Re: [PATCH v4 1/4] devfreq: Use mutex guard in governor_store()

[Chanwoo Choi <cwchoi00@gmail.com>]

On 7/29/2026 7:23 PM, Jie Zhan wrote:
> Use mutex guard in governor_store() so as to simplify the locking logic.
> 
> No functional impact intended.
> 
> Reviewed-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>  drivers/devfreq/devfreq.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index f08fc6966eae..7a70dd051644 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1394,23 +1394,20 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&devfreq_list_lock);
> +	guard(mutex)(&devfreq_list_lock);
>  	governor = try_then_request_governor(str_governor);
> -	if (IS_ERR(governor)) {
> -		ret = PTR_ERR(governor);
> -		goto out;
> -	}
> +	if (IS_ERR(governor))
> +		return PTR_ERR(governor);
> +
>  	if (!df->governor)
>  		goto start_new_governor;
>  
> -	if (df->governor == governor) {
> -		ret = 0;
> -		goto out;
> -	} else if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)
> -		|| IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (df->governor == governor)
> +		return count;
> +
> +	if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE) ||
> +	    IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE))
> +		return -EINVAL;
>  
>  	/*
>  	 * Stop the current governor and remove the specific sysfs files
> @@ -1420,7 +1417,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	if (ret) {
>  		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
>  			 __func__, df->governor->name, ret);
> -		goto out;
> +		return ret;
>  	}
>  
>  start_new_governor:
> @@ -1438,7 +1435,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  		/* Restore previous governor */
>  		df->governor = prev_governor;
>  		if (!df->governor)
> -			goto out;
> +			return ret;
>  
>  		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>  		if (ret) {
> @@ -1446,7 +1443,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  				"%s: reverting to Governor %s failed (%d)\n",
>  				__func__, prev_governor->name, ret);
>  			df->governor = NULL;
> -			goto out;
> +			return ret;
>  		}
>  	}
>  
> @@ -1455,13 +1452,10 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	 * the new governor, restore the sysfs files of previous governor.
>  	 */
>  	ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
> +	if (ret)
> +		return ret;
>  
> -out:
> -	mutex_unlock(&devfreq_list_lock);
> -
> -	if (!ret)
> -		ret = count;
> -	return ret;
> +	return count;
>  }
>  static DEVICE_ATTR_RW(governor);
>  

Hi,

include/linux/cleanup.h includes the following comment
"148  * Lastly, given that the benefit of cleanup helpers is removal of
149  * "goto", and that the "goto" statement can jump between scopes, the
150  * expectation is that usage of "goto" and cleanup helpers is never
151  * mixed in the same function. I.e. for a given routine, convert all
152  * resources that need a "goto" cleanup to scope-based cleanup, or
153  * convert none of them.
"

A Patch should be atomic. This patch still uses both guard and goto.
even if you removed goto on patch3.

-- 
Best Regards,
Chanwoo Choi
Principal Engineer, Samsung Electronics