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

[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 (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?

>  }
> +EXPORT_SYMBOL(b53_serdes_link_change);

[ ... ]

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