linux-kernel - Re: [PATCH v3] PM / devfreq: use _visible attribute to replace create/remove_sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <07c098d7-979b-44b5-aae0-7999581d8d90@hisilicon.com>
Date: Thu, 4 Dec 2025 16:41:11 +0800
From: Jie Zhan <zhanjie9@...ilicon.com>
To: "zhangpengjie (A)" <zhangpengjie2@...wei.com>, <myungjoo.ham@...sung.com>,
	<kyungmin.park@...sung.com>, <cw00.choi@...sung.com>
CC: <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<zhenglifeng1@...wei.com>, <lihuisong@...wei.com>, <yubowen8@...wei.com>,
	<linhongye@...artners.com>, <linuxarm@...wei.com>,
	<jonathan.cameron@...wei.com>
Subject: Re: [PATCH v3] PM / devfreq: use _visible attribute to replace
 create/remove_sysfs_files()



...

>>> +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 !
>