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

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

On Fri, 5 Dec 2025 16:37:24 +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,

Been a while since I looked at this series (sorry about that!)

A few trivial things inline and one suggestion for a possible follow up
patch.
None are significant enough that I'll look again at this so:

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
> changes in v4:
> -Remove the DEFINE_SYSFS_GROUP_VISIBLE macro and directly set the is_visible function.
> -Remove unnecessary ret variables
> Link to v3:https://lore.kernel.org/lkml/20251107031706.1698396-1-zhangpengjie2@huawei.com/
> 
> changes in v3:
> - Use guard() to simplify lock acquisition and destruction.
> - Eliminate redundant checks for df.
> Link to v2:https://lore.kernel.org/lkml/20251028022458.2824872-1-zhangpengjie2@huawei.com/
> 
> Changes in v2: 
> - Fix one problem reported by the kernel test robot.
> - Redirect all error paths in timer_store() to out to ensure locks are not 
>  left unReleased.
> Link to v1:https://lore.kernel.org/lkml/20251025135238.3576861-1-zhangpengjie2@huawei.com/
> 
>  drivers/devfreq/devfreq.c | 99 +++++++++++++++++++++++----------------
>  1 file changed, 58 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2e8d01d47f69..afcfc1be2e64 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -38,6 +38,7 @@
>  
>  static struct class *devfreq_class;
>  static struct dentry *devfreq_debugfs;
> +static const struct attribute_group gov_attr_group;
>  
>  /*
>   * devfreq core provides delayed work based load monitoring helper
> @@ -785,11 +786,6 @@ static void devfreq_dev_release(struct device *dev)
>  	kfree(devfreq);
>  }
>  
> -static void create_sysfs_files(struct devfreq *devfreq,
> -				const struct devfreq_governor *gov);
> -static void remove_sysfs_files(struct devfreq *devfreq,
> -				const struct devfreq_governor *gov);
> -
>  /**
>   * devfreq_add_device() - Add devfreq feature to the device
>   * @dev:	the device to add devfreq feature.
> @@ -956,7 +952,10 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  			 __func__);
>  		goto err_init;
>  	}
> -	create_sysfs_files(devfreq, devfreq->governor);
> +
> +	err = sysfs_update_group(&devfreq->dev.kobj, &gov_attr_group);
> +	if (err)
> +		goto err_init;
>  
>  	list_add(&devfreq->node, &devfreq_list);
>  
> @@ -998,9 +997,7 @@ int devfreq_remove_device(struct devfreq *devfreq)
>  	if (devfreq->governor) {

This file adopts (at least in first case I looked at) convention of no {}
when a single call is made in if blocks (even when it is multiline).
Coding standard has never been particularly clear on this corner case
so normally I'd advise following local style.

>  		devfreq->governor->event_handler(devfreq,
>  						 DEVFREQ_GOV_STOP, NULL);
> -		remove_sysfs_files(devfreq, devfreq->governor);
>  	}
> -

Technically an unrelated change that I'd be temped to undo just to slightly
reduce patch size.

>  	device_unregister(&devfreq->dev);
>  
>  	return 0;
> @@ -1460,7 +1457,6 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  			 __func__, df->governor->name, ret);
>  		goto out;
>  	}
> -	remove_sysfs_files(df, df->governor);
>  
>  	/*
>  	 * Start the new governor and create the specific sysfs files
> @@ -1489,7 +1485,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	 * Create the sysfs files for the new governor. But if failed to start
>  	 * the new governor, restore the sysfs files of previous governor.
>  	 */
> -	create_sysfs_files(df, df->governor);
> +	ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);

Not directly relevant to this patch, but this function smells like one
that would significantly benefit from guard() and early returns in error paths.

>  
>  out:
>  	mutex_unlock(&devfreq_list_lock);
> @@ -1807,14 +1803,16 @@ static struct attribute *devfreq_attrs[] = {
>  	&dev_attr_trans_stat.attr,
>  	NULL,
>  };

> @@ -1847,7 +1848,10 @@ static ssize_t timer_show(struct device *dev,
>  {
>  	struct devfreq *df = to_devfreq(dev);
>  
> -	if (!df->profile)
> +	guard(mutex)(&devfreq_list_lock);

For these I'd be tempted to add a trivial comment about the race they
are protecting against.  I think it is the window between setting
the IS_SUPPORTED_ATTR() and isvisible being called.

> +
> +	if (!df->profile || !df->governor ||
> +	    !IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
>  		return -EINVAL;
>  
>  	return sprintf(buf, "%s\n", timer_name[df->profile->timer]);
> @@ -1861,7 +1865,10 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
>  	int timer = -1;
>  	int ret = 0, i;
>  
> -	if (!df->governor || !df->profile)
> +	guard(mutex)(&devfreq_list_lock);
> +
> +	if (!df->governor || !df->profile ||
> +	    !IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
>  		return -EINVAL;
>  
>  	ret = sscanf(buf, "%16s", str_timer);