Re: [PATCH net 2/6 RESEND] net: dsa: b53: manage SerDes power through the PCS callbacks

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Baseline decision: discard.

Pre-existing issues:
- [Medium] Baseline decision: discard (pre-existing, medium).

--

net: dsa: b53: manage SerDes power through the PCS callbacks

This patch moves SerDes power management out of the b53 mac_link_down()
and mac_link_up() handlers and into new pcs_enable() and pcs_disable()
callbacks. It removes the serdes_link_set io op. It also adds a
b53_serdes_modify() helper that does read-modify-write of SerDes
registers under reg_mutex.

> diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
> index c90894877626e..87771e3788f7f 100644
> --- a/drivers/net/dsa/b53/b53_serdes.c
> +++ b/drivers/net/dsa/b53/b53_serdes.c

[ ... ]

> @@ -202,32 +235,31 @@ static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>  		state->pause |= MLO_PAUSE_TX;
>  }
>  
> -void b53_serdes_link_set(struct b53_device *dev, int port, unsigned int mode,
> -			 phy_interface_t interface, bool link_up)
> +static int b53_serdes_enable(struct phylink_pcs *pcs)
>  {
> -	u8 lane = b53_serdes_map_lane(dev, port);
> -	int reg;
> +	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
> +	int ret;
>  
> -	if (lane == B53_INVALID_LANE)
> -		return;
> +	ret = b53_serdes_modify(priv->dev, priv->lane,
> +				B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK,
> +				BMCR_PDOWN, 0);
> +	return ret < 0 ? ret : 0;
> +}

[Severity: Low]
Should b53_serdes_enable() also pass the error to b53_serdes_error(), as
b53_serdes_disable() does below?

The phylink core ignores the return value of pcs_enable() in
phylink_major_config():

drivers/net/phy/phylink.c:phylink_major_config() {
    ...
	if (pl->pcs_state == PCS_STATE_STARTING || pcs_changed)
		phylink_pcs_enable(pl->pcs);
    ...
}

If clearing BMCR_PDOWN fails, the lane stays powered down and the link
won't come up until the interface is cycled.

b53_srab_op() and b53_srab_request_grant() already hit WARN_ON() before
they return -EIO, so the failure does show up in the log. This is mostly
about keeping the enable and disable paths consistent.

>  
> -	reg = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
> -			      SERDES_MII_BLK);
> -	if (reg < 0)
> -		goto out;
> +static void b53_serdes_disable(struct phylink_pcs *pcs)
> +{
> +	struct b53_pcs *priv = pcs_to_b53_pcs(pcs);
> +	int ret;
>  
> -	if (link_up)
> -		reg &= ~BMCR_PDOWN;
> -	else
> -		reg |= BMCR_PDOWN;
> -	reg = b53_serdes_write(dev, lane, B53_SERDES_MII_REG(MII_BMCR),
> -			       SERDES_MII_BLK, reg);
> -out:
> -	b53_serdes_error(dev, lane, reg);
> +	ret = b53_serdes_modify(priv->dev, priv->lane,
> +				B53_SERDES_MII_REG(MII_BMCR), SERDES_MII_BLK,
> +				BMCR_PDOWN, BMCR_PDOWN);
> +	b53_serdes_error(priv->dev, priv->lane, ret);
>  }

[ ... ]

> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
> index b9939bbd2cd51..9818b3922a377 100644
> --- a/drivers/net/dsa/b53/b53_srab.c
> +++ b/drivers/net/dsa/b53/b53_srab.c
> @@ -493,7 +493,6 @@ static const struct b53_io_ops b53_srab_ops = {
>  #if IS_ENABLED(CONFIG_B53_SERDES)
>  	.phylink_mac_select_pcs = b53_serdes_phylink_mac_select_pcs,
>  	.serdes_map_lane = b53_srab_serdes_map_lane,
> -	.serdes_link_set = b53_serdes_link_set,
>  #endif
>  };

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The new
SerDes power paths go through the SRAB accessors, and the timeout check
in b53_srab_request_grant() does not match its loop bound:

drivers/net/dsa/b53/b53_srab.c:b53_srab_request_grant() {
    ...
	for (i = 0; i < 20; i++) {
		ctrls = readl(regs + B53_SRAB_CTRLS);
		if (ctrls & B53_SRAB_CTRLS_RCAGNT)
			break;
		usleep_range(10, 100);
	}
	if (WARN_ON(i == 5))
		return -EIO;
    ...
}

If all 20 polls time out, i ends up as 20 and the function returns 0. The
caller then goes ahead with the SRAB access without having seen RCAGNT.

If the grant shows up on the sixth poll, i is 5. The function then WARNs
and returns -EIO even though the grant was obtained.

Was the check meant to be i == 20? It dates back to commit 967dd82ffc52
("net: dsa: b53: Add support for Broadcom RoboSwitch"). It is reachable
from this patch's code through:

b53_serdes_enable()->b53_serdes_modify()->b53_serdes_write_blk()->
    b53_srab_write16()->b53_srab_request_grant()

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