On Wed, Mar 11, 2020 at 10:59:10AM +0100, Thomas Zimmermann wrote: > Hi > > Am 02.03.20 um 23:26 schrieb Daniel Vetter: > > 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. > > > > v2: Make pointer math around void ** consistent with what Laurent > > suggested. > > > > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > --- > > 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 23e5b0e7e041..29d106195ab3 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); > > -err_free: > > - kfree(minor); > > - return r; > > -} > > TBH, I think you're reducing code quality by removing the rollback code > from init functions, just for the sake of it. > > Specifically in this case here, you saved a few lines of code, but the > overall flow is way more complicated to follow. That's typically a > reliably source of bugs. This call to drmm_remove_action() just makes it > worse. > > Rather, see my remark on OOP destruction in patch 21. For now, I'd focus > on the device cleanup and leave init functions as they are. Ah, I can simplify this with add_action_or_reset. This removes the only user of drmm_remove_action, which I think is actually a good thing :-) -Daniel > > Best regards > Thomas > > > - > > -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 514d5bd42446..0883615c2088 100644 > > --- a/drivers/gpu/drm/drm_managed.c > > +++ b/drivers/gpu/drm/drm_managed.c > > @@ -142,6 +142,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) { > > + 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 89e6fce9f689..5280209dff92 100644 > > --- a/include/drm/drm_managed.h > > +++ b/include/drm/drm_managed.h > > @@ -17,6 +17,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; > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel