Re: [PATCH 03/51] drm: add managed resources tied to drm_device

From: Sam Ravnborg <sam@ravnborg.org>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Marco Felsch <m.felsch@pengutronix.de>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 03/51] drm: add managed resources tied to drm_device
Date: Sat, 29 Feb 2020 12:17:10 +0100	[thread overview]
Message-ID: <20200229111710.GB3674@ravnborg.org> (raw)
In-Reply-To: <CAKMK7uHPWZ=F2EyqnM7x1GpXY_SGu3e_jGXX4cg0OGyx_+C8ig@mail.gmail.com>

Hi Daniel.

> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 9fcd6ab3c154..3e5627d6eba6 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -629,6 +629,9 @@ int drm_dev_init(struct drm_device *dev,
> > >       dev->dev = get_device(parent);
> > >       dev->driver = driver;
> > >
> > > +     INIT_LIST_HEAD(&dev->managed.resources);
> > > +     spin_lock_init(&dev->managed.lock);
> > > +
> > >       /* no per-device feature limits by default */
> > >       dev->driver_features = ~0u;
> > >
> > > @@ -828,8 +831,16 @@ static void drm_dev_release(struct kref *ref)
> > >               dev->driver->release(dev);
> > >       } else {
> > >               drm_dev_fini(dev);
> > > -             kfree(dev);
> > > +             if (!dev->managed.final_kfree) {
> > > +                     WARN_ON(!list_empty(&dev->managed.resources));
> > > +                     kfree(dev);
> > > +             }
> >
> > This looks sub-optimal.
> > We cannot be sure a driver have used drmm_add_final_kfree() if it makes
> > use of drmm_.
> > So we may not WARN in all relavant cases.
> > Also, we cannot expect all drivers that uses devmm_ to have managed
> > to get rid of their ->release call-back.
> 
> The above is purely transition code. It gets cleaned up once all
> drivers call drmm_add_final_kfree(). This all disappears again, but
> indeed looks like the interim state isn't quite what we want.
> 
> > So the right thing looks to me like we should move it out to be
> > unconditional. Se we will WARN_ON(!list_empty(&dev->managed.resources))
> > always.
> 
> Until the driver has set drmm_add_final_kfree it's actually dangerous
> to use the drmm stuff. Exactly because of the use-after-free you point
> out below. Hence the warning to make sure there's no release actions.
> I'll shuffle this around to make sure we call kfree last for all
> possible paths and make sure this bisects all correctly.

I was just reviewing the code I had on hand, and did not look further in
the set of patches.
Very good if we can keep is bisectable.

> > > + *
> > > + * Based on drivers/base/devres.c
> > > + */
> > > +
> > > +#include <drm/drm_managed.h>
> > > +
> > > +#include <linux/list.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/spinlock.h>
> > > +
> > > +#include <drm/drm_device.h>
> > > +#include <drm/drm_print.h>
> >
> > It is good practice to group the include files.
> > And drm/ comes after linux/
> 
> I try to put the main header first to make sure it's stand-alone, but
> I guess that works with the header check now? Do I need to do anything
> to get that checked?

The header-check infrastructure was dropped again - see:
fcbb8461fd2376ba3782b5b8bd440c929b8e4980

So including it as the first header in the implmentation
file is likely the best way to keep it self contained.
We will spot errors sooner.

> > > +static __always_inline struct drmres * alloc_dr(drmres_release_t release,
> > > +                                             size_t size, gfp_t gfp, int nid)
> > Why do we force the compiler to inline this?
> > Seems a little agressive.
> 
> It's not for performance, but for kmalloc_trace_caller. No point if
> our caller is always some boring function from drm_managed.c that
> calls alloc_dr. If we force alloc_dr to inline, then we get the caller
> of the drm_managed.c function traced as allocator. Much better.
> 
> (I stole that trick from devres.c)
> 
> I'll add a comment to explain this.
Thanks.

> 
> > All the two users so far uses dev_to_node(dev->dev) for the nid.
> > Maybe let this function take a drm_device * and thus move the
> > calculation to this function?
> 
> Copypastes like that :-) I feel somewhat meh here ...
Well - keep the diff for devres smaller for now and leave it.
It was just an observation.

> > > +     /**
> > > +      * @managed:
> > > +      *
> > > +      * Managed resources linked to the lifetime of this &drm_device as
> > > +      * tracked by @ref.
> > > +      */
> > > +     struct {
> > > +             struct list_head resources;
> > > +             void *final_kfree;
> > > +             spinlock_t lock;
> > > +     } managed;
> >
> > I am missing kernel-doc here.
> > At least document that lock is used to guard access to resources.
> > (s/lock/lock_resources/ ?)
> 
> Dunno why, but the support for name sub-structures seems to have
> broken in kerneldoc. So I can type it, but it's not showing up, so I
> didn't bother. Well I had it, but deleted it again. It's still
> documented to work, but I have no idea what I'm doing wrong.

Most readers prefer the .c files as the source.
I personally read the generated kernel doc when I google
and when I check that my own stuff looks good in kernel-doc format.
So comments are still valueable despite not being picked up by
kernel-doc.
You know this - but I just wanted to encourage you to write the few
lines that may help me and others :-)

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel