Re: [PATCH v3] PM / devfreq: Add new name attribute for sysfs

[Chanwoo Choi]

Hi Greg,

On 11/25/19 5:50 PM, Greg KH wrote:
> On Mon, Nov 25, 2019 at 10:03:57AM +0900, Chanwoo Choi wrote:
>> The commit 4585fbcb5331 ("PM / devfreq: Modify the device name as devfreq(X) for
>> sysfs") changed the node name to devfreq(x). After this commit, it is not
>> possible to get the device name through /sys/class/devfreq/devfreq(X)/*.
>>
>> Add new name attribute in order to get device name.
>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: 4585fbcb5331 ("PM / devfreq: Modify the device name as devfreq(X) for sysfs")
>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> ---
>>  Changes from v2:
>> - Change the order of name_show() according to the sequence in devfreq_attrs[]
>>
>> Changes from v1:
>> - Update sysfs-class-devfreq documentation
>> - Show device name directly from 'devfreq->dev.parent'
>>
> 
> Shouldn't you just revert the original patch here?  Why did the sysfs
> file change?

The initial devfreq code used the parent device name for device name
corresponding to devfreq object instead of 'devfreq%d' style.
Before applied The commit 4585fbcb5331 ("PM / devfreq: Modify
the device name as devfreq(X) for sysfs"), the devfreq sysfs
showed the parent device name as following:

For example on Odroid-XU3 board before applied the commit 4585fbcb5331,
	/sys/class/devfreq/soc:bus_wcore
	/sys/class/devfreq/soc:bus_noc
	...(skip)


But, I think that devfreq subsystem had to show the consistent
sysfs entry name for devfreq device like input, thermal, hwmon subsystem.

For example on Odroid-XU3 board,
- The input subsystem show the 'input%d' style for input device.
$root@localhost:/# ls /sys/class/input/                                         
event0  event1  input0  input1  mice  mouse0

- The thermal subsystem show the 'cooling_device%d' style for thermal cooling device.
$ root@localhost:/# ls /sys/class/thermal/                                       
cooling_device0  cooling_device2  thermal_zone1  thermal_zone3
cooling_device1  thermal_zone0    thermal_zone2  thermal_zone4

- The hwmon subsystem show the 'hwmon%d' style for h/w monitor device.
$root@localhost:/# ls /sys/class/hwmon/                                         
hwmon0


So, I tried to make the consistent sysfs entry name for devfreq device
by contributing commit 4585fbcb5331 ("PM / devfreq: Modify the device name as
devfreq(X) for sysfs"). But, The commit 4585fbcb5331 have missed that sysfs
interface had to provide the real device name. Some subsystem like thermal
and hwmon provide the device type or device name through sysfs interface.
It is possible to make the user to find their own specific device by iteration
on user-space.

root@localhost:/# cat /sys/class/thermal/cooling_device0/type 
pwm-fan
root@localhost:/# cat /sys/class/thermal/cooling_device1/type                  
thermal-cpufreq-0
root@localhost:/# cat /sys/class/thermal/cooling_device2/type                  
thermal-cpufreq-1

root@localhost:/# cat /sys/class/hwmon/hwmon0/name                             
pwmfan


So, I add the new 'name' attribute of sysfs for devfreq device.

> 
>> Documentation/ABI/testing/sysfs-class-devfreq | 7 +++++++
>>  drivers/devfreq/devfreq.c                     | 9 +++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
>> index 01196e19afca..75897e2fde43 100644
>> --- a/Documentation/ABI/testing/sysfs-class-devfreq
>> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
>> @@ -7,6 +7,13 @@ Description:
>>  		The name of devfreq object denoted as ... is same as the
>>  		name of device using devfreq.
>>  
>> +What:		/sys/class/devfreq/.../name
>> +Date:		November 2019
>> +Contact:	Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> +Description:
>> +		The /sys/class/devfreq/.../name shows the name of device
>> +		of the corresponding devfreq object.
>> +
>>  What:		/sys/class/devfreq/.../governor
>>  Date:		September 2011
>>  Contact:	MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 65a4b6cf3fa5..6f4d93d2a651 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -1169,6 +1169,14 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
>>  }
>>  EXPORT_SYMBOL(devfreq_remove_governor);
>>  
>> +static ssize_t name_show(struct device *dev,
>> +			struct device_attribute *attr, char *buf)
>> +{
>> +	struct devfreq *devfreq = to_devfreq(dev);
>> +	return sprintf(buf, "%s\n", dev_name(devfreq->dev.parent));
> 
> Why is the parent's name being set here, and not the device name?

The device name style in struct devfreq is 'devfreq%d' instead of
parent device name in order to keep the consistent naming style for devfreq device
as I mentioned above. 'devfreq%d' name is consistent name style for devfreq device.
But, it don't show the real h/w device name. So, show the parent device name
which is specified on device-tree file.

> 
> The device name should be the name of the directory, and the parent's
> name is the name of the parent directory, why is a sysfs attribute for a
> name needed at all?

I add my comment why 'name' attribute is needed with the example of
other subsystem in the linux kernel. Instead of adding duplicate answer,
you could check my comment above.

> 
> confused,
> 
> greg k-h
> 
> 

Best Regards,
Chanwoo Choi