Re: [PATCH net-next v5 9/9] net: dsa: realtek: rtl8365mb: add support for RTL8367S-VB

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] The family-D SerDes re-latch work can be queued again after
  the reboot notifier has cancelled it.

--

net: dsa: realtek: rtl8365mb: add support for RTL8367S-VB

This patch adds the RTL8367S-VB (chip ID 0x6642, version 0x0010) to the
chip table. It is the first family D chip, so the family D handling added
in the earlier patches becomes reachable. On family D it also caps
priv->num_ports at RTL8365MB_D_MAX_NUM_PORTS.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 238a3fa765e97..352b15bd5508d 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -885,6 +892,19 @@ static const struct rtl8365mb_chip_info rtl8365mb_chip_infos[] = {
>  		.jam_table = rtl8365mb_init_jam_8365mb_vc,
>  		.jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
>  	},
> +	{
> +		.name = "RTL8367S-VB",
> +		.chip_id = 0x6642,
> +		.chip_ver = 0x0010,
> +		.family = RTL8365MB_FAMILY_D,

[Severity: Medium]

With this entry, the family D SerDes re-latch work and the reboot notifier
can now run. Can the sds_relatch work be queued again after the reboot
notifier has cancelled it?

The notifier cancels the work once. It sets no state that anything else
checks:

drivers/net/dsa/realtek/rtl8365mb_main.c:rtl8365mb_sds_reboot_notify() {
	/* Runs before device_shutdown() on reboot, halt, power off and kexec.
	 * cancel_delayed_work_sync() waits for a running pass, so the SerDes
	 * is not left parked and the bus is not touched while it is shut down.
	 */
	cancel_delayed_work_sync(&mb->sds_relatch);
    ...
}

rtl8365mb_pcs_config() queues the work for 2500base-x on family D
without any check:

	if (is_d && interface == PHY_INTERFACE_MODE_2500BASEX) {
		WRITE_ONCE(mb->sds_relatch_count, 1);
		schedule_delayed_work(&mb->sds_relatch, 0);
	}

After that, rtl8365mb_sds_relatch_work() re-arms itself every time it
exits:

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

pcs_config() could run after the reboot notifier chain in two ways:

- Probe overlaps reboot. rtl8365mb_detect() registers the notifier, and
  dsa_port_setup()->dsa_port_enable()->dsa_port_enable_rt()->
  phylink_start() then configures the HSGMII CPU port.
  kernel_restart_prepare() runs the notifiers before device_shutdown()
  calls wait_for_device_probe().

- A user port on the SerDes is brought up at the same time, through
  dsa_user_open()->phylink_start().

After that, rtl83xx_shutdown()->dsa_switch_shutdown() only closes the
conduits and detaches the user netdevs. It doesn't stop the CPU port
phylink, call rtl8365mb_teardown(), or cancel sds_relatch. The devm action
rtl8365mb_sds_relatch_cancel() doesn't run on shutdown either.

In that case, wouldn't the work keep parking SDS_MISC, sleeping for 20 ms
and doing regmap I/O over SMI/MDIO while the switch, and possibly the host
MDIO/GPIO controller, is being shut down? The notifier comment says this
cannot happen.

Would disable_delayed_work_sync() in the notifier work here? Another
option would be a shutdown flag that rtl8365mb_pcs_config() and the work
check before re-queueing.

> +		.extints = {
> +			{ 6, 0, PHY_INTF(SGMII) | PHY_INTF(HSGMII) },
> +			{ 7, 1, PHY_INTF(MII) | PHY_INTF(TMII) |
> +				PHY_INTF(RMII) | PHY_INTF(RGMII) },
> +		},
> +		.jam_table = rtl8365mb_init_jam_8365mb_vc,
> +		.jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> +	},
>  };

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/aa5fa12c-e43e-4e5e-9f77-c6eaf477abc4%40yahoo.com