Re: [PATCH net 3/6 RESEND] net: dsa: b53: constrain SerDes interfaces to initialized PCSes

[netdev-bot+sashiko@kernel.org]

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

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

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] Ports muxed to SGMII now fail phylink creation when SerDes
  discovery fails or CONFIG_B53_SERDES=n.

--

net: dsa: b53: constrain SerDes interfaces to initialized PCSes

This patch stores a supported interface bitmap on each b53 PCS that is
discovered, and uses that bitmap for capability reporting and PCS
selection. On SRAB ports whose mux selects a SerDes, it clears the
parallel interfaces inherited from the common code. It also stops
advertising 2500BASE-X.

> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
> index 9818b3922a377..3fabae80dc1a8 100644
> --- a/drivers/net/dsa/b53/b53_srab.c
> +++ b/drivers/net/dsa/b53/b53_srab.c
> @@ -451,9 +451,11 @@ static void b53_srab_phylink_get_caps(struct b53_device *dev, int port,
>  
>  	switch (p->mode) {
>  	case PHY_INTERFACE_MODE_SGMII:
> +		/* The mux connects this port to a SerDes, not a parallel bus. */
> +		phy_interface_zero(config->supported_interfaces);
>  #if IS_ENABLED(CONFIG_B53_SERDES)
>  		/* If p->mode indicates SGMII mode, that essentially means we
> -		 * are using a serdes. As the serdes for the capabilities.
> +		 * are using a SerDes. Ask it for the supported interfaces.
>  		 */
>  		b53_serdes_phylink_get_caps(dev, port, config);
>  #endif

[Severity: Medium]
Could an SGMII-muxed port end up here with an empty supported_interfaces
mask, so that phylink_create() fails for it?

b53_srab_mux_init() sets p->mode before it tries discovery. If discovery
fails, the loop continues with p->mode still set to SGMII and
pcs[lane].dev still NULL:

    case MUX_CONFIG_SGMII:
        p->mode = PHY_INTERFACE_MODE_SGMII;
        ret = b53_serdes_init(dev, port);
        if (ret) {
            dev_err(...);
            continue;
        }

In that state b53_serdes_phylink_get_caps() returns early on
!dev->pcs[lane].dev, so nothing goes back into the mask.

With CONFIG_B53_SERDES=n, the b53_serdes_init() stub in b53_serdes.h
always returns -ENODEV and the refill call is compiled out. Every
SGMII-muxed port then ends up in this state.

phylink_create() then rejects the port:

    if (phy_interface_empty(config->supported_interfaces)) {
        dev_err(config->dev,
            "phylink: error: empty supported_interfaces\n");
        return ERR_PTR(-EINVAL);
    }

DSA then sets the user port up as unused. Before this patch, these ports
kept GMII/MII/REVMII from b53_phylink_get_caps() and registered normally.

Ports that never select the PCS are hit as well. In
bcm958625-meraki-alamo.dtsi, SRAB port@4 and port@5 (sw0/sw1) use a
1000 Mb/s fixed-link with no phy-mode, so DSA passes
PHY_INTERFACE_MODE_NA. b53_serdes_phylink_mac_select_pcs() returns NULL
for that mode both before and after this patch.

These two ports are the conduits for the cascaded QCA8337 switches
(ethernet = <&sgmii0> / <&sgmii1>). Wouldn't those switches lose their
upstream link too?

The commit message says:

    A failed discovery must not leave a usable serial interface
    advertised without its PCS.

It does not mention that the port then fails to register at all, or that
fixed-link/NA ports are affected. Is that the intended result?

Later patches in the series do not change b53_srab_phylink_get_caps() or
b53_srab_mux_init(), so this still applies at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022225656.1387306-1-florian.fainelli%40broadcom.com