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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/26/26 16:47, netdev-bot+sashiko@kernel.org wrote:
> 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?

Yes it should, v2 will have that.

> 
> 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()

Yes, this pre-existing issue will be addressed separately.
-- 
Florian