On Sun, Sep 03, 2023 at 04:53:42PM +0100, Russell King (Oracle) wrote:
> On Fri, Sep 01, 2023 at 04:41:12PM -0700, Douglas Anderson wrote:
> > Based on grepping through the source code this driver appears to be
> > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > time. Among other things, this means that if a panel is in use that it
> > won't be cleanly powered off at system shutdown time.
> >
> > The fact that we should call drm_atomic_helper_shutdown() in the case
> > of OS shutdown/restart comes straight out of the kernel doc "driver
> > instance overview" in drm_drv.c.
> >
> > This driver was fairly easy to update. The drm_device is stored in the
> > drvdata so we just have to make sure the drvdata is NULL whenever the
> > device is not bound.
>
> ... and there I think you have a misunderstanding of the driver model.
> Please have a look at device_unbind_cleanup() which will be called if
> probe fails, or when the device is removed (in other words, when it is
> not bound to a driver.)
>
> Also, devices which aren't bound to a driver won't have their shutdown
> method called (because there is no driver currently bound to that
> device.) So, ->probe must have completed successfully, and ->remove
> must not have been called for that device.
>
> So, I think that all these dev_set_drvdata(dev, NULL) that you're
> adding are just asking for a kernel janitor to come along later and
> remove them because they serve no purpose... so best not introduce
> them in the first place.
What would that hypothetical janitor clean up exactly? Code making sure
that there's no dangling pointer? Doesn't look very wise to me.
Maxime
On Mon, Sep 04, 2023 at 09:36:10AM +0200, Maxime Ripard wrote:
> On Sun, Sep 03, 2023 at 04:53:42PM +0100, Russell King (Oracle) wrote:
> > On Fri, Sep 01, 2023 at 04:41:12PM -0700, Douglas Anderson wrote:
> > > Based on grepping through the source code this driver appears to be
> > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > time. Among other things, this means that if a panel is in use that it
> > > won't be cleanly powered off at system shutdown time.
> > >
> > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > instance overview" in drm_drv.c.
> > >
> > > This driver was fairly easy to update. The drm_device is stored in the
> > > drvdata so we just have to make sure the drvdata is NULL whenever the
> > > device is not bound.
> >
> > ... and there I think you have a misunderstanding of the driver model.
> > Please have a look at device_unbind_cleanup() which will be called if
> > probe fails, or when the device is removed (in other words, when it is
> > not bound to a driver.)
> >
> > Also, devices which aren't bound to a driver won't have their shutdown
> > method called (because there is no driver currently bound to that
> > device.) So, ->probe must have completed successfully, and ->remove
> > must not have been called for that device.
> >
> > So, I think that all these dev_set_drvdata(dev, NULL) that you're
> > adding are just asking for a kernel janitor to come along later and
> > remove them because they serve no purpose... so best not introduce
> > them in the first place.
>
> What would that hypothetical janitor clean up exactly? Code making sure
> that there's no dangling pointer? Doesn't look very wise to me.
How can there be a dangling pointer when the driver core removes the
pointer for the driver in these cases?
If I were to accept the argument that the driver core _might_ "forget"
to NULL out this pointer, then that argument by extension also means
that no one should make use of the devm_* stuff either, just in case
the driver core forgets to release that stuff. Best have every driver
manually release those resources. Nope, that doesn't work, because
driver authors tend to write buggy cleanup paths.
There are janitors that go around removing this stuff, and janitorial
patches tend to keep coming even if one says nak at any particular
point... and yes, janitors do go around removing this unnecessary
junk from drivers.
You will find examples of this removal in commit
ec3b1ce2ca34, 5cdade2d77dd, c7cb175bb1ef
Moreover:
7efb10383181
is also removing unnecessary driver code. Testing for driver data being
NULL when we know that a _successful_ probe has happened (because
->remove won't be called unless we were successful) and the probe
always sets drvdata non-NULL is also useless code.
If ultimately you don't trust the driver model to do what it's been
doing for more than the last decade, then I wonder whether you should
be trusting the kernel to manage your hardware!
Anyway, I've said no to this patch for a driver that I'm marked as
maintainer for, so at least do not make the changes I am objecting to
to that driver. Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!