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:

quoted

On Tue, Nov 09, 2021 at 01:54:34PM +0200, Vladimir Oltean wrote:

quoted

Your commit prefix does not reflect the fact that you are touching the
vsc73xx driver. Try "net: dsa: vsc73xx: ".

Oh, I missed that indeed.

quoted

On Tue, Nov 09, 2021 at 12:39:21PM +0100, Uwe Kleine-König wrote:

quoted

vsc73xx_remove() returns zero unconditionally and no caller checks the
returned value. So convert the function to return no value.

This I agree with.

quoted

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?

quoted

quoted

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.

quoted

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