net: dsa: mxl862xx: fix tag_8021q forwarding on firmware 1.0.85 - #213
Open
meehien wants to merge 3 commits into
Open
net: dsa: mxl862xx: fix tag_8021q forwarding on firmware 1.0.85#213meehien wants to merge 3 commits into
meehien wants to merge 3 commits into
Conversation
In tag_8021q mode, host FDB and MDB entries installed into a shared
bridge FID carry a portmap of virtual bridge ports. The bridging
engine ANDs that portmap with the ingress port's bridge_port_map, which
holds only that port's own VBP, so the intersection selects the ingress
port's VBP and with it the egress EVLAN that tags the frame with the
management VID identifying the source port to DSA.
Building the portmap from the current bridge members reads
priv->ports[].bridge_port_cpu for each member. A member whose VBP has
not been allocated yet reads back as 0, and
mxl862xx_fw_portmap_set_bit() sets bit 0 for it -- bit 0 is a valid
portmap bit belonging to another port, not a no-op, so the host entry
is installed pointing at the wrong bridge port. The resync and
drop-VBP callbacks run on bridge join and leave attempt to repair this
after the fact, which leaves a window in which host-terminated traffic
is misdirected, and depends on every reordering of DSA's setup sequence
being covered.
List every user port's VBP unconditionally instead, skipping ports
whose VBP is not allocated. The hardware AND against bridge_port_map
already encodes membership, so the wider portmap resolves to the same
single VBP per ingress port while removing both the ordering dependency
and the need to track membership changes in the entry itself.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
[Mihai Ordean: extracted from a7fdaf365e22 ("net: dsa: add 802.1Q VLAN-based
tag driver for MxL862xx") in https://github.com/dangowrt/linux, which
revises the earlier version of that commit already carried in this
tree; commit message and kernel-doc expanded to record the bit 0
aliasing, the code is unchanged from the original]
Signed-off-by: Mihai Ordean <research@mihaiordean.com>
The per-port PCE trap rules (IEEE 802.1D link-local, IGMP, MLDv1/v2)
redirect matching frames to the CPU port with an alternative portmap
and override the lookup FID to the dedicated snooping FID. The rule
action leaves learning_action at 0, so the hardware keeps learning the
source address of every trapped frame under the regular port and
bridge configuration.
Trapped management frames must not populate the MAC table: they are
diverted from the normal forwarding path, and any entry created from
them reflects the trap override rather than the bridge topology. On
switch firmware 1.0.85, tag_8021q mode stops forwarding entirely (R3)
while the native SpTag mode keeps working; in tag_8021q mode correct
forwarding depends on static host FDB entries carrying virtual bridge
port portmaps, which learned entries for the same {MAC, FID} can
displace or shadow. This trap-rule learning is the last behavioural
difference between this driver's trap path and the reference driver
in the dangowrt/linux tree at 07c32ae88874, which sets
LEARNING_FORCE_NOT here and passes bridge selftests in tag_8021q mode
on firmware >= 1.0.84.
Set learning_action to LEARNING_FORCE_NOT in the shared trap action
fill helper so the source addresses of trapped frames are never
learned, and add the mxl862xx_pce_action_learning enum, which this
driver documented in prose but never defined.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
[meehien: extracted from f82da48b6323 ("net: dsa: mxl862xx: trap
link-local and multicast snooping frames to CPU") in
https://github.com/dangowrt/linux, which revises the earlier version of
that commit already carried in this tree; commit message rewritten to
explain the effect on tag_8021q forwarding under firmware 1.0.85, the
code is unchanged from the original]
Signed-off-by: Mihai Ordean <research@mihaiordean.com>
In tag_8021q mode every user port gets a virtual bridge port on the CPU RX path. mxl862xx_setup_virtual_bridge_port() populates its bridge port map with the single user port the VBP belongs to, and nothing writes that field again: mxl862xx_set_cpu_vbp() updates the same bridge port with a mask covering only the forwarding ID and the egress sub-meters, leaving bridge_port_map zeroed in the request. That relies on the firmware preserving a field its caller did not select. On MxL86252C firmware 1.0.85 it does not, and mxl862xx_complete_tag_8021q_setup() calls mxl862xx_set_cpu_vbp() for every port immediately after the VBPs are allocated, so the map is already gone before the first frame is forwarded. The map is how a CPU-originated frame reaches the wire: the CPU port's ingress rules reassign the frame onto the VBP, the destination lookup runs in the VBP's per-port FID where nothing was learned, and the resulting flood has no member to flood to. The symptom is that the switch forwards between its own ports normally while every frame the host originates is lost, in both directions and on every user port. Write the port map on every VBP update and select it in the mask, so the driver never depends on the firmware retaining an unselected field. This is correct on any firmware; where the field was being preserved the write is idempotent. Confirmed on a BPI-R4 Pro (MxL86252C, firmware 1.0.85 build 85): without this change host-originated traffic is lost entirely; with it host and switch-local traffic work in every direction, including DHCP and DHCPv6-PD. Selecting the mask bit while writing an empty map reproduces the failure exactly, so it is the map contents that matter and not the act of selecting the field. Signed-off-by: Mihai Ordean <research@mihaiordean.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three patches that make
tag_8021qmode forward correctly on MxL86252C firmware1.0.85. Without them the switch probes cleanly and every port links, but no frame
the host originates reaches the wire — and traffic between user ports keeps
working, so it presents as a routing or firewall fault rather than a switch one.
This is the follow-up to the firmware-enablement series merged in
mxl862xx-fw-1.0.85. That series got the driver past probe on 1.0.85; this onegets it forwarding.
mt7988a-bananapi-bpi-r4-pro.dtsiselectsdsa-tag-protocol = "mxl862xx-8021q", so this is the default path on the R4 Pro,not an opt-in one.
The series
map every allocated VBP in host bridge entries— Daniel GolleHost FDB/MDB entries in a shared bridge FID build their portmap from the
current bridge members. A member whose VBP is not yet allocated reads back as
0, and
mxl862xx_fw_portmap_set_bit()then sets bit 0 — a valid portmap bitbelonging to another port, not a no-op — so the entry is installed pointing at
the wrong bridge port. Listing every user port's VBP unconditionally resolves
to the same single VBP after the hardware AND against
bridge_port_map, andremoves the ordering dependency entirely.
never learn from CPU-trapped management frames— Daniel GolleThe per-port trap rules for link-local, IGMP and MLDv1/v2 leave
learning_actionat 0, so the hardware learns the source address of everytrapped frame under the regular port configuration. In
tag_8021qmodecorrect forwarding depends on static host FDB entries carrying VBP portmaps,
which learned entries for the same {MAC, FID} can displace or shadow.
always write the virtual bridge port mapmxl862xx_set_cpu_vbp()updates each VBP with a mask covering onlyBRIDGE_IDandEGRESS_SUB_METER, leavingbridge_port_mapzeroed in therequest. 1.0.85 does not preserve a field the caller did not select, and
mxl862xx_complete_tag_8021q_setup()calls it for every port immediatelyafter allocation — so the map is gone before the first frame is forwarded.
The map is the delivery path: the CPU port's ingress rules reassign a
CPU-originated frame onto the VBP, the destination lookup runs in the VBP's
per-port FID where nothing is learned, and the resulting flood has no member
to flood to. Correct on any firmware; where the field was being preserved the
write is idempotent.
Attribution
Patches 1 and 2 are Daniel's, extracted from
a7fdaf365e22andf82da48b6323in https://github.com/dangowrt/linux (branch
wip). Both revise commits thistree already carries in an earlier form, so they are backports of his own later
work rather than new changes. Their code is unchanged from his originals; I
expanded the commit messages and one kernel-doc block, recorded in the bracketed
notes. His
Signed-off-by:is preserved from the original commits.Patch 3 is not in his tree — his
mxl862xx_set_cpu_vbp()does not select orwrite
bridge_port_map.Note for a future dangowrt sync
Daniel's
wipbranch has since grown6ab6dd06b95c("recover switch stuck inMCUboot rescue mode"), which is not here and is worth picking up now that people
are flashing 1.0.85. His revised trap commit also routes PCE rules through
GSW_TFLOW_PCERULELOGICWRITEvia his ownmxl862xx_pce_rule_write()helper,which overlaps the helper merged in the firmware-enablement series. Nothing
conflicts today, but the next sync will need to reconcile the two.