All of lore.kernel.org
 help / color / mirror / Atom feed
From: ryan@bluewatersys.com (Ryan Mallon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
Date: Wed, 24 Mar 2010 11:01:39 +1300	[thread overview]
Message-ID: <4BA93A43.2010807@bluewatersys.com> (raw)
In-Reply-To: <0D753D10438DA54287A00B02708426976368E58561@AUSP01VMBX24.collaborationhost.net>

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

WARNING: multiple messages have this Message-ID (diff)
From: Ryan Mallon <ryan@bluewatersys.com>
To: H Hartley Sweeten <hartleys@visionengravers.com>
Cc: linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/2] arm: add a /proc/cpuinfo platform extension
Date: Wed, 24 Mar 2010 11:01:39 +1300	[thread overview]
Message-ID: <4BA93A43.2010807@bluewatersys.com> (raw)
In-Reply-To: <0D753D10438DA54287A00B02708426976368E58561@AUSP01VMBX24.collaborationhost.net>

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

  reply	other threads:[~2010-03-23 22:01 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4BA93A43.2010807@bluewatersys.com \
    --to=ryan@bluewatersys.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.