Re: [PATCH net-next] net: dsa: Some cleanups in remove code

On Tue, Nov 09, 2021 at 06:50:55PM +0100, Uwe Kleine-König wrote:

Hello,

Cc += gregkh, maybe he has something to say on this matter

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?

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.

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?

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.

quoted

quoted

Also setting driver data to NULL is not necessary, this is already done
in the driver core in __device_release_driver(), so drop this from the
remove callback, too.

And this was also intentional, for visibility more or less. I would like
you to ack that you understand the problems surrounding ->remove/->shutdown
ordering for devices on buses, prior to making seemingly trivial cleanups.

I see that the change is not so obviously correct as I thought. I'll
have to think about this and will respin if and when I find a sane way
forward.

A way forward towards what? This is literally a cosmetic patch that
would happen to break some stuff, were it to be applied.