Re: [PATCH net 5/6 RESEND] net: dsa: b53: report link state through the PCS

[Florian Fainelli]

On 10/26/26 16:47, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 (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] b53_srab_port_thread()
>    (drivers/net/dsa/b53/b53_srab.c:371-380) now calls…
> - [Low] In b53_serdes_link_change()
>    (drivers/net/dsa/b53/b53_serdes.c:317-330), if the…
>
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>    b53_srab_request_grant() (drivers/net/dsa/b53/b53_srab.c:91-112) has a…
>
> --
>
> net: dsa: b53: report link state through the PCS
>
> This commit changes b53_serdes_get_state() to decode the Clause 22 BMSR
> and LPA registers with phylink_mii_c22_pcs_decode_state(). The SRAB
> port interrupt now notifies phylink through phylink_pcs_change(), using
> the SerDes digital status LINK_STATUS bit instead of the MAC link status.
> The patch also removes b53_port_event() and turns on PCS polling.
>
> > diff --git a/drivers/net/dsa/b53/b53_serdes.c b/drivers/net/dsa/b53/b53_serdes.c
> > index fc20e211e67e1..c7f3e131f2def 100644
> > --- a/drivers/net/dsa/b53/b53_serdes.c
> > +++ b/drivers/net/dsa/b53/b53_serdes.c
> > @@ -291,50 +291,44 @@ static void b53_serdes_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
> >   static void b53_serdes_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> >   				 struct phylink_link_state *state)
> >   {
>
> [ ... ]
>
> >   	bmsr = b53_serdes_read(dev, lane, B53_SERDES_MII_REG(MII_BMSR),
> >   			       SERDES_MII_BLK);
> > -	if (bmsr < 0) {
> > -		state->link = false;
> > -		b53_serdes_error(dev, lane, bmsr);
> > -		return;
> > -	}
> > +	if (bmsr < 0)
> > +		goto error;
>
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch.
> The SRAB accessors behind b53_serdes_read() have a mismatched timeout
> check in b53_srab_request_grant() in drivers/net/dsa/b53/b53_srab.c:
>
> 	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 the grant never arrives, the loop exits with i == 20 and the function
> returns 0. The register access then goes ahead without the RCA grant.
>
> If the grant arrives on the sixth poll (i == 5), the function warns and
> returns -EIO for an access that should have succeeded.
> b53_serdes_get_state() then reports that error as link down.
>
> Should this check be i == 20?
>
> [ ... ]
>
> > +void b53_serdes_link_change(struct b53_device *dev, int port)
> > +{
> > +	u8 lane = b53_serdes_map_lane(dev, port);
> > +	int dig;
> >   
>
> [ ... ]
>
> > +	if (lane >= B53_N_PCS || !dev->pcs[lane].dev)
> > +		return;
> > +
> > +	/* Use the live PCS status, not latched BMSR or MAC override status. */
> > +	dig = b53_serdes_read(dev, lane, B53_SERDES_DIGITAL_STATUS,
> > +			      SERDES_DIGITAL_BLK);
> > +	b53_serdes_error(dev, lane, dig);
> > +	phylink_pcs_change(&dev->pcs[lane].pcs, dig >= 0 && (dig & LINK_STATUS));
>
> [Severity: Medium]
> Can this force a fixed-link SerDes port down?
>
> b53_srab_port_thread() calls b53_serdes_link_change() for every port
> whose mux mode is SGMII. Nothing on this path checks the phylink AN
> mode. b53_serdes_phylink_mac_select_pcs() returns the PCS for SGMII and
> 1000BASE-X in every mode, so pcs->phylink is also set for fixed-link
> ports.
>
> When the SerDes has no sync, for example because the peer is not up
> yet, this is the call chain:
>
> b53_srab_port_thread()
>    b53_serdes_link_change()
>      phylink_pcs_change(pcs, false)
>        phylink_link_changed()
>          pl->link_failed = true;
>
> phylink_resolve() checks link_failed before the fixed-link branch:
>
> 	} else if (pl->link_failed) {
> 		link_state = pl->link_config;
> 		link_state.link = false;
> 		retrigger = true;
> 	} else if (pl->act_link_an_mode == MLO_AN_FIXED) {
>
> As a result, b53_phylink_mac_link_down() calls
> b53_force_link(dev, port, false). The retriggered resolve then takes
> the MLO_AN_FIXED path, and b53_phylink_mac_link_up() calls
> b53_force_link(dev, port, true).
>
> Before this patch, b53_port_event() read B53_LINK_STAT. That register
> follows the forced override, so a fixed link always reported up=true
> and this interrupt never took it down.
>
> The commit message says MAC link status "can be overridden by
> mac_link_up/down()". Does the link_state_pN interrupt also fire when
> those override bits change? If so, each forced toggle would raise
> another interrupt that reports SerDes down again. The forced MAC link
> and the carrier would then keep flapping until the SerDes gets sync. I
> can't confirm that hardware behaviour from the code.
>
> Would it make sense to call phylink_pcs_change() only when the PCS
> provides the link state (in-band), or to keep the previous MAC-status
> notification for the other modes? The commit message also doesn't
> mention this change in fixed-link behaviour.
>
> [Severity: Low]
> What happens here when the B53_SERDES_DIGITAL_STATUS read fails?
>
> b53_serdes_read() can return a negative errno, for example -EIO from
> b53_srab_request_grant() or a b53_srab_op() timeout. In that case the
> expression evaluates to false and phylink_pcs_change(pcs, false) is
> called. phylink_link_changed() then sets pl->link_failed. As a result,
> phylink_resolve() forces a link down/up cycle while the real link state
> is unknown.
>
> The phylink_pcs_change() kernel-doc says:
>
>    If in doubt about the link state at interrupt time, then pass true if
>    pcs_get_state() returns the latched link-down state, otherwise pass
>    false.
>
> b53_serdes_get_state() now reads MII_BMSR, which latches link-down.
> Should the error case pass true, so that a transient access failure in
> the IRQ thread doesn't cause a link flap?

Agreed on both findings. The next revision restricts PCS notifications 
to in-band links. On an IRQ status-read failure, it requests resolution 
without forcing link down, since pcs_get_state() reads the latched BMSR.

The pre-existing SRAB grant timeout check is also corrected in a 
separate patch.

pw-bot: cr
--
Florian