Several GEM core functions manually managed mutex_lock() and mutex_unlock() over single scopes or error paths. This adds boilerplate and carries the risk of lock leaks if error paths are refactored.
Modernize these locks by deploying the <linux/cleanup.h> scoped_guard() macro. This ensures that the locks are reliably dropped when the block exits, cleanly removing goto out_unlock paths and tightening the lifecycle.
Signed-off-by: Biren Pandya birenpandya@gmail.com
Compiled locally, but requires IGT validation by the DRM CI. --- drivers/gpu/drm/drm_gem.c | 66 ++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 891c3bff5ae0..d3a061d42ba7 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -346,13 +346,13 @@ void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) * we checked for a name. */
- mutex_lock(&dev->object_name_lock); - if (--obj->handle_count == 0) { - drm_gem_object_handle_free(obj); - drm_gem_object_exported_dma_buf_free(obj); - final = true; + scoped_guard(mutex, &dev->object_name_lock) { + if (--obj->handle_count == 0) { + drm_gem_object_handle_free(obj); + drm_gem_object_exported_dma_buf_free(obj); + final = true; + } } - mutex_unlock(&dev->object_name_lock);
if (final) drm_gem_object_put(obj); @@ -374,11 +374,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data) if (obj->funcs->close) obj->funcs->close(obj, file_priv);
- mutex_lock(&file_priv->prime.lock); - - drm_prime_remove_buf_handle(&file_priv->prime, id); - - mutex_unlock(&file_priv->prime.lock); + scoped_guard(mutex, &file_priv->prime.lock) + drm_prime_remove_buf_handle(&file_priv->prime, id);
drm_vma_node_revoke(&obj->vma_node, file_priv);
@@ -1021,37 +1018,34 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, goto out; }
- mutex_lock(&file_priv->prime.lock); + scoped_guard(mutex, &file_priv->prime.lock) { + spin_lock(&file_priv->table_lock); + ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1, + GFP_NOWAIT); + spin_unlock(&file_priv->table_lock);
- spin_lock(&file_priv->table_lock); - ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1, - GFP_NOWAIT); - spin_unlock(&file_priv->table_lock); + if (ret < 0) + break;
- if (ret < 0) - goto out_unlock; + if (obj->dma_buf) { + ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf, + handle); + if (ret < 0) { + spin_lock(&file_priv->table_lock); + idr_remove(&file_priv->object_idr, handle); + spin_unlock(&file_priv->table_lock); + break; + }
- if (obj->dma_buf) { - ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf, - handle); - if (ret < 0) { - spin_lock(&file_priv->table_lock); - idr_remove(&file_priv->object_idr, handle); - spin_unlock(&file_priv->table_lock); - goto out_unlock; + drm_prime_remove_buf_handle(&file_priv->prime, args->handle); }
- drm_prime_remove_buf_handle(&file_priv->prime, args->handle); - } - - ret = 0; - - spin_lock(&file_priv->table_lock); - idr_remove(&file_priv->object_idr, args->handle); - spin_unlock(&file_priv->table_lock); + ret = 0;
-out_unlock: - mutex_unlock(&file_priv->prime.lock); + spin_lock(&file_priv->table_lock); + idr_remove(&file_priv->object_idr, args->handle); + spin_unlock(&file_priv->table_lock); + } out: drm_gem_object_put(obj);
base-commit: 72c25183cac9bc584c9de21797a5883af44bcc7a
On Tue, Jun 16, 2026 at 11:49:57PM +0530, Biren Pandya wrote:
Several GEM core functions manually managed mutex_lock() and mutex_unlock() over single scopes or error paths. This adds boilerplate and carries the risk of lock leaks if error paths are refactored.
Modernize these locks by deploying the <linux/cleanup.h> scoped_guard() macro. This ensures that the locks are reliably dropped when the block exits, cleanly removing goto out_unlock paths and tightening the lifecycle.
What's the reason for doing so in in drm_gem and not other areas in DRM ?
Signed-off-by: Biren Pandya birenpandya@gmail.com
Compiled locally, but requires IGT validation by the DRM CI.
drivers/gpu/drm/drm_gem.c | 66 ++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 891c3bff5ae0..d3a061d42ba7 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -346,13 +346,13 @@ void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) * we checked for a name. */
- mutex_lock(&dev->object_name_lock);
- if (--obj->handle_count == 0) {
drm_gem_object_handle_free(obj);drm_gem_object_exported_dma_buf_free(obj);final = true;
- scoped_guard(mutex, &dev->object_name_lock) {
if (--obj->handle_count == 0) {drm_gem_object_handle_free(obj);drm_gem_object_exported_dma_buf_free(obj);final = true;}
- mutex_unlock(&dev->object_name_lock);
if (final) drm_gem_object_put(obj); @@ -374,11 +374,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data) if (obj->funcs->close) obj->funcs->close(obj, file_priv);
- mutex_lock(&file_priv->prime.lock);
- drm_prime_remove_buf_handle(&file_priv->prime, id);
- mutex_unlock(&file_priv->prime.lock);
- scoped_guard(mutex, &file_priv->prime.lock)
drm_prime_remove_buf_handle(&file_priv->prime, id);drm_vma_node_revoke(&obj->vma_node, file_priv); @@ -1021,37 +1018,34 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, goto out; }
- mutex_lock(&file_priv->prime.lock);
- scoped_guard(mutex, &file_priv->prime.lock) {
spin_lock(&file_priv->table_lock);ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,GFP_NOWAIT);spin_unlock(&file_priv->table_lock);
And why don't you use guards for the spinlock as well ?
- spin_lock(&file_priv->table_lock);
- ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,
GFP_NOWAIT);- spin_unlock(&file_priv->table_lock);
if (ret < 0)break;
- if (ret < 0)
goto out_unlock;
if (obj->dma_buf) {ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,handle);if (ret < 0) {spin_lock(&file_priv->table_lock);idr_remove(&file_priv->object_idr, handle);spin_unlock(&file_priv->table_lock);break;}
- if (obj->dma_buf) {
ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,handle);if (ret < 0) {spin_lock(&file_priv->table_lock);idr_remove(&file_priv->object_idr, handle);spin_unlock(&file_priv->table_lock);goto out_unlock;
drm_prime_remove_buf_handle(&file_priv->prime, args->handle);
drm_prime_remove_buf_handle(&file_priv->prime, args->handle);- }
- ret = 0;
- spin_lock(&file_priv->table_lock);
- idr_remove(&file_priv->object_idr, args->handle);
- spin_unlock(&file_priv->table_lock);
ret = 0;-out_unlock:
- mutex_unlock(&file_priv->prime.lock);
spin_lock(&file_priv->table_lock);idr_remove(&file_priv->object_idr, args->handle);spin_unlock(&file_priv->table_lock);- }
out: drm_gem_object_put(obj);
base-commit: 72c25183cac9bc584c9de21797a5883af44bcc7a
On Mon, Jun 22, 2026 at 5:25 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Tue, Jun 16, 2026 at 11:49:57PM +0530, Biren Pandya wrote:
Several GEM core functions manually managed mutex_lock() and mutex_unlock() over single scopes or error paths. This adds boilerplate and carries the risk of lock leaks if error paths are refactored.
Modernize these locks by deploying the <linux/cleanup.h> scoped_guard() macro. This ensures that the locks are reliably dropped when the block exits, cleanly removing goto out_unlock paths and tightening the lifecycle.
What's the reason for doing so in in drm_gem and not other areas in DRM ?
Hi Laurent,
Thanks for taking a look. No deeper reason than it being where I happened to start — I didn't mean to single it out, and I'd rather the treatment be consistent than piecemeal.
@@ -1021,37 +1018,34 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, goto out; }
mutex_lock(&file_priv->prime.lock);
scoped_guard(mutex, &file_priv->prime.lock) {spin_lock(&file_priv->table_lock);ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,GFP_NOWAIT);spin_unlock(&file_priv->table_lock);And why don't you use guards for the spinlock as well ?
Fair point — the spinlocks here are equally good candidates; I only kept v1 to mutexes to keep it small.
That said, this is a pure cleanup with no functional change, so it's entirely your call whether it's worth carrying. If you'd like it, I'll send a v2 that converts both the mutexes and the spinlocks in drm_gem.c consistently. If you'd prefer not to take cleanup-only churn, I'm happy to drop it — no problem either way.
Thanks, Biren
On Mon, Jun 22, 2026 at 06:22:27PM +0530, biren pandya wrote:
On Mon, Jun 22, 2026 at 5:25 PM Laurent Pinchart wrote:
On Tue, Jun 16, 2026 at 11:49:57PM +0530, Biren Pandya wrote:
Several GEM core functions manually managed mutex_lock() and mutex_unlock() over single scopes or error paths. This adds boilerplate and carries the risk of lock leaks if error paths are refactored.
Modernize these locks by deploying the <linux/cleanup.h> scoped_guard() macro. This ensures that the locks are reliably dropped when the block exits, cleanly removing goto out_unlock paths and tightening the lifecycle.
What's the reason for doing so in in drm_gem and not other areas in DRM ?
Hi Laurent,
Thanks for taking a look. No deeper reason than it being where I happened to start — I didn't mean to single it out, and I'd rather the treatment be consistent than piecemeal.
@@ -1021,37 +1018,34 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, goto out; }
mutex_lock(&file_priv->prime.lock);
scoped_guard(mutex, &file_priv->prime.lock) {spin_lock(&file_priv->table_lock);ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,GFP_NOWAIT);spin_unlock(&file_priv->table_lock);And why don't you use guards for the spinlock as well ?
Fair point — the spinlocks here are equally good candidates; I only kept v1 to mutexes to keep it small.
That said, this is a pure cleanup with no functional change, so it's entirely your call whether it's worth carrying. If you'd like it, I'll send a v2 that converts both the mutexes and the spinlocks in drm_gem.c consistently. If you'd prefer not to take cleanup-only churn, I'm happy to drop it — no problem either way.
I'll let the maintainers of that code decide (I'm not one of them).
Hi
Am 16.06.26 um 20:19 schrieb Biren Pandya:
Several GEM core functions manually managed mutex_lock() and mutex_unlock() over single scopes or error paths. This adds boilerplate and carries the risk of lock leaks if error paths are refactored.
Modernize these locks by deploying the <linux/cleanup.h> scoped_guard() macro. This ensures that the locks are reliably dropped when the block exits, cleanly removing goto out_unlock paths and tightening the lifecycle.
Signed-off-by: Biren Pandya birenpandya@gmail.com
Compiled locally, but requires IGT validation by the DRM CI.
drivers/gpu/drm/drm_gem.c | 66 ++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 891c3bff5ae0..d3a061d42ba7 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -346,13 +346,13 @@ void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) * we checked for a name. */
- mutex_lock(&dev->object_name_lock);
- if (--obj->handle_count == 0) {
drm_gem_object_handle_free(obj);drm_gem_object_exported_dma_buf_free(obj);final = true;
- scoped_guard(mutex, &dev->object_name_lock) {
if (--obj->handle_count == 0) {drm_gem_object_handle_free(obj);drm_gem_object_exported_dma_buf_free(obj);final = true;}
- mutex_unlock(&dev->object_name_lock);
if (final) drm_gem_object_put(obj); @@ -374,11 +374,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data) if (obj->funcs->close) obj->funcs->close(obj, file_priv);
- mutex_lock(&file_priv->prime.lock);
- drm_prime_remove_buf_handle(&file_priv->prime, id);
- mutex_unlock(&file_priv->prime.lock);
- scoped_guard(mutex, &file_priv->prime.lock)
drm_prime_remove_buf_handle(&file_priv->prime, id);drm_vma_node_revoke(&obj->vma_node, file_priv); @@ -1021,37 +1018,34 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
There have been a number of revisions to this function recently and you want to make sure that you have the latest. At least the code in v7.1 looks different from yours.
goto out;}
- mutex_lock(&file_priv->prime.lock);
- scoped_guard(mutex, &file_priv->prime.lock) {
spin_lock(&file_priv->table_lock);
Could this spin lock also use a guard?
Best regards Thomas
ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,GFP_NOWAIT);spin_unlock(&file_priv->table_lock);
- spin_lock(&file_priv->table_lock);
- ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,
GFP_NOWAIT);- spin_unlock(&file_priv->table_lock);
if (ret < 0)break;
- if (ret < 0)
goto out_unlock;
if (obj->dma_buf) {ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,handle);if (ret < 0) {spin_lock(&file_priv->table_lock);idr_remove(&file_priv->object_idr, handle);spin_unlock(&file_priv->table_lock);break;}
- if (obj->dma_buf) {
ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf,handle);if (ret < 0) {spin_lock(&file_priv->table_lock);idr_remove(&file_priv->object_idr, handle);spin_unlock(&file_priv->table_lock);goto out_unlock;
drm_prime_remove_buf_handle(&file_priv->prime, args->handle);
drm_prime_remove_buf_handle(&file_priv->prime, args->handle);- }
- ret = 0;
- spin_lock(&file_priv->table_lock);
- idr_remove(&file_priv->object_idr, args->handle);
- spin_unlock(&file_priv->table_lock);
ret = 0;-out_unlock:
- mutex_unlock(&file_priv->prime.lock);
spin_lock(&file_priv->table_lock);idr_remove(&file_priv->object_idr, args->handle);spin_unlock(&file_priv->table_lock);- } out: drm_gem_object_put(obj);
base-commit: 72c25183cac9bc584c9de21797a5883af44bcc7a
linaro-mm-sig@lists.linaro.org
-
Biren Pandya -
biren pandya
-
Laurent Pinchart -
Thomas Zimmermann