From mboxrd@z Thu Jan 1 00:00:00 1970 From: Erik van Konijnenburg Date: Wed, 23 Feb 2005 00:44:04 +0000 Subject: Re: [ANNOUNCE] hotplug-ng 001 release Message-Id: <20050223014404.E2736@banaan.localdomain> List-Id: References: <20050211004033.GA26624@suse.de> In-Reply-To: <20050211004033.GA26624@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-hotplug@vger.kernel.org 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_id396&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