[PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Jeffrey Mahoney]

This patch adds sysfs nodes that the hotplug userspace can use to load the
appropriate modules.

In order for hotplug to work with macio devices, patches to module-init-tools
and hotplug must be applied. Those patches are available at:

ftp://ftp.suse.com/pub/people/jeffm/linux/macio-hotplug/

Signed-off-by: Jeff Mahoney <jeffm@suse.com>

diff -rupN -X dontdiff linux-2.6.8/drivers/macintosh/macio_sysfs.c linux-2.6.8.devel/drivers/macintosh/macio_sysfs.c
--- linux-2.6.8/drivers/macintosh/macio_sysfs.c	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.6.8.devel/drivers/macintosh/macio_sysfs.c	2004-09-16 17:12:37.244919704 -0400
@@ -0,0 +1,49 @@
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/stat.h>
+#include <asm/macio.h>
+
+
+#define macio_config_of_attr(field, format_string)              \
+static ssize_t                                                  \
+field##_show (struct device *dev, char *buf)                    \
+{                                                               \
+    struct macio_dev *mdev = to_macio_device (dev);             \
+    return sprintf (buf, format_string, mdev->ofdev.node->field);     \
+}
+
+static ssize_t
+compatible_show (struct device *dev, char *buf)        
+{
+        struct of_device *of;
+        char *compat;
+        int cplen;
+        int length = 0;
+        
+        of = &to_macio_device (dev)->ofdev;
+	compat = (char *) get_property(of->node, "compatible", &cplen);
+	if (!compat) {
+		*buf = '\0';
+		return 0;
+	}
+	while (cplen > 0) {
+		int l;
+		length += sprintf (buf, "%s%s", length ? "," : "", compat);
+		buf += length;
+		l = strlen (compat) + 1;
+		compat += l;
+		cplen -= l;
+	}
+
+	return length;
+}
+
+macio_config_of_attr (name, "%s");
+macio_config_of_attr (type, "%s");
+
+struct device_attribute macio_dev_attrs[] = {
+	__ATTR_RO(name),
+	__ATTR_RO(type),
+	__ATTR_RO(compatible),
+	__ATTR_NULL
+};
diff -rupN -X dontdiff linux-2.6.8/drivers/macintosh/Makefile linux-2.6.8.devel/drivers/macintosh/Makefile
--- linux-2.6.8/drivers/macintosh/Makefile	2004-08-14 01:37:40.000000000 -0400
+++ linux-2.6.8.devel/drivers/macintosh/Makefile	2004-09-16 17:12:37.252918488 -0400
@@ -4,7 +4,7 @@
 
 # Each configuration option enables a list of files.
 
-obj-$(CONFIG_PPC_PMAC)		+= macio_asic.o
+obj-$(CONFIG_PPC_PMAC)		+= macio_asic.o macio_sysfs.o
 
 obj-$(CONFIG_PMAC_PBOOK)	+= mediabay.o
 obj-$(CONFIG_MAC_SERIAL)	+= macserial.o

diff -rupN -X dontdiff linux-2.6.8/drivers/macintosh/macio_asic.c linux-2.6.8.devel/drivers/macintosh/macio_asic.c
--- linux-2.6.8/drivers/macintosh/macio_asic.c	2004-08-14 01:36:45.000000000 -0400
+++ linux-2.6.8.devel/drivers/macintosh/macio_asic.c	2004-09-16 17:12:37.242920008 -0400
@@ -126,10 +126,13 @@ static int macio_device_resume(struct de
 	return 0;
 }
 
+extern struct device_attribute macio_dev_attrs[];
+
 struct bus_type macio_bus_type = {
        .name	= "macio",
        .match	= macio_bus_match,
        .suspend	= macio_device_suspend,
        .resume	= macio_device_resume,
+       .dev_attrs = macio_dev_attrs,
 };
 
 static int __init macio_bus_driver_init(void)
-- 
Jeff Mahoney
SuSE Labs

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Benjamin Herrenschmidt]

On Tue, 2005-03-01 at 16:18 -0500, Jeffrey Mahoney wrote:
> This patch adds sysfs nodes that the hotplug userspace can use to load the
> appropriate modules.
> 
> In order for hotplug to work with macio devices, patches to module-init-tools
> and hotplug must be applied. Those patches are available at:
> 
> ftp://ftp.suse.com/pub/people/jeffm/linux/macio-hotplug/
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

> +static ssize_t
> +compatible_show (struct device *dev, char *buf)        
> +{
> +        struct of_device *of;
> +        char *compat;
> +        int cplen;
> +        int length = 0;
> +        
> +        of = &to_macio_device (dev)->ofdev;
> +	compat = (char *) get_property(of->node, "compatible", &cplen);
> +	if (!compat) {
> +		*buf = '\0';
> +		return 0;
> +	}
> +	while (cplen > 0) {
> +		int l;
> +		length += sprintf (buf, "%s%s", length ? "," : "", compat);
> +		buf += length;
> +		l = strlen (compat) + 1;
> +		compat += l;
> +		cplen -= l;
> +	}
> +
> +	return length;
> +}
> +

There is a problem here. "," is a valid character within a "compatible"
property, and is actually regulary used. Normally, "compatible" is a
list, with '\0' beeing the separator. I suggest using CRLF instead.

Ben.
 

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Benjamin Herrenschmidt wrote:
> On Tue, 2005-03-01 at 16:18 -0500, Jeffrey Mahoney wrote:
> 
>>This patch adds sysfs nodes that the hotplug userspace can use to load the
>>appropriate modules.
>>
>>In order for hotplug to work with macio devices, patches to module-init-tools
>>and hotplug must be applied. Those patches are available at:
>>
>>ftp://ftp.suse.com/pub/people/jeffm/linux/macio-hotplug/
>>
>>Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> 
> 
>>+static ssize_t
>>+compatible_show (struct device *dev, char *buf)        
>>+{
>>+        struct of_device *of;
>>+        char *compat;
>>+        int cplen;
>>+        int length = 0;
>>+        
>>+        of = &to_macio_device (dev)->ofdev;
>>+	compat = (char *) get_property(of->node, "compatible", &cplen);
>>+	if (!compat) {
>>+		*buf = '\0';
>>+		return 0;
>>+	}
>>+	while (cplen > 0) {
>>+		int l;
>>+		length += sprintf (buf, "%s%s", length ? "," : "", compat);
>>+		buf += length;
>>+		l = strlen (compat) + 1;
>>+		compat += l;
>>+		cplen -= l;
>>+	}
>>+
>>+	return length;
>>+}
>>+
> 
> 
> There is a problem here. "," is a valid character within a "compatible"
> property, and is actually regulary used. Normally, "compatible" is a
> list, with '\0' beeing the separator. I suggest using CRLF instead.

Reviewing the 'compatible' values in my device-tree, I definately agree.
I can hack the pmac_zilog driver to test this out further - I've just
been using my airport card.

Are there any other "invalid" characters for the compatible property?
CRLF would work, but these values (as a group) need to be put into
modules.ofmap as well as passed via environment variables for hotplug.
As such, CRLF isn't really easiest choice to work with.

Is whitespace (in any form) allowed in the compatible value?

- -Jeff

- --
Jeff Mahoney
SuSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD8DBQFCJ1U2LPWxlyuTD7IRAopmAJ44hoUKoCrhdBSyAnCp+jzSauAZ8gCfXt7k
tSZa3KiwEybqOoVhPHsQ5Lg=
=9Cvt
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Olaf Hering]

 On Thu, Mar 03, Jeff Mahoney wrote:

> Is whitespace (in any form) allowed in the compatible value?

Yes, whitespace is used at least in the toplevel compatible file, like
'Power Macintosh' in some Pismo models.

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Olaf Hering wrote:
>  On Thu, Mar 03, Jeff Mahoney wrote:
> 
> 
>>Is whitespace (in any form) allowed in the compatible value?
> 
> 
> Yes, whitespace is used at least in the toplevel compatible file, like
> 'Power Macintosh' in some Pismo models.
> 

Oh well, it was wishful thinking anyway. ;)

I see two potential solutions:
* Ideally, I'd like to find a character (pipe?) that isn't used in the
  Apple OF compatible property. I've been unable to find any
  documentation that specifies to this level of detail. (Well, without
  paying for the IEEE-1275 reference, and it may not even be there.)

* Looking at other hotplug agents, it seems acceptable to use $DEVPATH
  to read attributes directly from sysfs. This wouldn't be difficult to
  add to my macio agent, but doesn't seem as nice.

To be frank, my experiences with OF are limited to getting my airport
card to work with this code. That was the initial goal, and I figured it
was code other people might want to use as well. If someone has the
answers to these questions, it would be appreciated.

- -Jeff

- --
Jeff Mahoney
SuSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD8DBQFCJ37XLPWxlyuTD7IRAimZAJ4kAWQwFur1mBB4RDpC3OfDCVpOWACeKQGg
YOoSQu4IGt9zmKNCmTjd6UI=
=yICe
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Olaf Hering]

 On Thu, Mar 03, Jeff Mahoney wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Olaf Hering wrote:
> >  On Thu, Mar 03, Jeff Mahoney wrote:
> > 
> > 
> >>Is whitespace (in any form) allowed in the compatible value?
> > 
> > 
> > Yes, whitespace is used at least in the toplevel compatible file, like
> > 'Power Macintosh' in some Pismo models.
> > 
> 
> Oh well, it was wishful thinking anyway. ;)

The same thing needs to be solved for vio devices, the properties can
contain spaces. depmod and modprobe have to deal with it to generate a
valid module.alias file.

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Olaf Hering wrote:
>  On Thu, Mar 03, Jeff Mahoney wrote:
> 
> 
>>-----BEGIN PGP SIGNED MESSAGE-----
>>Hash: SHA1
>>
>>Olaf Hering wrote:
>>
>>> On Thu, Mar 03, Jeff Mahoney wrote:
>>>
>>>
>>>
>>>>Is whitespace (in any form) allowed in the compatible value?
>>>
>>>
>>>Yes, whitespace is used at least in the toplevel compatible file, like
>>>'Power Macintosh' in some Pismo models.
>>>
>>
>>Oh well, it was wishful thinking anyway. ;)
> 
> 
> The same thing needs to be solved for vio devices, the properties can
> contain spaces. depmod and modprobe have to deal with it to generate a
> valid module.alias file.

The solution I ended up going with was the original CRLF suggestion, and
then I pass around a group of environment vars in userspace rather than
a single line. This works as before, but works with commas and isn't as
ugly as I had thought it might be.

As far as the modules.aliases file goes, I'm not sure what our options
are there. Is the only requirement that the aliases be consistent in the
kernel and in userspace? Since aliases for OF devices haven't previously
existed, I defined the format. Is there any reason I couldn't define the
format such that whitepsace is replaced by underscores, thus eliminating
this issue?

- -Jeff

- --
Jeff Mahoney
SuSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD8DBQFCKI4+LPWxlyuTD7IRAvNOAJ0aopD1JcpUpoAMeuI1EVSBVbICCQCgmHVN
QKswIpQ//5SxjnXIk02PBts=
=K/kB
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Segher Boessenkool]

Sorry to follow up this late...

>>> Is whitespace (in any form) allowed in the compatible value?

No.  Only printable characters are allowed, that is, byte values
0x21..0x7e and 0xa1..0xfe; each text string is terminated by a
0x00; there can be several text strings concatenated in one
"compatible" property.

>> Yes, whitespace is used at least in the toplevel compatible file, like
>> 'Power Macintosh' in some Pismo models.

So those OF implementations violate the OF specification.

> Oh well, it was wishful thinking anyway. ;)
>
> I see two potential solutions:
> * Ideally, I'd like to find a character (pipe?) that isn't used in the
>   Apple OF compatible property. I've been unable to find any
>   documentation that specifies to this level of detail. (Well, without
>   paying for the IEEE-1275 reference, and it may not even be there.)

See 2.3.75 and 3.2.2.1.2 in the OF spec.


Segher

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Benjamin Herrenschmidt]

On Sun, 2005-03-13 at 16:17 +0100, Segher Boessenkool wrote:
> Sorry to follow up this late...
> 
> >>> Is whitespace (in any form) allowed in the compatible value?
> 
> No.  Only printable characters are allowed, that is, byte values
> 0x21..0x7e and 0xa1..0xfe; each text string is terminated by a
> 0x00; there can be several text strings concatenated in one
> "compatible" property.
> 
> >> Yes, whitespace is used at least in the toplevel compatible file, like
> >> 'Power Macintosh' in some Pismo models.
> 
> So those OF implementations violate the OF specification.

Well, we have an unmaintained spec on one side that can't even be
ordered from IEEE anymore and actual imlementations that work today,
what do you chose ? ;)

Ben.

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Olaf Hering]

 On Tue, Mar 15, Benjamin Herrenschmidt wrote:

> what do you chose ? ;)

I'm sure Rusty will prefer the non-whitespace version for depmod and
module.alias

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Segher Boessenkool]

>>>>> Is whitespace (in any form) allowed in the compatible value?
>>
>> No.  Only printable characters are allowed, that is, byte values
>> 0x21..0x7e and 0xa1..0xfe; each text string is terminated by a
>> 0x00; there can be several text strings concatenated in one
>> "compatible" property.
>>
>>>> Yes, whitespace is used at least in the toplevel compatible file, 
>>>> like
>>>> 'Power Macintosh' in some Pismo models.
>>
>> So those OF implementations violate the OF specification.
>
> Well, we have an unmaintained spec on one side that can't even be
> ordered from IEEE anymore and actual imlementations that work today,
> what do you chose ? ;)

I choose the spec.  If an implementation is not conformant to the spec,
it doesn't "work".

Not to say that Linux doesn't have to work around bugs in actual
implementations, of course.  And there's a lot of those.  Too bad ;-)


Segher

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Benjamin Herrenschmidt]

> > Well, we have an unmaintained spec on one side that can't even be
> > ordered from IEEE anymore and actual imlementations that work today,
> > what do you chose ? ;)
> 
> I choose the spec.  If an implementation is not conformant to the spec,
> it doesn't "work".
> 
> Not to say that Linux doesn't have to work around bugs in actual
> implementations, of course.  And there's a lot of those.  Too bad ;-)

Yah, well.. ok, let's say we have a spec... and an implementation that
represents about 90% of the machines concerned. Those 90% have the
"bug"... what do you chose ? :) 

The separator in "compatible", afaik, is \0, not space btw.

On possibiliy would be to have the kernel replace spaces with
underscores for the sake of matching. That would make life easier for
everybody.

Ben.

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Benjamin Herrenschmidt wrote:
>>>Well, we have an unmaintained spec on one side that can't even be
>>>ordered from IEEE anymore and actual imlementations that work today,
>>>what do you chose ? ;)
>>
>>I choose the spec.  If an implementation is not conformant to the spec,
>>it doesn't "work".
>>
>>Not to say that Linux doesn't have to work around bugs in actual
>>implementations, of course.  And there's a lot of those.  Too bad ;-)
> 
> 
> Yah, well.. ok, let's say we have a spec... and an implementation that
> represents about 90% of the machines concerned. Those 90% have the
> "bug"... what do you chose ? :) 
> 
> The separator in "compatible", afaik, is \0, not space btw.
> 
> On possibiliy would be to have the kernel replace spaces with
> underscores for the sake of matching. That would make life easier for
> everybody.

I had suggested this a few days ago, but got no response. Can we agree
that this would be an acceptable solution?

- -Jeff

- --
Jeff Mahoney
SuSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)

iD8DBQFCNauDLPWxlyuTD7IRAt+IAJ9mJRrrYT5trhv03qbGGPrwIwosqwCgpLS6
K/xQZE+dB4BPZc+R/FW74Nw=
=9Pgi
-----END PGP SIGNATURE-----

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Segher Boessenkool]

>> I choose the spec.  If an implementation is not conformant to the 
>> spec,
>> it doesn't "work".
>>
>> Not to say that Linux doesn't have to work around bugs in actual
>> implementations, of course.  And there's a lot of those.  Too bad ;-)
>
> Yah, well.. ok, let's say we have a spec... and an implementation that
> represents about 90% of the machines concerned. Those 90% have the
> "bug"... what do you chose ? :)

What do you mean?  I already said we have to work around this bug --
but it IS a bug.  That's all.

> The separator in "compatible", afaik, is \0, not space btw.

Please re-read my original message?  Yes the "separator" is 0x00;
of course it isn't space, as space isn't allowed at all.

> On possibiliy would be to have the kernel replace spaces with
> underscores for the sake of matching. That would make life easier for
> everybody.

Yes, that'll probably work just fine.  Or use 0xb1, showing that this
is "plus-minus" correct :-)


Segher

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Benjamin Herrenschmidt]

On Mon, 2005-03-14 at 16:19 +0100, Segher Boessenkool wrote:

> > On possibiliy would be to have the kernel replace spaces with
> > underscores for the sake of matching. That would make life easier for
> > everybody.
> 
> Yes, that'll probably work just fine.  Or use 0xb1, showing that this
> is "plus-minus" correct :-)

I'd rather avoid above 0x7f :) I think underscore is fine. Let's replace
spaces with underscores when matching to userspace.

Ben.

Re: [PATCH 2/3] openfirmware: adds sysfs nodes for openfirmware devices

[Benjamin Herrenschmidt]

> Reviewing the 'compatible' values in my device-tree, I definately agree.
> I can hack the pmac_zilog driver to test this out further - I've just
> been using my airport card.
> 
> Are there any other "invalid" characters for the compatible property?
> CRLF would work, but these values (as a group) need to be put into
> modules.ofmap as well as passed via environment variables for hotplug.
> As such, CRLF isn't really easiest choice to work with.

I don't think any value is "invalid". Not sure what is best to use ...
maybe '#' ? never seen it in there so far ... but who knows ...

> Is whitespace (in any form) allowed in the compatible value?

Yes. For example: "Power Macintosh" is a typical compatible value in the
root of the devive tree.

Maybe "tab" ?

Ben.