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

[zhangpengjie (A)]

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@xxxxxxxxxx>
> 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 !