Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20230901234202.566951-1-dianders@chromium.org>
 <20230901164111.RFT.1.I3d5598bd73a59b5ded71430736c93f67dc5dea61@changeid>
 <ZPSsBhbekKY7VyDg@shell.armlinux.org.uk> <CAD=FV=WT4Hf1XVA641WtNFg4WRYFKarU1WOkLPEbr0eiVQuZPg@mail.gmail.com>
In-Reply-To: <CAD=FV=WT4Hf1XVA641WtNFg4WRYFKarU1WOkLPEbr0eiVQuZPg@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Wed, 13 Sep 2023 08:34:45 -0700
Message-ID: <CAD=FV=UpJEFKcsnHSzPqEiGaWusp50DrcEh=mcra4eLeRQT5NQ@mail.gmail.com>
Subject: Re: [RFT PATCH 01/15] drm/armada: Call drm_atomic_helper_shutdown()
 at shutdown time
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     dri-devel@lists.freedesktop.org,
        Maxime Ripard <mripard@kernel.org>, airlied@gmail.com,
        daniel@ffwll.ch, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Tue, Sep 5, 2023 at 7:23=E2=80=AFAM Doug Anderson <dianders@chromium.org=
> wrote:
>
> Hi,
>
> On Sun, Sep 3, 2023 at 8:53=E2=80=AFAM 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 i=
t
> > > 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 th=
e
> > > 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