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

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

[Greg Kroah-Hartman]

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
> From: Sudeep Holla <sudeep.holla@arm.com>
> 
> This patch adds initial support for providing processor cache information
> to userspace through sysfs interface. This is based on x86 implementation
> and hence the interface is intended to be fully compatible.
> 
> A per-cpu array of cache information maintained is used mainly for
> sysfs-related book keeping.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/base/Makefile     |   2 +-
>  drivers/base/cacheinfo.c  | 296 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cacheinfo.h |  43 +++++++
>  3 files changed, 340 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/cacheinfo.c
>  create mode 100644 include/linux/cacheinfo.h

You are creating sysfs files, yet you didn't add Documentation/ABI/
information, which is required.  Please fix that.

greg k-h

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

[Sudeep Holla]

On 08/01/14 20:26, Greg Kroah-Hartman wrote:
> On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
>> From: Sudeep Holla <sudeep.holla@arm.com>
>>
>> This patch adds initial support for providing processor cache informatio=
n
>> to userspace through sysfs interface. This is based on x86 implementatio=
n
>> and hence the interface is intended to be fully compatible.
>>
>> A per-cpu array of cache information maintained is used mainly for
>> sysfs-related book keeping.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/base/Makefile     |   2 +-
>>  drivers/base/cacheinfo.c  | 296 +++++++++++++++++++++++++++++++++++++++=
+++++++
>>  include/linux/cacheinfo.h |  43 +++++++
>>  3 files changed, 340 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/base/cacheinfo.c
>>  create mode 100644 include/linux/cacheinfo.h
>=20
> You are creating sysfs files, yet you didn't add Documentation/ABI/
> information, which is required.  Please fix that.
>=20
Ah, I overlooked it. But I am not creating any new sysfs files in this seri=
es.
I am just trying to unify duplicated code in various architectures.

Since these sysfs files are already created in:
1. arch/ia64/kernel/topology.c
2. arch/powerpc/kernel/cacheinfo.c
3. arch/s390/kernel/cache.c
4. arch/x86/kernel/cpu/intel_cacheinfo.c and
also already used by user-space tools like `lscpu` I assumed it's already
documented.

I will add it in next version.

Regards,
Sudeep

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

[Greg Kroah-Hartman]

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
> From: Sudeep Holla <sudeep.holla@arm.com>
> 
> This patch adds initial support for providing processor cache information
> to userspace through sysfs interface. This is based on x86 implementation
> and hence the interface is intended to be fully compatible.
> 
> A per-cpu array of cache information maintained is used mainly for
> sysfs-related book keeping.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/base/Makefile     |   2 +-
>  drivers/base/cacheinfo.c  | 296 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cacheinfo.h |  43 +++++++
>  3 files changed, 340 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/base/cacheinfo.c
>  create mode 100644 include/linux/cacheinfo.h
> 
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 94e8a80..76f07c8 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -4,7 +4,7 @@ obj-y			:= core.o bus.o dd.o syscore.o \
>  			   driver.o class.o platform.o \
>  			   cpu.o firmware.o init.o map.o devres.o \
>  			   attribute_container.o transport_class.o \
> -			   topology.o
> +			   topology.o cacheinfo.o
>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
>  obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
>  obj-y			+= power/
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> new file mode 100644
> index 0000000..f436c31
> --- /dev/null
> +++ b/drivers/base/cacheinfo.c
> @@ -0,0 +1,296 @@
> +/*
> + * cacheinfo support - processor cache information via sysfs
> + *
> + * Copyright (C) 2013 ARM Ltd.
> + * All Rights Reserved
> + *
> + * Author: Sudeep Holla <sudeep.holla@arm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/bitops.h>
> +#include <linux/cacheinfo.h>
> +#include <linux/compiler.h>
> +#include <linux/cpu.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/kobject.h>
> +#include <linux/of.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/smp.h>
> +#include <linux/sysfs.h>
> +
> +struct cache_attr {
> +	struct attribute attr;
> +	 ssize_t(*show) (unsigned int, unsigned short, char *);
> +	 ssize_t(*store) (unsigned int, unsigned short, const char *, size_t);
> +};
> +
> +/* pointer to kobject for cpuX/cache */
> +static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
> +#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu))
> +
> +struct index_kobject {
> +	struct kobject kobj;
> +	unsigned int cpu;
> +	unsigned short index;
> +};
> +
> +static cpumask_t cache_dev_map;
> +
> +/* pointer to array of kobjects for cpuX/cache/indexY */

Please don't use "raw" kobjects for this, use the device attribute
groups, that's what they are there for.  Bonus is that your code should
get a lot simpler when you do that.

thanks,

greg k-h

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

[Sudeep Holla]

On 08/01/14 20:27, Greg Kroah-Hartman wrote:
> On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
>> From: Sudeep Holla <sudeep.holla@arm.com>
>>
>> This patch adds initial support for providing processor cache informatio=
n
>> to userspace through sysfs interface. This is based on x86 implementatio=
n
>> and hence the interface is intended to be fully compatible.
>>
>> A per-cpu array of cache information maintained is used mainly for
>> sysfs-related book keeping.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/base/Makefile     |   2 +-
>>  drivers/base/cacheinfo.c  | 296 +++++++++++++++++++++++++++++++++++++++=
+++++++
>>  include/linux/cacheinfo.h |  43 +++++++
>>  3 files changed, 340 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/base/cacheinfo.c
>>  create mode 100644 include/linux/cacheinfo.h
>>
>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>> index 94e8a80..76f07c8 100644
>> --- a/drivers/base/Makefile
>> +++ b/drivers/base/Makefile
>> @@ -4,7 +4,7 @@ obj-y=09=09=09:=3D core.o bus.o dd.o syscore.o \
>>  =09=09=09   driver.o class.o platform.o \
>>  =09=09=09   cpu.o firmware.o init.o map.o devres.o \
>>  =09=09=09   attribute_container.o transport_class.o \
>> -=09=09=09   topology.o
>> +=09=09=09   topology.o cacheinfo.o
>>  obj-$(CONFIG_DEVTMPFS)=09+=3D devtmpfs.o
>>  obj-$(CONFIG_DMA_CMA) +=3D dma-contiguous.o
>>  obj-y=09=09=09+=3D power/
>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> new file mode 100644
>> index 0000000..f436c31
>> --- /dev/null
>> +++ b/drivers/base/cacheinfo.c
>> @@ -0,0 +1,296 @@
>> +/*
>> + * cacheinfo support - processor cache information via sysfs
>> + *
>> + * Copyright (C) 2013 ARM Ltd.
>> + * All Rights Reserved
>> + *
>> + * Author: Sudeep Holla <sudeep.holla@arm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <linux/bitops.h>
>> +#include <linux/cacheinfo.h>
>> +#include <linux/compiler.h>
>> +#include <linux/cpu.h>
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/kobject.h>
>> +#include <linux/of.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/smp.h>
>> +#include <linux/sysfs.h>
>> +
>> +struct cache_attr {
>> +=09struct attribute attr;
>> +=09 ssize_t(*show) (unsigned int, unsigned short, char *);
>> +=09 ssize_t(*store) (unsigned int, unsigned short, const char *, size_t=
);
>> +};
>> +
>> +/* pointer to kobject for cpuX/cache */
>> +static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
>> +#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu))
>> +
>> +struct index_kobject {
>> +=09struct kobject kobj;
>> +=09unsigned int cpu;
>> +=09unsigned short index;
>> +};
>> +
>> +static cpumask_t cache_dev_map;
>> +
>> +/* pointer to array of kobjects for cpuX/cache/indexY */
>=20
> Please don't use "raw" kobjects for this, use the device attribute
> groups, that's what they are there for.  Bonus is that your code should
> get a lot simpler when you do that.
>=20

Yes I now understand device attribute group simplifies the code, but I thin=
k
kobjects are still needed as we need to track both cpu and cache index.
By reusing only cpu device kobject, we can track cpu only.

Please correct me if I am missing to understand something here.

One thought I have is to make cache_info structure common to all architectu=
re
(for now its ARM specific) and introduce kobject in that similar to ia64
implementation. That even eliminates lot of weak functions defined.

Regards,
Sudeep

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

[Greg Kroah-Hartman]

On Thu, Jan 09, 2014 at 07:19:00PM +0000, Sudeep Holla wrote:
> On 08/01/14 20:27, Greg Kroah-Hartman wrote:
> > On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
> >> From: Sudeep Holla <sudeep.holla@arm.com>
> >>
> >> This patch adds initial support for providing processor cache information
> >> to userspace through sysfs interface. This is based on x86 implementation
> >> and hence the interface is intended to be fully compatible.
> >>
> >> A per-cpu array of cache information maintained is used mainly for
> >> sysfs-related book keeping.
> >>
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>  drivers/base/Makefile     |   2 +-
> >>  drivers/base/cacheinfo.c  | 296 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/cacheinfo.h |  43 +++++++
> >>  3 files changed, 340 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/base/cacheinfo.c
> >>  create mode 100644 include/linux/cacheinfo.h
> >>
> >> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> >> index 94e8a80..76f07c8 100644
> >> --- a/drivers/base/Makefile
> >> +++ b/drivers/base/Makefile
> >> @@ -4,7 +4,7 @@ obj-y			:= core.o bus.o dd.o syscore.o \
> >>  			   driver.o class.o platform.o \
> >>  			   cpu.o firmware.o init.o map.o devres.o \
> >>  			   attribute_container.o transport_class.o \
> >> -			   topology.o
> >> +			   topology.o cacheinfo.o
> >>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
> >>  obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
> >>  obj-y			+= power/
> >> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> >> new file mode 100644
> >> index 0000000..f436c31
> >> --- /dev/null
> >> +++ b/drivers/base/cacheinfo.c
> >> @@ -0,0 +1,296 @@
> >> +/*
> >> + * cacheinfo support - processor cache information via sysfs
> >> + *
> >> + * Copyright (C) 2013 ARM Ltd.
> >> + * All Rights Reserved
> >> + *
> >> + * Author: Sudeep Holla <sudeep.holla@arm.com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> >> + * kind, whether express or implied; without even the implied warranty
> >> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +#include <linux/bitops.h>
> >> +#include <linux/cacheinfo.h>
> >> +#include <linux/compiler.h>
> >> +#include <linux/cpu.h>
> >> +#include <linux/device.h>
> >> +#include <linux/init.h>
> >> +#include <linux/kobject.h>
> >> +#include <linux/of.h>
> >> +#include <linux/sched.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/smp.h>
> >> +#include <linux/sysfs.h>
> >> +
> >> +struct cache_attr {
> >> +	struct attribute attr;
> >> +	 ssize_t(*show) (unsigned int, unsigned short, char *);
> >> +	 ssize_t(*store) (unsigned int, unsigned short, const char *, size_t);
> >> +};
> >> +
> >> +/* pointer to kobject for cpuX/cache */
> >> +static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
> >> +#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu))
> >> +
> >> +struct index_kobject {
> >> +	struct kobject kobj;
> >> +	unsigned int cpu;
> >> +	unsigned short index;
> >> +};
> >> +
> >> +static cpumask_t cache_dev_map;
> >> +
> >> +/* pointer to array of kobjects for cpuX/cache/indexY */
> > 
> > Please don't use "raw" kobjects for this, use the device attribute
> > groups, that's what they are there for.  Bonus is that your code should
> > get a lot simpler when you do that.
> > 
> 
> Yes I now understand device attribute group simplifies the code, but I think
> kobjects are still needed as we need to track both cpu and cache index.
> By reusing only cpu device kobject, we can track cpu only.

I don't understand, you are putting things under the cpu device object,
why do you care about a "cache" kobject?

> One thought I have is to make cache_info structure common to all architecture
> (for now its ARM specific) and introduce kobject in that similar to ia64
> implementation. That even eliminates lot of weak functions defined.

Please don't use raw kobjects if at all possible, it's not good for a
variety of reasons (no userspace events, have to roll your own code,
etc.)

thanks,

greg k-h

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

[Sudeep Holla]

On 09/01/14 19:31, Greg Kroah-Hartman wrote:
> On Thu, Jan 09, 2014 at 07:19:00PM +0000, Sudeep Holla wrote:
>> On 08/01/14 20:27, Greg Kroah-Hartman wrote:
>>> On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
>>>> From: Sudeep Holla <sudeep.holla@arm.com>
>>>>
>>>> This patch adds initial support for providing processor cache informat=
ion
>>>> to userspace through sysfs interface. This is based on x86 implementat=
ion
>>>> and hence the interface is intended to be fully compatible.
>>>>
>>>> A per-cpu array of cache information maintained is used mainly for
>>>> sysfs-related book keeping.
>>>>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>  drivers/base/Makefile     |   2 +-
>>>>  drivers/base/cacheinfo.c  | 296 +++++++++++++++++++++++++++++++++++++=
+++++++++
>>>>  include/linux/cacheinfo.h |  43 +++++++
>>>>  3 files changed, 340 insertions(+), 1 deletion(-)
>>>>  create mode 100644 drivers/base/cacheinfo.c
>>>>  create mode 100644 include/linux/cacheinfo.h
>>>>
>>>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>>>> index 94e8a80..76f07c8 100644
>>>> --- a/drivers/base/Makefile
>>>> +++ b/drivers/base/Makefile
>>>> @@ -4,7 +4,7 @@ obj-y=09=09=09:=3D core.o bus.o dd.o syscore.o \
>>>>  =09=09=09   driver.o class.o platform.o \
>>>>  =09=09=09   cpu.o firmware.o init.o map.o devres.o \
>>>>  =09=09=09   attribute_container.o transport_class.o \
>>>> -=09=09=09   topology.o
>>>> +=09=09=09   topology.o cacheinfo.o
>>>>  obj-$(CONFIG_DEVTMPFS)=09+=3D devtmpfs.o
>>>>  obj-$(CONFIG_DMA_CMA) +=3D dma-contiguous.o
>>>>  obj-y=09=09=09+=3D power/
>>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>>>> new file mode 100644
>>>> index 0000000..f436c31
>>>> --- /dev/null
>>>> +++ b/drivers/base/cacheinfo.c
>>>> @@ -0,0 +1,296 @@
>>>> +/*
>>>> + * cacheinfo support - processor cache information via sysfs
>>>> + *
>>>> + * Copyright (C) 2013 ARM Ltd.
>>>> + * All Rights Reserved
>>>> + *
>>>> + * Author: Sudeep Holla <sudeep.holla@arm.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modi=
fy
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>>> + * kind, whether express or implied; without even the implied warrant=
y
>>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/cacheinfo.h>
>>>> +#include <linux/compiler.h>
>>>> +#include <linux/cpu.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/kobject.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/sched.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/smp.h>
>>>> +#include <linux/sysfs.h>
>>>> +
>>>> +struct cache_attr {
>>>> +=09struct attribute attr;
>>>> +=09 ssize_t(*show) (unsigned int, unsigned short, char *);
>>>> +=09 ssize_t(*store) (unsigned int, unsigned short, const char *, size=
_t);
>>>> +};
>>>> +
>>>> +/* pointer to kobject for cpuX/cache */
>>>> +static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
>>>> +#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu=
))
>>>> +
>>>> +struct index_kobject {
>>>> +=09struct kobject kobj;
>>>> +=09unsigned int cpu;
>>>> +=09unsigned short index;
>>>> +};
>>>> +
>>>> +static cpumask_t cache_dev_map;
>>>> +
>>>> +/* pointer to array of kobjects for cpuX/cache/indexY */
>>>
>>> Please don't use "raw" kobjects for this, use the device attribute
>>> groups, that's what they are there for.  Bonus is that your code should
>>> get a lot simpler when you do that.
>>>
>>
>> Yes I now understand device attribute group simplifies the code, but I t=
hink
>> kobjects are still needed as we need to track both cpu and cache index.
>> By reusing only cpu device kobject, we can track cpu only.
>=20
> I don't understand, you are putting things under the cpu device object,
> why do you care about a "cache" kobject?
>=20
Yes though the cache attributes are under cpu objects, it's hierarchical
something like:
/sys/devices/system/cpu/cpu<n>/cache/index<m>/<attribute_x>
<attribute_x> is unique for each pair of (cpu<n>, index<m>
index is more like cache level, but with 2 indices if they are separate(I$,=
D$)

>> One thought I have is to make cache_info structure common to all archite=
cture
>> (for now its ARM specific) and introduce kobject in that similar to ia64
>> implementation. That even eliminates lot of weak functions defined.
>=20
> Please don't use raw kobjects if at all possible, it's not good for a
> variety of reasons (no userspace events, have to roll your own code,
> etc.)
>=20
Yes I understand, will try to explore other feasible solutions.

Regards,
Sudeep

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

[Greg Kroah-Hartman]

On Thu, Jan 09, 2014 at 07:47:47PM +0000, Sudeep Holla wrote:
> On 09/01/14 19:31, Greg Kroah-Hartman wrote:
> > On Thu, Jan 09, 2014 at 07:19:00PM +0000, Sudeep Holla wrote:
> >> On 08/01/14 20:27, Greg Kroah-Hartman wrote:
> >>> On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
> >>>> From: Sudeep Holla <sudeep.holla@arm.com>
> >>>>
> >>>> This patch adds initial support for providing processor cache information
> >>>> to userspace through sysfs interface. This is based on x86 implementation
> >>>> and hence the interface is intended to be fully compatible.
> >>>>
> >>>> A per-cpu array of cache information maintained is used mainly for
> >>>> sysfs-related book keeping.
> >>>>
> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >>>> ---
> >>>>  drivers/base/Makefile     |   2 +-
> >>>>  drivers/base/cacheinfo.c  | 296 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/linux/cacheinfo.h |  43 +++++++
> >>>>  3 files changed, 340 insertions(+), 1 deletion(-)
> >>>>  create mode 100644 drivers/base/cacheinfo.c
> >>>>  create mode 100644 include/linux/cacheinfo.h
> >>>>
> >>>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> >>>> index 94e8a80..76f07c8 100644
> >>>> --- a/drivers/base/Makefile
> >>>> +++ b/drivers/base/Makefile
> >>>> @@ -4,7 +4,7 @@ obj-y			:= core.o bus.o dd.o syscore.o \
> >>>>  			   driver.o class.o platform.o \
> >>>>  			   cpu.o firmware.o init.o map.o devres.o \
> >>>>  			   attribute_container.o transport_class.o \
> >>>> -			   topology.o
> >>>> +			   topology.o cacheinfo.o
> >>>>  obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
> >>>>  obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
> >>>>  obj-y			+= power/
> >>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> >>>> new file mode 100644
> >>>> index 0000000..f436c31
> >>>> --- /dev/null
> >>>> +++ b/drivers/base/cacheinfo.c
> >>>> @@ -0,0 +1,296 @@
> >>>> +/*
> >>>> + * cacheinfo support - processor cache information via sysfs
> >>>> + *
> >>>> + * Copyright (C) 2013 ARM Ltd.
> >>>> + * All Rights Reserved
> >>>> + *
> >>>> + * Author: Sudeep Holla <sudeep.holla@arm.com>
> >>>> + *
> >>>> + * This program is free software; you can redistribute it and/or modify
> >>>> + * it under the terms of the GNU General Public License version 2 as
> >>>> + * published by the Free Software Foundation.
> >>>> + *
> >>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> >>>> + * kind, whether express or implied; without even the implied warranty
> >>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>>> + * GNU General Public License for more details.
> >>>> + */
> >>>> +#include <linux/bitops.h>
> >>>> +#include <linux/cacheinfo.h>
> >>>> +#include <linux/compiler.h>
> >>>> +#include <linux/cpu.h>
> >>>> +#include <linux/device.h>
> >>>> +#include <linux/init.h>
> >>>> +#include <linux/kobject.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/sched.h>
> >>>> +#include <linux/slab.h>
> >>>> +#include <linux/smp.h>
> >>>> +#include <linux/sysfs.h>
> >>>> +
> >>>> +struct cache_attr {
> >>>> +	struct attribute attr;
> >>>> +	 ssize_t(*show) (unsigned int, unsigned short, char *);
> >>>> +	 ssize_t(*store) (unsigned int, unsigned short, const char *, size_t);
> >>>> +};
> >>>> +
> >>>> +/* pointer to kobject for cpuX/cache */
> >>>> +static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
> >>>> +#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu))
> >>>> +
> >>>> +struct index_kobject {
> >>>> +	struct kobject kobj;
> >>>> +	unsigned int cpu;
> >>>> +	unsigned short index;
> >>>> +};
> >>>> +
> >>>> +static cpumask_t cache_dev_map;
> >>>> +
> >>>> +/* pointer to array of kobjects for cpuX/cache/indexY */
> >>>
> >>> Please don't use "raw" kobjects for this, use the device attribute
> >>> groups, that's what they are there for.  Bonus is that your code should
> >>> get a lot simpler when you do that.
> >>>
> >>
> >> Yes I now understand device attribute group simplifies the code, but I think
> >> kobjects are still needed as we need to track both cpu and cache index.
> >> By reusing only cpu device kobject, we can track cpu only.
> > 
> > I don't understand, you are putting things under the cpu device object,
> > why do you care about a "cache" kobject?
> > 
> Yes though the cache attributes are under cpu objects, it's hierarchical
> something like:
> /sys/devices/system/cpu/cpu<n>/cache/index<m>/<attribute_x>
> <attribute_x> is unique for each pair of (cpu<n>, index<m>
> index is more like cache level, but with 2 indices if they are separate(I$,D$)

Then make the "cache" a struct device, and then create the attribute
group under that?  You'll want that anyway as you don't have any
visibility to userspace tools by using raw kobjects, they don't see them
at all (i.e. libudev and the like.)

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
> From: Sudeep Holla <sudeep.holla@arm.com>
> +#define define_one_ro(_name) \
> +static struct cache_attr _name = \
> +	__ATTR(_name, 0444, show_##_name, NULL)

In the future, we do have __ATTR_RO(), which should be used instead.
You should never use __ATTR() on it's own, if at all possible.  I'm
sweeping the tree for all usages and fixing them slowly up over time.

thanks,

greg k-h

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

[Sudeep Holla]

On 08/01/14 20:28, Greg Kroah-Hartman wrote:
> On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:
>> From: Sudeep Holla <sudeep.holla@arm.com>
>> +#define define_one_ro(_name) \
>> +static struct cache_attr _name =3D \
>> +=09__ATTR(_name, 0444, show_##_name, NULL)
>=20
> In the future, we do have __ATTR_RO(), which should be used instead.
> You should never use __ATTR() on it's own, if at all possible.  I'm
> sweeping the tree for all usages and fixing them slowly up over time.
>=20

Understood, will fix it.

Regards,
Sudeep

Re: [PATCH RFC 2/3] ARM: kernel: add support for cpu cache information

[Russell King - ARM Linux]

On Wed, Jan 08, 2014 at 07:26:07PM +0000, Sudeep Holla wrote:
> +#if __LINUX_ARM_ARCH__ < 7 /* pre ARMv7 */
> +
> +#define MAX_CACHE_LEVEL		1	/* Only 1 level supported */
> +#define CTR_CTYPE_SHIFT		24
> +#define CTR_CTYPE_MASK		(1 << CTR_CTYPE_SHIFT)
> +
> +static inline unsigned int get_ctr(void)
> +{
> +	unsigned int ctr;
> +	asm volatile ("mrc p15, 0, %0, c0, c0, 1" : "=r" (ctr));
> +	return ctr;
> +}
> +
> +static enum cache_type get_cache_type(int level)
> +{
> +	if (level > MAX_CACHE_LEVEL)
> +		return CACHE_TYPE_NOCACHE;
> +	return get_ctr() & CTR_CTYPE_MASK ?
> +		CACHE_TYPE_SEPARATE : CACHE_TYPE_UNIFIED;

So, what do we do for CPUs that don't implement the CTR?  Just return
random rubbish based on decoding the CPU Identity register as if it
were the cache type register?

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH RFC 2/3] ARM: kernel: add support for cpu cache information

[Sudeep Holla]

On 08/01/14 20:57, Russell King - ARM Linux wrote:
> On Wed, Jan 08, 2014 at 07:26:07PM +0000, Sudeep Holla wrote:
>> +#if __LINUX_ARM_ARCH__ < 7 /* pre ARMv7 */
>> +
>> +#define MAX_CACHE_LEVEL=09=091=09/* Only 1 level supported */
>> +#define CTR_CTYPE_SHIFT=09=0924
>> +#define CTR_CTYPE_MASK=09=09(1 << CTR_CTYPE_SHIFT)
>> +
>> +static inline unsigned int get_ctr(void)
>> +{
>> +=09unsigned int ctr;
>> +=09asm volatile ("mrc p15, 0, %0, c0, c0, 1" : "=3Dr" (ctr));
>> +=09return ctr;
>> +}
>> +
>> +static enum cache_type get_cache_type(int level)
>> +{
>> +=09if (level > MAX_CACHE_LEVEL)
>> +=09=09return CACHE_TYPE_NOCACHE;
>> +=09return get_ctr() & CTR_CTYPE_MASK ?
>> +=09=09CACHE_TYPE_SEPARATE : CACHE_TYPE_UNIFIED;
>=20
> So, what do we do for CPUs that don't implement the CTR?  Just return
> random rubbish based on decoding the CPU Identity register as if it
> were the cache type register?
>=20

I assume you referring to some particular CPUs which don't implement this.
I could not find it as optional or IMPLEMENTATION defined in ARM ARM.
I might be missing to find it or there may be exceptions.
Can you please provide more information on that ?

Regards,
Sudeep

Re: [PATCH RFC 2/3] ARM: kernel: add support for cpu cache information

[Russell King - ARM Linux]

On Thu, Jan 09, 2014 at 07:35:03PM +0000, Sudeep Holla wrote:
> I assume you referring to some particular CPUs which don't implement this.
> I could not find it as optional or IMPLEMENTATION defined in ARM ARM.
> I might be missing to find it or there may be exceptions.
> Can you please provide more information on that ?

This is where _not_ relying on the most up to date ARM architecture
reference manual, but instead referring back to the ARM architecture
manual revision appropriate to the architecture is a far better plan.

For example, DDI0100E, Part B, 2.3.2:

| 2.3.2 Cache Type register
| If present, the Cache Type register supplies the following details about
| the cache:

Note the "if present" - it's a fact that not all ARMv4 CPUs support this
register.  2.3 also tells you how to detect when these registers are
implemented:

| ID registers other than the main ID register are defined so that when
| implemented, their value cannot be equal to that of the main ID register.
| Software can therefore determine whether they exist by reading both
| the main ID register and the desired register and comparing their values.
| If the two values are not equal, the desired register exists.

I can go back further to one of the initial revisions of the ARM ARM,
but that's a paper copy.

I can also refer you to DDI0087E (ARM720T) section 4.3 - this is an
ARMv4T CPU, and it has no cache type register.  StrongARM is another
example where the CTR is not implemented.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".