Re: [PATCH net v3] net: dsa: mxl-gsw1xx: manually clear RANEG bit

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Dec 08, 2025 at 01:27:04AM +0000, Daniel Golle wrote:
>  static void gsw1xx_remove(struct mdio_device *mdiodev)
>  {
>  	struct gswip_priv *priv = dev_get_drvdata(&mdiodev->dev);
> +	struct gsw1xx_priv *gsw1xx_priv;
>  
>  	if (!priv)
>  		return;
>  
> +	gsw1xx_priv = container_of(priv, struct gsw1xx_priv, gswip);
> +	cancel_delayed_work_sync(&gsw1xx_priv->clear_raneg);
> +
>  	gswip_disable_switch(priv);
>  
>  	dsa_unregister_switch(priv->ds);

Can we please pay attention to ->remove methods, and code them properly
please?

There are two golden rules of driver programming.

1. Do not publish the device during probe until hardware setup is
   complete. If you publish before hardware setup is complete, userspace
   is free to race with the hardware setup and start using the device.
   This is especially true of recent systems which use hotplug events
   via udev and systemd to do stuff.

2. Do not start tearing down a device until the user interfaces have
   been unpublished. Similar to (1), while the user interface is
   published, uesrspace is completely free to interact with the device
   in any way it sees fit.

In this case, what I'm concerned with is the call above to
cancel_delayed_work_sync() before dsa_unregister_switch(). While
cancel_delayed_work_sync() will stop this work and wait for the handler
to finish running before returning (which is safe) there is a window
between this call and dsa_unregister_switch() where the user _could_
issue a badly timed ethtool command which invokes
gsw1xx_pcs_an_restart(), which would re-schedule the delayed work,
thus undoing the cancel_delayed_work_sync() effect in this path.

So please, always unpublish and then tear-down.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!