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

[Jie Zhan]

...

>>> +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;
>     }
> 
Yeah.

Could be cleaner like:
	if (attr == &dev_attr_polling_interval.attr &&
	    IS_SUPPORTED_ATTR(df->governor->attrs, POLLING_INTERVAL))
		return attr->mode;

	if (attr == &dev_attr_timer.attr &&
	    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|.
Exactly.
> 
> thanks !
>