All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
@ 2009-12-23 18:42 H Hartley Sweeten
  2010-03-23 20:30   ` Ryan Mallon
  0 siblings, 1 reply; 13+ messages in thread
From: H Hartley Sweeten @ 2009-12-23 18:42 UTC (permalink / raw)
  To: linux-arm-kernel

Add an optional platform specific extension to /proc/cpuinfo.

Many platforms have custom cpu information that could be exposed
to user space using /proc/cpuinfo.

Patch 1/2 adds the necessary core support to allow a platform
specific callback to dump this information.

Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
edb93xx platforms.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
  2009-12-23 18:42 [PATCH 0/2] arm: add a /proc/cpuinfo platform extension H Hartley Sweeten
@ 2010-03-23 20:30   ` Ryan Mallon
  0 siblings, 0 replies; 13+ messages in thread
From: Ryan Mallon @ 2010-03-23 20:30 UTC (permalink / raw)
  To: H Hartley Sweeten; +Cc: linux-arm-kernel, linux kernel

H Hartley Sweeten wrote:
> Add an optional platform specific extension to /proc/cpuinfo.
> 
> Many platforms have custom cpu information that could be exposed
> to user space using /proc/cpuinfo.
> 
> Patch 1/2 adds the necessary core support to allow a platform
> specific callback to dump this information.
> 
> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
> edb93xx platforms.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I think this is unlikely to get merged in its current state. Russell has
mentioned issues with breaking userspace by changing /proc/cpuinfo. The
other problem I see is that you have a single callback for registering
the arch specific information. In you ep93xx example, each of the ep93xx
boards must add:

  .arch_cpuinfo = ep93xx_cpuinfo,

If one of the boards has some additional information to make available,
it would need to reimplement the entire callback, which gets messy.

I think a better approach would be to have a separate file (say
/proc/archinfo) and use a list approach for displaying data. I'm
guessing that the data displayed in the archinfo file would be
immutable, so something like this (very rough, this would be
kernel/archinfo.c, or arch/arm/kernel/archinfo.c, or whatever):

struct archinfo_entry {
	const char 		*name;
	const char 		*value;
	struct list_head 	list;
};

static LIST_HEAD(archinfo_entries);

int archinfo_add_entry(const char *name, const char *value)
{
	struct archinfo_entry *entry;

	entry = kzalloc(sizeof(struct archinfo_entry), GFP_KERNEL);
	if (!entry)
		return -ENOMEM;

	entry->name = kzalloc(strlen(name) + 1), GFP_KERNEL);
	if (!entry->name) {
		kfree(entry);
		return -ENOMEM;
	}
	strcpy(entry->name, name);
		
	entry->value = kzalloc(strlen(value) + 1, GFP_KERNEL);
	if (!entry->value) {
		kfree(entry->name);
		kfree(entry);
		return -ENOMEM;
	}

	list_add(&entry->list, &archinfo_entries);
	return 0;	
}

static int archinfo_show(struct seq_file *s, void *v)
{
	struct archinfo_entry *entry;

	list_for_each_entry(entry, &archinfo_entries, list)
		seq_printf(s, "%-20s: %s\n", entry->name, entry->value);

	return 0;
}

static int archinfo_open(struct inode *inode, struct file *file)
{
	return single_open(file, archinfo_show, NULL);
}

static const struct file_operations archinfo_ops = {
	.open		= archinfo_open,
	.read		= seq_read,
	.llseek		= seq_lseek,
	.release	= single_release,
};

static int __init init_archinfo(void)
{
	struct proc_dir_entry *proc;

	if (list_empty(&archinfo_entries))
		return 0;

	proc = proc_create("archinfo", 0444, NULL, &archinfo_ops);
	if (!proc)
		return -ENOMEM;
	return 0;
}

lateinit_call(init_archinfo);

A given board/arch can then have something like the following in its
init function:

static void __init myboard_init_archinfo(void)
{
	char buffer[64];

	snprintf(buffer, sizeof(buffer), "some stuff %d, %d\n",
		 val1, val2);
	archinfo_add_entry("stuff", buffer);
}

That way, the arch core (eg arch/arm/mach-ep93xx/core.c) can create a
base set of entries, and the individual platforms/board files can append
additional information to it.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
@ 2010-03-23 20:30   ` Ryan Mallon
  0 siblings, 0 replies; 13+ messages in thread
From: Ryan Mallon @ 2010-03-23 20:30 UTC (permalink / raw)
  To: linux-arm-kernel

H Hartley Sweeten wrote:
> Add an optional platform specific extension to /proc/cpuinfo.
> 
> Many platforms have custom cpu information that could be exposed
> to user space using /proc/cpuinfo.
> 
> Patch 1/2 adds the necessary core support to allow a platform
> specific callback to dump this information.
> 
> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
> edb93xx platforms.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I think this is unlikely to get merged in its current state. Russell has
mentioned issues with breaking userspace by changing /proc/cpuinfo. The
other problem I see is that you have a single callback for registering
the arch specific information. In you ep93xx example, each of the ep93xx
boards must add:

  .arch_cpuinfo = ep93xx_cpuinfo,

If one of the boards has some additional information to make available,
it would need to reimplement the entire callback, which gets messy.

I think a better approach would be to have a separate file (say
/proc/archinfo) and use a list approach for displaying data. I'm
guessing that the data displayed in the archinfo file would be
immutable, so something like this (very rough, this would be
kernel/archinfo.c, or arch/arm/kernel/archinfo.c, or whatever):

struct archinfo_entry {
	const char 		*name;
	const char 		*value;
	struct list_head 	list;
};

static LIST_HEAD(archinfo_entries);

int archinfo_add_entry(const char *name, const char *value)
{
	struct archinfo_entry *entry;

	entry = kzalloc(sizeof(struct archinfo_entry), GFP_KERNEL);
	if (!entry)
		return -ENOMEM;

	entry->name = kzalloc(strlen(name) + 1), GFP_KERNEL);
	if (!entry->name) {
		kfree(entry);
		return -ENOMEM;
	}
	strcpy(entry->name, name);
		
	entry->value = kzalloc(strlen(value) + 1, GFP_KERNEL);
	if (!entry->value) {
		kfree(entry->name);
		kfree(entry);
		return -ENOMEM;
	}

	list_add(&entry->list, &archinfo_entries);
	return 0;	
}

static int archinfo_show(struct seq_file *s, void *v)
{
	struct archinfo_entry *entry;

	list_for_each_entry(entry, &archinfo_entries, list)
		seq_printf(s, "%-20s: %s\n", entry->name, entry->value);

	return 0;
}

static int archinfo_open(struct inode *inode, struct file *file)
{
	return single_open(file, archinfo_show, NULL);
}

static const struct file_operations archinfo_ops = {
	.open		= archinfo_open,
	.read		= seq_read,
	.llseek		= seq_lseek,
	.release	= single_release,
};

static int __init init_archinfo(void)
{
	struct proc_dir_entry *proc;

	if (list_empty(&archinfo_entries))
		return 0;

	proc = proc_create("archinfo", 0444, NULL, &archinfo_ops);
	if (!proc)
		return -ENOMEM;
	return 0;
}

lateinit_call(init_archinfo);

A given board/arch can then have something like the following in its
init function:

static void __init myboard_init_archinfo(void)
{
	char buffer[64];

	snprintf(buffer, sizeof(buffer), "some stuff %d, %d\n",
		 val1, val2);
	archinfo_add_entry("stuff", buffer);
}

That way, the arch core (eg arch/arm/mach-ep93xx/core.c) can create a
base set of entries, and the individual platforms/board files can append
additional information to it.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
  2010-03-23 20:30   ` Ryan Mallon
@ 2010-03-23 20:53     ` H Hartley Sweeten
  -1 siblings, 0 replies; 13+ messages in thread
From: H Hartley Sweeten @ 2010-03-23 20:53 UTC (permalink / raw)
  To: Ryan Mallon; +Cc: linux-arm-kernel, linux kernel

On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
> H Hartley Sweeten wrote:
>> Add an optional platform specific extension to /proc/cpuinfo.
>> 
>> Many platforms have custom cpu information that could be exposed
>> to user space using /proc/cpuinfo.
>> 
>> Patch 1/2 adds the necessary core support to allow a platform
>> specific callback to dump this information.
>> 
>> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
>> edb93xx platforms.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> I think this is unlikely to get merged in its current state. Russell has
> mentioned issues with breaking userspace by changing /proc/cpuinfo. 

I don't agree with this point.

On my Xeon based host running Debian, /proc/cpuinfo looks like this:

$ cat /proc/cpuinfo
Processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 15
model name      : Intel(R) Xeon(R) CPU            5130  @ 2.00GHz
stepping        : 6
cpu MHz         : 1994.954
cache size      : 4096 KB
fdiv_bug        : no
hlt_bug         : no
f00f_bug        : no
coma_bug        : no
fpu             : yes
fpu_exception   : yes
cpuid level     : 10
wp              : yes
Flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss nx lm constant_tsc up arch_perfmon pebs bts pni ssse3 cx16 lahf_lm
Bogomips        : 3998.17
clflush size    : 64
power management:

On my ep93xx system, /proc/cpuinfo normally looks like this:

Processor       : ARM920T rev 0 (v4l)
BogoMIPS        : 99.53
Features        : swp half thumb crunch
CPU implementer : 0x41
CPU architecture: 4T
CPU variant     : 0x1
CPU part        : 0x920
CPU revision    : 0

Hardware        : Vision Engraving Systems EP9307
Revision        : 0001
Serial          : 4943410027260017

Even something as trivial as the BogoMIPS is in a different place in
the two outputs and is spelled differently (due to caps).

The outputs are completely different.  Other architectures in
mainline also have very different outputs.

I can't see any reason why adding additional fields will break
user space, as long as an existing heading in the output is not
duplicated.  Even that "really" shouldn't break anything since
any application parsing this file has to do it sequentially and
the new headings are located at the end of the file.

> The other problem I see is that you have a single callback for registering
> the arch specific information. In you ep93xx example, each of the ep93xx
> boards must add:
>
>  .arch_cpuinfo = ep93xx_cpuinfo,
>
> If one of the boards has some additional information to make available,
> it would need to reimplement the entire callback, which gets messy.

Not necessarily.

If a board, such as the ts72xx, wanted to add additional information
it just has to register it's private callback then call the ep93xx core
supplied callback at the desired point in it's private one.

The ts72xx currently does this exact thing with the .map_io callback.
It supplies it's own private one to map the external FPGA.  It first calls
the ep93xx core to map the ahb/apb space then it does an iotable_init to
map the FPGA.

> I think a better approach would be to have a separate file (say
> /proc/archinfo) and use a list approach for displaying data. I'm
> guessing that the data displayed in the archinfo file would be
> immutable, so something like this (very rough, this would be
> kernel/archinfo.c, or arch/arm/kernel/archinfo.c, or whatever):
> 
> struct archinfo_entry {
> 	const char 		*name;
> 	const char 		*value;
> 	struct list_head 	list;
> };
> 
> static LIST_HEAD(archinfo_entries);
> 
> int archinfo_add_entry(const char *name, const char *value)
> {
> 	struct archinfo_entry *entry;
> 
> 	entry = kzalloc(sizeof(struct archinfo_entry), GFP_KERNEL);
> 	if (!entry)
> 		return -ENOMEM;
> 
> 	entry->name = kzalloc(strlen(name) + 1), GFP_KERNEL);
> 	if (!entry->name) {
> 		kfree(entry);
> 		return -ENOMEM;
> 	}
> 	strcpy(entry->name, name);
> 		
> 	entry->value = kzalloc(strlen(value) + 1, GFP_KERNEL);
> 	if (!entry->value) {
> 		kfree(entry->name);
> 		kfree(entry);
> 		return -ENOMEM;
> 	}
> 
> 	list_add(&entry->list, &archinfo_entries);
> 	return 0;	
> }
> 
> static int archinfo_show(struct seq_file *s, void *v)
> {
> 	struct archinfo_entry *entry;
> 
> 	list_for_each_entry(entry, &archinfo_entries, list)
> 		seq_printf(s, "%-20s: %s\n", entry->name, entry->value);
> 
> 	return 0;
> }
> 
> static int archinfo_open(struct inode *inode, struct file *file)
> {
> 	return single_open(file, archinfo_show, NULL);
> }
> 
> static const struct file_operations archinfo_ops = {
> 	.open		= archinfo_open,
> 	.read		= seq_read,
> 	.llseek		= seq_lseek,
> 	.release	= single_release,
> };
> 
> static int __init init_archinfo(void)
> {
> 	struct proc_dir_entry *proc;
> 
> 	if (list_empty(&archinfo_entries))
> 		return 0;
> 
> 	proc = proc_create("archinfo", 0444, NULL, &archinfo_ops);
> 	if (!proc)
> 		return -ENOMEM;
> 	return 0;
> }
> 
> lateinit_call(init_archinfo);
> 
> A given board/arch can then have something like the following in its
> init function:
> 
> static void __init myboard_init_archinfo(void)
> {
> 	char buffer[64];
> 
> 	snprintf(buffer, sizeof(buffer), "some stuff %d, %d\n",
> 		 val1, val2);
> 	archinfo_add_entry("stuff", buffer);
> }
> 
> That way, the arch core (eg arch/arm/mach-ep93xx/core.c) can create a
> base set of entries, and the individual platforms/board files can append
> additional information to it.

I agree this is probably another way to handle this but it seems like
overkill.

Regards,
Hartley

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
@ 2010-03-23 20:53     ` H Hartley Sweeten
  0 siblings, 0 replies; 13+ messages in thread
From: H Hartley Sweeten @ 2010-03-23 20:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
> H Hartley Sweeten wrote:
>> Add an optional platform specific extension to /proc/cpuinfo.
>> 
>> Many platforms have custom cpu information that could be exposed
>> to user space using /proc/cpuinfo.
>> 
>> Patch 1/2 adds the necessary core support to allow a platform
>> specific callback to dump this information.
>> 
>> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
>> edb93xx platforms.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> I think this is unlikely to get merged in its current state. Russell has
> mentioned issues with breaking userspace by changing /proc/cpuinfo. 

I don't agree with this point.

On my Xeon based host running Debian, /proc/cpuinfo looks like this:

$ cat /proc/cpuinfo
Processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 15
model name      : Intel(R) Xeon(R) CPU            5130  @ 2.00GHz
stepping        : 6
cpu MHz         : 1994.954
cache size      : 4096 KB
fdiv_bug        : no
hlt_bug         : no
f00f_bug        : no
coma_bug        : no
fpu             : yes
fpu_exception   : yes
cpuid level     : 10
wp              : yes
Flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss nx lm constant_tsc up arch_perfmon pebs bts pni ssse3 cx16 lahf_lm
Bogomips        : 3998.17
clflush size    : 64
power management:

On my ep93xx system, /proc/cpuinfo normally looks like this:

Processor       : ARM920T rev 0 (v4l)
BogoMIPS        : 99.53
Features        : swp half thumb crunch
CPU implementer : 0x41
CPU architecture: 4T
CPU variant     : 0x1
CPU part        : 0x920
CPU revision    : 0

Hardware        : Vision Engraving Systems EP9307
Revision        : 0001
Serial          : 4943410027260017

Even something as trivial as the BogoMIPS is in a different place in
the two outputs and is spelled differently (due to caps).

The outputs are completely different.  Other architectures in
mainline also have very different outputs.

I can't see any reason why adding additional fields will break
user space, as long as an existing heading in the output is not
duplicated.  Even that "really" shouldn't break anything since
any application parsing this file has to do it sequentially and
the new headings are located at the end of the file.

> The other problem I see is that you have a single callback for registering
> the arch specific information. In you ep93xx example, each of the ep93xx
> boards must add:
>
>  .arch_cpuinfo = ep93xx_cpuinfo,
>
> If one of the boards has some additional information to make available,
> it would need to reimplement the entire callback, which gets messy.

Not necessarily.

If a board, such as the ts72xx, wanted to add additional information
it just has to register it's private callback then call the ep93xx core
supplied callback at the desired point in it's private one.

The ts72xx currently does this exact thing with the .map_io callback.
It supplies it's own private one to map the external FPGA.  It first calls
the ep93xx core to map the ahb/apb space then it does an iotable_init to
map the FPGA.

> I think a better approach would be to have a separate file (say
> /proc/archinfo) and use a list approach for displaying data. I'm
> guessing that the data displayed in the archinfo file would be
> immutable, so something like this (very rough, this would be
> kernel/archinfo.c, or arch/arm/kernel/archinfo.c, or whatever):
> 
> struct archinfo_entry {
> 	const char 		*name;
> 	const char 		*value;
> 	struct list_head 	list;
> };
> 
> static LIST_HEAD(archinfo_entries);
> 
> int archinfo_add_entry(const char *name, const char *value)
> {
> 	struct archinfo_entry *entry;
> 
> 	entry = kzalloc(sizeof(struct archinfo_entry), GFP_KERNEL);
> 	if (!entry)
> 		return -ENOMEM;
> 
> 	entry->name = kzalloc(strlen(name) + 1), GFP_KERNEL);
> 	if (!entry->name) {
> 		kfree(entry);
> 		return -ENOMEM;
> 	}
> 	strcpy(entry->name, name);
> 		
> 	entry->value = kzalloc(strlen(value) + 1, GFP_KERNEL);
> 	if (!entry->value) {
> 		kfree(entry->name);
> 		kfree(entry);
> 		return -ENOMEM;
> 	}
> 
> 	list_add(&entry->list, &archinfo_entries);
> 	return 0;	
> }
> 
> static int archinfo_show(struct seq_file *s, void *v)
> {
> 	struct archinfo_entry *entry;
> 
> 	list_for_each_entry(entry, &archinfo_entries, list)
> 		seq_printf(s, "%-20s: %s\n", entry->name, entry->value);
> 
> 	return 0;
> }
> 
> static int archinfo_open(struct inode *inode, struct file *file)
> {
> 	return single_open(file, archinfo_show, NULL);
> }
> 
> static const struct file_operations archinfo_ops = {
> 	.open		= archinfo_open,
> 	.read		= seq_read,
> 	.llseek		= seq_lseek,
> 	.release	= single_release,
> };
> 
> static int __init init_archinfo(void)
> {
> 	struct proc_dir_entry *proc;
> 
> 	if (list_empty(&archinfo_entries))
> 		return 0;
> 
> 	proc = proc_create("archinfo", 0444, NULL, &archinfo_ops);
> 	if (!proc)
> 		return -ENOMEM;
> 	return 0;
> }
> 
> lateinit_call(init_archinfo);
> 
> A given board/arch can then have something like the following in its
> init function:
> 
> static void __init myboard_init_archinfo(void)
> {
> 	char buffer[64];
> 
> 	snprintf(buffer, sizeof(buffer), "some stuff %d, %d\n",
> 		 val1, val2);
> 	archinfo_add_entry("stuff", buffer);
> }
> 
> That way, the arch core (eg arch/arm/mach-ep93xx/core.c) can create a
> base set of entries, and the individual platforms/board files can append
> additional information to it.

I agree this is probably another way to handle this but it seems like
overkill.

Regards,
Hartley

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
  2010-03-23 20:53     ` H Hartley Sweeten
@ 2010-03-23 22:01       ` Ryan Mallon
  -1 siblings, 0 replies; 13+ messages in thread
From: Ryan Mallon @ 2010-03-23 22:01 UTC (permalink / raw)
  To: H Hartley Sweeten; +Cc: linux-arm-kernel, linux kernel

H Hartley Sweeten wrote:
> On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
>> H Hartley Sweeten wrote:
>>> Add an optional platform specific extension to /proc/cpuinfo.
>>>
>>> Many platforms have custom cpu information that could be exposed
>>> to user space using /proc/cpuinfo.
>>>
>>> Patch 1/2 adds the necessary core support to allow a platform
>>> specific callback to dump this information.
>>>
>>> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
>>> edb93xx platforms.
>>>
>>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> I think this is unlikely to get merged in its current state. Russell has
>> mentioned issues with breaking userspace by changing /proc/cpuinfo. 
> 
> I don't agree with this point.
> 

[snip]

> 
> Even something as trivial as the BogoMIPS is in a different place in
> the two outputs and is spelled differently (due to caps).
> 
> The outputs are completely different.  Other architectures in
> mainline also have very different outputs.
> 
> I can't see any reason why adding additional fields will break
> user space, as long as an existing heading in the output is not
> duplicated.  Even that "really" shouldn't break anything since
> any application parsing this file has to do it sequentially and
> the new headings are located at the end of the file.

I'm really not sure. There may be some crappy userspace tools out there
which will break. I don't really mind either way if the info goes in
/proc/cpuinfo, or some new /proc/archinfo, just as long as it doesn't
break userspace in some way.

>> The other problem I see is that you have a single callback for registering
>> the arch specific information. In you ep93xx example, each of the ep93xx
>> boards must add:
>>
>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>
>> If one of the boards has some additional information to make available,
>> it would need to reimplement the entire callback, which gets messy.
> 
> Not necessarily.
> 
> If a board, such as the ts72xx, wanted to add additional information
> it just has to register it's private callback then call the ep93xx core
> supplied callback at the desired point in it's private one.
> 
> The ts72xx currently does this exact thing with the .map_io callback.
> It supplies it's own private one to map the external FPGA.  It first calls
> the ep93xx core to map the ahb/apb space then it does an iotable_init to
> map the FPGA.

Okay, fair point. I still don't like having the seq_file callback being
in machine_desc. It means that all of the board files have to be edited
to add the callback. It should be something which happens automagically
in the platform core. Perhaps using a weak function for the callback, or
a #define check.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
@ 2010-03-23 22:01       ` Ryan Mallon
  0 siblings, 0 replies; 13+ messages in thread
From: Ryan Mallon @ 2010-03-23 22:01 UTC (permalink / raw)
  To: linux-arm-kernel

H Hartley Sweeten wrote:
> On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
>> H Hartley Sweeten wrote:
>>> Add an optional platform specific extension to /proc/cpuinfo.
>>>
>>> Many platforms have custom cpu information that could be exposed
>>> to user space using /proc/cpuinfo.
>>>
>>> Patch 1/2 adds the necessary core support to allow a platform
>>> specific callback to dump this information.
>>>
>>> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
>>> edb93xx platforms.
>>>
>>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> I think this is unlikely to get merged in its current state. Russell has
>> mentioned issues with breaking userspace by changing /proc/cpuinfo. 
> 
> I don't agree with this point.
> 

[snip]

> 
> Even something as trivial as the BogoMIPS is in a different place in
> the two outputs and is spelled differently (due to caps).
> 
> The outputs are completely different.  Other architectures in
> mainline also have very different outputs.
> 
> I can't see any reason why adding additional fields will break
> user space, as long as an existing heading in the output is not
> duplicated.  Even that "really" shouldn't break anything since
> any application parsing this file has to do it sequentially and
> the new headings are located at the end of the file.

I'm really not sure. There may be some crappy userspace tools out there
which will break. I don't really mind either way if the info goes in
/proc/cpuinfo, or some new /proc/archinfo, just as long as it doesn't
break userspace in some way.

>> The other problem I see is that you have a single callback for registering
>> the arch specific information. In you ep93xx example, each of the ep93xx
>> boards must add:
>>
>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>
>> If one of the boards has some additional information to make available,
>> it would need to reimplement the entire callback, which gets messy.
> 
> Not necessarily.
> 
> If a board, such as the ts72xx, wanted to add additional information
> it just has to register it's private callback then call the ep93xx core
> supplied callback at the desired point in it's private one.
> 
> The ts72xx currently does this exact thing with the .map_io callback.
> It supplies it's own private one to map the external FPGA.  It first calls
> the ep93xx core to map the ahb/apb space then it does an iotable_init to
> map the FPGA.

Okay, fair point. I still don't like having the seq_file callback being
in machine_desc. It means that all of the board files have to be edited
to add the callback. It should be something which happens automagically
in the platform core. Perhaps using a weak function for the callback, or
a #define check.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
  2010-03-23 22:01       ` Ryan Mallon
@ 2010-03-23 22:35         ` H Hartley Sweeten
  -1 siblings, 0 replies; 13+ messages in thread
From: H Hartley Sweeten @ 2010-03-23 22:35 UTC (permalink / raw)
  To: Ryan Mallon; +Cc: linux-arm-kernel, linux kernel

On Tuesday, March 23, 2010 3:02 PM, Ryan Mallon wrote:
> H Hartley Sweeten wrote:
>> On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
>>> H Hartley Sweeten wrote:
>>>> Add an optional platform specific extension to /proc/cpuinfo.
>>>>
>>>> Many platforms have custom cpu information that could be exposed
>>>> to user space using /proc/cpuinfo.
>>>>
>>>> Patch 1/2 adds the necessary core support to allow a platform
>>>> specific callback to dump this information.
>>>>
>>>> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
>>>> edb93xx platforms.
>>>>
>>>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> I think this is unlikely to get merged in its current state. Russell has
>>> mentioned issues with breaking userspace by changing /proc/cpuinfo. 
>> 
>> I don't agree with this point.
>> 
>
> [snip]
>
>> 
>> Even something as trivial as the BogoMIPS is in a different place in
>> the two outputs and is spelled differently (due to caps).
>> 
>> The outputs are completely different.  Other architectures in
>> mainline also have very different outputs.
>> 
>> I can't see any reason why adding additional fields will break
>> user space, as long as an existing heading in the output is not
>> duplicated.  Even that "really" shouldn't break anything since
>> any application parsing this file has to do it sequentially and
>> the new headings are located at the end of the file.
>
> I'm really not sure. There may be some crappy userspace tools out there
> which will break. I don't really mind either way if the info goes in
> /proc/cpuinfo, or some new /proc/archinfo, just as long as it doesn't
> break userspace in some way.

I did a grep of glibc's source (cvs-2.9, the only one currently on my
system) to see what it tries to parse out of /proc/cpuinfo.  These are
the only ones I could find:

./sysdeps/unix/sysv/linux/arm/ioperm.c
	"Hardware\t: %256[^\n]\n"
./sysdeps/unix/sysv/linux/m68k/getsysstats.c
	"CPU:"
./sysdeps/unix/sysv/linux/mips/getsysstats.c
	"cpu model"
./nptl/perf.c
	"cpu MHz"
./nptl/sockperf.c
	"cpu MHz"
./sysdeps/unix/sysv/linux/getsysstats.c
	"processor"
./sysdeps/unix/sysv/linux/alpha/getsysstats.c
	"cpus active : %d"
	"CPUs probed %*d active %d"
	"cpus detected : %d"
	"CPUs probed %d"
./sysdeps/unix/sysv/linux/alpha/ioperm.c
	"system type : %256[^\n]\n"
	"system variation : %256[^\n]\n"
	"cpu model :%256[^\n]\n"
./sysdeps/unix/sysv/linux/i386/get_clockfreq.c
	"cpu MHz"
./sysdeps/unix/sysv/linux/ia64/get_clockfreq.c
	"itc MHz"
./sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
	"timebase"
./sysdeps/unix/sysv/linux/sparc/getsysstats.c
	"ncpus active : %d"
	"ncpus probed : %d"
./sysdeps/unix/sysv/linux/sparc/sparc64/get_clockfreq.c
	"Cpu0ClkTck"

There may be other userspace tools out there that look at this file
but I would assume they still would parse it using fopen/sscanf/fclose.
Nothing should break as long as the new data is at the end of the file.

>>> The other problem I see is that you have a single callback for registering
>>> the arch specific information. In you ep93xx example, each of the ep93xx
>>> boards must add:
>>>
>>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>>
>>> If one of the boards has some additional information to make available,
>>> it would need to reimplement the entire callback, which gets messy.
>> 
>> Not necessarily.
>> 
>> If a board, such as the ts72xx, wanted to add additional information
>> it just has to register it's private callback then call the ep93xx core
>> supplied callback at the desired point in it's private one.
>> 
>> The ts72xx currently does this exact thing with the .map_io callback.
>> It supplies it's own private one to map the external FPGA.  It first calls
>> the ep93xx core to map the ahb/apb space then it does an iotable_init to
>> map the FPGA.
>
> Okay, fair point. I still don't like having the seq_file callback being
> in machine_desc. It means that all of the board files have to be edited
> to add the callback. It should be something which happens automagically
> in the platform core. Perhaps using a weak function for the callback, or
> a #define check.

The callback is copied from the machine_desc in arch/arm/kernel/setup.c
just like the other architecture-specific pointers.  If an architecture
does not have any additional data to dump they don't have to provide the
callback.  If it's not provided, the main seq_file callback (c_show in
arch/arm/kernel/setup.c) will not call it.  See Patch 1/3 of the series.

Regards,
Hartley

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
@ 2010-03-23 22:35         ` H Hartley Sweeten
  0 siblings, 0 replies; 13+ messages in thread
From: H Hartley Sweeten @ 2010-03-23 22:35 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday, March 23, 2010 3:02 PM, Ryan Mallon wrote:
> H Hartley Sweeten wrote:
>> On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
>>> H Hartley Sweeten wrote:
>>>> Add an optional platform specific extension to /proc/cpuinfo.
>>>>
>>>> Many platforms have custom cpu information that could be exposed
>>>> to user space using /proc/cpuinfo.
>>>>
>>>> Patch 1/2 adds the necessary core support to allow a platform
>>>> specific callback to dump this information.
>>>>
>>>> Patch 2/2 adds a callback to mach-ep93xx and hooks up all the
>>>> edb93xx platforms.
>>>>
>>>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> I think this is unlikely to get merged in its current state. Russell has
>>> mentioned issues with breaking userspace by changing /proc/cpuinfo. 
>> 
>> I don't agree with this point.
>> 
>
> [snip]
>
>> 
>> Even something as trivial as the BogoMIPS is in a different place in
>> the two outputs and is spelled differently (due to caps).
>> 
>> The outputs are completely different.  Other architectures in
>> mainline also have very different outputs.
>> 
>> I can't see any reason why adding additional fields will break
>> user space, as long as an existing heading in the output is not
>> duplicated.  Even that "really" shouldn't break anything since
>> any application parsing this file has to do it sequentially and
>> the new headings are located at the end of the file.
>
> I'm really not sure. There may be some crappy userspace tools out there
> which will break. I don't really mind either way if the info goes in
> /proc/cpuinfo, or some new /proc/archinfo, just as long as it doesn't
> break userspace in some way.

I did a grep of glibc's source (cvs-2.9, the only one currently on my
system) to see what it tries to parse out of /proc/cpuinfo.  These are
the only ones I could find:

./sysdeps/unix/sysv/linux/arm/ioperm.c
	"Hardware\t: %256[^\n]\n"
./sysdeps/unix/sysv/linux/m68k/getsysstats.c
	"CPU:"
./sysdeps/unix/sysv/linux/mips/getsysstats.c
	"cpu model"
./nptl/perf.c
	"cpu MHz"
./nptl/sockperf.c
	"cpu MHz"
./sysdeps/unix/sysv/linux/getsysstats.c
	"processor"
./sysdeps/unix/sysv/linux/alpha/getsysstats.c
	"cpus active : %d"
	"CPUs probed %*d active %d"
	"cpus detected : %d"
	"CPUs probed %d"
./sysdeps/unix/sysv/linux/alpha/ioperm.c
	"system type : %256[^\n]\n"
	"system variation : %256[^\n]\n"
	"cpu model :%256[^\n]\n"
./sysdeps/unix/sysv/linux/i386/get_clockfreq.c
	"cpu MHz"
./sysdeps/unix/sysv/linux/ia64/get_clockfreq.c
	"itc MHz"
./sysdeps/unix/sysv/linux/powerpc/get_clockfreq.c
	"timebase"
./sysdeps/unix/sysv/linux/sparc/getsysstats.c
	"ncpus active : %d"
	"ncpus probed : %d"
./sysdeps/unix/sysv/linux/sparc/sparc64/get_clockfreq.c
	"Cpu0ClkTck"

There may be other userspace tools out there that look at this file
but I would assume they still would parse it using fopen/sscanf/fclose.
Nothing should break as long as the new data is at the end of the file.

>>> The other problem I see is that you have a single callback for registering
>>> the arch specific information. In you ep93xx example, each of the ep93xx
>>> boards must add:
>>>
>>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>>
>>> If one of the boards has some additional information to make available,
>>> it would need to reimplement the entire callback, which gets messy.
>> 
>> Not necessarily.
>> 
>> If a board, such as the ts72xx, wanted to add additional information
>> it just has to register it's private callback then call the ep93xx core
>> supplied callback at the desired point in it's private one.
>> 
>> The ts72xx currently does this exact thing with the .map_io callback.
>> It supplies it's own private one to map the external FPGA.  It first calls
>> the ep93xx core to map the ahb/apb space then it does an iotable_init to
>> map the FPGA.
>
> Okay, fair point. I still don't like having the seq_file callback being
> in machine_desc. It means that all of the board files have to be edited
> to add the callback. It should be something which happens automagically
> in the platform core. Perhaps using a weak function for the callback, or
> a #define check.

The callback is copied from the machine_desc in arch/arm/kernel/setup.c
just like the other architecture-specific pointers.  If an architecture
does not have any additional data to dump they don't have to provide the
callback.  If it's not provided, the main seq_file callback (c_show in
arch/arm/kernel/setup.c) will not call it.  See Patch 1/3 of the series.

Regards,
Hartley

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
  2010-03-23 22:35         ` H Hartley Sweeten
@ 2010-03-23 23:01           ` Ryan Mallon
  -1 siblings, 0 replies; 13+ messages in thread
From: Ryan Mallon @ 2010-03-23 23:01 UTC (permalink / raw)
  To: H Hartley Sweeten; +Cc: linux-arm-kernel, linux kernel

H Hartley Sweeten wrote:
> On Tuesday, March 23, 2010 3:02 PM, Ryan Mallon wrote:
>> H Hartley Sweeten wrote:
>>> On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
>>>> H Hartley Sweeten wrote:
>>> I can't see any reason why adding additional fields will break
>>> user space, as long as an existing heading in the output is not
>>> duplicated.  Even that "really" shouldn't break anything since
>>> any application parsing this file has to do it sequentially and
>>> the new headings are located at the end of the file.
>> I'm really not sure. There may be some crappy userspace tools out there
>> which will break. I don't really mind either way if the info goes in
>> /proc/cpuinfo, or some new /proc/archinfo, just as long as it doesn't
>> break userspace in some way.
> 
> I did a grep of glibc's source (cvs-2.9, the only one currently on my
> system) to see what it tries to parse out of /proc/cpuinfo.  These are
> the only ones I could find:

According to Google Codesearch there are a few utilities around which
read /proc/cpuinfo. I honestly don't know if any of them will break,
though I suspect not. I think that seemingly innocent userspace API
changes like this have broken things in the past though.

>>>> The other problem I see is that you have a single callback for registering
>>>> the arch specific information. In you ep93xx example, each of the ep93xx
>>>> boards must add:
>>>>
>>>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>>>
>>>> If one of the boards has some additional information to make available,
>>>> it would need to reimplement the entire callback, which gets messy.
>>> Not necessarily.
>>>
>>> If a board, such as the ts72xx, wanted to add additional information
>>> it just has to register it's private callback then call the ep93xx core
>>> supplied callback at the desired point in it's private one.
>>>
>>> The ts72xx currently does this exact thing with the .map_io callback.
>>> It supplies it's own private one to map the external FPGA.  It first calls
>>> the ep93xx core to map the ahb/apb space then it does an iotable_init to
>>> map the FPGA.
>> Okay, fair point. I still don't like having the seq_file callback being
>> in machine_desc. It means that all of the board files have to be edited
>> to add the callback. It should be something which happens automagically
>> in the platform core. Perhaps using a weak function for the callback, or
>> a #define check.
> 
> The callback is copied from the machine_desc in arch/arm/kernel/setup.c
> just like the other architecture-specific pointers.  If an architecture
> does not have any additional data to dump they don't have to provide the
> callback.  If it's not provided, the main seq_file callback (c_show in
> arch/arm/kernel/setup.c) will not call it.  See Patch 1/3 of the series.

My point was that you end up adding to the machine_desc for _every_
board on the platform. In the ep93xx patch example the callback is the
same in every case. It doesn't belong in machine_desc, which is the
descriptor for a specific machine or board, it belongs to the platform
(ie ep93xx), so it should be hidden from the board specific files unless
they are providing extensions to the core arch info.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
@ 2010-03-23 23:01           ` Ryan Mallon
  0 siblings, 0 replies; 13+ messages in thread
From: Ryan Mallon @ 2010-03-23 23:01 UTC (permalink / raw)
  To: linux-arm-kernel

H Hartley Sweeten wrote:
> On Tuesday, March 23, 2010 3:02 PM, Ryan Mallon wrote:
>> H Hartley Sweeten wrote:
>>> On Tuesday, March 23, 2010 1:30 PM, Ryan Mallon wrote:
>>>> H Hartley Sweeten wrote:
>>> I can't see any reason why adding additional fields will break
>>> user space, as long as an existing heading in the output is not
>>> duplicated.  Even that "really" shouldn't break anything since
>>> any application parsing this file has to do it sequentially and
>>> the new headings are located at the end of the file.
>> I'm really not sure. There may be some crappy userspace tools out there
>> which will break. I don't really mind either way if the info goes in
>> /proc/cpuinfo, or some new /proc/archinfo, just as long as it doesn't
>> break userspace in some way.
> 
> I did a grep of glibc's source (cvs-2.9, the only one currently on my
> system) to see what it tries to parse out of /proc/cpuinfo.  These are
> the only ones I could find:

According to Google Codesearch there are a few utilities around which
read /proc/cpuinfo. I honestly don't know if any of them will break,
though I suspect not. I think that seemingly innocent userspace API
changes like this have broken things in the past though.

>>>> The other problem I see is that you have a single callback for registering
>>>> the arch specific information. In you ep93xx example, each of the ep93xx
>>>> boards must add:
>>>>
>>>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>>>
>>>> If one of the boards has some additional information to make available,
>>>> it would need to reimplement the entire callback, which gets messy.
>>> Not necessarily.
>>>
>>> If a board, such as the ts72xx, wanted to add additional information
>>> it just has to register it's private callback then call the ep93xx core
>>> supplied callback at the desired point in it's private one.
>>>
>>> The ts72xx currently does this exact thing with the .map_io callback.
>>> It supplies it's own private one to map the external FPGA.  It first calls
>>> the ep93xx core to map the ahb/apb space then it does an iotable_init to
>>> map the FPGA.
>> Okay, fair point. I still don't like having the seq_file callback being
>> in machine_desc. It means that all of the board files have to be edited
>> to add the callback. It should be something which happens automagically
>> in the platform core. Perhaps using a weak function for the callback, or
>> a #define check.
> 
> The callback is copied from the machine_desc in arch/arm/kernel/setup.c
> just like the other architecture-specific pointers.  If an architecture
> does not have any additional data to dump they don't have to provide the
> callback.  If it's not provided, the main seq_file callback (c_show in
> arch/arm/kernel/setup.c) will not call it.  See Patch 1/3 of the series.

My point was that you end up adding to the machine_desc for _every_
board on the platform. In the ep93xx patch example the callback is the
same in every case. It doesn't belong in machine_desc, which is the
descriptor for a specific machine or board, it belongs to the platform
(ie ep93xx), so it should be hidden from the board specific files unless
they are providing extensions to the core arch info.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
  2010-03-23 23:01           ` Ryan Mallon
@ 2010-03-23 23:31             ` H Hartley Sweeten
  -1 siblings, 0 replies; 13+ messages in thread
From: H Hartley Sweeten @ 2010-03-23 23:31 UTC (permalink / raw)
  To: Ryan Mallon; +Cc: linux-arm-kernel, linux kernel

On Tuesday, March 23, 2010 4:01 PM, Ryan Mallon wrote:
>> I did a grep of glibc's source (cvs-2.9, the only one currently on my
>> system) to see what it tries to parse out of /proc/cpuinfo.  These are
>> the only ones I could find:
>
> According to Google Codesearch there are a few utilities around which
> read /proc/cpuinfo. I honestly don't know if any of them will break,
> though I suspect not. I think that seemingly innocent userspace API
> changes like this have broken things in the past though.

Fair enough.

>>>>> The other problem I see is that you have a single callback for registering
>>>>> the arch specific information. In you ep93xx example, each of the ep93xx
>>>>> boards must add:
>>>>>
>>>>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>>>>
>>>>> If one of the boards has some additional information to make available,
>>>>> it would need to reimplement the entire callback, which gets messy.
>>>> Not necessarily.
>>>>
>>>> If a board, such as the ts72xx, wanted to add additional information
>>>> it just has to register it's private callback then call the ep93xx core
>>>> supplied callback at the desired point in it's private one.
>>>>
>>>> The ts72xx currently does this exact thing with the .map_io callback.
>>>> It supplies it's own private one to map the external FPGA.  It first calls
>>>> the ep93xx core to map the ahb/apb space then it does an iotable_init to
>>>> map the FPGA.
>>> Okay, fair point. I still don't like having the seq_file callback being
>>> in machine_desc. It means that all of the board files have to be edited
>>> to add the callback. It should be something which happens automagically
>>> in the platform core. Perhaps using a weak function for the callback, or
>>> a #define check.
>> 
>> The callback is copied from the machine_desc in arch/arm/kernel/setup.c
>> just like the other architecture-specific pointers.  If an architecture
>> does not have any additional data to dump they don't have to provide the
>> callback.  If it's not provided, the main seq_file callback (c_show in
>> arch/arm/kernel/setup.c) will not call it.  See Patch 1/3 of the series.
>
> My point was that you end up adding to the machine_desc for _every_
> board on the platform. In the ep93xx patch example the callback is the
> same in every case. It doesn't belong in machine_desc, which is the
> descriptor for a specific machine or board, it belongs to the platform
> (ie ep93xx), so it should be hidden from the board specific files unless
> they are providing extensions to the core arch info.

The same argument could be made for the .map_io and .init_irq callbacks as
well as the .timer pointer.

The .map_io call back is the same for every ep93xx platform, except for ts72xx
which needs to map it's external FPGA.  The .init_irq callback and the .timer
pointer are the same for all the existing ep93xx platforms.

The point being, they might start out the same now but I can see at least the
ts72xx one changing to dump the jumper settings provided by the external FPGA.

My out-of-tree init file also uses a private .arch_cpuinfo to dump some
information that is used by my user space applications to determine what
features are available.

Also, if a platform maintainer decides that the extra information is of no
use on their platform, they don't need to provide the callback.

Regards,
Hartley

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
@ 2010-03-23 23:31             ` H Hartley Sweeten
  0 siblings, 0 replies; 13+ messages in thread
From: H Hartley Sweeten @ 2010-03-23 23:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday, March 23, 2010 4:01 PM, Ryan Mallon wrote:
>> I did a grep of glibc's source (cvs-2.9, the only one currently on my
>> system) to see what it tries to parse out of /proc/cpuinfo.  These are
>> the only ones I could find:
>
> According to Google Codesearch there are a few utilities around which
> read /proc/cpuinfo. I honestly don't know if any of them will break,
> though I suspect not. I think that seemingly innocent userspace API
> changes like this have broken things in the past though.

Fair enough.

>>>>> The other problem I see is that you have a single callback for registering
>>>>> the arch specific information. In you ep93xx example, each of the ep93xx
>>>>> boards must add:
>>>>>
>>>>>  .arch_cpuinfo = ep93xx_cpuinfo,
>>>>>
>>>>> If one of the boards has some additional information to make available,
>>>>> it would need to reimplement the entire callback, which gets messy.
>>>> Not necessarily.
>>>>
>>>> If a board, such as the ts72xx, wanted to add additional information
>>>> it just has to register it's private callback then call the ep93xx core
>>>> supplied callback at the desired point in it's private one.
>>>>
>>>> The ts72xx currently does this exact thing with the .map_io callback.
>>>> It supplies it's own private one to map the external FPGA.  It first calls
>>>> the ep93xx core to map the ahb/apb space then it does an iotable_init to
>>>> map the FPGA.
>>> Okay, fair point. I still don't like having the seq_file callback being
>>> in machine_desc. It means that all of the board files have to be edited
>>> to add the callback. It should be something which happens automagically
>>> in the platform core. Perhaps using a weak function for the callback, or
>>> a #define check.
>> 
>> The callback is copied from the machine_desc in arch/arm/kernel/setup.c
>> just like the other architecture-specific pointers.  If an architecture
>> does not have any additional data to dump they don't have to provide the
>> callback.  If it's not provided, the main seq_file callback (c_show in
>> arch/arm/kernel/setup.c) will not call it.  See Patch 1/3 of the series.
>
> My point was that you end up adding to the machine_desc for _every_
> board on the platform. In the ep93xx patch example the callback is the
> same in every case. It doesn't belong in machine_desc, which is the
> descriptor for a specific machine or board, it belongs to the platform
> (ie ep93xx), so it should be hidden from the board specific files unless
> they are providing extensions to the core arch info.

The same argument could be made for the .map_io and .init_irq callbacks as
well as the .timer pointer.

The .map_io call back is the same for every ep93xx platform, except for ts72xx
which needs to map it's external FPGA.  The .init_irq callback and the .timer
pointer are the same for all the existing ep93xx platforms.

The point being, they might start out the same now but I can see at least the
ts72xx one changing to dump the jumper settings provided by the external FPGA.

My out-of-tree init file also uses a private .arch_cpuinfo to dump some
information that is used by my user space applications to determine what
features are available.

Also, if a platform maintainer decides that the extra information is of no
use on their platform, they don't need to provide the callback.

Regards,
Hartley

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2010-03-23 23:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-12-23 18:42 [PATCH 0/2] arm: add a /proc/cpuinfo platform extension H Hartley Sweeten
2010-03-23 20:30 ` Ryan Mallon
2010-03-23 20:30   ` Ryan Mallon
2010-03-23 20:53   ` H Hartley Sweeten
2010-03-23 20:53     ` H Hartley Sweeten
2010-03-23 22:01     ` Ryan Mallon
2010-03-23 22:01       ` Ryan Mallon
2010-03-23 22:35       ` H Hartley Sweeten
2010-03-23 22:35         ` H Hartley Sweeten
2010-03-23 23:01         ` Ryan Mallon
2010-03-23 23:01           ` Ryan Mallon
2010-03-23 23:31           ` H Hartley Sweeten
2010-03-23 23:31             ` H Hartley Sweeten

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.