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

[Pozsar Balazs <pozsy@uhulinux.hu>]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/