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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2467 bytes --]

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

> 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()? 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?!

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]