Date:   Wed, 10 Nov 2021 22:03:46 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Vladimir Oltean <olteanv@...il.com>
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

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.

> 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?

> > > 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.

> > 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/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists