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

["zhangpengjie (A)" <zhangpengjie2@huawei.com>]

hello，Jie

On 12/4/2025 3:36 PM, Jie Zhan wrote:
> On 11/7/2025 11:17 AM, Pengjie Zhang 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 Pengjie,
>
> The motivation looks fine but the implementation of gov_group_visible() and
> gov_attr_visible() doesn't work right in my view.
>
> See comments below.
>
> Thanks!
> Jie
>>   /*
>>    * devfreq core provides delayed work based load monitoring helper
>> @@ -785,11 +786,6 @@ static void devfreq_dev_release(struct device *dev)
>>   	kfree(devfreq);
>>   }
>>   
>>   
>>   static ssize_t polling_interval_show(struct device *dev,
>>   				     struct device_attribute *attr, char *buf)
>>   {
>>   	struct devfreq *df = to_devfreq(dev);
>> +	int ret;
> No need of this if restore the last line.
>>   
>> -	if (!df->profile)
>> +	guard(mutex)(&devfreq_list_lock);
>> +
>> +	if (!df->profile || !df->governor ||
>> +	    !IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL))
>>   		return -EINVAL;
>>   
>> -	return sprintf(buf, "%d\n", df->profile->polling_ms);
> It's fine to keep this line.
>> +	ret = sprintf(buf, "%d\n", df->profile->polling_ms);
>> +
>> +	return ret;
>>   }
>>   
>>
>>   
>> -/* Create the specific sysfs files which depend on each governor. */
>> -static void create_sysfs_files(struct devfreq *devfreq,
>> -				const struct devfreq_governor *gov)
>> +static umode_t gov_attr_visible(struct kobject *kobj,
>> +				struct attribute *attr, int n)
>>   {
>> -	if (IS_SUPPORTED_ATTR(gov->attrs, POLLING_INTERVAL))
>> -		CREATE_SYSFS_FILE(devfreq, polling_interval);
>> -	if (IS_SUPPORTED_ATTR(gov->attrs, TIMER))
>> -		CREATE_SYSFS_FILE(devfreq, timer);
>> +	struct device *dev = kobj_to_dev(kobj);
>> +	struct devfreq *df = to_devfreq(dev);
>> +
>> +	if (!df->governor || !df->governor->attrs)
>> +		return 0;
>> +
>> +	if (IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL))
>> +		return attr->mode;
>> +	if (IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
>> +		return attr->mode;
> This would expose both 'timer' and 'polling_interval' if either of them is
> supported, which is wrong.

ok，i see，may be

     if (attr == &dev_attr_polling_interval.attr) {
         if (IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL))
             return attr->mode;
         return 0;
     }

     if (attr == &dev_attr_timer.attr) {
         if (IS_SUPPORTED_ATTR(df->governor->attrs, TIMER))
             return attr->mode;
         return 0;
     }

>> +
>> +	return 0;
>>   }
>>   
>> -/* 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);
>> +
>> +	if (!dev)
>> +		return false;
> kobj_to_dev(kobj) can't fail.  This check is unnecessary.
>> +
>> +	return true;
>>   }
>> +DEFINE_SYSFS_GROUP_VISIBLE(gov);
> The implementation of gov_group_visible() and gov_attr_visible() doesn't
> seem to comply with the design of DEFINE_SYSFS_GROUP_VISIBLE().
>
> DEFINE_SYSFS_GROUP_VISIBLE is defined as:
>
> #define DEFINE_SYSFS_GROUP_VISIBLE(name)                             \
>          static inline umode_t sysfs_group_visible_##name(            \
>                  struct kobject *kobj, struct attribute *attr, int n) \
>          {                                                            \
>                  if (n == 0 && !name##_group_visible(kobj))           \
>                          return SYSFS_GROUP_INVISIBLE;                \
>                  return name##_attr_visible(kobj, attr, n);           \
>          }
>
> That means:
> _group_visible controls whether to hide all the attrs in this group;
> _attr_visible further decides whether to show each attr.
>
> Hence,
> 1. gov_group_visible() should check if any attr in 'governor_attrs' should
> be present, i.e. checking df->governor->attrs.
> 2. gov_attr_visible identifies which attr it is and checks whether the
> governor supports it.
>
> Neither is done in the above code, so this should be updated.

we can move the check|if (!df->governor || !df->governor->attrs)|

from|gov_attr_visible|to|gov_group_visible|.

thanks !