Re: [PATCH net-next v4 8/9] net: dsa: realtek: rtl8365mb: re-latch the family D SerDes

[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 · 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] rtl8365mb_sds_relatch_work() relies on a lockless `if
  (!dev_get_drvdata(priv->dev)) return;` check at entry (line 1812), not…
- [Low] The comment on RTL8365MB_D_SDS_RESTORE_TRIES (lines 315-318) says
  a failed restore write "is retried this many times" (3).

--

net: dsa: realtek: rtl8365mb: re-latch the family D SerDes

On family D switches, this patch adds a delayed work item that keeps
re-driving the DISABLE -> HSGMII edge in SDS_MISC until the SerDes
receiver latches, then health-checks the link every 10 s. pcs_config()
starts the work for 2500base-x. pcs_config(), pcs_disable(), teardown()
and a devres action cancel it. A new sds_lock serializes the SDS_INDACS
indirect access sequences.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index d23e80f1d745d..bef95f91eea50 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -299,6 +300,28 @@
>  #define   RTL8365MB_SDS_MISC_SGMII_SPD_MASK		GENMASK(8, 7)
>  #define   RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK	BIT(6)
>  

[ ... ]

> +/* The SerDes stays parked until the restore write succeeds, so a failed
> + * write is retried this many times before giving up until the next run.
> + */
> +#define RTL8365MB_D_SDS_RESTORE_TRIES		3

[Severity: Low]
This isn't a bug, but does this comment match how the macro is used?
rtl8365mb_sds_relatch_restore() uses it as the total number of writes,
counting the first one:

	for (i = 0; i < RTL8365MB_D_SDS_RESTORE_TRIES; i++) {
		ret = regmap_update_bits(...);
		if (!ret)
			return 0;
		usleep_range(1000, 2000);
	}

So a failed first write gets two retries, not three. The loop also sleeps
1-2 ms after the last failed write before it gives up.

[ ... ]

> @@ -1669,8 +1725,192 @@ static int rtl8365mb_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,

[ ... ]

> +static void rtl8365mb_sds_relatch_work(struct work_struct *work)
> +{
> +	struct rtl8365mb *mb = container_of(to_delayed_work(work),
> +					    struct rtl8365mb, sds_relatch);
> +	struct realtek_priv *priv = mb->priv;

[ ... ]

> +	/* rtl83xx_shutdown() clears the driver data and nothing stops the PCS
> +	 * then: do not touch the hardware or rearm after that.
> +	 */
> +	if (!dev_get_drvdata(priv->dev))
> +		return;

[Severity: Medium]
Is this check enough to keep the work away from the hardware after
shutdown?

rtl83xx_shutdown() clears the driver data with a plain store and never
cancels sds_relatch:

drivers/net/dsa/realtek/rtl83xx.c:rtl83xx_shutdown() {
	dsa_switch_shutdown(ds);

	dev_set_drvdata(priv->dev, NULL);
}

dsa_switch_shutdown() does not tear down the CPU port's phylink, so neither
rtl8365mb_pcs_disable() nor rtl8365mb_teardown() runs on this path.

Suppose a run passes the dev_get_drvdata() test just before the store. It
then reads SDS_MISC, runs the SDS_INDACS sequence under sds_lock, parks
SDS_MISC, sleeps 20 ms in usleep_range(), restores, and calls
schedule_delayed_work() again. All of that can happen after
rtl83xx_shutdown() has returned, while device_shutdown() moves on to the
parent MDIO/SMI bus device.

Closing the conduit in dsa_switch_shutdown() drops the trunk, so the work
is likely to take the park path at about that moment. If the reboot or
kexec lands in the middle of a park, could the SerDes stay in DISABLE
mode?

The commit message says:

  rtl83xx_shutdown() clears the driver data and nothing stops the PCS
  then, so the work does nothing once the driver data is gone

That seems true only for runs that start after the store. Would a
cancel_delayed_work_sync() of sds_relatch on the shutdown path be more
reliable than this lockless drvdata check?

[ ... ]

> +	usleep_range(20000, 21000);
> +
> +	if (rtl8365mb_sds_relatch_restore(mb)) {

[ ... ]

> +rearm:
> +	schedule_delayed_work(&mb->sds_relatch, delay);
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/38872e6f-1264-4a93-9f81-8212173e909d%40yahoo.com