Re: [PATCH RFC/RFT v2 1/8] drivers: base: support cpu cache information interface to userspace via sysfs

[Sudeep Holla <Sudeep.Holla@arm.com>]

Hi Greg,

On 18/02/14 21:53, Greg Kroah-Hartman wrote:
> On Thu, Feb 13, 2014 at 03:55:47PM +0000, Sudeep Holla wrote:
>> Hi Greg,
>> On 11/02/14 00:13, Greg Kroah-Hartman wrote:
[...]
>>> Make the cpu devices be part of a class?
>>
>> I was able to convert these to use struct device instead of raw kobjects. But it
>> requires some changes in order to retain the existing sysfs path mainly due to
>> the fact that cpus are using legacy subsys_system_register and introducing
>> cpu_class conflicts with cpu bus. The base changes in the driver core is as
>> below. Is this acceptable ? or any other alternate suggestions ?
>>
>> --->8
>>  drivers/base/bus.c  | 2 ++
>>  drivers/base/core.c | 8 ++++++++
>>  drivers/base/cpu.c  | 7 +++++++
>>  include/linux/cpu.h | 2 ++
>>  4 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>> index 59dc808..c33bfdb 100644
>> --- a/drivers/base/bus.c
>> +++ b/drivers/base/bus.c
>> @@ -518,10 +518,12 @@ int bus_add_device(struct device *dev)
>>  						&dev->kobj, dev_name(dev));
>>  		if (error)
>>  			goto out_id;
>> +		if (!dev->class) {
>>  			error = sysfs_create_link(&dev->kobj,
>>  				&dev->bus->p->subsys.kobj, "subsystem");
>>  			if (error)
>>  				goto out_subsys;
>> +		}
>>  		klist_add_tail(&dev->p->knode_bus, &bus->p->klist_devices);
>>  	}
>>  	return 0;
> 
> This change worries me, does it cause anything else to happen in sysfs
> that looks differently from what it does today?
> 
Yes, I found that out 2 days back and have now fixed it, the above change is
removed. It's now handled in device_{add,remove}_class_symlinks correctly.

>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2b56717..ef81984 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -10,6 +10,7 @@
>>   *
>>   */
>>
>> +#include <linux/cpu.h>
>>  #include <linux/device.h>
>>  #include <linux/err.h>
>>  #include <linux/init.h>
>> @@ -759,6 +760,8 @@ static struct kobject *get_device_parent(struct device *dev,
>>  			return &block_class.p->subsys.kobj;
>>  		}
>>  #endif
>> +		if (dev->class == cpu_class)
>> +			return &parent->kobj;
> 
> I don't think this is ok :)
> 
> Oh wait, we do this for disk classes today, ick, ok, nevermind, but
> comment it really well please.
> 

Yes I was initially worried but I don't see any other way to retain the sysfs
paths as is. I have commented it well enough now in the patch IMO. I will send
v3 series soon.

Regards,
Sudeep