Re: [PATCH net-next v5 3/3] net: dsa: let user ports wait for a PHY that probes late

[netdev-bot+sashiko]

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] This patch arms phylink's self-requeueing late-PHY poller
  (pl->late_phy_poll on system_freezable_power_efficient_wq) for…
- [Low] The comment this patch adds on struct
  dsa_switch::phy_may_probe_late (include/net/dsa.h:458-460) says a…

--

net: dsa: let user ports wait for a PHY that probes late

This adds a phy_may_probe_late bit to struct dsa_switch. dsa_user_phy_setup()
passes it to phylink so that a user port can wait for a PHY whose driver
needs host firmware and probes late. mt7530 opts in.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 7781a63b4e6fd..e4c155605e27f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3548,6 +3548,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
>  	priv->ds->priv = priv;
>  	priv->ds->ops = &mt7530_switch_ops;
>  	priv->ds->phylink_mac_ops = &mt753x_phylink_mac_ops;
> +	priv->ds->phy_may_probe_late = true;

[Severity: Medium]

With this opt-in, phylink's late PHY poller (pl->late_phy_poll) can be
pending on mt7530 user ports. Does anything on the switch shutdown path
cancel it?

mt7530_shutdown() and the mmio shutdown handler both call
dsa_switch_shutdown(). That function closes the conduits, detaches and
unlinks the user netdevs, and clears conduit->dsa_ptr:

net/dsa/dsa.c:dsa_switch_shutdown() {
    ...
	dsa_switch_for_each_user_port(dp, ds) {
		conduit = dsa_port_to_conduit(dp);
		user_dev = dp->user;

		netif_device_detach(user_dev);
		netdev_upper_dev_unlink(conduit, user_dev);
	}
    ...
}

It never calls phylink_disconnect_phy() or phylink_destroy(). Those two
functions and phylink_fwnode_phy_connect() are the only callers of
phylink_late_phy_cancel(). phylink_stop() does not cancel the work either.

Suppose the PHY driver binds shortly before a reboot or kexec. A poll that
is still queued (up to 30 seconds out after backoff) can then run after
.shutdown has returned:

phylink_late_phy_poll()
    phylink_attach_phy()
        phy_attach_direct()
            phy_init_hw()
            phy_resume()
    phylink_bringup_phy()
        phy_request_interrupt()

Could this cause MDIO traffic, a PHY soft reset and config_init, and an
IRQ request on hardware that has already been shut down? Could it also
leave that interrupt enabled for a kexec'd kernel?

For comparison, sfp_shutdown() calls cancel_delayed_work_sync() on
sfp->poll and sfp->timeout.

>  	mutex_init(&priv->reg_mutex);
>  	spin_lock_init(&priv->stats_lock);
>  	INIT_DELAYED_WORK(&priv->stats_work, mt7530_stats_poll);
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 5d12191b6f6f5..f7e78c3a123d3 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -455,6 +455,11 @@ struct dsa_switch {
>  	 */
>  	u32			dscp_prio_mapping_is_global:1;
>  
> +	/* Drivers whose .port_enable copes with a NULL phy may set this to
> +	 * let user ports wait for a PHY that needs host firmware.
> +	 */
> +	u32			phy_may_probe_late:1;

[Severity: Low]

Is "copes with a NULL phy" the whole requirement for opting in?

The only caller of .port_enable is dsa_user_open() ->
dsa_port_enable_rt(dp, dev->phydev). When phylink_late_phy_poll() attaches
the PHY later, it forces a major config and calls phy_start(). It does not
call .port_enable again with the new phy.

gswip_port_enable() already handles a NULL phy:

	if (phydev)
		mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;

So gswip meets the rule as written. Yet the commit message says gswip must
not opt in, because the PHY address would stay wrong until the next open.

The same applies to qca8k. Adding an if (phy) check around
phy_support_asym_pause(phy) in qca8k_port_enable() would satisfy this
comment. A PHY that attaches late would then never advertise asym pause.

Should the comment say that the driver must not depend on the phy argument
of .port_enable at all? That would match the reasoning in the commit
message.

> +
>  	/* Listener for switch fabric events */
>  	struct notifier_block	nb;
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001130208.105558-1-f%40lex.la