Re: [PATCH v3 2/7] dt-bindings: net: dsa: mediatek,mt7530: add passthrough mode

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The binding tells DT authors to model passthrough as a two-
  switch DSA cascade ('the upstream switch is configured with a DSA link…
- [Low] The passthrough paragraph added to the description of
  Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml (lines…

--

dt-bindings: net: dsa: mediatek,mt7530: add passthrough mode

This commit documents a "passthrough" mode for MT7530/MT7531 switches in
the mediatek,mt7530 binding description. In this mode the upstream switch
forwards traffic between its CPU port and a DSA link to a downstream
switch, and it does not alter the MediaTek tag.

> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index 90b3582b76190..e29fb86934981 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -73,6 +73,13 @@ description: |
>  
>        Check out example 7.
>  
> +  The MT7530 and MT7531 switches are capable of being operated in "passthrough"
> +  mode. In this mode, traffic is unconditionally passed between the CPU port
> +  and the DSA port which links to a second switch. To use passthrough mode, the
> +  upstream switch is configured with a DSA link to the downstream switch. In

[Severity: Medium]
Should this also say which DSA switch index each switch has to use?

The MediaTek tag carries only a 3-bit source port and no switch ID.
mtk_tag_rcv() always looks the port up on switch index 0:

net/dsa/tag_mtk.c:mtk_tag_rcv() {
    ...
	port = (hdr & MTK_HDR_RECV_SOURCE_PORT_MASK);

	skb->dev = dsa_conduit_find_user(dev, 0, port);
	if (!skb->dev) {
		kfree_skb(skb);
		return NULL;
	}
    ...
}

dsa_conduit_find_user() only matches DSA_PORT_TYPE_USER ports on the
given switch index. In passthrough mode the upstream switch forwards the
downstream switch's tag unchanged, so port is a downstream port number.

The two switches can't share an index, because
dsa_switch_parse_member_of() returns -EEXIST for a duplicate.

Take a DT that uses the natural ordering: the CPU-facing upstream switch
at dsa,member switch index 0 and the downstream switch at index 1. Would
every received frame then be looked up on the upstream switch?

That switch has no user ports in passthrough mode. The lookup would
return NULL and the frame would be freed, so all RX traffic on the
downstream user ports would be lost.

No later patch in the series seems to change this. tag_mtk.c is
unchanged, the driver doesn't touch ds->index, and the final binding has
no passthrough example or dsa,member guidance.

Could the text say that the downstream switch must be switch index 0 in
dsa,member and the upstream switch must use a non-zero index? An example
showing this would also work.

> +  passthrough mode the upstream switch is restricted, with no other ports or
> +  switch configuration allowed.

[Severity: Low]
These topology rules are stated only in prose. Could they be encoded in
the schema as well?

Ports match ^(ethernet-)?port@[0-6]$ with additionalProperties: true.
The per-port reg and phy-mode rules apply only to ports that have an
ethernet property. Those rules are the top-level if: required:
[ ethernet ] block and the builtin-dsa-port, mt7530-dsa-port and
mt7531-dsa-port $defs.

As a result, a port with a link phandle on the upstream switch gets no
reg or phy-mode check. A node with extra user ports next to the CPU and
link ports also passes dt_binding_check.

mt753x_check_passthrough() in the driver does reject that case at probe
with "Only two ports allowed in passthrough mode". The schema does not.

The port reg description still reads:

    Port address described must be 5 or 6 for CPU port and from 0 to 5
    for user ports.

Should it mention DSA link ports too?

Later in the series, "dt-bindings: net: dsa: mediatek,mt7530: add
econet,en751221" adds mt7530-passthrough-dsa-port, which requires the
link port to be reg 6 and trgmii. That $def applies only to the
downstream econet,en751221-gswitch. The upstream econet,en751221-switch
uses builtin-dsa-port, so its link port is still unconstrained.

There is also a small wording mismatch. The commit message says:

    MT7530 switches can be configured to blindly pass packets between their

but the binding text says "The MT7530 and MT7531 switches". Could these
be made to match?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028011317.1557238-1-cjd%40cjdns.fr