Message-ID: <20211110225611.h6klnoscntufdsv3@skbuf>
Date:   Thu, 11 Nov 2021 00:56:11 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        kernel@...gutronix.de,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next] net: dsa: Some cleanups in remove code

On Wed, Nov 10, 2021 at 10:03:46PM +0100, Uwe Kleine-König wrote:
> Hello Vladimir,
> 
> On Wed, Nov 10, 2021 at 03:15:40PM +0200, Vladimir Oltean wrote:
> > On Tue, Nov 09, 2021 at 06:50:55PM +0100, Uwe Kleine-König wrote:
> > > On Tue, Nov 09, 2021 at 01:54:34PM +0200, Vladimir Oltean wrote:
> > > > Your commit prefix does not reflect the fact that you are touching the
> > > > vsc73xx driver. Try "net: dsa: vsc73xx: ".
> > > 
> > > Oh, I missed that indeed.
> > > 
> > > > On Tue, Nov 09, 2021 at 12:39:21PM +0100, Uwe Kleine-König wrote:
> > > > > vsc73xx_remove() returns zero unconditionally and no caller checks the
> > > > > returned value. So convert the function to return no value.
> > > > 
> > > > This I agree with.
> > > > 
> > > > > For both the platform and the spi variant ..._get_drvdata() will never
> > > > > return NULL in .remove() because the remove callback is only called after
> > > > > the probe callback returned successfully and in this case driver data was
> > > > > set to a non-NULL value.
> > > > 
> > > > Have you read the commit message of 0650bf52b31f ("net: dsa: be
> > > > compatible with masters which unregister on shutdown")?
> > > 
> > > No. But I did now. I consider it very surprising that .shutdown() calls
> > > the .remove() callback and would recommend to not do this. The commit
> > > log seems to prove this being difficult.
> > 
> > Why do you consider it surprising?
> 
> In my book .shutdown should be minimal and just silence the device, such
> that it e.g. doesn't do any DMA any more.

To me, the more important thing to consider is that many drivers lack
any ->shutdown hook at all, and making their ->shutdown simply call
->remove is often times the least-effort path of doing something
reasonable towards quiescing the hardware. Not to mention the lesser
evil compared to not having a ->shutdown at all.

That's not to say I am not in favor of a minimal shutdown procedure if
possible. Notice how DSA has dsa_switch_shutdown() vs dsa_unregister_switch().
But judging what should go into dsa_switch_shutdown() was definitely not
simple and there might still be corner cases that I missed - although it
works for now, knock on wood.

The reality is that you'll have a very hard time convincing people to
write a dedicated code path for shutdown, if you can convince them to
write one at all. They wouldn't even know if it does all the right
things - it's not like you kexec every day (unless you're using Linux as
a bootloader - but then again, if you do that you're kind of asking for
trouble - the reason why this is the case is exactly because not having
a ->shutdown hook implemented by drivers is an option, and the driver
core doesn't e.g. fall back to calling the ->remove method, even with
all the insanity that might ensue).

> > Many drivers implement ->shutdown by calling ->remove for the simple
> > reason that ->remove provides for a well-tested code path already, and
> > leaves the hardware in a known state, workable for kexec and others.
> > 
> > Many drivers have buses beneath them. Those buses go away when these
> > drivers unregister, and so do their children.
> > 
> > ==============================================
> > 
> > => some drivers do both => children of these buses should expect to be
> > potentially unregistered after they've been shut down.
> 
> Do you know this happens, or do you "only" fear it might happen?

Are you asking whether there are SPI controllers that implement
->shutdown as ->remove? Just search for "\.shutdown" in drivers/spi.
3 out of 3 implementations call ->remove.

If you really have time to waste, here, have fun: Lino Sanfilippo had
not one, but two (!!!) reboot problems with his ksz9897 Ethernet switch
connected to a Raspberry Pi, both of which were due to other drivers
implementing their ->shutdown as ->remove. First driver was the DSA
master/host port (bcmgenet), the other was the bcm2835_spi controller.
https://patchwork.kernel.org/project/netdevbpf/cover/20210909095324.12978-1-LinoSanfilippo@gmx.de/
https://patchwork.kernel.org/project/netdevbpf/cover/20210912120932.993440-1-vladimir.oltean@nxp.com/
https://patchwork.kernel.org/project/netdevbpf/cover/20210917133436.553995-1-vladimir.oltean@nxp.com/
As soon as we implemented ->shutdown in DSA drivers (which we had mostly
not done up until that thread) we ran into the surprise that ->remove
will get called too. Yay. It wasn't trivial to sort out, but we did it
eventually in a more systematic way. Not sure whether there's anything
to change at the drivers/base/ level.

Since any SPI-controlled DSA switch can fundamentally be connected to
mostly any SPI controller, then yes, I have no doubt at all that the
same can happen even with the vsc73xx driver you're patching here.

> > > > To remove the check for dev_get_drvdata == NULL in ->remove, you need to
> > > > prove that ->remove will never be called after ->shutdown. For platform
> > > > devices this is pretty easy to prove, for SPI devices not so much.
> > > > I intentionally kept the code structure the same because code gets
> > > > copied around a lot, it is easy to copy from the wrong place.
> > > 
> > > Alternatively remove spi_set_drvdata(spi, NULL); from
> > > vsc73xx_spi_shutdown()?
> > 
> > What is the end goal exactly?
> 
> My end goal is:
> 
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index eb7ac8a1e03c..183cf15fbdd2 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -280,7 +280,7 @@ struct spi_message;
>  struct spi_driver {
>         const struct spi_device_id *id_table;
>         int                     (*probe)(struct spi_device *spi);
> -       int                     (*remove)(struct spi_device *spi);
> +       void                    (*remove)(struct spi_device *spi);
>         void                    (*shutdown)(struct spi_device *spi);
>         struct device_driver    driver;
>  };
> 
> As (nearly) all spi drivers must be touched in the same commit, the
> preparing goal is to have these remove callbacks simple, such that I
> only have to replace their "return 0;" by "return;" (or nothing if it's
> at the end of the function). Looking at vsc73xx's remove function I
> didn't stop at this minimal goal and simplified the stuff that I thought
> to be superflous.

Yeah, well I guess you can stop at the minimal goal.

> > > Also I'm not aware how platform devices are
> > > different to spi devices that the ordering of .remove and shutdown() is
> > > more or less obvious than on the other bus?!
> > 
> > Not sure what you mean. See the explanation above. For the "platform"
> > bus, there simply isn't any code path that unregisters children on the
> > ->shutdown callback. For other buses, there is.
> 
> OK, with your last mail I understood that now, thanks.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Powered by blists - more mailing lists