| Message ID | 20230901164111.RFT.1.I3d5598bd73a59b5ded71430736c93f67dc5dea61@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: non-drm-misc drivers call drm_atomic_helper_shutdown() at the right times | expand |
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. Thanks.
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 Fri, 1 Sep 2023 16:41:12 -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. > > > [ ... ] Reviewed-by: Maxime Ripard <mripard@kernel.org> Thanks! 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.
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. To make things simpler, > drm_atomic_helper_shutdown() has been modified to consider a NULL > drm_device as a noop in the patch ("drm/atomic-helper: > drm_atomic_helper_shutdown(NULL) should be a noop"). > > Suggested-by: Maxime Ripard <mripard@kernel.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> As the listed driver author for the reasons I've stated - NAK.
On Mon, Sep 04, 2023 at 08:55:43AM +0100, Russell King (Oracle) wrote: > 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? You can still access that pointer from remove after the call to component_del(). It's unlikely, sure, but the issue is still there. > 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. It's funny that you go for that argument, because using devm is known to be a design issue in KMS (and the rest of the kernel to some extent), so yeah, I very much agree with you there. > Nope, that doesn't work, because driver authors tend to write buggy > cleanup paths. And using devm is just as buggy for a KMS driver. We even discourage its use in the documentation. But really, I'm not sure what your point is there. Does devm lead to bugs? Sure. Is it less buggy that manually unrolling an exit path by hand? Probably. I seriously don't get the relation to some code clearing a pointer after it's been freed though. > 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 Other people doing it doesn't make it right (or wrong). And really, I'm not arguing that it's right, I'm saying that it's not wrong. It's probably being over-cautious, especially in that driver, but it's not wrong. > 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. Again, I fail to see what the relationship is there. > 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! It's not the kernel driver model that I don't trust, it's C's (lack of) memory safety and management. And like you said yourself, "driver authors tend to write buggy" > 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. You're entitled to that opinion indeed. Maxime
Hi, On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle) <linux@armlinux.org.uk> 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.) ...and there I think you didn't read this patch closely enough and perhaps that you have a misunderstanding of the component model. Please have a look at the difference between armada_drm_unbind() and armada_drm_remove() and also check which of those two functions is being modified by my patch. Were this patch adding a call to "dev_set_drvdata(dev, NULL)" in armada_drm_remove() then your NAK would be justified. However, I am not aware of anything in the component unbind path nor in the failure case of component bind that would NULL the drvdata. Kindly look at the patch a second time with this in mind. -Doug
Hi, On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle) > <linux@armlinux.org.uk> 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.) > > ...and there I think you didn't read this patch closely enough and > perhaps that you have a misunderstanding of the component model. > Please have a look at the difference between armada_drm_unbind() and > armada_drm_remove() and also check which of those two functions is > being modified by my patch. Were this patch adding a call to > "dev_set_drvdata(dev, NULL)" in armada_drm_remove() then your NAK > would be justified. However, I am not aware of anything in the > component unbind path nor in the failure case of component bind that > would NULL the drvdata. > > Kindly look at the patch a second time with this in mind. Since I didn't see any further response, I'll assume that my explanation here has addressed your concerns. If not, I can re-post it without NULLing the "drvdata". While I still believe this is unsafe in some corner cases because of the component model used by this driver, at least it would get the shutdown call in. In any case, what's the process for landing patches to this driver? Running `./scripts/get_maintainer.pl --scm -f drivers/gpu/drm/armada/armada_drv.c` seems to indicate that this should go through the git tree: git git://git.armlinux.org.uk/~rmk/linux-arm.git drm-armada-devel ...but it doesn't appear that recent changes to this driver have gone that way. Should this land through drm-misc? -Doug
Maxime, On Wed, Sep 13, 2023 at 8:34 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle) > > <linux@armlinux.org.uk> 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.) > > > > ...and there I think you didn't read this patch closely enough and > > perhaps that you have a misunderstanding of the component model. > > Please have a look at the difference between armada_drm_unbind() and > > armada_drm_remove() and also check which of those two functions is > > being modified by my patch. Were this patch adding a call to > > "dev_set_drvdata(dev, NULL)" in armada_drm_remove() then your NAK > > would be justified. However, I am not aware of anything in the > > component unbind path nor in the failure case of component bind that > > would NULL the drvdata. > > > > Kindly look at the patch a second time with this in mind. > > Since I didn't see any further response, I'll assume that my > explanation here has addressed your concerns. If not, I can re-post it > without NULLing the "drvdata". While I still believe this is unsafe in > some corner cases because of the component model used by this driver, > at least it would get the shutdown call in. > > In any case, what's the process for landing patches to this driver? > Running `./scripts/get_maintainer.pl --scm -f > drivers/gpu/drm/armada/armada_drv.c` seems to indicate that this > should go through the git tree: > > git git://git.armlinux.org.uk/~rmk/linux-arm.git drm-armada-devel > > ...but it doesn't appear that recent changes to this driver have gone > that way. Should this land through drm-misc? Do you have any advice here? Should I land this through drm-misc-next, put it on ice for a while, or resend without the calls to NULL our the drvdata? Thanks! -Doug
On Wed, Sep 20, 2023 at 11:03:32AM -0700, Doug Anderson wrote: > Maxime, > > On Wed, Sep 13, 2023 at 8:34 AM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle) > > > <linux@armlinux.org.uk> 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.) > > > > > > ...and there I think you didn't read this patch closely enough and > > > perhaps that you have a misunderstanding of the component model. > > > Please have a look at the difference between armada_drm_unbind() and > > > armada_drm_remove() and also check which of those two functions is > > > being modified by my patch. Were this patch adding a call to > > > "dev_set_drvdata(dev, NULL)" in armada_drm_remove() then your NAK > > > would be justified. However, I am not aware of anything in the > > > component unbind path nor in the failure case of component bind that > > > would NULL the drvdata. > > > > > > Kindly look at the patch a second time with this in mind. > > > > Since I didn't see any further response, I'll assume that my > > explanation here has addressed your concerns. If not, I can re-post it > > without NULLing the "drvdata". While I still believe this is unsafe in > > some corner cases because of the component model used by this driver, > > at least it would get the shutdown call in. > > > > In any case, what's the process for landing patches to this driver? > > Running `./scripts/get_maintainer.pl --scm -f > > drivers/gpu/drm/armada/armada_drv.c` seems to indicate that this > > should go through the git tree: > > > > git git://git.armlinux.org.uk/~rmk/linux-arm.git drm-armada-devel > > > > ...but it doesn't appear that recent changes to this driver have gone > > that way. Should this land through drm-misc? > > Do you have any advice here? Should I land this through drm-misc-next, > put it on ice for a while, or resend without the calls to NULL our the > drvdata? Sorry, I haven't had a chance to look at it, but I think you're probably right, so I withdraw my objection. Please take it through drm-misc for the time being. Thanks.
Hi, On Wed, Sep 20, 2023 at 11:58 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Wed, Sep 20, 2023 at 11:03:32AM -0700, Doug Anderson wrote: > > Maxime, > > > > On Wed, Sep 13, 2023 at 8:34 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > > > Hi, > > > > > > > > On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle) > > > > <linux@armlinux.org.uk> 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.) > > > > > > > > ...and there I think you didn't read this patch closely enough and > > > > perhaps that you have a misunderstanding of the component model. > > > > Please have a look at the difference between armada_drm_unbind() and > > > > armada_drm_remove() and also check which of those two functions is > > > > being modified by my patch. Were this patch adding a call to > > > > "dev_set_drvdata(dev, NULL)" in armada_drm_remove() then your NAK > > > > would be justified. However, I am not aware of anything in the > > > > component unbind path nor in the failure case of component bind that > > > > would NULL the drvdata. > > > > > > > > Kindly look at the patch a second time with this in mind. > > > > > > Since I didn't see any further response, I'll assume that my > > > explanation here has addressed your concerns. If not, I can re-post it > > > without NULLing the "drvdata". While I still believe this is unsafe in > > > some corner cases because of the component model used by this driver, > > > at least it would get the shutdown call in. > > > > > > In any case, what's the process for landing patches to this driver? > > > Running `./scripts/get_maintainer.pl --scm -f > > > drivers/gpu/drm/armada/armada_drv.c` seems to indicate that this > > > should go through the git tree: > > > > > > git git://git.armlinux.org.uk/~rmk/linux-arm.git drm-armada-devel > > > > > > ...but it doesn't appear that recent changes to this driver have gone > > > that way. Should this land through drm-misc? > > > > Do you have any advice here? Should I land this through drm-misc-next, > > put it on ice for a while, or resend without the calls to NULL our the > > drvdata? > > Sorry, I haven't had a chance to look at it, but I think you're probably > right, so I withdraw my objection. Please take it through drm-misc for > the time being. Thanks. Pushed to drm-misc-next: c478768ce807 drm/armada: Call drm_atomic_helper_shutdown() at shutdown time
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index e8d2fe955909..fa1c67598706 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -148,6 +148,7 @@ static int armada_drm_bind(struct device *dev) err_kms: drm_mode_config_cleanup(&priv->drm); drm_mm_takedown(&priv->linear); + dev_set_drvdata(dev, NULL); return ret; } @@ -166,6 +167,7 @@ static void armada_drm_unbind(struct device *dev) drm_mode_config_cleanup(&priv->drm); drm_mm_takedown(&priv->linear); + dev_set_drvdata(dev, NULL); } static void armada_add_endpoints(struct device *dev, @@ -230,6 +232,11 @@ static int armada_drm_remove(struct platform_device *pdev) return 0; } +static void armada_drm_shutdown(struct platform_device *pdev) +{ + drm_atomic_helper_shutdown(platform_get_drvdata(pdev)); +} + static const struct platform_device_id armada_drm_platform_ids[] = { { .name = "armada-drm", @@ -243,6 +250,7 @@ MODULE_DEVICE_TABLE(platform, armada_drm_platform_ids); static struct platform_driver armada_drm_platform_driver = { .probe = armada_drm_probe, .remove = armada_drm_remove, + .shutdown = armada_drm_shutdown, .driver = { .name = "armada-drm", },
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. To make things simpler, drm_atomic_helper_shutdown() has been modified to consider a NULL drm_device as a noop in the patch ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop"). Suggested-by: Maxime Ripard <mripard@kernel.org> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- This commit is only compile-time tested. drivers/gpu/drm/armada/armada_drv.c | 8 ++++++++ 1 file changed, 8 insertions(+)