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

[Greg KH]

On Tue, Nov 26, 2019 at 12:08:18PM +0900, Chanwoo Choi wrote:
> 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.

I'm sorry, but I still do not understand.  Can you show me the directory
tree before and after here?

thanks,

greg k-h