[PATCH 23/52] drm: manage drm_minor cleanup with drmm_
Daniel Vetter
daniel.vetter at ffwll.ch
Wed Feb 19 15:34:41 UTC 2020
On Wed, Feb 19, 2020 at 3:47 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> On Wed, Feb 19, 2020 at 11:20:53AM +0100, Daniel Vetter wrote:
> > The cleanup here is somewhat tricky, since we can't tell apart the
> > allocated minor index from 0. So register a cleanup action first, and
> > if the index allocation fails, unregister that cleanup action again to
> > avoid bad mistakes.
> >
> > The kdev for the minor already handles NULL, so no problem there.
> >
> > Hence add drmm_remove_action() to the drm_managed library.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> > drivers/gpu/drm/drm_drv.c | 74 +++++++++++++----------------------
> > drivers/gpu/drm/drm_managed.c | 28 +++++++++++++
> > include/drm/drm_managed.h | 4 ++
> > 3 files changed, 59 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 1f7ab88d9435..03a1fb377830 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -93,19 +93,35 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
> > }
> > }
> >
> > +static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> > +{
> > + struct drm_minor *minor = data;
> > + unsigned long flags;
> > +
> > + put_device(minor->kdev);
> > +
> > + spin_lock_irqsave(&drm_minor_lock, flags);
> > + idr_remove(&drm_minors_idr, minor->index);
> > + spin_unlock_irqrestore(&drm_minor_lock, flags);
> > +}
> > +
> > static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> > {
> > struct drm_minor *minor;
> > unsigned long flags;
> > int r;
> >
> > - minor = kzalloc(sizeof(*minor), GFP_KERNEL);
> > + minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> > if (!minor)
> > return -ENOMEM;
> >
> > minor->type = type;
> > minor->dev = dev;
> >
> > + r = drmm_add_action(dev, drm_minor_alloc_release, minor);
> > + if (r)
> > + return r;
> > +
> > idr_preload(GFP_KERNEL);
> > spin_lock_irqsave(&drm_minor_lock, flags);
> > r = idr_alloc(&drm_minors_idr,
> > @@ -116,47 +132,18 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> > spin_unlock_irqrestore(&drm_minor_lock, flags);
> > idr_preload_end();
> >
> > - if (r < 0)
> > - goto err_free;
> > + if (r < 0) {
> > + drmm_remove_action(dev, drm_minor_alloc_release, minor);
> > + return r;
> > + }
> >
> > minor->index = r;
> > -
> > minor->kdev = drm_sysfs_minor_alloc(minor);
> > - if (IS_ERR(minor->kdev)) {
> > - r = PTR_ERR(minor->kdev);
> > - goto err_index;
> > - }
> > + if (IS_ERR(minor->kdev))
> > + return PTR_ERR(minor->kdev);
> >
> > *drm_minor_get_slot(dev, type) = minor;
> > return 0;
> > -
> > -err_index:
> > - spin_lock_irqsave(&drm_minor_lock, flags);
> > - idr_remove(&drm_minors_idr, minor->index);
> > - spin_unlock_irqrestore(&drm_minor_lock, flags);
>
> The need to do the drmm_remove_action() dance, with the need for
> drmm_remove_action() in the first place, just to remove those three
> lines of manual cleanup really seems overkill to me. Automation is nice,
> but not everything is a nail even if all you have is a hammer.
Still the same thing, I wanted to have onion unwinding for everything.
If we keep some things outside of drmm_ then I have to carefully
interleave the drm_managed_release with the cleanup actions, and
review that across all drivers. Which I had to do anyway, but this way
it's at least somewhat of a split-up.
Essentially for a safe conversion you need to look at what's the last
thing the code right before drm_managed_release manually cleans up
(whether drm core or drivers doesn't matter, so lots of reviewing).
And then converting that over to drmm_. Repeat until everything is
handled.
If you decide to not handle something because it's not worth it the
review complexity across all our drivers goes through the roof (and
there's soooooooo many special cases). Plus the core cleanup sequence
gets real nasty where you have to interleave all kinds of things.
I guess you could say I could just register one action for the
drm_dev_fini stuff, but I kinda wanted to start out with an example of
what this could look like in driver cleanup code. Ofc since it's core
code there's not going to be amplified code savings, but as an example
it's still useful I think.
-Daniel
> > -err_free:
> > - kfree(minor);
> > - return r;
> > -}
> > -
> > -static void drm_minor_free(struct drm_device *dev, unsigned int type)
> > -{
> > - struct drm_minor **slot, *minor;
> > - unsigned long flags;
> > -
> > - slot = drm_minor_get_slot(dev, type);
> > - minor = *slot;
> > - if (!minor)
> > - return;
> > -
> > - put_device(minor->kdev);
> > -
> > - spin_lock_irqsave(&drm_minor_lock, flags);
> > - idr_remove(&drm_minors_idr, minor->index);
> > - spin_unlock_irqrestore(&drm_minor_lock, flags);
> > -
> > - kfree(minor);
> > - *slot = NULL;
> > }
> >
> > static int drm_minor_register(struct drm_device *dev, unsigned int type)
> > @@ -678,16 +665,16 @@ int drm_dev_init(struct drm_device *dev,
> > if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> > ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
> > if (ret)
> > - goto err_minors;
> > + goto err;
> > }
> >
> > ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
> > if (ret)
> > - goto err_minors;
> > + goto err;
> >
> > ret = drm_legacy_create_map_hash(dev);
> > if (ret)
> > - goto err_minors;
> > + goto err;
> >
> > drm_legacy_ctxbitmap_init(dev);
> >
> > @@ -695,7 +682,7 @@ int drm_dev_init(struct drm_device *dev,
> > ret = drm_gem_init(dev);
> > if (ret) {
> > DRM_ERROR("Cannot initialize graphics execution manager (GEM)\n");
> > - goto err_ctxbitmap;
> > + goto err;
> > }
> > }
> >
> > @@ -708,10 +695,6 @@ int drm_dev_init(struct drm_device *dev,
> > err_setunique:
> > if (drm_core_check_feature(dev, DRIVER_GEM))
> > drm_gem_destroy(dev);
> > -err_ctxbitmap:
> > -err_minors:
> > - drm_minor_free(dev, DRM_MINOR_PRIMARY);
> > - drm_minor_free(dev, DRM_MINOR_RENDER);
> > err:
> > drm_managed_release(dev);
> >
> > @@ -776,9 +759,6 @@ void drm_dev_fini(struct drm_device *dev)
> >
> > if (drm_core_check_feature(dev, DRIVER_GEM))
> > drm_gem_destroy(dev);
> > -
> > - drm_minor_free(dev, DRM_MINOR_PRIMARY);
> > - drm_minor_free(dev, DRM_MINOR_RENDER);
> > }
> > EXPORT_SYMBOL(drm_dev_fini);
> >
> > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
> > index d8a484e19830..fb44fe65c2cd 100644
> > --- a/drivers/gpu/drm/drm_managed.c
> > +++ b/drivers/gpu/drm/drm_managed.c
> > @@ -132,6 +132,34 @@ int __drmm_add_action(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(__drmm_add_action);
> >
> > +void drmm_remove_action(struct drm_device *dev,
> > + drmres_release_t action,
> > + void *data)
> > +{
> > + struct drmres *dr = NULL, *tmp;
> > + unsigned long flags;
> > +
> > + if (!data)
> > + return;
> > +
> > + spin_lock_irqsave(&dev->managed.lock, flags);
> > + list_for_each_entry(tmp, &dev->managed.resources, node.entry) {
> > + if (tmp->node.release == action &&
> > + * (void **) tmp->data == data) {
>
> As before, &tmp->data, and let's rename tmp.
>
> > + dr = tmp;
> > + del_dr(dev, dr);
> > + break;
> > + }
> > + }
> > + spin_unlock_irqrestore(&dev->managed.lock, flags);
> > +
> > + if (WARN_ON(!dr))
> > + return;
> > +
> > + kfree(dr);
> > +}
> > +EXPORT_SYMBOL(drmm_remove_action);
> > +
> > void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp)
> > {
> > struct drmres *dr;
> > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> > index 240edd395e88..df30f9355902 100644
> > --- a/include/drm/drm_managed.h
> > +++ b/include/drm/drm_managed.h
> > @@ -14,6 +14,10 @@ int __must_check __drmm_add_action(struct drm_device *dev,
> > drmres_release_t action,
> > void *data, const char *name);
> >
> > +void drmm_remove_action(struct drm_device *dev,
> > + drmres_release_t action,
> > + void *data);
> > +
> > void drmm_add_final_kfree(struct drm_device *dev, void *parent);
> >
> > void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp) __malloc;
>
> --
> Regards,
>
> Laurent Pinchart
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list