Re: [PATCH v2] PM / devfreq: use _visible attribute to replace create/remove_sysfs_files()

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Tue, 28 Oct 2025 10:24:58 +0800
Pengjie Zhang <zhangpengjie2@huawei.com> wrote:

> Previously, non-generic attributes (polling_interval, timer) used separate
> create/delete logic, leading to race conditions during concurrent access in
> creation/deletion. Multi-threaded operations also caused inconsistencies
> between governor capabilities and attribute states.
> 
> 1.Use is_visible + sysfs_update_group() to unify management of these
> attributes, eliminating creation/deletion races.
> 2.Add locks and validation to these attributes, ensuring consistency
> between current governor capabilities and attribute operations in
> multi-threaded environments.
> 
> Signed-off-by: Pengjie Zhang <zhangpengjie2@huawei.com>
Hi

Various comments inline.

Jonathan

> @@ -1805,19 +1801,26 @@ static struct attribute *devfreq_attrs[] = {
>  	&dev_attr_min_freq.attr,
>  	&dev_attr_max_freq.attr,
>  	&dev_attr_trans_stat.attr,
> -	NULL,
> +	NULL
Whilst a good change, doesn't belong in a patch doing anything more
substantial.  So not in here.

>  };
> -ATTRIBUTE_GROUPS(devfreq);

>  static ssize_t polling_interval_store(struct device *dev,
> @@ -1828,15 +1831,22 @@ static ssize_t polling_interval_store(struct device *dev,
>  	unsigned int value;
>  	int ret;
>  
> -	if (!df->governor)
> +	mutex_lock(&devfreq_list_lock);

As below I'd use guard() to simplify this.  Applies in various other places in this
patch.


> +	if (!df->governor ||
> +	    !IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL)) {
> +		mutex_unlock(&devfreq_list_lock);
>  		return -EINVAL;
> +	}
>  
>  	ret = sscanf(buf, "%u", &value);
> -	if (ret != 1)
> +	if (ret != 1) {
> +		mutex_unlock(&devfreq_list_lock);
>  		return -EINVAL;
> +	}
>  
>  	df->governor->event_handler(df, DEVFREQ_GOV_UPDATE_INTERVAL, &value);
>  	ret = count;
> +	mutex_unlock(&devfreq_list_lock);
>  
>  	return ret;
>  }
> @@ -1846,11 +1856,19 @@ static ssize_t timer_show(struct device *dev,
>  			     struct device_attribute *attr, char *buf)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> +	int ret;
>  
> -	if (!df->profile)
> +	mutex_lock(&devfreq_list_lock);

guard() would be useful here.

> +	if (!df->profile || !df->governor ||
> +	    !IS_SUPPORTED_ATTR(df->governor->attrs, TIMER)) {
> +		mutex_unlock(&devfreq_list_lock);
>  		return -EINVAL;
> +	}
> +
> +	ret = sprintf(buf, "%s\n", timer_name[df->profile->timer]);
> +	mutex_unlock(&devfreq_list_lock);
>  
> -	return sprintf(buf, "%s\n", timer_name[df->profile->timer]);
> +	return ret;
>  }
>  
>  static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
> @@ -1861,12 +1879,18 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
>  	int timer = -1;
>  	int ret = 0, i;
>  
> -	if (!df->governor || !df->profile)
> -		return -EINVAL;
> +	mutex_lock(&devfreq_list_lock);

Perhaps a follow up but this code would be more readable with use of guard()
and returns wherever we are done with anything other than unlocking.


> +	if (!df->governor || !df->profile ||
> +	    !IS_SUPPORTED_ATTR(df->governor->attrs, TIMER)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	ret = sscanf(buf, "%16s", str_timer);
> -	if (ret != 1)
> -		return -EINVAL;
> +	if (ret != 1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	for (i = 0; i < DEVFREQ_TIMER_NUM; i++) {
>  		if (!strncmp(timer_name[i], str_timer, DEVFREQ_NAME_LEN)) {
> @@ -1901,40 +1925,64 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
>  		dev_warn(dev, "%s: Governor %s not started(%d)\n",
>  			 __func__, df->governor->name, ret);
>  out:
> +	mutex_unlock(&devfreq_list_lock);
>  	return ret ? ret : count;
>  }
>  static DEVICE_ATTR_RW(timer);


>  
> -/* Remove the specific sysfs files which depend on each governor. */
> -static void remove_sysfs_files(struct devfreq *devfreq,
> -				const struct devfreq_governor *gov)
> +static bool gov_group_visible(struct kobject *kobj)
>  {
> -	if (IS_SUPPORTED_ATTR(gov->attrs, POLLING_INTERVAL))
> -		sysfs_remove_file(&devfreq->dev.kobj,
> -				&dev_attr_polling_interval.attr);
> -	if (IS_SUPPORTED_ATTR(gov->attrs, TIMER))
> -		sysfs_remove_file(&devfreq->dev.kobj, &dev_attr_timer.attr);
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct devfreq *df;
> +
> +	if (!dev)
> +		return false;
> +
> +	df = to_devfreq(dev);
> +	if (!df)
> +		return false;

Isn't to_devfreq() just a container_of() wrapper?
If that's the case it will always be there if dev is not NULL.

As such I not seeing how this group is likely to ever be
invisible.


> +
> +	return true;
>  }
> +DEFINE_SYSFS_GROUP_VISIBLE(gov);