Re: [PATCH net-next] net: dsa: Some cleanups in remove code
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-11-10 22:56:16
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:quoted
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.
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).
quoted
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.
quoted hunk ↗ jump to hunk
quoted
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.
Yeah, well I guess you can stop at the minimal goal.
quoted
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/ |