file2alias - incorrect? aliases for USB

file2alias - incorrect? aliases for USB

[Andrey Borzenkov]

file2aliases puts in alias device ID high and low numbers directly from match 
specifications. E.g. for this match table entry:

usb-storage          0x000f      0x04e6   0x0006    0x0100       0x0205 ...

it generates alias

alias usb:v04E6p0006dl0100dh0205dc*dsc*dp*ic*isc*ip* usb_storage

unfortunately real device attribute does not include high and low - rather it 
has single device ID (as part of PRODUCT) that should be contained in these 
bounds:

        length += snprintf (scratch, buffer_size - length, "PRODUCT=%x/%x/%x",
                            usb_dev->descriptor.idVendor,
                            usb_dev->descriptor.idProduct,
                            usb_dev->descriptor.bcdDevice);

or bcdDevice file in sysfs.

This makes those aliases rather useless for the purpose of matching reported 
device. It may take the same route as PCI and reject all device ID table 
entries that have High != Low but there are quite a few of them available.

I am rather confused because I do not see how this condition (low <= bcdDevice 
<= high) can be expressed using simple glob pattern (unless we are going to 
take glob library from Zsh :)

thank you

-andrey

Re: file2alias - incorrect? aliases for USB

[Greg KH]

On Sun, Nov 09, 2003 at 09:55:19PM +0300, Andrey Borzenkov wrote:
> file2aliases puts in alias device ID high and low numbers directly from match 
> specifications. E.g. for this match table entry:
> 
> usb-storage          0x000f      0x04e6   0x0006    0x0100       0x0205 ...
> 
> it generates alias
> 
> alias usb:v04E6p0006dl0100dh0205dc*dsc*dp*ic*isc*ip* usb_storage
> 
> unfortunately real device attribute does not include high and low - rather it 
> has single device ID (as part of PRODUCT) that should be contained in these 
> bounds:
> 
>         length += snprintf (scratch, buffer_size - length, "PRODUCT=%x/%x/%x",
>                             usb_dev->descriptor.idVendor,
>                             usb_dev->descriptor.idProduct,
>                             usb_dev->descriptor.bcdDevice);
> 
> or bcdDevice file in sysfs.
> 
> This makes those aliases rather useless for the purpose of matching reported 
> device. It may take the same route as PCI and reject all device ID table 
> entries that have High != Low but there are quite a few of them available.
> 
> I am rather confused because I do not see how this condition (low <= bcdDevice 
> <= high) can be expressed using simple glob pattern (unless we are going to 
> take glob library from Zsh :)

I would suggest just ignoring the bcdDevice value, and loading all
modules that match the idVendor and idProduct values, and let the kernel
sort it out :)

So for your example, you would just:
	modprobe usb:v04E6p0006dl*dh*dc*dsc*dp*ic*isc*ip* 

Hm, but that's no good either, because the visor driver trips over that
with its entry:
	MODULE_ALIAS("usb:v*p*dl*dh*dc*dsc*dp*ic*isc*ip*");
and the improper module is loaded.  That needs to be fixed up...

Rusty, any reason why the module alias code is turning an empty
MODULE_PARAM structure, as is declared in drivers/usb/serial/visor.c
with the line:
        { },                                    /* optional parameter entry */ 

Into the above MODULE_ALIAS?  I don't think that's correct.

thanks,

greg k-h

Re[2]: file2alias - incorrect? aliases for USB

["Andrey Borzenkov"]

-----Original Message-----

> 
> On Sun, Nov 09, 2003 at 09:55:19PM +0300, Andrey Borzenkov wrote:
> > file2aliases puts in alias device ID high and low numbers directly from match 
> > specifications. E.g. for this match table entry:
> > 
> > usb-storage          0x000f      0x04e6   0x0006    0x0100       0x0205 ...
> > 
> > it generates alias
> > 
> > alias usb:v04E6p0006dl0100dh0205dc*dsc*dp*ic*isc*ip* usb_storage
> > 
> > unfortunately real device attribute does not include high and low - rather it 
> > has single device ID (as part of PRODUCT) that should be contained in these 
> > bounds:
> > 
> >         length += snprintf (scratch, buffer_size - length, "PRODUCT=%x/%x/%x",
> >                             usb_dev->descriptor.idVendor,
> >                             usb_dev->descriptor.idProduct,
> >                             usb_dev->descriptor.bcdDevice);
> > 
> > or bcdDevice file in sysfs.
> > 
> > This makes those aliases rather useless for the purpose of matching reported 
> > device. It may take the same route as PCI and reject all device ID table 
> > entries that have High != Low but there are quite a few of them available.
> > 
> > I am rather confused because I do not see how this condition (low <= bcdDevice 
> > <= high) can be expressed using simple glob pattern (unless we are going to 
> > take glob library from Zsh :)
> 
> I would suggest just ignoring the bcdDevice value, and loading all
> modules that match the idVendor and idProduct values, and let the kernel
> sort it out :)
> 
> So for your example, you would just:
> 	modprobe usb:v04E6p0006dl*dh*dc*dsc*dp*ic*isc*ip* 
> 

any reason we put in alias fields that apparently won't be used
at all?

> Hm, but that's no good either, because the visor driver trips over that
> with its entry:
> 	MODULE_ALIAS("usb:v*p*dl*dh*dc*dsc*dp*ic*isc*ip*");
> and the improper module is loaded.  That needs to be fixed up...
> 
> Rusty, any reason why the module alias code is turning an empty
> MODULE_PARAM structure, as is declared in drivers/usb/serial/visor.c
> with the line:
>         { },                                    /* optional parameter entry */ 
> 
> Into the above MODULE_ALIAS?  I don't think that's correct.
> 

Subject:  [PATCH][2.6.0-test9] prevent catch-all USB aliases in modules.alias

http://marc.theaimsgroup.com/?l=linux-kernel&m=106787897124700&w=2

regards

-andrey

Re: file2alias - incorrect? aliases for USB

[Greg KH]

On Mon, Nov 10, 2003 at 01:26:39PM +0300, "Andrey Borzenkov"  wrote:
> > I would suggest just ignoring the bcdDevice value, and loading all
> > modules that match the idVendor and idProduct values, and let the kernel
> > sort it out :)
> > 
> > So for your example, you would just:
> > 	modprobe usb:v04E6p0006dl*dh*dc*dsc*dp*ic*isc*ip* 
> > 
> 
> any reason we put in alias fields that apparently won't be used
> at all?

Hm, don't know.  I didn't think of these as ranges, which some of the
fields are.  Possibly we might want to drop those variables from the
strings.

> > Hm, but that's no good either, because the visor driver trips over that
> > with its entry:
> > 	MODULE_ALIAS("usb:v*p*dl*dh*dc*dsc*dp*ic*isc*ip*");
> > and the improper module is loaded.  That needs to be fixed up...
> > 
> > Rusty, any reason why the module alias code is turning an empty
> > MODULE_PARAM structure, as is declared in drivers/usb/serial/visor.c
> > with the line:
> >         { },                                    /* optional parameter entry */ 
> > 
> > Into the above MODULE_ALIAS?  I don't think that's correct.
> > 
> 
> Subject:  [PATCH][2.6.0-test9] prevent catch-all USB aliases in modules.alias
> 
> http://marc.theaimsgroup.com/?l=linux-kernel&m=106787897124700&w=2

Ah, missed that.  Care to send it to Rusty?  It looks like a simple,
needed fix.

thanks,

greg k-h

Re: file2alias - incorrect? aliases for USB

[Rusty Russell]

In message <20031110093703.GA5449@kroah.com> you write:
> Hm, but that's no good either, because the visor driver trips over that
> with its entry:
> 	MODULE_ALIAS("usb:v*p*dl*dh*dc*dsc*dp*ic*isc*ip*");
> and the improper module is loaded.  That needs to be fixed up...
> 
> Rusty, any reason why the module alias code is turning an empty
> MODULE_PARAM structure, as is declared in drivers/usb/serial/visor.c
> with the line:
>         { },                                    /* optional parameter entry */ 
> 
> Into the above MODULE_ALIAS?  I don't think that's correct.

Sorry for the delay, was away and am catching up on mail.

Thanks to Andrey for the fix.  Greg, did you want to add something
else?  Either way, please forward to Linus.

Andrey: the reason everything is in there is I didn't know what Greg
wanted.  He OK'd it, but I'm happy for them to be trimmed, too.

Subject:  [PATCH][2.6.0-test9] prevent catch-all USB aliases in modules.alias
From:     Andrey Borzenkov
Date:     2003-11-03 16:45:03

visor.c defines one empty slot in USB ids table that can be filled in at 
runtime using module parameters. file2alias generates catch-all alias for it:

alias usb:v*p*dl*dh*dc*dsc*dp*ic*isc*ip* visor

patch adds the same sanity check as in depmod to scripts/file2alias.

regards

-andrey

*["2.6.0-test9-file2alias_visor.patch" (text/x-diff)]*

--- ../tmp/linux-2.6.0-test9/scripts/file2alias.c	2003-07-27 22:29:49.000000000 +0400
+++ linux-2.6.0-test9/scripts/file2alias.c	2003-11-03 19:40:47.744746456 +0300
@@ -52,6 +52,13 @@ static int do_usb_entry(const char *file
 	id->bcdDevice_lo = TO_NATIVE(id->bcdDevice_lo);
 	id->bcdDevice_hi = TO_NATIVE(id->bcdDevice_hi);
 
+	/*
+	 * Some modules (visor) have empty slots as placeholder for
+	 * run-time specification that results in catch-all alias
+	 */
+	if (!(id->idVendor | id->bDeviceClass | id->bInterfaceClass))
+		return 1;
+
 	strcpy(alias, "usb:");
 	ADD(alias, "v", id->match_flags&USB_DEVICE_ID_MATCH_VENDOR,
 	    id->idVendor);

--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: file2alias - incorrect? aliases for USB

["Andrey Borzenkov"]

> 
> Thanks to Andrey for the fix.  Greg, did you want to add something
> else?  Either way, please forward to Linus.
> 

thank you.

> Andrey: the reason everything is in there is I didn't know what Greg
> wanted.  He OK'd it, but I'm happy for them to be trimmed, too.
> 

May I ask reponsible persons - Greg, Rusty - make some decision?
As I have been hammered by Rusty to use modules.alias I am going
to send functions for other subsystems as well - at least USB.
Ironically for all others it makes code much smaller (and possibly
a bit faster). For input it does not bring much :(

regards

-andrey

Re: file2alias - incorrect? aliases for USB

[Andrey Borzenkov]

On Monday 10 November 2003 12:37, Greg KH wrote:
> On Sun, Nov 09, 2003 at 09:55:19PM +0300, Andrey Borzenkov wrote:
> > file2aliases puts in alias device ID high and low numbers directly from
> > match specifications. E.g. for this match table entry:
> >
> > usb-storage          0x000f      0x04e6   0x0006    0x0100       0x0205
> > ...
> >
> > it generates alias
> >
> > alias usb:v04E6p0006dl0100dh0205dc*dsc*dp*ic*isc*ip* usb_storage
> >
[...]
>
> I would suggest just ignoring the bcdDevice value, and loading all
> modules that match the idVendor and idProduct values, and let the kernel
> sort it out :)
>

the obvious point that this leaves unneeded modules in kernel aside ...

that won't work that easy. You have to build name to match line from 
modules.alias. But to match you have to put exact values for `dl' and `dh'. 
Those values are simply not available when hotplug agent is called.

i.e. for the above line to match you have to supply exact values for vendor, 
product, device low, device high. Any other values are not important but 
these must be exact. But neither device low nor device high are known.

apparently the only way is to remove them and hope that usually the same 
vendor/product combination is handled by single driver.

> So for your example, you would just:
> 	modprobe usb:v04E6p0006dl*dh*dc*dsc*dp*ic*isc*ip*
>

that won't match, sorry. wildcards can be in aliases; they can't be in module 
names.

-andrey

Re: file2alias - incorrect? aliases for USB

[Greg KH]

On Mon, Nov 17, 2003 at 02:09:32PM +1100, Rusty Russell wrote:
> In message <20031110093703.GA5449@kroah.com> you write:
> > Hm, but that's no good either, because the visor driver trips over that
> > with its entry:
> > 	MODULE_ALIAS("usb:v*p*dl*dh*dc*dsc*dp*ic*isc*ip*");
> > and the improper module is loaded.  That needs to be fixed up...
> > 
> > Rusty, any reason why the module alias code is turning an empty
> > MODULE_PARAM structure, as is declared in drivers/usb/serial/visor.c
> > with the line:
> >         { },                                    /* optional parameter entry */ 
> > 
> > Into the above MODULE_ALIAS?  I don't think that's correct.
> 
> Sorry for the delay, was away and am catching up on mail.
> 
> Thanks to Andrey for the fix.  Greg, did you want to add something
> else?  Either way, please forward to Linus.

Just finally did that, thanks.

greg k-h