[PATCH] [PNP] 'modalias' sysfs export

[PATCH] [PNP] 'modalias' sysfs export

[Pierre Ossman]

User space hardware detection need the 'modalias' attributes in the
sysfs tree. This patch adds support to the PNP bus.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---

 drivers/pnp/card.c      |   12 ++++++++++++
 drivers/pnp/interface.c |   12 ++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/pnp/card.c b/drivers/pnp/card.c
index aaa568a..d33a88f 100644
--- a/drivers/pnp/card.c
+++ b/drivers/pnp/card.c
@@ -159,10 +159,22 @@ static ssize_t pnp_show_card_ids(struct 
 
 static DEVICE_ATTR(card_id,S_IRUGO,pnp_show_card_ids,NULL);
 
+static ssize_t pnp_card_modalias_show(struct device *dmdev, struct device_attribute *attr, char *buf)
+{
+	struct pnp_card *card = to_pnp_card(dmdev);
+	struct pnp_id * pos = card->id;
+
+	/* FIXME: modalias can only do one alias */
+	return sprintf(buf, "pnp:c%s\n", pos->id);
+}
+
+static DEVICE_ATTR(modalias,S_IRUGO,pnp_card_modalias_show,NULL);
+
 static int pnp_interface_attach_card(struct pnp_card *card)
 {
 	device_create_file(&card->dev,&dev_attr_name);
 	device_create_file(&card->dev,&dev_attr_card_id);
+	device_create_file(&card->dev,&dev_attr_modalias);
 	return 0;
 }
 
diff --git a/drivers/pnp/interface.c b/drivers/pnp/interface.c
index a2d8ce7..67bd17c 100644
--- a/drivers/pnp/interface.c
+++ b/drivers/pnp/interface.c
@@ -459,10 +459,22 @@ static ssize_t pnp_show_current_ids(stru
 
 static DEVICE_ATTR(id,S_IRUGO,pnp_show_current_ids,NULL);
 
+static ssize_t pnp_modalias_show(struct device *dmdev, struct device_attribute *attr, char *buf)
+{
+	struct pnp_dev *dev = to_pnp_dev(dmdev);
+	struct pnp_id * pos = dev->id;
+
+	/* FIXME: modalias can only do one alias */
+	return sprintf(buf, "pnp:d%s\n", pos->id);
+}
+
+static DEVICE_ATTR(modalias,S_IRUGO,pnp_modalias_show,NULL);
+
 int pnp_interface_attach_device(struct pnp_dev *dev)
 {
 	device_create_file(&dev->dev,&dev_attr_options);
 	device_create_file(&dev->dev,&dev_attr_resources);
 	device_create_file(&dev->dev,&dev_attr_id);
+	device_create_file(&dev->dev,&dev_attr_modalias);
 	return 0;
 }

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Mon, Feb 27, 2006 at 10:40:19PM +0100, Pierre Ossman wrote:
> User space hardware detection need the 'modalias' attributes in the
> sysfs tree. This patch adds support to the PNP bus.

> +
> +	/* FIXME: modalias can only do one alias */
> +	return sprintf(buf, "pnp:c%s\n", pos->id);

Without the FIXME actually "fixed", this does not make sense. You want to
match always on _all_ aliases. In most cases where you have more than
one, the first one is the vendor specific one which isn't interesting at
all on Linux. If you have more than one entry usually the second one is the
one you are looking for.

So eighter we find a way to encode _all_ id's in one modalias string which can
be matched by a wildcard or keep the current solution which iterates over the
sysfs "id" file and calls modprobe for every entry.

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Pierre Ossman]

Kay Sievers wrote:
> On Mon, Feb 27, 2006 at 10:40:19PM +0100, Pierre Ossman wrote:
>> User space hardware detection need the 'modalias' attributes in the
>> sysfs tree. This patch adds support to the PNP bus.
> 
>> +
>> +	/* FIXME: modalias can only do one alias */
>> +	return sprintf(buf, "pnp:c%s\n", pos->id);
> 
> Without the FIXME actually "fixed", this does not make sense. You want to
> match always on _all_ aliases. In most cases where you have more than
> one, the first one is the vendor specific one which isn't interesting at
> all on Linux. If you have more than one entry usually the second one is the
> one you are looking for.
> 
> So eighter we find a way to encode _all_ id's in one modalias string which can
> be matched by a wildcard or keep the current solution which iterates over the
> sysfs "id" file and calls modprobe for every entry.
> 

That's a bit harsh. Although the FIXME is a downer, this patch is a
strict addition of functionality, not removal. It solves a real problem
for me, and it does so without removing any functionality for anyone
else. The fact is that most PNP devices do not have multiple id:s (at
least the ACPI variant which is the most common in todays machines), so
the problem is not near as big as you make it out to be.

That said, I agree that it would be desirable to fix this. First of all
we would need to synchronise this with userspace. Currently I guess that
means udev. Allowing 'modalias' to contain multiple lines should be a
simple enough solution (provided we don't fill the available buffer space).

The PNP cards are also a bit of a problem, but this isn't something new.
When matching a device to a driver, the card ID must match and also all
device ID:s. The problem is that the device ID:s are sets, not lists.
I.e. we compare the unordered contents of the two, with no regard to
ordering.

Rgds
Pierre

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Thu, Mar 02, 2006 at 09:39:03AM +0100, Pierre Ossman wrote:
> Kay Sievers wrote:
> > On Mon, Feb 27, 2006 at 10:40:19PM +0100, Pierre Ossman wrote:
> >> User space hardware detection need the 'modalias' attributes in the
> >> sysfs tree. This patch adds support to the PNP bus.
> > 
> >> +
> >> +	/* FIXME: modalias can only do one alias */
> >> +	return sprintf(buf, "pnp:c%s\n", pos->id);
> > 
> > Without the FIXME actually "fixed", this does not make sense. You want to
> > match always on _all_ aliases. In most cases where you have more than
> > one, the first one is the vendor specific one which isn't interesting at
> > all on Linux. If you have more than one entry usually the second one is the
> > one you are looking for.
> > 
> > So eighter we find a way to encode _all_ id's in one modalias string which can
> > be matched by a wildcard or keep the current solution which iterates over the
> > sysfs "id" file and calls modprobe for every entry.
> > 
> 
> That's a bit harsh. Although the FIXME is a downer, this patch is a
> strict addition of functionality, not removal. It solves a real problem
> for me, and it does so without removing any functionality for anyone
> else. The fact is that most PNP devices do not have multiple id:s (at
> least the ACPI variant which is the most common in todays machines), so
> the problem is not near as big as you make it out to be.

Sorry, but your patch is just incomplete and in some cases just wrong.
On my new ThinkPad, 3 of 12 devices would not work with your patch, so this
is far from "most common" and not acceptable. So eighter we get a fully
working modalias or we better stay without one for PNP and handle that
with the custom script we already use today.

Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Pierre Ossman]

Kay Sievers wrote:
>
> Sorry, but your patch is just incomplete and in some cases just wrong.
> On my new ThinkPad, 3 of 12 devices would not work with your patch, so this
> is far from "most common" and not acceptable. So eighter we get a fully
> working modalias or we better stay without one for PNP and handle that
> with the custom script we already use today.
>
>   

Well then you shouldn't have any problems with adding multi-line support
of modalias in udev. Then I can do another patch that exports all aliases.

Rgds
Pierre

Re: [PATCH] [PNP] 'modalias' sysfs export

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

Here is a patch for doing multi line modalias for PNP devices. This will
break udev, so that needs to be updated first.

I had a longer look at the card part and it seems that module aliases
cannot be reliably used for it. Not without restructuring the system at
least. The possible combinations explode when you notice that the driver
ids needs to be just at subset of the card, without any ordering.

If I got my calculations right, a PNP card would have to have roughly
2^(2n) aliases, where n is the number of device ids. So right now, I
lean towards only adding modalias support for the non-card part of the
PNP layer.

Andrew, do you want a fix for the patch in -mm or can you remove the
part of it that modifies drivers/pnp/card.c by yourself?

Rgds
Pierre


[-- Attachment #2: pnp-sysfs-modalias-multiline.patch --]
[-- Type: text/x-patch, Size: 1042 bytes --]

[PNP] Export all aliases through the modalias attribute

From: Pierre Ossman <drzeus@drzeus.cx>

In order to be backwards compatible, we previously only exported the first
of all the PNP module aliases. We should instead export all aliases,
delimited by newlines.
---

 drivers/pnp/interface.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pnp/interface.c b/drivers/pnp/interface.c
index 67bd17c..8e78923 100644
--- a/drivers/pnp/interface.c
+++ b/drivers/pnp/interface.c
@@ -461,11 +461,15 @@ static DEVICE_ATTR(id,S_IRUGO,pnp_show_c
 
 static ssize_t pnp_modalias_show(struct device *dmdev, struct device_attribute *attr, char *buf)
 {
+	char *str = buf;
 	struct pnp_dev *dev = to_pnp_dev(dmdev);
 	struct pnp_id * pos = dev->id;
 
-	/* FIXME: modalias can only do one alias */
-	return sprintf(buf, "pnp:d%s\n", pos->id);
+	while (pos) {
+		str += sprintf(str,"pnp:d%s\n", pos->id);
+		pos = pos->next;
+	}
+	return (str - buf);
 }
 
 static DEVICE_ATTR(modalias,S_IRUGO,pnp_modalias_show,NULL);

Re: [PATCH] [PNP] 'modalias' sysfs export

[Arjan van de Ven]

On Sat, 2006-03-11 at 17:05 +0100, Pierre Ossman wrote:
> Here is a patch for doing multi line modalias for PNP devices. This will
> break udev, so that needs to be updated first.


how could this EVER be acceptable???

Re: [PATCH] [PNP] 'modalias' sysfs export

[Pierre Ossman]

Arjan van de Ven wrote:
> On Sat, 2006-03-11 at 17:05 +0100, Pierre Ossman wrote:
>   
>> Here is a patch for doing multi line modalias for PNP devices. This will
>> break udev, so that needs to be updated first.
>>     
>
>
> how could this EVER be acceptable???
>
>   

Soon I would hope. The modalias attribute currently only supports one
alias (i.e. one line). This isn't enough for PNP, so if we want to
support that bus (which I assume we do) we need to extend the interface.
udev could be updated and be backwards compatible, the kernel can not
(excluding adding another interface to the same data). So this patch
should lag the update to udev a bit (i.e. I'm not suggesting it be
applied now).

Rgds
Pierre

Re: [PATCH] [PNP] 'modalias' sysfs export

[Andrew Morton]

Pierre Ossman <drzeus-list@drzeus.cx> wrote:
>
>  Here is a patch for doing multi line modalias for PNP devices. This will
>  break udev, so that needs to be updated first.
> 
>  I had a longer look at the card part and it seems that module aliases
>  cannot be reliably used for it. Not without restructuring the system at
>  least. The possible combinations explode when you notice that the driver
>  ids needs to be just at subset of the card, without any ordering.
> 
>  If I got my calculations right, a PNP card would have to have roughly
>  2^(2n) aliases, where n is the number of device ids. So right now, I
>  lean towards only adding modalias support for the non-card part of the
>  PNP layer.
> 
>  Andrew, do you want a fix for the patch in -mm or can you remove the
>  part of it that modifies drivers/pnp/card.c by yourself?

I assume you mean that the drivers/pnp/card.c patch of
pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
on top of the result.

But I don't want to break udev.

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Sat, Mar 11, 2006 at 05:38:47PM -0800, Andrew Morton wrote:
> Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> >
> >  Here is a patch for doing multi line modalias for PNP devices. This will
> >  break udev, so that needs to be updated first.
> > 
> >  I had a longer look at the card part and it seems that module aliases
> >  cannot be reliably used for it. Not without restructuring the system at
> >  least. The possible combinations explode when you notice that the driver
> >  ids needs to be just at subset of the card, without any ordering.
> > 
> >  If I got my calculations right, a PNP card would have to have roughly
> >  2^(2n) aliases, where n is the number of device ids. So right now, I
> >  lean towards only adding modalias support for the non-card part of the
> >  PNP layer.
> > 
> >  Andrew, do you want a fix for the patch in -mm or can you remove the
> >  part of it that modifies drivers/pnp/card.c by yourself?
> 
> I assume you mean that the drivers/pnp/card.c patch of
> pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> on top of the result.
> 
> But I don't want to break udev.

Right, we should not start multiline modalias sysfs files. Eighter we
get all aliases encoded in a single string, maybe like macio is doing it:
  http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;hb=HEAD;f=drivers/macintosh/macio_sysfs.c#l42
and we can pass that single string to modprobe, or we better stay with
the current one-line udev PNP rule which uses /bin/sh to do the job, which
works just fine.

Also MODALIAS in the event environment is required at the same time
the sysfs file is added. And that should also not be a multi-line
value.

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Adam Belay]

On Sat, Mar 11, 2006 at 05:38:47PM -0800, Andrew Morton wrote:
> Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> >
> >  Here is a patch for doing multi line modalias for PNP devices. This will
> >  break udev, so that needs to be updated first.
> > 
> >  I had a longer look at the card part and it seems that module aliases
> >  cannot be reliably used for it. Not without restructuring the system at
> >  least. The possible combinations explode when you notice that the driver
> >  ids needs to be just at subset of the card, without any ordering.
> > 
> >  If I got my calculations right, a PNP card would have to have roughly
> >  2^(2n) aliases, where n is the number of device ids. So right now, I
> >  lean towards only adding modalias support for the non-card part of the
> >  PNP layer.
> > 
> >  Andrew, do you want a fix for the patch in -mm or can you remove the
> >  part of it that modifies drivers/pnp/card.c by yourself?
> 
> I assume you mean that the drivers/pnp/card.c patch of
> pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> on top of the result.
> 
> But I don't want to break udev.

I think supporting multiple IDs per node is a reasonable expectation to
have from udev (even for subsystems beyond PnP).  Kay, would this be
difficult to add?

However, I'm a little confused as to why we're exporting these "modalias"
strings from a kernel interface in the first place.  Wouldn't it be better
from an abstraction barrier standpoint to have userspace generate such
strings from information it gathered through bus-specific interfaces? As
can be seen from this example, when the modalias (or essentially a device
identification key) is imposed at the kernel level driver model interface,
one has to make policy decisions (e.g. what legacy features such as isapnp
are we going to only psuedo-support).

Thanks,
Adam

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Sat, Mar 11, 2006 at 11:29:57PM -0500, Adam Belay wrote:
> On Sat, Mar 11, 2006 at 05:38:47PM -0800, Andrew Morton wrote:
> > Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> > >
> > >  Here is a patch for doing multi line modalias for PNP devices. This will
> > >  break udev, so that needs to be updated first.
> > > 
> > >  I had a longer look at the card part and it seems that module aliases
> > >  cannot be reliably used for it. Not without restructuring the system at
> > >  least. The possible combinations explode when you notice that the driver
> > >  ids needs to be just at subset of the card, without any ordering.
> > > 
> > >  If I got my calculations right, a PNP card would have to have roughly
> > >  2^(2n) aliases, where n is the number of device ids. So right now, I
> > >  lean towards only adding modalias support for the non-card part of the
> > >  PNP layer.
> > > 
> > >  Andrew, do you want a fix for the patch in -mm or can you remove the
> > >  part of it that modifies drivers/pnp/card.c by yourself?
> > 
> > I assume you mean that the drivers/pnp/card.c patch of
> > pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> > on top of the result.
> > 
> > But I don't want to break udev.
> 
> I think supporting multiple IDs per node is a reasonable expectation to
> have from udev (even for subsystems beyond PnP).  Kay, would this be
> difficult to add?

Udev does not care about $MODALIAS at all about the string, it just
runs configured programs when a device is added. It's unlikely,
that we will ever need/have a built-in MODALIAS handling in udev.
Distros handle that all differently, most just do "modprobe $MODALIAS"
with the device event.

> However, I'm a little confused as to why we're exporting these "modalias"
> strings from a kernel interface in the first place.  Wouldn't it be better
> from an abstraction barrier standpoint to have userspace generate such
> strings from information it gathered through bus-specific interfaces?

Well, the modalias match strings are native part of the kernel modules, so
it is just convenient to have the kernel to create the modalias to match
against these strings too. Userspace and modprobe usually doesn't care about
the actual content of the strings, as both come from the kernel and just need
to match each other. In theory, there is no need to keep userspace updated,
if a new bus is added, or the format is changed, which is nice.

> As
> can be seen from this example, when the modalias (or essentially a device
> identification key) is imposed at the kernel level driver model interface,
> one has to make policy decisions (e.g. what legacy features such as isapnp
> are we going to only psuedo-support).

What "policy"? We only need a way to export the multiple matches in a
sane way and using a multiline value is not really nice, especially when
added to the environment.

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Adam Belay]

On Sun, Mar 12, 2006 at 06:09:55AM +0100, Kay Sievers wrote:
> 259-0000
> 
> On Sat, Mar 11, 2006 at 11:29:57PM -0500, Adam Belay wrote:
> > On Sat, Mar 11, 2006 at 05:38:47PM -0800, Andrew Morton wrote:
> > > Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> > > >
> > > >  Here is a patch for doing multi line modalias for PNP devices. This will
> > > >  break udev, so that needs to be updated first.
> > > > 
> > > >  I had a longer look at the card part and it seems that module aliases
> > > >  cannot be reliably used for it. Not without restructuring the system at
> > > >  least. The possible combinations explode when you notice that the driver
> > > >  ids needs to be just at subset of the card, without any ordering.
> > > > 
> > > >  If I got my calculations right, a PNP card would have to have roughly
> > > >  2^(2n) aliases, where n is the number of device ids. So right now, I
> > > >  lean towards only adding modalias support for the non-card part of the
> > > >  PNP layer.
> > > > 
> > > >  Andrew, do you want a fix for the patch in -mm or can you remove the
> > > >  part of it that modifies drivers/pnp/card.c by yourself?
> > > 
> > > I assume you mean that the drivers/pnp/card.c patch of
> > > pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> > > on top of the result.
> > > 
> > > But I don't want to break udev.
> > 
> > I think supporting multiple IDs per node is a reasonable expectation to
> > have from udev (even for subsystems beyond PnP).  Kay, would this be
> > difficult to add?
> 
> Udev does not care about $MODALIAS at all about the string, it just
> runs configured programs when a device is added. It's unlikely,
> that we will ever need/have a built-in MODALIAS handling in udev.
> Distros handle that all differently, most just do "modprobe $MODALIAS"
> with the device event.

Alright, so it would seem changing to multiple line MODALIAS values could be
potentially inconvenient for the current conventions.  Therefore, this sort of
change probably shouldn't be made for PnP.  However, I think pcmcia and acpi
will eventually have similar issues.

> 
> > However, I'm a little confused as to why we're exporting these "modalias"
> > strings from a kernel interface in the first place.  Wouldn't it be better
> > from an abstraction barrier standpoint to have userspace generate such
> > strings from information it gathered through bus-specific interfaces?
> 
> Well, the modalias match strings are native part of the kernel modules, so
> it is just convenient to have the kernel to create the modalias to match
> against these strings too. Userspace and modprobe usually doesn't care about
> the actual content of the strings, as both come from the kernel and just need
> to match each other. In theory, there is no need to keep userspace updated,
> if a new bus is added, or the format is changed, which is nice.

I appreciate the explanation.  I guess I've always favored a more
userspace-centric driver matching mechanism than our current system.  There
are some cases where multiple drivers exist and the user might want to select
a specific driver for each device instance, and I think we're very limited in
these situations because of the module level granularity of driver matching.

In any case, a native part of kernel modules is not necessarily also native to
the kernel itself.  In this case, as I understand it, modalias strings are a
sort of metadata embedded in the module binary that's not necessarily referenced
by the kernel.  A userspace script could rather easily read bus-specific vendor
and device ID components from sysfs and generate a string compatible with this
format.

Regards,
Adam

Re: [PATCH] [PNP] 'modalias' sysfs export

[Pierre Ossman]

Andrew Morton wrote:
> I assume you mean that the drivers/pnp/card.c patch of
> pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> on top of the result.
>
> But I don't want to break udev.
>   

I suppose I wasn't entirely clear there. I'd like you to do the first
part (remove the card.c part), but not apply this second patch. I just
sent that in as a means of getting the ball rolling again.

The reason I'm pushing this issue is that Red Hat decided to drop all
magical scripts that figured out what modules to load and instead only
use the modalias attribute. They consider the right way to go is to get
the PNP layer to export modalias, so that's what I'm trying to do.

Bugzilla entry for those interested:

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=146405

Rgds
Pierre

Re: [PATCH] [PNP] 'modalias' sysfs export

[Matthieu CASTET]

Hi,

Le Sun, 12 Mar 2006 12:17:19 +0100, Pierre Ossman a écrit :
> The reason I'm pushing this issue is that Red Hat decided to drop all
> magical scripts that figured out what modules to load and instead only
> use the modalias attribute. They consider the right way to go is to get
> the PNP layer to export modalias, so that's what I'm trying to do.
> 
> Bugzilla entry for those interested:
> 
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=146405
> 
IIRC debian maintainers want also to use only modalias stuff.
see
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=337004
and http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=334238

Matthieu

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Sun, Mar 12, 2006 at 12:17:19PM +0100, Pierre Ossman wrote:
> Andrew Morton wrote:
> > I assume you mean that the drivers/pnp/card.c patch of
> > pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> > on top of the result.
> >
> > But I don't want to break udev.
> >   
> 
> I suppose I wasn't entirely clear there. I'd like you to do the first
> part (remove the card.c part), but not apply this second patch. I just
> sent that in as a means of getting the ball rolling again.

Again, multiline sysfs modalias files are not going to happen. Find a
sane way to encode the list of devices into a single string, or don't do
it at all. And it must be available in the event environment too.

> The reason I'm pushing this issue is that Red Hat decided to drop all
> magical scripts that figured out what modules to load and instead only
> use the modalias attribute. They consider the right way to go is to get
> the PNP layer to export modalias, so that's what I'm trying to do.

There is no need to rush out with this half-baken solution. This simple
udev rule does the job for you, if you want pnp module autoloading with
the current kernel:
  SUBSYSTEM=="pnp", RUN+="/bin/sh -c 'while read id; do /sbin/modprobe pnp:d$$id; done < /sys$devpath/id'"

Andrew, please make sure, that this patch does not hit mainline until
there is a _sane_ solution to the multiple id's exported for a single
device problem.

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Andrew Morton]

Kay Sievers <kay.sievers@vrfy.org> wrote:
>
> On Sun, Mar 12, 2006 at 12:17:19PM +0100, Pierre Ossman wrote:
> > Andrew Morton wrote:
> > > I assume you mean that the drivers/pnp/card.c patch of
> > > pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> > > on top of the result.
> > >
> > > But I don't want to break udev.
> > >   
> > 
> > I suppose I wasn't entirely clear there. I'd like you to do the first
> > part (remove the card.c part), but not apply this second patch. I just
> > sent that in as a means of getting the ball rolling again.
> 
> Again, multiline sysfs modalias files are not going to happen. Find a
> sane way to encode the list of devices into a single string, or don't do
> it at all. And it must be available in the event environment too.
> 
> > The reason I'm pushing this issue is that Red Hat decided to drop all
> > magical scripts that figured out what modules to load and instead only
> > use the modalias attribute. They consider the right way to go is to get
> > the PNP layer to export modalias, so that's what I'm trying to do.
> 
> There is no need to rush out with this half-baken solution. This simple
> udev rule does the job for you, if you want pnp module autoloading with
> the current kernel:
>   SUBSYSTEM=="pnp", RUN+="/bin/sh -c 'while read id; do /sbin/modprobe pnp:d$$id; done < /sys$devpath/id'"
> 
> Andrew, please make sure, that this patch does not hit mainline until
> there is a _sane_ solution to the multiple id's exported for a single
> device problem.
> 

The only patch I presently have is:

--- devel/drivers/pnp/interface.c~pnp-modalias-sysfs-export	2006-03-12 03:27:01.000000000 -0800
+++ devel-akpm/drivers/pnp/interface.c	2006-03-12 03:27:01.000000000 -0800
@@ -459,10 +459,22 @@ static ssize_t pnp_show_current_ids(stru
 
 static DEVICE_ATTR(id,S_IRUGO,pnp_show_current_ids,NULL);
 
+static ssize_t pnp_modalias_show(struct device *dmdev, struct device_attribute *attr, char *buf)
+{
+	struct pnp_dev *dev = to_pnp_dev(dmdev);
+	struct pnp_id * pos = dev->id;
+
+	/* FIXME: modalias can only do one alias */
+	return sprintf(buf, "pnp:d%s\n", pos->id);
+}
+
+static DEVICE_ATTR(modalias,S_IRUGO,pnp_modalias_show,NULL);
+
 int pnp_interface_attach_device(struct pnp_dev *dev)
 {
 	device_create_file(&dev->dev,&dev_attr_options);
 	device_create_file(&dev->dev,&dev_attr_resources);
 	device_create_file(&dev->dev,&dev_attr_id);
+	device_create_file(&dev->dev,&dev_attr_modalias);
 	return 0;
 }
_

Is that OK?

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Sun, Mar 12, 2006 at 02:55:43PM -0800, Andrew Morton wrote:
> Kay Sievers <kay.sievers@vrfy.org> wrote:
> > On Sun, Mar 12, 2006 at 12:17:19PM +0100, Pierre Ossman wrote:
> > > Andrew Morton wrote:
> > > > I assume you mean that the drivers/pnp/card.c patch of
> > > > pnp-modalias-sysfs-export.patch needs to be removed and this patch applies
> > > > on top of the result.
> > > >
> > > > But I don't want to break udev.
> > > >   
> > > I suppose I wasn't entirely clear there. I'd like you to do the first
> > > part (remove the card.c part), but not apply this second patch. I just
> > > sent that in as a means of getting the ball rolling again.
> > 
> > Again, multiline sysfs modalias files are not going to happen. Find a
> > sane way to encode the list of devices into a single string, or don't do
> > it at all. And it must be available in the event environment too.
> > 
> > > The reason I'm pushing this issue is that Red Hat decided to drop all
> > > magical scripts that figured out what modules to load and instead only
> > > use the modalias attribute. They consider the right way to go is to get
> > > the PNP layer to export modalias, so that's what I'm trying to do.
> > 
> > There is no need to rush out with this half-baken solution. This simple
> > udev rule does the job for you, if you want pnp module autoloading with
> > the current kernel:
> >   SUBSYSTEM=="pnp", RUN+="/bin/sh -c 'while read id; do /sbin/modprobe pnp:d$$id; done < /sys$devpath/id'"
> > 
> > Andrew, please make sure, that this patch does not hit mainline until
> > there is a _sane_ solution to the multiple id's exported for a single
> > device problem.
> > 
> The only patch I presently have is:

> +	/* FIXME: modalias can only do one alias */
> +	return sprintf(buf, "pnp:d%s\n", pos->id);

> Is that OK?

No, it claims to export the modalias for the PnP device, but it is in
some cases (3 of 12 on my recent Laptop) not correct and we should not
have it at all until this issue is solved. If it would be _that_ easy,
it would have been long done, as several people already run into this
problem. :)
Let me explain it in detail, maybe someone has an idea how to solve that.

The problem is that we use the modalias value exported/created from the
kernel to lookup a matching kernel module name.

Kernel modules can contain entries from the module device table with
wildcard matches, which depmod collects and puts all of them into a
single file: /lib/modules/`uname -r`/modules.alias.

Now the enumerated/probed bus device instances export a modalias string
which contains the native id's of the bus. You can pass that string
directly to modprobe and modprobe will look up a matching driver for
that device and load the module.

That made the whole device <-> kernel module as easy as calling modprobe
once for every device the kernel creates, compared to the complex
userspace mess with map files we did in the past with shell scripts.
Here is a tg3 pci network card:
  $ cat /sys/bus/pci/devices/0000:02:00.0/modalias
  pci:v000014E4d0000167Dsv00001014sd00000577bc02sc00i00

  $ modprobe -v -n --first-time pci:v000014E4d0000167Dsv00001014sd00000577bc02sc00i00
  FATAL: Module tg3 already in kernel.

PnP, as some other buses, face the problem, that they don't have a
single identifier like a pci:$vendor-$product, or usb:$vendor-$product,
they can have multiple identifiers, which all have to be passed to
modprobe at once or one after the other. Exporting only one of these id's
is just wrong.
Here we see multiple id's for the same device:
  $ grep . /sys/bus/pnp/devices/*/id
  /sys/bus/pnp/devices/00:01/id:PNP0a08
  /sys/bus/pnp/devices/00:01/id:PNP0a03

  /sys/bus/pnp/devices/00:08/id:IBM0057
  /sys/bus/pnp/devices/00:08/id:PNP0f13

The problem is to represent multiple id's in a single string or find
another sane way to export multiple id's to userspace. Introducing
multiline values in sysfs for modalias is probably no the way to go
and if we really would want to do this, we need to prepare that
very carefully. There is currently already a PnP specific file called
"id", which is a multiline file and can be easily used to trick around
the problem without messing up "modalias".

Also at the same time we export a modalias file, we require a $MODALIAS
value in the event environment to be available, which is kind of ugly
for a multiline value.

Macio solved the problem by adding all devices to a single string and
let the device table match one of these id's in that single string:
  http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;hb=HEAD;f=drivers/macintosh/macio_sysfs.c#l42

We should first check if that is possible for PnP too, or solve that
problem in general at that level before we introduce such a hack.

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Adam Belay]

On Mon, Mar 13, 2006 at 05:14:58AM +0100, Kay Sievers wrote:
> 
> Macio solved the problem by adding all devices to a single string and
> let the device table match one of these id's in that single string:
>   http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;hb=HEAD;f=drivers/macintosh/macio_sysfs.c#l42
> 
> We should first check if that is possible for PnP too, or solve that
> problem in general at that level before we introduce such a hack.

I do have some concerns about merging every ID into a single string.  The
orginal design goal of having multiple IDs was to allow vendors to specify
a single high priority ID that a driver that supports the device's complete
feature set could match against.  If that driver is unavailable, it is
acceptable to search for other drivers that might match against a
compatibility ID and support a partial feature set.  Now if we just search
for the first driver that matches anything in a single ID string without
regard to the order IDs are presented, then we're not supporting the
specification.

More generally speaking, it seems to me there are four main options:

1.) We remove the modalias strings from all buses, and generate them in
userspace exclusively.  We may loose the ability to support new buses
without specialized userspace software, but we gain a great deal of
flexibility and can eventually implement more advanced hardware detection
schemes without depreciating existing kernel interfaces or parsing strings
that are limiting when compared to bus-specific data.  Also, at least we
have a uniform sysfs interface.

2.) We selectively export modalias strings on buses where this sort of
thing works and use hacks for other buses.

3.) We export multiline sysfs modalias attributes and tell userspace
hotplug developers that they're wrong and must change their assumptions.

4.) We export a single line modalias with each ID appended to the previous ID.
Userspace must pay careful attention to the order, but because the format is
bus-specific, it will have to be handled in a very specialized way. (e.g. PCI
has class codes, PnP has compatibility IDs, etc)

In the long run, I think option 1 is the best choice.  I'm more concerned with
flexibility than having a simplistic but limited hardware detection mechanism.
Also, I prefer to keep code out of the kernel when there isn't a clear
functionality advantage.  "file2alias" is not a kernel-level interface, but
rather implementation specific to modutils and various module scripts included
with the kernel source.  Therefore, I don't think that sysfs is obligated to be
specially tailored toward modprobe, even if it is convenient for some buses.

But I'm also interested in a practical short-term solution.  What are your
thoughts?  Would method #2 be acceptable?

Thanks,
Adam

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Mon, Mar 13, 2006 at 01:02:21AM -0500, Adam Belay wrote:
> On Mon, Mar 13, 2006 at 05:14:58AM +0100, Kay Sievers wrote:
> > 
> > Macio solved the problem by adding all devices to a single string and
> > let the device table match one of these id's in that single string:
> >   http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;hb=HEAD;f=drivers/macintosh/macio_sysfs.c#l42
> > 
> > We should first check if that is possible for PnP too, or solve that
> > problem in general at that level before we introduce such a hack.
> 
> I do have some concerns about merging every ID into a single string.  The
> orginal design goal of having multiple IDs was to allow vendors to specify
> a single high priority ID that a driver that supports the device's complete
> feature set could match against.  If that driver is unavailable, it is
> acceptable to search for other drivers that might match against a
> compatibility ID and support a partial feature set.  Now if we just search
> for the first driver that matches anything in a single ID string without
> regard to the order IDs are presented, then we're not supporting the
> specification.
> 
> More generally speaking, it seems to me there are four main options:
> 
> 1.) We remove the modalias strings from all buses, and generate them in
> userspace exclusively.  We may loose the ability to support new buses
> without specialized userspace software, but we gain a great deal of
> flexibility and can eventually implement more advanced hardware detection
> schemes without depreciating existing kernel interfaces or parsing strings
> that are limiting when compared to bus-specific data.  Also, at least we
> have a uniform sysfs interface.

This is what we are coming from. Just look at the input.agent in any older
installation and you may think twice about this. :) I'm all for having
that created by the kernel.

> 2.) We selectively export modalias strings on buses where this sort of
> thing works and use hacks for other buses.

This is what we have today, right? PnP does not have modalias at all for the
reason, we couldn't figure out how to do this. We can use the "id" file
just fine, even when it's kind of ugly.

> 3.) We export multiline sysfs modalias attributes and tell userspace
> hotplug developers that they're wrong and must change their assumptions.

I'm pretty sure, we don't want multiline values. How do you stick them
in the environment?

> 4.) We export a single line modalias with each ID appended to the previous ID.
> Userspace must pay careful attention to the order, but because the format is
> bus-specific, it will have to be handled in a very specialized way. (e.g. PCI
> has class codes, PnP has compatibility IDs, etc)

What's the problem you see? It's all about loading modules if a piece of
hardware appears, It's even completely uncritical to load a module that
does not do anything in the end, cause it decides not to bind to that
hardware.
If you want fine grained policy in userspace, just implement it matching
on the other values in sysfs (or whatever policy) before you run the
"default" brute-force "modprobe $MODALIAS". I don't see any problem with
that approach and having it work without any specific userspace setup is
very nice. You still have full control what you can do, cause the device event
still travels through udev/hotplug and you can do whatever a system decides
what's needed - it's not that a modalias values would make the kernel load
a module on its own.

> In the long run, I think option 1 is the best choice.  I'm more concerned with
> flexibility than having a simplistic but limited hardware detection mechanism.
> Also, I prefer to keep code out of the kernel when there isn't a clear
> functionality advantage.  "file2alias" is not a kernel-level interface, but
> rather implementation specific to modutils and various module scripts included
> with the kernel source.  Therefore, I don't think that sysfs is obligated to be
> specially tailored toward modprobe, even if it is convenient for some buses.

It's about making it "just work", even for currently unknown buses, which
is very nice.

> But I'm also interested in a practical short-term solution.  What are your
> thoughts?  Would method #2 be acceptable?

What do you have in mind? #2 is what we have today, right?

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Adam Belay]

On Mon, Mar 13, 2006 at 07:21:12AM +0100, Kay Sievers wrote:
> On Mon, Mar 13, 2006 at 01:02:21AM -0500, Adam Belay wrote:
> > On Mon, Mar 13, 2006 at 05:14:58AM +0100, Kay Sievers wrote:
> > > 
> > > Macio solved the problem by adding all devices to a single string and
> > > let the device table match one of these id's in that single string:
> > >   http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;hb=HEAD;f=drivers/macintosh/macio_sysfs.c#l42
> > > 
> > > We should first check if that is possible for PnP too, or solve that
> > > problem in general at that level before we introduce such a hack.
> > 
> > I do have some concerns about merging every ID into a single string.  The
> > orginal design goal of having multiple IDs was to allow vendors to specify
> > a single high priority ID that a driver that supports the device's complete
> > feature set could match against.  If that driver is unavailable, it is
> > acceptable to search for other drivers that might match against a
> > compatibility ID and support a partial feature set.  Now if we just search
> > for the first driver that matches anything in a single ID string without
> > regard to the order IDs are presented, then we're not supporting the
> > specification.
> > 
> > More generally speaking, it seems to me there are four main options:
> > 
> > 1.) We remove the modalias strings from all buses, and generate them in
> > userspace exclusively.  We may loose the ability to support new buses
> > without specialized userspace software, but we gain a great deal of
> > flexibility and can eventually implement more advanced hardware detection
> > schemes without depreciating existing kernel interfaces or parsing strings
> > that are limiting when compared to bus-specific data.  Also, at least we
> > have a uniform sysfs interface.
> 
> This is what we are coming from. Just look at the input.agent in any older
> installation and you may think twice about this. :) I'm all for having
> that created by the kernel.

There are certainly cleaner ways of making this happen.

> 
> > 2.) We selectively export modalias strings on buses where this sort of
> > thing works and use hacks for other buses.
> 
> This is what we have today, right? PnP does not have modalias at all for the
> reason, we couldn't figure out how to do this. We can use the "id" file
> just fine, even when it's kind of ugly.

Yes, exactly.

> 
> > 3.) We export multiline sysfs modalias attributes and tell userspace
> > hotplug developers that they're wrong and must change their assumptions.
> 
> I'm pretty sure, we don't want multiline values. How do you stick them
> in the environment?

I'm suggesting that sticking them in as an environmental variable might be
incorrect in the first place.

> 
> > 4.) We export a single line modalias with each ID appended to the previous ID.
> > Userspace must pay careful attention to the order, but because the format is
> > bus-specific, it will have to be handled in a very specialized way. (e.g. PCI
> > has class codes, PnP has compatibility IDs, etc)
> 
> What's the problem you see? It's all about loading modules if a piece of
> hardware appears, It's even completely uncritical to load a module that
> does not do anything in the end, cause it decides not to bind to that
> hardware.

But it's inefficient, and occasionally these modules will touch hardware they
don't actually support. (e.g. acpi PCI hotplug driver)

> If you want fine grained policy in userspace, just implement it matching
> on the other values in sysfs (or whatever policy) before you run the
> "default" brute-force "modprobe $MODALIAS". I don't see any problem with
> that approach and having it work without any specific userspace setup is
> very nice. You still have full control what you can do, cause the device event
> still travels through udev/hotplug and you can do whatever a system decides
> what's needed - it's not that a modalias values would make the kernel load
> a module on its own.

Sure, but we're still tailoring the kernel interface to a specific
userspace implementation rather than requiring usage of the already available
bus-specific interfaces.  I agree it makes things easier, but as a general
convention, I don't think a specific userspace implementation should dictate
the kernel interface, especially when it doesn't fit well with some buses.

> 
> > In the long run, I think option 1 is the best choice.  I'm more concerned with
> > flexibility than having a simplistic but limited hardware detection mechanism.
> > Also, I prefer to keep code out of the kernel when there isn't a clear
> > functionality advantage.  "file2alias" is not a kernel-level interface, but
> > rather implementation specific to modutils and various module scripts included
> > with the kernel source.  Therefore, I don't think that sysfs is obligated to be
> > specially tailored toward modprobe, even if it is convenient for some buses.
> 
> It's about making it "just work", even for currently unknown buses, which
> is very nice.

With the increased popularity of userspace drivers, I think we're eventually
going to need some driver matching infustructure in userspace anyway.  If we
move away from loading modules as a means of binding drivers to devices, and
allow userspace to match drivers to specific hardware, then we can support
really useful features like configuration caches that maintain a list of
hardware detected during the last boot-up.  This would reduce boot time and
allow for persistent configuration of driver settings for each device
instance.  Also we could support driver priorities (e.g. discriminating
between drivers that partially support a feature set and drivers that
completely support a feature set).  This is currently not possible in
kernel-space.  Finally, a fair ammount of complexity could be moved out of
the kernel.  I think these sorts of things would go a long way toward the
"just works" factor.

> 
> > But I'm also interested in a practical short-term solution.  What are your
> > thoughts?  Would method #2 be acceptable?
> 
> What do you have in mind? #2 is what we have today, right?

Yes, I think it may be a workable immediate solution.

Thanks,
Adam

Re: [PATCH] [PNP] 'modalias' sysfs export

[Adam Belay]

I did some research, and interestingly enough, the ACPI _CID method allows
for compatible IDs even for PCI devices.  These also would present a problem
for the modalias sysfs attribute.

Thanks,
Adam

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Mon, Mar 13, 2006 at 02:26:54AM -0500, Adam Belay wrote:
> I did some research, and interestingly enough, the ACPI _CID method allows
> for compatible IDs even for PCI devices.  These also would present a problem
> for the modalias sysfs attribute.

Again, you can do every "advanced" setup already today with poking
around in the bind/unbind files in sysfs. Userspace just receives an
event from the kernel and can do whatever it wants to do with the event:
ignore it, load a specific module, start a userspace driver, or just ask
modprobe to load the kernel supplied default module.

The modalias is just a convenient way to provide a "default" module
autoloading and is not expected to become a system management
replacement with full featured policy integration. I don't really see
a "real world" problem here. If some day we support this stuff and need
a new interface we can just do this if someone proposes a better
solution. For now modalias works just fine. As long as we have device
table matches _in_ the kernel modules, there is no reason not to export
the match value from the kernel at the same time.

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Bill Nottingham]

Kay Sievers (kay.sievers@vrfy.org) said: 
> There is no need to rush out with this half-baken solution. This simple
> udev rule does the job for you, if you want pnp module autoloading with
> the current kernel:
>   SUBSYSTEM=="pnp", RUN+="/bin/sh -c 'while read id; do /sbin/modprobe pnp:d$$id; done < /sys$devpath/id'"

See:
  https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178998

This doesn't work for everything.

Bill

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Mon, Mar 13, 2006 at 11:57:19AM -0500, Bill Nottingham wrote:
> Kay Sievers (kay.sievers@vrfy.org) said: 
> > There is no need to rush out with this half-baken solution. This simple
> > udev rule does the job for you, if you want pnp module autoloading with
> > the current kernel:
> >   SUBSYSTEM=="pnp", RUN+="/bin/sh -c 'while read id; do /sbin/modprobe pnp:d$$id; done < /sys$devpath/id'"
> 
> See:
>   https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178998
> 
> This doesn't work for everything.

Sure not, and I don't think "everything" in PnP will ever work. :) But
it does the same as the modalias patch to the kernel we are talking about.
There are device table entries completely missing and some other don't
match. Some of them can be fixed by adding the aliases as modprobe.conf
entries.

Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Bill Nottingham]

Kay Sievers (kay.sievers@vrfy.org) said: 
> > See:
> >   https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178998
> > 
> > This doesn't work for everything.
> 
> Sure not, and I don't think "everything" in PnP will ever work. :) But
> it does the same as the modalias patch to the kernel we are talking about.
> There are device table entries completely missing and some other don't
> match. Some of them can be fixed by adding the aliases as modprobe.conf
> entries.

Well, it's just that if this is the solution proposed, I'd like it if
it worked for any of the people who are seeing problems - in our bugs,
it hasn't helped any of them yet.

Bill

Re: [PATCH] [PNP] 'modalias' sysfs export

[Adam Belay]

On Mon, Mar 13, 2006 at 08:36:12AM +0100, Kay Sievers wrote:
> On Mon, Mar 13, 2006 at 02:26:54AM -0500, Adam Belay wrote:
> > I did some research, and interestingly enough, the ACPI _CID method allows
> > for compatible IDs even for PCI devices.  These also would present a problem
> > for the modalias sysfs attribute.
> 
> Again, you can do every "advanced" setup already today with poking
> around in the bind/unbind files in sysfs. Userspace just receives an
> event from the kernel and can do whatever it wants to do with the event:
> ignore it, load a specific module, start a userspace driver, or just ask
> modprobe to load the kernel supplied default module.
> 
> The modalias is just a convenient way to provide a "default" module
> autoloading and is not expected to become a system management
> replacement with full featured policy integration. I don't really see
> a "real world" problem here. If some day we support this stuff and need
> a new interface we can just do this if someone proposes a better
> solution. For now modalias works just fine. As long as we have device
> table matches _in_ the kernel modules, there is no reason not to export
> the match value from the kernel at the same time.
> 
> Thanks,
> Kay

Alright, I was just trying to make it clear that there are minor problems
with hotplug and some aspects of ACPI, PCMCIA, PNP, etc.  Some of the
exceptions (e.g. multiple PnP IDs) are more common than others (e.g. ACPI
_CID methods for non-acpi devices).  Also, some aren't difficult to work
around with things like the script that parses the "id" attribute.  I'm
interested in seeing how future solutions might attempt to implement these
features, even the corner cases.  I agree, though, that having a simplistic
default like the modalias approach has an important "real world" advantage.

Thanks,
Adam

Re: [PATCH] [PNP] 'modalias' sysfs export

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 4477 bytes --]

On Mon, 13 Mar 2006 17:26:44 -0500 Bill Nottingham wrote:

> Kay Sievers (kay.sievers@vrfy.org) said: 
> > > See:
> > >   https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178998

I have written a comment in that bugzilla entry, but it is probably too
terse, so I'll try to explain the problem and my proposed solution
better.

> > > This doesn't work for everything.
> > 
> > Sure not, and I don't think "everything" in PnP will ever work. :) But
> > it does the same as the modalias patch to the kernel we are talking about.
> > There are device table entries completely missing and some other don't
> > match. Some of them can be fixed by adding the aliases as modprobe.conf
> > entries.
> 
> Well, it's just that if this is the solution proposed, I'd like it if
> it worked for any of the people who are seeing problems - in our bugs,
> it hasn't helped any of them yet.

There are two kinds of PNP drivers:

1) PNP device drivers, which use struct pnp_driver.  These drivers use
   struct pnp_device_id in id_table entries; struct pnp_device_id
   contains a single device ID field which is used for matching.
   Aliases for these drivers have the form:

	pnp:dXXXYYYY*

   where XXXYYYY is the PNP ID which is matched.

2) PNP card drivers, which use struct pnp_card_driver.  These drivers
   use struct pnp_card_device_id in id_table entries; struct
   pnp_card_device_id contains ID for the card itself and a variable
   number of logical device IDs.  drivers/pnp/card.c:match_card() uses
   these rules for matching struct pnp_card_device_id to a device:

    a) the card IDs must match;
    b) all device IDs mentioned in struct pnp_card_device_id must be
       present in the card, but can be in any order (and there may be
       more devices than listed in the ID table).

   Aliases for card drivers currently have the form:

	pnp:cXXXYYYYdXXXYYYYdXXXYYYY*

   The first "cXXXYYYY" part is the card ID, and "dXXXYYYY" parts are
   device IDs (there may be up to PNP_MAX_DEVICES == 8 of them).

Now, for the drivers of the first type the only problem is that the
devices can have several compatible IDs in addition to the primary ID,
and this requires either a multiline "modalias" attribute, or a helper
script to call modprobe multiple times with the pnp:dXXXYYYY alias for
all available IDs.  

Drivers of the second type - PNP card drivers - are only used for isapnp
(pnpbios and pnpacpi have only plain devices).  Cards itself have only a
single ID (there are no compatible IDs for cards), but every logical
device on a card can have up to DEVICE_COUNT_COMPATIBLE == 4 compatible
IDs in addition to the primary ID.

(BTW, #define DEVICE_COUNT_COMPATIBLE 4 is duplicated in <linux/pci.h>
and <linux/isapnp.h> - if these values were different, it would be a
nasty bug.)

For the card drivers, in addition to the problem with compatible IDs, we
have another problem - the alias format for them is wrong!  The problem
is that if device IDs in the alias happen to be in a different order
than the same IDs in the actual device (or even in the same order, but
some devices are not mentioned in the ID table), fnmatch() used by
modprobe will not match this alias.

To solve this problem, I suggest to do this:

1) Change the alias format for PNP card drivers to require logical
   device IDs to be sorted, and add an "*" before every device ID part.
   The alias format becomes:

	pnp:cXXXYYYY*dXXXYYYY*dXXXYYYY*

2) Update scripts/mod/file2alias.c:do_pnp_card_entry() to write the new
   alias format (it should sort device IDs - no need to change all
   drivers to list device IDs in sorted order and create potential for
   bugs when someone adds a non-sorted entry).

3) Update /etc/udev/isapnp script mentioned in bugzilla entry to sort
   device ID values before concatenating them.

After dust settles, we can then add "modalias" attribute generation for
PNP card devices to the kernel - note that this attribute would have
only a single value which would list the card ID and all (primary and
compatible) IDs of all logical devices in sorted order.

BTW, we can change the alias format for PNP device drivers to

	pnp:*dXXXYYYY*

(note the additional "*" before the device ID).  This would allow us to
have a single-value "modalias" attribute for PNP logical devices too -
it would have the form

	pnp:dXXXYYYYdXXXYYYYdXXXYYYY

(listing all IDs, in this case sorting is not required, because each
driver will match at most only a single dXXXYYYY entry).

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [PNP] 'modalias' sysfs export

[Pierre Ossman]

Sergey Vlasov wrote:
> BTW, we can change the alias format for PNP device drivers to
>
> 	pnp:*dXXXYYYY*
>
> (note the additional "*" before the device ID).  This would allow us to
> have a single-value "modalias" attribute for PNP logical devices too -
> it would have the form
>
> 	pnp:dXXXYYYYdXXXYYYYdXXXYYYY
>
> (listing all IDs, in this case sorting is not required, because each
> driver will match at most only a single dXXXYYYY entry).
>   

How do you guarantee that the modules are tried in the correct order? Is
it well defined in modprobe that pnp:*dABC0001* would match before
pnp:*dXYZ0001* if the modalias is pnp:dABC0001dXYZ0001 ?

Rgds
Pierre

Re: [PATCH] [PNP] 'modalias' sysfs export

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

On Tue, Mar 14, 2006 at 01:47:47PM +0100, Pierre Ossman wrote:
> Sergey Vlasov wrote:
> > BTW, we can change the alias format for PNP device drivers to
> >
> > 	pnp:*dXXXYYYY*
> >
> > (note the additional "*" before the device ID).  This would allow us to
> > have a single-value "modalias" attribute for PNP logical devices too -
> > it would have the form
> >
> > 	pnp:dXXXYYYYdXXXYYYYdXXXYYYY
> >
> > (listing all IDs, in this case sorting is not required, because each
> > driver will match at most only a single dXXXYYYY entry).
> >   
> 
> How do you guarantee that the modules are tried in the correct order? Is
> it well defined in modprobe that pnp:*dABC0001* would match before
> pnp:*dXYZ0001* if the modalias is pnp:dABC0001dXYZ0001 ?

No, the order is undefined.  However, defining it will not really help -
what if there is another similar device in the system, which is discovered
earlier and brings in the generic driver before the second device is
considered?  In this case defining the module load order buys you nothing.
Currently the only reliable solution to prevent a generic driver from
driving a device which has a more specific driver is to blacklist the
problematic device in the generic driver (e.g., usbhid has lots of
blacklist entries because vendors like to abuse the HID class).

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [PNP] 'modalias' sysfs export

[Pozsar Balazs]

On Tue, Mar 14, 2006 at 03:29:44PM +0300, Sergey Vlasov wrote:
> On Mon, 13 Mar 2006 17:26:44 -0500 Bill Nottingham wrote:
> 
> > Kay Sievers (kay.sievers@vrfy.org) said: 
> > > > See:
> > > >   https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178998
> 
> I have written a comment in that bugzilla entry, but it is probably too
> terse, so I'll try to explain the problem and my proposed solution
> better.
> 
> > > > This doesn't work for everything.
> > > 
> > > Sure not, and I don't think "everything" in PnP will ever work. :) But
> > > it does the same as the modalias patch to the kernel we are talking about.
> > > There are device table entries completely missing and some other don't
> > > match. Some of them can be fixed by adding the aliases as modprobe.conf
> > > entries.
> > 
> > Well, it's just that if this is the solution proposed, I'd like it if
> > it worked for any of the people who are seeing problems - in our bugs,
> > it hasn't helped any of them yet.
> 
> There are two kinds of PNP drivers:
> 
> 1) PNP device drivers, which use struct pnp_driver.  These drivers use
>    struct pnp_device_id in id_table entries; struct pnp_device_id
>    contains a single device ID field which is used for matching.
>    Aliases for these drivers have the form:
> 
> 	pnp:dXXXYYYY*
> 
>    where XXXYYYY is the PNP ID which is matched.
> 
> 2) PNP card drivers, which use struct pnp_card_driver.  These drivers
>    use struct pnp_card_device_id in id_table entries; struct
>    pnp_card_device_id contains ID for the card itself and a variable
>    number of logical device IDs.  drivers/pnp/card.c:match_card() uses
>    these rules for matching struct pnp_card_device_id to a device:
> 
>     a) the card IDs must match;
>     b) all device IDs mentioned in struct pnp_card_device_id must be
>        present in the card, but can be in any order (and there may be
>        more devices than listed in the ID table).
> 
>    Aliases for card drivers currently have the form:
> 
> 	pnp:cXXXYYYYdXXXYYYYdXXXYYYY*
> 
>    The first "cXXXYYYY" part is the card ID, and "dXXXYYYY" parts are
>    device IDs (there may be up to PNP_MAX_DEVICES == 8 of them).
> 
> Now, for the drivers of the first type the only problem is that the
> devices can have several compatible IDs in addition to the primary ID,
> and this requires either a multiline "modalias" attribute, or a helper
> script to call modprobe multiple times with the pnp:dXXXYYYY alias for
> all available IDs.  
> 
> Drivers of the second type - PNP card drivers - are only used for isapnp
> (pnpbios and pnpacpi have only plain devices).  Cards itself have only a
> single ID (there are no compatible IDs for cards), but every logical
> device on a card can have up to DEVICE_COUNT_COMPATIBLE == 4 compatible
> IDs in addition to the primary ID.
> 
> (BTW, #define DEVICE_COUNT_COMPATIBLE 4 is duplicated in <linux/pci.h>
> and <linux/isapnp.h> - if these values were different, it would be a
> nasty bug.)
> 
> For the card drivers, in addition to the problem with compatible IDs, we
> have another problem - the alias format for them is wrong!  The problem
> is that if device IDs in the alias happen to be in a different order
> than the same IDs in the actual device (or even in the same order, but
> some devices are not mentioned in the ID table), fnmatch() used by
> modprobe will not match this alias.
> 
> To solve this problem, I suggest to do this:
> 
> 1) Change the alias format for PNP card drivers to require logical
>    device IDs to be sorted, and add an "*" before every device ID part.
>    The alias format becomes:
> 
> 	pnp:cXXXYYYY*dXXXYYYY*dXXXYYYY*
> 
> 2) Update scripts/mod/file2alias.c:do_pnp_card_entry() to write the new
>    alias format (it should sort device IDs - no need to change all
>    drivers to list device IDs in sorted order and create potential for
>    bugs when someone adds a non-sorted entry).
> 
> 3) Update /etc/udev/isapnp script mentioned in bugzilla entry to sort
>    device ID values before concatenating them.
> 
> After dust settles, we can then add "modalias" attribute generation for
> PNP card devices to the kernel - note that this attribute would have
> only a single value which would list the card ID and all (primary and
> compatible) IDs of all logical devices in sorted order.
> 
> BTW, we can change the alias format for PNP device drivers to
> 
> 	pnp:*dXXXYYYY*
> 
> (note the additional "*" before the device ID).  This would allow us to
> have a single-value "modalias" attribute for PNP logical devices too -
> it would have the form
> 
> 	pnp:dXXXYYYYdXXXYYYYdXXXYYYY
> 
> (listing all IDs, in this case sorting is not required, because each
> driver will match at most only a single dXXXYYYY entry).


Hi all,

Sorry for the long quotation, but I think you might have forgotten what 
this thread was about.

Basically I implemented the above things, to be precise:
 - the alias for the pnp device drivers are in the form "pnp:*dXXXYYYY*" 
   instead of the old "pnp:dXXXYYYY*"
 - the alias for the pnp card drivers are in the form
     "pnp:cXXXYYYY*dXXXYYYY*dXXXYYYY*"
   instead of the old
     "pnp:cXXXYYYYdXXXYYYYdXXXYYYY*"
   _and_ the device id part are ordered
 - add a "modalias" file under sysfs for each pnp device, containing
     "pnp:dXXXYYYYdXXXYYYY..."
   where "dXXXYYYY" is appended for each pnp id the device has
 - add a "modalias" file under sysfs for each pnp card, containing
     "pnp:cXXXYYYYdXXXYYYYdXXXYYYY..."
   where "cXXXYYYY" is the card_id, and the device ids are appended 
   after it, _ordered_.


With this applied, I think we are close to be able to drop 
special-casing the pnp bus in udev rules.

What still needs to be done is exporting the MODALIAS env variable.
(Sorry, I do not see how it could be added elegantly.)

Please tell me what you think of it.

thanks,
pozsy



diff -urd a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
--- a/scripts/mod/file2alias.c	2006-05-02 23:38:44.000000000 +0200
+++ b/scripts/mod/file2alias.c	2006-05-08 21:57:33.000000000 +0200
@@ -273,21 +273,31 @@
 static int do_pnp_entry(const char *filename,
 			struct pnp_device_id *id, char *alias)
 {
-	sprintf(alias, "pnp:d%s", id->id);
+	sprintf(alias, "pnp:*d%s", id->id);
 	return 1;
 }
 
+static int _compare_pnp_id(const void *a, const void *b)
+{
+	const char *aa = a;
+	const char *bb = b;
+	return strcmp(aa, bb);
+}
+
 /* looks like: "pnp:cCdD..." */
 static int do_pnp_card_entry(const char *filename,
 			struct pnp_card_device_id *id, char *alias)
 {
-	int i;
+	int i, j;
 
 	sprintf(alias, "pnp:c%s", id->id);
-	for (i = 0; i < PNP_MAX_DEVICES; i++) {
-		if (! *id->devs[i].id)
-			break;
-		sprintf(alias + strlen(alias), "d%s", id->devs[i].id);
+	for (j = 0; j < PNP_MAX_DEVICES; j++) {
+		if (! *id->devs[j].id)
+		    break;
+	}
+	qsort(id->devs, j, sizeof(id->devs[0]), _compare_pnp_id);
+	for (i = 0; i < j; i++) {
+		sprintf(alias + strlen(alias), "*d%s", id->devs[i].id);
 	}
 	return 1;
 }
diff -Naurd a/drivers/pnp/card.c b/drivers/pnp/card.c
--- a/drivers/pnp/card.c	2006-05-02 23:38:44.000000000 +0200
+++ b/drivers/pnp/card.c	2006-05-09 19:25:08.000000000 +0200
@@ -8,6 +8,7 @@
 #include <linux/config.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/sort.h>
 #include <linux/pnp.h>
 #include "base.h"
 
@@ -159,10 +160,34 @@
 
 static DEVICE_ATTR(card_id,S_IRUGO,pnp_show_card_ids,NULL);
 
+static ssize_t pnp_card_modalias_show(struct device *dmdev, struct device_attribute *attr, char *buf)
+{
+	char *str = buf;
+	struct pnp_card *card = to_pnp_card(dmdev);
+	struct pnp_id * pos = card->id;
+	struct pnp_dev *dev;
+	int i, count = 0;
+	char ids[PNP_MAX_DEVICES][PNP_ID_LEN];
+
+	card_for_each_dev(card, dev) {
+		strcpy(&ids[count++], dev->id);
+	}
+	sort(ids, count, PNP_ID_LEN, strcmp, NULL);
+	str += sprintf(str, "pnp:c%s", pos->id);
+	for (i = 0; i < count; i++) {
+		str += sprintf(str, "d%s", ids[i]);
+	}
+	str += sprintf(str, "\n");
+	return (str - buf);
+}
+
+static DEVICE_ATTR(modalias,S_IRUGO,pnp_card_modalias_show,NULL);
+
 static int pnp_interface_attach_card(struct pnp_card *card)
 {
 	device_create_file(&card->dev,&dev_attr_name);
 	device_create_file(&card->dev,&dev_attr_card_id);
+	device_create_file(&card->dev,&dev_attr_modalias);
 	return 0;
 }
 
diff -Naurd a/drivers/pnp/interface.c b/drivers/pnp/interface.c
--- a/drivers/pnp/interface.c	2006-05-02 23:38:44.000000000 +0200
+++ b/drivers/pnp/interface.c	2006-05-09 19:25:08.000000000 +0200
@@ -459,10 +459,28 @@
 
 static DEVICE_ATTR(id,S_IRUGO,pnp_show_current_ids,NULL);
 
+static ssize_t pnp_modalias_show(struct device *dmdev, struct device_attribute *attr, char *buf)
+{
+	char *str = buf;
+	struct pnp_dev *dev = to_pnp_dev(dmdev);
+	struct pnp_id * pos = dev->id;
+
+	str += sprintf(str, "pnp:");
+	while (pos) {
+		str += sprintf(str,"d%s", pos->id);
+		pos = pos->next;
+	}
+	str += sprintf(str, "\n");
+	return (str - buf);
+}
+
+static DEVICE_ATTR(modalias,S_IRUGO,pnp_modalias_show,NULL);
+
 int pnp_interface_attach_device(struct pnp_dev *dev)
 {
 	device_create_file(&dev->dev,&dev_attr_options);
 	device_create_file(&dev->dev,&dev_attr_resources);
 	device_create_file(&dev->dev,&dev_attr_id);
+	device_create_file(&dev->dev,&dev_attr_modalias);
 	return 0;
 }



-- 
pozsy

Re: [PATCH] [PNP] 'modalias' sysfs export

[Kay Sievers]

On Tue, May 09, 2006 at 07:41:44PM +0200, Pozsar Balazs wrote:

> Basically I implemented the above things, to be precise:
>  - the alias for the pnp device drivers are in the form "pnp:*dXXXYYYY*" 
>    instead of the old "pnp:dXXXYYYY*"
>  - the alias for the pnp card drivers are in the form
>      "pnp:cXXXYYYY*dXXXYYYY*dXXXYYYY*"
>    instead of the old
>      "pnp:cXXXYYYYdXXXYYYYdXXXYYYY*"
>    _and_ the device id part are ordered
>  - add a "modalias" file under sysfs for each pnp device, containing
>      "pnp:dXXXYYYYdXXXYYYY..."
>    where "dXXXYYYY" is appended for each pnp id the device has
>  - add a "modalias" file under sysfs for each pnp card, containing
>      "pnp:cXXXYYYYdXXXYYYYdXXXYYYY..."
>    where "cXXXYYYY" is the card_id, and the device ids are appended 
>    after it, _ordered_.
> 
> 
> With this applied, I think we are close to be able to drop 
> special-casing the pnp bus in udev rules.

That looks promising.

> What still needs to be done is exporting the MODALIAS env variable.
> (Sorry, I do not see how it could be added elegantly.)

Yeah, we really want the environment variable. You may do the composition
of the string in a separate function, that just writes to a bufferi, and use
it to fill the buffer of the uevent environment and the page of the sysfs
attribute. Like we did for input:
  http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=bd37e5a951ad2123d3f51f59c407b5242946b6ba


Thanks,
Kay

Re: [PATCH] [PNP] 'modalias' sysfs export

[Andrey Borzenkov]

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

> Arjan van de Ven wrote:
> > On Sat, 2006-03-11 at 17:05 +0100, Pierre Ossman wrote:
> >> Here is a patch for doing multi line modalias for PNP devices. This will
> >> break udev, so that needs to be updated first.
> >
> > how could this EVER be acceptable???
>
> Soon I would hope. The modalias attribute currently only supports one
> alias (i.e. one line). This isn't enough for PNP, so if we want to
> support that bus (which I assume we do) we need to extend the interface.
> udev could be updated and be backwards compatible, the kernel can not
> (excluding adding another interface to the same data). So this patch
> should lag the update to udev a bit (i.e. I'm not suggesting it be
> applied now).

actually it is not that much udev but modprobe issue and modprobe already 
supports multiple modules on command line (modprobe --all, module-init-tools 
3.3.2 that I have here). So assuming module aliases cannot have embedded 
spaces and udev properly space-splits command line (I have not checked, but 
it should be the case IIRC) udev simply has to use 'modprobe --all $modalias' 
to be compatible with this patch. It also remains backwards compatible with 
single-alias modalias.

Or do I miss something obvious here? I understand that alternative is to make 
every alias appear as separate device in sysfs, but I do not know PNP 
structure well enough to decide if it makes sense.

- -andrey
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (GNU/Linux)

iD8DBQFEEwPNR6LMutpd94wRAhpTAJ9DQ6gj4SM+6Arxqxb3hM5PA01cHACgjZQs
yrONSgp3+TAo1p2qzR1tAHg=
=eq0n
-----END PGP SIGNATURE-----