drivers/gpu/drm/drm_auth.c | 64 ++++++++++++++------------------------ 1 file changed, 23 insertions(+), 41 deletions(-)
Replace open-coded mutex handling with cleanup.h guard(mutex). This
simplifies the code and removes the "goto unlock" pattern.
Tested with igt tests core_auth and core_setmaster.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
For more information about guard(mutex):
https://www.kernel.org/doc/html/latest/core-api/cleanup.html
---
drivers/gpu/drm/drm_auth.c | 64 ++++++++++++++------------------------
1 file changed, 23 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 22aa015df387..d6bf605b4b90 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -95,7 +95,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
struct drm_auth *auth = data;
int ret = 0;
- mutex_lock(&dev->master_mutex);
+ guard(mutex)(&dev->master_mutex);
if (!file_priv->magic) {
ret = idr_alloc(&file_priv->master->magic_map, file_priv,
1, 0, GFP_KERNEL);
@@ -103,7 +103,6 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
file_priv->magic = ret;
}
auth->magic = file_priv->magic;
- mutex_unlock(&dev->master_mutex);
drm_dbg_core(dev, "%u\n", auth->magic);
@@ -118,13 +117,12 @@ int drm_authmagic(struct drm_device *dev, void *data,
drm_dbg_core(dev, "%u\n", auth->magic);
- mutex_lock(&dev->master_mutex);
+ guard(mutex)(&dev->master_mutex);
file = idr_find(&file_priv->master->magic_map, auth->magic);
if (file) {
file->authenticated = 1;
idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
}
- mutex_unlock(&dev->master_mutex);
return file ? 0 : -EINVAL;
}
@@ -248,41 +246,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
{
int ret;
- mutex_lock(&dev->master_mutex);
+ guard(mutex)(&dev->master_mutex);
ret = drm_master_check_perm(dev, file_priv);
if (ret)
- goto out_unlock;
+ return ret;
if (drm_is_current_master_locked(file_priv))
- goto out_unlock;
+ return ret;
- if (dev->master) {
- ret = -EBUSY;
- goto out_unlock;
- }
+ if (dev->master)
+ return -EBUSY;
- if (!file_priv->master) {
- ret = -EINVAL;
- goto out_unlock;
- }
+ if (!file_priv->master)
+ return -EINVAL;
- if (!file_priv->is_master) {
- ret = drm_new_set_master(dev, file_priv);
- goto out_unlock;
- }
+ if (!file_priv->is_master)
+ return drm_new_set_master(dev, file_priv);
if (file_priv->master->lessor != NULL) {
drm_dbg_lease(dev,
"Attempt to set lessee %d as master\n",
file_priv->master->lessee_id);
- ret = -EINVAL;
- goto out_unlock;
+ return -EINVAL;
}
drm_set_master(dev, file_priv, false);
-out_unlock:
- mutex_unlock(&dev->master_mutex);
+
return ret;
}
@@ -299,33 +289,27 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
{
int ret;
- mutex_lock(&dev->master_mutex);
+ guard(mutex)(&dev->master_mutex);
ret = drm_master_check_perm(dev, file_priv);
if (ret)
- goto out_unlock;
+ return ret;
- if (!drm_is_current_master_locked(file_priv)) {
- ret = -EINVAL;
- goto out_unlock;
- }
+ if (!drm_is_current_master_locked(file_priv))
+ return -EINVAL;
- if (!dev->master) {
- ret = -EINVAL;
- goto out_unlock;
- }
+ if (!dev->master)
+ return -EINVAL;
if (file_priv->master->lessor != NULL) {
drm_dbg_lease(dev,
"Attempt to drop lessee %d as master\n",
file_priv->master->lessee_id);
- ret = -EINVAL;
- goto out_unlock;
+ return -EINVAL;
}
drm_drop_master(dev, file_priv);
-out_unlock:
- mutex_unlock(&dev->master_mutex);
+
return ret;
}
@@ -337,7 +321,7 @@ int drm_master_open(struct drm_file *file_priv)
/* if there is no current master make this fd it, but do not create
* any master object for render clients
*/
- mutex_lock(&dev->master_mutex);
+ guard(mutex)(&dev->master_mutex);
if (!dev->master) {
ret = drm_new_set_master(dev, file_priv);
} else {
@@ -345,7 +329,6 @@ int drm_master_open(struct drm_file *file_priv)
file_priv->master = drm_master_get(dev->master);
spin_unlock(&file_priv->master_lookup_lock);
}
- mutex_unlock(&dev->master_mutex);
return ret;
}
@@ -355,7 +338,7 @@ void drm_master_release(struct drm_file *file_priv)
struct drm_device *dev = file_priv->minor->dev;
struct drm_master *master;
- mutex_lock(&dev->master_mutex);
+ guard(mutex)(&dev->master_mutex);
master = file_priv->master;
if (file_priv->magic)
idr_remove(&file_priv->master->magic_map, file_priv->magic);
@@ -376,7 +359,6 @@ void drm_master_release(struct drm_file *file_priv)
/* drop the master reference held by the file priv */
if (file_priv->master)
drm_master_put(&file_priv->master);
- mutex_unlock(&dev->master_mutex);
}
/**
--
2.49.0
Hi
Am 09.05.25 um 16:26 schrieb André Almeida:
> Replace open-coded mutex handling with cleanup.h guard(mutex). This
> simplifies the code and removes the "goto unlock" pattern.
>
> Tested with igt tests core_auth and core_setmaster.
>
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
but with questions below
> ---
>
> For more information about guard(mutex):
> https://www.kernel.org/doc/html/latest/core-api/cleanup.html
This page lists issues with guards, so conversion from manual locking
should be decided on a case-by-case base IMHO.
> ---
> drivers/gpu/drm/drm_auth.c | 64 ++++++++++++++------------------------
> 1 file changed, 23 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 22aa015df387..d6bf605b4b90 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -95,7 +95,7 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
> struct drm_auth *auth = data;
> int ret = 0;
>
> - mutex_lock(&dev->master_mutex);
> + guard(mutex)(&dev->master_mutex);
These guard statements are hidden variable declarations. Shouldn't they
rather go to the function top with the other declarations? This would
also help to prevent the problem listed in cleanup.html to some extend.
Best regards
Thomas
> if (!file_priv->magic) {
> ret = idr_alloc(&file_priv->master->magic_map, file_priv,
> 1, 0, GFP_KERNEL);
> @@ -103,7 +103,6 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv)
> file_priv->magic = ret;
> }
> auth->magic = file_priv->magic;
> - mutex_unlock(&dev->master_mutex);
>
> drm_dbg_core(dev, "%u\n", auth->magic);
>
> @@ -118,13 +117,12 @@ int drm_authmagic(struct drm_device *dev, void *data,
>
> drm_dbg_core(dev, "%u\n", auth->magic);
>
> - mutex_lock(&dev->master_mutex);
> + guard(mutex)(&dev->master_mutex);
> file = idr_find(&file_priv->master->magic_map, auth->magic);
> if (file) {
> file->authenticated = 1;
> idr_replace(&file_priv->master->magic_map, NULL, auth->magic);
> }
> - mutex_unlock(&dev->master_mutex);
>
> return file ? 0 : -EINVAL;
> }
> @@ -248,41 +246,33 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data,
> {
> int ret;
>
> - mutex_lock(&dev->master_mutex);
> + guard(mutex)(&dev->master_mutex);
>
> ret = drm_master_check_perm(dev, file_priv);
> if (ret)
> - goto out_unlock;
> + return ret;
>
> if (drm_is_current_master_locked(file_priv))
> - goto out_unlock;
> + return ret;
>
> - if (dev->master) {
> - ret = -EBUSY;
> - goto out_unlock;
> - }
> + if (dev->master)
> + return -EBUSY;
>
> - if (!file_priv->master) {
> - ret = -EINVAL;
> - goto out_unlock;
> - }
> + if (!file_priv->master)
> + return -EINVAL;
>
> - if (!file_priv->is_master) {
> - ret = drm_new_set_master(dev, file_priv);
> - goto out_unlock;
> - }
> + if (!file_priv->is_master)
> + return drm_new_set_master(dev, file_priv);
>
> if (file_priv->master->lessor != NULL) {
> drm_dbg_lease(dev,
> "Attempt to set lessee %d as master\n",
> file_priv->master->lessee_id);
> - ret = -EINVAL;
> - goto out_unlock;
> + return -EINVAL;
> }
>
> drm_set_master(dev, file_priv, false);
> -out_unlock:
> - mutex_unlock(&dev->master_mutex);
> +
> return ret;
> }
>
> @@ -299,33 +289,27 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
> {
> int ret;
>
> - mutex_lock(&dev->master_mutex);
> + guard(mutex)(&dev->master_mutex);
>
> ret = drm_master_check_perm(dev, file_priv);
> if (ret)
> - goto out_unlock;
> + return ret;
>
> - if (!drm_is_current_master_locked(file_priv)) {
> - ret = -EINVAL;
> - goto out_unlock;
> - }
> + if (!drm_is_current_master_locked(file_priv))
> + return -EINVAL;
>
> - if (!dev->master) {
> - ret = -EINVAL;
> - goto out_unlock;
> - }
> + if (!dev->master)
> + return -EINVAL;
>
> if (file_priv->master->lessor != NULL) {
> drm_dbg_lease(dev,
> "Attempt to drop lessee %d as master\n",
> file_priv->master->lessee_id);
> - ret = -EINVAL;
> - goto out_unlock;
> + return -EINVAL;
> }
>
> drm_drop_master(dev, file_priv);
> -out_unlock:
> - mutex_unlock(&dev->master_mutex);
> +
> return ret;
> }
>
> @@ -337,7 +321,7 @@ int drm_master_open(struct drm_file *file_priv)
> /* if there is no current master make this fd it, but do not create
> * any master object for render clients
> */
> - mutex_lock(&dev->master_mutex);
> + guard(mutex)(&dev->master_mutex);
> if (!dev->master) {
> ret = drm_new_set_master(dev, file_priv);
> } else {
> @@ -345,7 +329,6 @@ int drm_master_open(struct drm_file *file_priv)
> file_priv->master = drm_master_get(dev->master);
> spin_unlock(&file_priv->master_lookup_lock);
> }
> - mutex_unlock(&dev->master_mutex);
>
> return ret;
> }
> @@ -355,7 +338,7 @@ void drm_master_release(struct drm_file *file_priv)
> struct drm_device *dev = file_priv->minor->dev;
> struct drm_master *master;
>
> - mutex_lock(&dev->master_mutex);
> + guard(mutex)(&dev->master_mutex);
> master = file_priv->master;
> if (file_priv->magic)
> idr_remove(&file_priv->master->magic_map, file_priv->magic);
> @@ -376,7 +359,6 @@ void drm_master_release(struct drm_file *file_priv)
> /* drop the master reference held by the file priv */
> if (file_priv->master)
> drm_master_put(&file_priv->master);
> - mutex_unlock(&dev->master_mutex);
> }
>
> /**
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Hi Thomas, Em 12/05/2025 03:52, Thomas Zimmermann escreveu: > Hi > > Am 09.05.25 um 16:26 schrieb André Almeida: >> Replace open-coded mutex handling with cleanup.h guard(mutex). This >> simplifies the code and removes the "goto unlock" pattern. >> >> Tested with igt tests core_auth and core_setmaster. >> >> Signed-off-by: André Almeida <andrealmeid@igalia.com> > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Do you mind applying this patch at drm-misc? Thanks! André
Hi Am 22.05.25 um 20:28 schrieb André Almeida: > Hi Thomas, > > Em 12/05/2025 03:52, Thomas Zimmermann escreveu: >> Hi >> >> Am 09.05.25 um 16:26 schrieb André Almeida: >>> Replace open-coded mutex handling with cleanup.h guard(mutex). This >>> simplifies the code and removes the "goto unlock" pattern. >>> >>> Tested with igt tests core_auth and core_setmaster. >>> >>> Signed-off-by: André Almeida <andrealmeid@igalia.com> >> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> >> > Do you mind applying this patch at drm-misc? Merged into drm-misc-next. Thanks for the patch. Best regards Thomas > > Thanks! > André -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Hi Thomas, Thanks for the feedback. Em 12/05/2025 03:52, Thomas Zimmermann escreveu: > Hi > > Am 09.05.25 um 16:26 schrieb André Almeida: >> Replace open-coded mutex handling with cleanup.h guard(mutex). This >> simplifies the code and removes the "goto unlock" pattern. >> >> Tested with igt tests core_auth and core_setmaster. >> >> Signed-off-by: André Almeida <andrealmeid@igalia.com> > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > > but with questions below > >> --- >> >> For more information about guard(mutex): >> https://www.kernel.org/doc/html/latest/core-api/cleanup.html > > This page lists issues with guards, so conversion from manual locking > should be decided on a case-by-case base IMHO. > Sure, agreed. The places that I have converted to guard(mutex) here looks like a good fit for this conversion, where the scope of the mutex is well defined inside a function without conditional locking. >> --- >> drivers/gpu/drm/drm_auth.c | 64 ++++++++++++++------------------------ >> 1 file changed, 23 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c >> index 22aa015df387..d6bf605b4b90 100644 >> --- a/drivers/gpu/drm/drm_auth.c >> +++ b/drivers/gpu/drm/drm_auth.c >> @@ -95,7 +95,7 @@ int drm_getmagic(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> struct drm_auth *auth = data; >> int ret = 0; >> - mutex_lock(&dev->master_mutex); >> + guard(mutex)(&dev->master_mutex); > > These guard statements are hidden variable declarations. Shouldn't they > rather go to the function top with the other declarations? This would > also help to prevent the problem listed in cleanup.html to some extend. > The guard statements should go exactly where the lock should be taken, as it not only declares anonymous variables but also really takes the lock. The lock is then release when the mutex goes out of scope. File drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c has some usage of guard(mutex) as well, where Mario did a similar cleanup: f123fda19752 drm/amd/display: Use scoped guards for handle_hpd_irq_helper() aca9ec9b050c drm/amd/display: Use scoped guard for amdgpu_dm_update_connector_after_detect() f24a74d59e14 drm/amd/display: Use scoped guard for dm_resume() > Best regards > Thomas >
© 2016 - 2025 Red Hat, Inc.