Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughIn Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Hardware without additional MAC address registers can receive secondary unicast traffic through the existing promiscuous fallback, with no merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
defcom5-rockchip
left a comment
There was a problem hiding this comment.
The analysis matches the code and the fix does what it says for RK3528 — withdrawing the flag is the right way to let __dev_set_rx_mode() do the fallback. One thing before merge, and I measured it rather than guessed:
In rk-6.1, GMAC_HW_FEAT_ADDMAC is BIT(18) (dwmac4.h:240) and dma_cap->multi_addr = (hw_cap & GMAC_HW_FEAT_ADDMAC) >> 18 (dwmac4_dma.c:387). ADDMACADRSEL in MAC_HW_Feature0 is a 5-bit field, bits 22:18, so in this tree multi_addr is the low bit of the count, not the count. !priv->dma_cap.multi_addr is therefore true for 0 additional registers and for 2, 4, 6…30.
Measured on RK3588 (Orange Pi 5B, GMAC1 @ 0xfe1c0000, MAC_VERSION = 0x3051, DWMAC 5.10a):
HW_FEATURE0 = 0x1a1173f7 -> ADDMACADRSEL = 4 MACADR32SEL = 0 MACADR64SEL = 0
Four additional address registers, bit 18 zero. So !priv->dma_cap.multi_addr is true on RK3588, and this patch as written clears IFF_UNICAST_FLT on a core that has four working registers — any macvlan/bridge/DSA secondary address would push it to promiscuous instead of MAC_ADDRESS1, on every RK3588 in the tree.
Mainline fixed the parsing in 384ee2379a66 ("net: stmmac: dwmac4: Read the UC filter size from hardware capabilities", 2026-08-31, in torvalds/linux):
-#define GMAC_HW_FEAT_ADDMAC BIT(18)
+#define GMAC_HW_FEAT_MACADR64SEL BIT(24)
+#define GMAC_HW_FEAT_MACADR32SEL BIT(23)
+#define GMAC_HW_FEAT_ADDMAC GENMASK(22, 18)
Folding that hunk in ahead of this change makes the condition exact: RK3588 reads 4 and keeps its filter, RK3528 reads 0 and gets the fallback. Since the banks are independently selectable, !multi_addr && !additional_32_addr && !additional_64_addr would also cover a core that has only the 32/64 bank (the driver programs from register 1, so those would still miss).
Fuller option: 384ee2379a66 + 6815415d68b9 together with the net fixes "Account for primary MAC in UC filtering" (d29b399150b0..96e8cb5527ce) compute the real filter size from HW features and fall to PR inside dwmac4_set_filter() without touching the flag — RK3528 works, and RK3588 gets its five slots instead of the default one. Bigger backport; this patch plus the GENMASK hunk is fine for rkr7.2 if that isn't wanted yet.
Could you add RK3528's HW_FEATURE0 value (bits 22:18, 23, 24) to the commit message so the "no additional registers" claim is on record? The dwmac1000 path is fine as-is — DMA_HW_FEAT_ADDMAC really is a single bit on the 3.x cores.
… capabilities commit 384ee2379a66a86903e1acccc30c2f87a3020809 upstream. dwmac4 has multiple banks of perfect filter entries, independently configurable during IP integration. The multi_addr bank reports a number between 0 and 31 corresponding to the actual number of entries in that bank, while the 32 and 64 banks are all-or-nothing. Expose these caps over debugfs as well. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de> Link: https://patch.msgid.link/20260831070121.349778-2-maxime.chevallier@bootlin.com Signed-off-by: Paolo Abeni <pabeni@redhat.com> [rpardini: 6.1 has no plat->core_type, key the debugfs output on plat->has_gmac4. Needed so multi_addr is the register count, not bit 18.] Signed-off-by: Ricardo Pardini <ricardo@pardini.net> Assisted-By: Claude Opus 5 <noreply@anthropic.com>
dwmac4_setup() and dwmac1000_setup() set IFF_UNICAST_FLT unconditionally, and unicast_filter_entries defaults to 1. On a core with no additional MAC address registers, a secondary address is written to the missing MAC_ADDRESS1 and never matches, while the flag keeps __dev_set_rx_mode() from falling back to promiscuous mode. RK3528 is such a core: DSA user ports get no unicast until a second port pushes netdev_uc_count() past the filter size. Withdraw the flag when the 1-31 address bank is empty. The 32/64 banks don't count, since the filter is programmed from register 1. XGMAC does not report the count, so leave it alone. MAC_HW_FEATURE0, GMAC1: RK3528 (Radxa E24C): 0x100163e7 ADDMACADRSEL=0 32SEL=0 64SEL=0 RK3588 (Orange Pi 5B): 0x1a1173f7 ADDMACADRSEL=4 32SEL=0 64SEL=0 Signed-off-by: Ricardo Pardini <ricardo@pardini.net> Assisted-By: Claude Opus 5 <noreply@anthropic.com>
de051fc to
4daf91f
Compare
|
@defcom5-rockchip done, I think... could you check? |
defcom5-rockchip
left a comment
There was a problem hiding this comment.
Checked — this does what the commit message says now.
- The backport's functional hunks match 384ee2379a66 exactly (ADDMAC as GENMASK(22, 18), MACADR32SEL/MACADR64SEL, the multi_addr / additional_32_addr / additional_64_addr parsing). The only difference is the debugfs block keyed on has_gmac4 instead of core_type, as your backport note says.
- With that parsing, 2/2 behaves as described: RK3588 (HW_FEATURE0 0x1a1173f7) reads multi_addr = 4 and keeps IFF_UNICAST_FLT; RK3528 (0x100163e7) reads 0 and gets the promiscuous fallback. On dwmac1000, multi_addr is still the 1-bit ADDMAC flag, where 0 = no additional registers is the right reading.
- The new block sits inside the hw_cap_support branch, after stmmac_hwif_init(), which is where dwmac4/dwmac1000 set IFF_UNICAST_FLT; nothing sets it again afterwards, and with unicast_filter_entries = 0 dwmac4_set_filter() goes promiscuous for any secondary address.
- Both commits apply cleanly on rk-6.1-rkr7.2 and build for arm64 with W=1, no new warnings. I have not run this revision on hardware; on RK3588 it is a no-op by construction.
Thanks for folding in the backport and the register values.
An LLM assistant (Claude, Anthropic) did the code reading and the compile test for this review and drafted this reply.
net: stmmac: don't claim IFF_UNICAST_FLT without address registers
dwmac4_setup() and dwmac1000_setup() set IFF_UNICAST_FLT unconditionally,
and unicast_filter_entries defaults to 1. On a core with no additional
MAC address registers, a secondary address is written to the missing
MAC_ADDRESS1 and never matches, while the flag keeps __dev_set_rx_mode()
from falling back to promiscuous mode.
RK3528 is such a core: DSA user ports get no unicast until a second port
pushes netdev_uc_count() past the filter size.
Withdraw the flag when the 1-31 address bank is empty. The 32/64 banks
don't count, since the filter is programmed from register 1. XGMAC does
not report the count, so leave it alone.
[BACKPORT] net: stmmac: dwmac4: Read the UC filter size from hardware capabilities
commit 384ee2379a66a86903e1acccc30c2f87a3020809 upstream.