Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 29 Nov 2018 13:00:35 +1100
From:   Sam Bobroff <sbobroff@linux.ibm.com>
To:     Dave Airlie <airlied@gmail.com>
Cc:     Dave Airlie <airlied@redhat.com>, Dave Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] drm/ast: Fix connector leak during driver unload
References: <cover.1541397462.git.sbobroff@linux.ibm.com>
 <3350c5dd5ea08c71e4769ea3801290e7f9238a6c.1541397462.git.sbobroff@linux.ibm.com>
 <CAPM=9txRjd6E3DxdcM14mzWG1P-Cr70P61Eyce95bBhJtqfH7A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="mYCpIKhGyMATD0i+"
Content-Disposition: inline
In-Reply-To: <CAPM=9txRjd6E3DxdcM14mzWG1P-Cr70P61Eyce95bBhJtqfH7A@mail.gmail.com>
User-Agent: Mutt/1.9.3 (2018-01-21)
Message-Id: <20181129020034.GA10328@tungsten.ozlabs.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--mYCpIKhGyMATD0i+
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Nov 29, 2018 at 09:40:53AM +1000, Dave Airlie wrote:
> On Mon, 5 Nov 2018 at 15:59, Sam Bobroff <sbobroff@linux.ibm.com> wrote:
> >
> > When unloading the ast driver, a warning message is printed by
> > drm_mode_config_cleanup() because a reference is still held to one of
> > the drm_connector structs.
> >
> > Correct this by calling drm_framebuffer_remove() in
> > ast_fbdev_destroy().
> >
> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > ---
> >  drivers/gpu/drm/ast/ast_fb.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> > index 0cd827e11fa2..655372ea81e9 100644
> > --- a/drivers/gpu/drm/ast/ast_fb.c
> > +++ b/drivers/gpu/drm/ast/ast_fb.c
> > @@ -263,6 +263,10 @@ static void ast_fbdev_destroy(struct drm_device *d=
ev,
> >  {
> >         struct ast_framebuffer *afb =3D &afbdev->afb;
> >
> > +       /* drm_framebuffer_remove() expects us to hold a ref, which it
> > +        * will drop, so take one: */
> > +       drm_framebuffer_get(&afb->base);
> > +       drm_framebuffer_remove(&afb->base);
>=20
> This doesn't seem corret, no other driver does this pattern, and I
> can't believe ast is special here.
>
> The get just doesn't make sense.

Thanks for having a look at this, as I said in the cover letter I was
concerned that it might not be a good fix.

But the AST driver does seem to be special (or just old?) because it
embeds the drm_framebuffer directly into ast_fbdev and (almost all)
other drivers dynamically allocate and reference count theirs.

The drm_framebuffer_get() certainly looks weird but it is there in order
to cause drm_framebuffer_remove() to call legacy_remove_fb(), which it
won't do unless the refcount is at least 2. (And because the
drm_framebuffer isn't dynamically allocated in this case we don't really
care about the reference count anyway.)

An alternative might be to call legacy_remove_fb() directly, but it's
declared static.  Do you think it would be better to expose it and call
it directly from the AST driver code? Or is there some other better way
to put the drm_connectors?

> Dave.

Cheers,
Sam.

--mYCpIKhGyMATD0i+
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEELWWF8pdtWK5YQRohMX8w6AQl/iIFAlv/SDoACgkQMX8w6AQl
/iJoGAf9FQRdN5z13PVoWcgv/sZQqLDpv5MeRN0JyKIf3J4af4T+TgYpoJ2pjzEU
Fz9m5pSJCFoXp2ZQHhkVYu5bJ95gbNLf0m+LwSK/p/BVOVdbCMI6U+EwMGxFQf9a
05he/anVPdMLz+DJImv5W/znCn1zhVapO7EETjMq+mfCbEOu8gHhvvVDVL8tTNNe
zVgh4ytcI3dxkHaadKgenRaMTZ8UESqA1AgpvB/TkoDr3GdIWv+qPcrkZix7hpOo
Z+J6x0tqnvY1jm52mpV9HIJ7TKtzzLwHQdYfUGqiYdGDarEd3PmqvFRJ4QJP2kWo
JQgOodb2pTGbcPoarNhIaBiWFSC/3Q==
=MMgY
-----END PGP SIGNATURE-----

--mYCpIKhGyMATD0i+--