Re: [PATCH 3/4] 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: cleanup.h scope.lock access and shadowing in msm_gpu_open()
--

commit 320eeedaaf2618e89f113d51dc4647a97f5a4d17
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)
instead of goto-based error handling.

> 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) {
> +			msm_gpu_hw_init(gpu);
> +			show_priv->state = gpu->funcs->gpu_state_get(gpu);
> +		}
> +	}

[Severity: Low]
In msm_gpu_open(), both __scoped_guard() and __scoped_cond_guard() in
include/linux/cleanup.h expand to for-loops that declare a local variable
named scope:

    #define __scoped_guard(_name, _label, args...) \
        for (CLASS(_name, scope)(args); ...

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

Nesting scoped_cond_guard() inside scoped_guard() causes the inner scope
declaration to shadow the outer one, which triggers a -Wshadow warning when
building with W=2.

In addition, passing return PTR_ERR(scope.lock) as the failure statement
reaches into the internal macro variable scope and the internal .lock field
where DEFINE_LOCK_GUARD_1_COND_4() stores ERR_PTR(_RET) on lock acquisition
failure.

Could this return -EINTR directly (or use ACQUIRE()/ACQUIRE_ERR()) instead
of referencing scope.lock, and avoid nesting the two scoped guard macros?

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