Re: [ANNOUNCE] hotplug-ng 001 release

[Erik van Konijnenburg <ekonijn@xs4all.nl>]

Some notes in the patch.

On Tue, Feb 22, 2005 at 01:35:03PM +0300, Roman Kagan wrote:
> On Mon, Feb 21, 2005 at 09:40:04PM +0100, Erik van Konijnenburg wrote:
> > Here are some samples from modules.alias:
> > 	alias usb:v097Ap0001dl0000dh0001dc*dsc*dp*ic*isc*ip* usb_storage
> > 	alias usb:v05A9p0511dl*dh*dc*dsc*dp*ic*isc*ip* ov511
> > 
> > And the read_config_file function in modprobe.c simply does a shell
> > style wildcard match.
> > 
> > How can wildcard matching in modprobe against these aliases give the same
> > result as a numeric range comparison on DEV_LO and DEV_HI in kernel and
> > hotplug agents?
> > 
> > Since I don't know what modules.aliases was originally intended for,
> > I may be missing something obvious.  Any hints appreciated.
> 
> That wasn't you who missed something obvious :)  Actually this problem
> existed since day 0 and presumably was due to the fact that USB was (and
> still is) the only kind which used numeric ranges and thus had two
> parameters to match a single value against, which was probably
> overlooked when scripts/mod/file2alias.c was written.
> 
> The only solution I could come up with was to modify that file to
> generate multiple patterns (and thus MODULE_ALIAS strings) out of a
> usb_match_id which defined a range of bcdDevice values, e.g. given a
> range between 0x0001 and 0xa345 it would create 7 patterns:
> 
> 000[1-F]*
> 00[1-F]*
> 0[1-F]*
> [1-9]*
> A[0-2]*
> A3[0-3]*
> A34[0-5]*
> 
> Since bcdDevice is 2 bytes wide = 4 digits in hex representation, the
> max no. of patters is 2 * 4 - 1 = 7.

Nice!  Far fewer patterns needed than I feared.

> 
> The attached patch does this.  Please comment and consider applying.
> 
> Thanks,
>   Roman.
> 
> 
> --- linux-2.6.11-rc4/scripts/mod/file2alias.c.orig	2005-02-13 10:06:23.000000000 +0300
> +++ linux-2.6.11-rc4/scripts/mod/file2alias.c	2005-02-22 12:58:05.849081056 +0300
> @@ -47,32 +47,19 @@
>                  sprintf(str + strlen(str), "*");                \
>  } while(0)
>  
> -/* Looks like "usb:vNpNdlNdhNdcNdscNdpNicNiscNipN" */
> -static int do_usb_entry(const char *filename,
> -			struct usb_device_id *id, char *alias)
> +/* USB is special because the bcdDevice can be matched against a numeric range */
> +/* Looks like "usb:vNpNdNdcNdscNdpNicNiscNipN" */

Hmm.  A change in the alias pattern: dl*dh* => d*, but given the meaning of
the pattern it's probably the right thing.  Who generates stuff
that needs to be matched against this?

> +static void do_usb_entry(struct usb_device_id *id, const char *bcdDevice_pattern,
> +			 struct module *mod)
>  {
> -	id->match_flags = TO_NATIVE(id->match_flags);
> -	id->idVendor = TO_NATIVE(id->idVendor);
> -	id->idProduct = TO_NATIVE(id->idProduct);
> -	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;
> -
> +	char alias[500];
>  	strcpy(alias, "usb:");
>  	ADD(alias, "v", id->match_flags&USB_DEVICE_ID_MATCH_VENDOR,
>  	    id->idVendor);
>  	ADD(alias, "p", id->match_flags&USB_DEVICE_ID_MATCH_PRODUCT,
>  	    id->idProduct);
> -	ADD(alias, "dl", id->match_flags&USB_DEVICE_ID_MATCH_DEV_LO,
> -	    id->bcdDevice_lo);
> -	ADD(alias, "dh", id->match_flags&USB_DEVICE_ID_MATCH_DEV_HI,
> -	    id->bcdDevice_hi);
> +	strcat(alias, "d");
> +	strcat(alias, bcdDevice_pattern);
>  	ADD(alias, "dc", id->match_flags&USB_DEVICE_ID_MATCH_DEV_CLASS,
>  	    id->bDeviceClass);
>  	ADD(alias, "dsc",
> @@ -90,7 +77,104 @@
>  	ADD(alias, "ip",
>  	    id->match_flags&USB_DEVICE_ID_MATCH_INT_PROTOCOL,
>  	    id->bInterfaceProtocol);
> -	return 1;
> +
> +	/* Always end in a wildcard, for future extension */
> +	if (alias[strlen(alias)-1] != '*')
> +		strcat(alias, "*");
> +	buf_printf(&mod->dev_table_buf,
> +		   "MODULE_ALIAS(\"%s\");\n", alias);
> +}
> +
> +
> +static inline size_t sprint_pattern(char *str, int ndigits, unsigned int initial,
> +				  unsigned char clo, unsigned char chi)
> +{
> +	if (ndigits) {
> +		if (clo = chi)
> +			return sprintf(str, "%0*X%X*", ndigits, initial, clo);
> +		if (clo = 0x0 && chi = 0xf)
> +			return sprintf(str, "%0*X*", ndigits, initial);
> +		return sprintf(str, "%0*X[%X-%X]*", ndigits, initial, clo, chi);
> +	}
> +	else {
> +		if (clo = chi)
> +			return sprintf(str, "%X*", clo);
> +		if (clo = 0x0 && chi = 0xf)
> +			return sprintf(str, "*");
> +		return sprintf(str, "[%X-%X]*", clo, chi);
> +	}
> +}
> +
> +static void do_usb_entry_multi(struct usb_device_id *id, struct module *mod)
> +{
> +	unsigned int devlo, devhi;
> +	unsigned char chi, clo;
> +	int ndigits;
> +	char pattern[32];
> +
> +	id->match_flags = TO_NATIVE(id->match_flags);
> +	id->idVendor = TO_NATIVE(id->idVendor);
> +	id->idProduct = TO_NATIVE(id->idProduct);
> +
> +	devlo = id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO ?
> +		TO_NATIVE(id->bcdDevice_lo) : 0x0U;
> +	devhi = id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI ?
> +		TO_NATIVE(id->bcdDevice_hi) : ~0x0U;
> +
> +	/*
> +	 * 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;
> +
> +	/* Convert numeric bcdDevice range into fnmatch-able pattern(s) */
> +	for (ndigits = sizeof(id->bcdDevice_lo) * 2 - 1; 1; ndigits--) {

By changing the termination condition to devhi < devlo, we could
avoid ghost matches for ranges such as 6009-4002.  That's a useless but
well-defined range, and some driver author might find it convenient not to
filter such stuff from his array; silenty misinterpreting it seems unkind.

The loop body is tricky; at first sight it looks OK, but I did not do a full
blown analysis with loop invariants.

		/* ndigits is the number of digits that is copied unchanged
		 * from the lo/hi numbers to the pattern in this iteration.
		 */

> +		clo = devlo & 0xf;
> +		chi = devhi & 0xf;
> +		devlo >>= 4;
> +		devhi >>= 4;
> +
> +		if (devlo = devhi || !ndigits) {
> +			if (clo <= chi) {
> +				sprint_pattern(pattern, ndigits, devlo, clo, chi);
> +				do_usb_entry(id, pattern, mod);
> +				printf("%s\n", pattern);
> +			}
> +			break;
> +		}
> +
> +		if (clo != 0x0) {
> +			sprint_pattern(pattern, ndigits, devlo, clo, 0xf);
> +			do_usb_entry(id, pattern, mod);
> +			printf("%s\n", pattern);
> +			devlo++;
> +		}
> +
> +		if (chi != 0xf) {
> +			sprint_pattern(pattern, ndigits, devhi, 0x0, chi);
> +			do_usb_entry(id, pattern, mod);
> +			printf("%s\n", pattern);
> +			devhi--;

Signedness issues?  Consider the range 0001-0002: at start of the loop
body you split in 000 + 1, 000 + 2, here you decrement unsigned devhi
from 0 to 0xfffffff.

> +		}
> +	}
> +}
> +
> +static void do_usb_table(void *symval, unsigned long size,
> +			 struct module *mod)
> +{
> +	unsigned int i;
> +	const unsigned long id_size = sizeof(struct usb_device_id);
> +
> +	if (size % id_size || size < id_size) {
> +		fprintf(stderr, "*** Warning: %s ids %lu bad size "
> +			"(each on %lu)\n", mod->name, size, id_size);
> +	}
> +	/* Leave last one: it's the terminator. */
> +	size -= id_size;
> +
> +	for (i = 0; i < size; i += id_size)
> +		do_usb_entry_multi(symval + i, mod);
>  }
>  
>  /* Looks like: ieee1394:venNmoNspNverN */
> @@ -262,8 +346,8 @@
>  		do_table(symval, sym->st_size, sizeof(struct pci_device_id),
>  			 do_pci_entry, mod);
>  	else if (sym_is(symname, "__mod_usb_device_table"))
> -		do_table(symval, sym->st_size, sizeof(struct usb_device_id),
> -			 do_usb_entry, mod);
> +		/* special case to handle bcdDevice ranges */
> +		do_usb_table(symval, sym->st_size, mod);
>  	else if (sym_is(symname, "__mod_ieee1394_device_table"))
>  		do_table(symval, sym->st_size, sizeof(struct ieee1394_device_id),
>  			 do_ieee1394_entry, mod);

Regards,
Erik


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_ide95&alloc_id\x14396&op=click
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel