Re: [PATCH v4 11/19] drm/msm/debugfs: Use cleanup.h for gpu state dump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/msm/debugfs: internal cleanup.h scope.lock access in msm_gpu_open()
--

commit dbb53fa1a9b9e541e82f4c7d41aa75f373848b93
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/debugfs: Use cleanup.h for gpu state dump

This commit converts msm_gpu_open() in drivers/gpu/drm/msm/msm_debugfs.c to
use cleanup.h helpers (__free, scoped_guard, and scoped_cond_guard) to
simplify error handling when capturing the GPU state.

> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
> index d05307bfa1235..e1b872576c7b1 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -70,43 +70,36 @@ static int msm_gpu_open(struct inode *inode, struct file *file)
>  	struct drm_device *dev = inode->i_private;
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_gpu *gpu = priv->gpu;
> -	struct msm_gpu_show_priv *show_priv;
>  	int ret;
>  
>  	if (!gpu || !gpu->funcs->gpu_state_get)
>  		return -ENODEV;
>  
> -	show_priv = kmalloc_obj(*show_priv);
> +	struct msm_gpu_show_priv *show_priv __free(kfree)
> +		= kmalloc_obj(*show_priv);
>  	if (!show_priv)
>  		return -ENOMEM;
>  
> -	ret = mutex_lock_interruptible(&gpu->lock);
> -	if (ret)
> -		goto free_priv;
> -
> -	pm_runtime_get_sync(&gpu->pdev->dev);
> -	msm_gpu_hw_init(gpu);
> -	show_priv->state = gpu->funcs->gpu_state_get(gpu);
> -	pm_runtime_put_sync(&gpu->pdev->dev);
> -
> -	mutex_unlock(&gpu->lock);
> +	scoped_guard (pm_runtime_active, &gpu->pdev->dev) {
> +		scoped_cond_guard (mutex_intr, return PTR_ERR(scope.lock), &gpu->lock) {

[Severity: Low]
This isn't a runtime bug, but in msm_gpu_open() in
drivers/gpu/drm/msm/msm_debugfs.c, does referencing scope.lock inside
scoped_cond_guard() reach into internal implementation details of
include/linux/cleanup.h while also shadowing the outer scope variable?

Both __scoped_guard() and __scoped_cond_guard() in include/linux/cleanup.h
synthesize a for-loop variable named scope:

#define __scoped_cond_guard(_name, _fail, _label, args...)		\
	for (CLASS(_name, scope)(args); true; ({ goto _label; }))	\
		if (!__guard_ptr(_name)(&scope)) {			\
			BUILD_BUG_ON(!__is_cond_ptr(_name));		\
			_fail;						\

Because scoped_cond_guard() is nested inside scoped_guard(), the inner
scope variable shadows the outer one, and .lock is an internal struct
member where DEFINE_LOCK_GUARD_1_COND_4() stores ERR_PTR(_RET) for
consumption via ACQUIRE() and ACQUIRE_ERR().

Would it be cleaner to pass return -EINTR as the failure statement to
scoped_cond_guard(mutex_intr, ...), or use ACQUIRE(mutex_intr, ...) with
ACQUIRE_ERR() if the exact return value is needed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030211928.45761-1-robin.clark@oss.qualcomm.com?part=11