Hi,
On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle)
> <[email protected]> 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 <[email protected]> wrote:
>
> Hi,
>
> On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle)
> > <[email protected]> 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 <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle)
> > > <[email protected]> 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Hi,
On Wed, Sep 20, 2023 at 11:58 AM Russell King (Oracle)
<[email protected]> 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 <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Sep 5, 2023 at 7:23 AM Doug Anderson <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Sun, Sep 3, 2023 at 8:53 AM Russell King (Oracle)
> > > > <[email protected]> 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