Hi Daniel,
(On a side note, git-format-patch accepts a -v argument to specify the version, I didn't realize you were not aware of it :-))
On Mon, Mar 23, 2020 at 03:49:18PM +0100, Daniel Vetter wrote:
A few things:
- Update the example driver in the documentation.
- We can drop the old kfree in drm_dev_release.
- Add a WARN_ON check in drm_dev_register to make sure everyone calls drmm_add_final_kfree and there's no leaks.
v2: Restore the full cleanup, I accidentally left some moved code behind when fixing the bisectability of the series.
Acked-by: Sam Ravnborg sam@ravnborg.org Acked-by: Thomas Zimmermann tzimmermann@suse.de Cc: Sam Ravnborg sam@ravnborg.org Cc: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_drv.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 877ded348b6e..7f9d7ea543a0 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -297,8 +297,6 @@ void drm_minor_release(struct drm_minor *minor)
drm_mode_config_cleanup(drm);drm_dev_fini(drm);
kfree(priv->userspace_facing);
kfree(priv);- }
- static struct drm_driver driver_drm_driver = {
@@ -326,10 +324,11 @@ void drm_minor_release(struct drm_minor *minor)
kfree(drm);return ret;}
drmm_add_final_kfree(drm, priv);drm_mode_config_init(drm);
priv->userspace_facing = kzalloc(..., GFP_KERNEL);
priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL);if (!priv->userspace_facing)return -ENOMEM;@@ -837,10 +836,7 @@ static void drm_dev_release(struct kref *ref)
drm_managed_release(dev);
- if (!dev->driver->release && !dev->managed.final_kfree) {
WARN_ON(!list_empty(&dev->managed.resources));kfree(dev);- } else if (dev->managed.final_kfree)
- if (dev->managed.final_kfree) kfree(dev->managed.final_kfree);
}
@@ -961,6 +957,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (!driver->load) drm_mode_config_validate(dev);
- WARN_ON(!dev->managed.final_kfree);
That's too aggressive. Driver freeing their private object in .release() isn't wrong. I'd even go as far as saying that it should be the norm, until we manage to find a better way to handle that (which this series doesn't achieve). Many drivers need to allocate resources at probe time before they get a chance to init the drm device. Those resources must be released in the error handling paths of probe. By requiring drmm_add_final_kfree(), you're making that much more complex. I can't release those resources in the error path anymore after calling drmm_add_final_kfree(), or they will be released twice. And I can't rely on drmm_* to release them in all cases, as the error path may be hit before touching anything drm-related.
Until we figure out a good way forward and test it on a significant number of drivers, let's not add WARN_ON() that will be hit with the majority of drivers, forcing them to be converted to something that is clearly half-baked.
- if (drm_dev_needs_global_mutex(dev)) mutex_lock(&drm_global_mutex);