LinuxLists.cc - [PATCH] Module alias and table support

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

On Thu, Nov 14, 2002 at 12:19:51PM +1100, Rusty Russell wrote:
> Hi guys,
>
> Below is a preliminary patch which implements module aliases
> and reimplements support for MODULE_DEVICE_TABLE on top of it. Tested
> on drivers/usb/net/pegasus.c, seems to work. The reason for doing
> this is so that userspace tools are shielded from ever needing to know
> the layout of the xxx_id structures.

Nice, I like that.

> The idea that modules can contain a ".modalias" section of
> strings which are aliases for the module. Every module goes through a
> "finishing" stage (scripts/modfinish) which looks for __module_table*
> symbols and uses scripts/table2alias.c to add aliases such as
> "usb:v0506p4601dl*dh*dc*dsc*dp*ic*isc*ip*" which hotplug can use to
> probe for matching modules.

How are you going to be able to handle hex values in that string? Or
are you just going to rely on the fact that no valid hex value can be a
identifier? You might want to add a ':' or something between fields to
make it easier to parse, unless you've already written a bash and C
parser that I can use :)

Hm, there goes my wonderful % decrease in size claims of diethotplug if
we shink the original size of the module_table files...

> +
> +#define USB_DEVICE_ID_MATCH_VENDOR 0x0001
> +#define USB_DEVICE_ID_MATCH_PRODUCT 0x0002
> +#define USB_DEVICE_ID_MATCH_DEV_LO 0x0004
> +#define USB_DEVICE_ID_MATCH_DEV_HI 0x0008
> +#define USB_DEVICE_ID_MATCH_DEV_CLASS 0x0010
> +#define USB_DEVICE_ID_MATCH_DEV_SUBCLASS 0x0020
> +#define USB_DEVICE_ID_MATCH_DEV_PROTOCOL 0x0040
> +#define USB_DEVICE_ID_MATCH_INT_CLASS 0x0080
> +#define USB_DEVICE_ID_MATCH_INT_SUBCLASS 0x0100
> +#define USB_DEVICE_ID_MATCH_INT_PROTOCOL 0x0200
> +
> +struct usb_device_id {

We're already including other kernel header files, why not include usb.h
too? That way we don't have to remember to update the structures in two
places.

> + /* which fields to match against? */
> + uint16_t match_flags;
> +
> + /* Used for product specific matches; range is inclusive */
> + uint16_t idVendor;
> + uint16_t idProduct;
> + uint16_t bcdDevice_lo;
> + uint16_t bcdDevice_hi;
> +
> + /* Used for device class matches */
> + uint8_t bDeviceClass;
> + uint8_t bDeviceSubClass;
> + uint8_t bDeviceProtocol;
> +
> + /* Used for interface class matches */
> + uint8_t bInterfaceClass;
> + uint8_t bInterfaceSubClass;
> + uint8_t bInterfaceProtocol;
> +
> + /* not matched against */
> + kernel_long driver_info;

Or is it because of "kernel_long"? I'm pretty sure this field is only
used within the kernel, and userspace does not care at all about it.

> +/* Looks like "usb:vNpNdlNdhNdcNdscNdpNicNiscNipN" */
> +static int do_usb_table(void)
> +{
> + struct usb_device_id id;
> + int r;
> + char alias[200];
> +
> + while ((r = xread(STDIN_FILENO, &id, sizeof(id))) == sizeof(id)) {
> + TO_NATIVE(id.match_flags);
> + TO_NATIVE(id.idVendor);
> + TO_NATIVE(id.idProduct);
> + TO_NATIVE(id.bcdDevice_lo);
> + TO_NATIVE(id.bcdDevice_hi);
> +
> + strcpy(alias, "usb:");
> + ADD(alias, "v", id.match_flags&USB_DEVICE_ID_MATCH_VENDOR,
> + id.idVendor);

Why are you doing this "preprocessing" of the flags and the different
fields? If you do this, you mess with the logic of the current
/sbin/hotplug tools a lot, as they expect to have to do this.

Granted, it's much nicer to do this only once, at this moment in time,
but it is a change that we need to be aware of.

Also realize that if you do this, you can't generate the existing
modules.*map files from the exported values.

> +struct pci_device_id {
> + unsigned int vendor, device; /* Vendor and device ID or PCI_ANY_ID */
> + unsigned int subvendor, subdevice; /* Subsystem ID's or PCI_ANY_ID */
> + unsigned int class, class_mask; /* (class,subclass,prog-if) triplet */
> + kernel_long driver_data; /* Data private to the driver */
> +};

You should specify a proper size of vendor, device, and other fields of
size "int" like you did above for USB. Also, driver_data isn't used by
userspace, so don't worry about keeping the size properly.

Or just include <linux/pci.h> and don't worry about it :)

> +/* Looks like: pci:vNdNsvNsdNcN. */
> +static int do_pci_table(void)
> +{
> + struct pci_device_id id;
> + int r;
> + char alias[200];
> +
> + while ((r = xread(STDIN_FILENO, &id, sizeof(id))) == sizeof(id)) {
> + TO_NATIVE(id.vendor);
> + TO_NATIVE(id.device);
> + TO_NATIVE(id.subvendor);
> + TO_NATIVE(id.subdevice);
> + TO_NATIVE(id.class);
> + TO_NATIVE(id.class_mask);
> +
> + strcpy(alias, "pci:");
> + ADD(alias, "v", id.vendor != PCI_ANY_ID, id.vendor);

Again, same "preprocessing" comment as made above for USB.

In summary, I like the idea, and removing userspace knowledge of kernel
structures is very nice. Just a few tweaks are needed.

thanks,

greg k-h

2002-11-25 00:23:59

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

In message <[email protected]> you write:
> On Thu, Nov 14, 2002 at 12:19:51PM +1100, Rusty Russell wrote:
> > The idea that modules can contain a ".modalias" section of
> > strings which are aliases for the module. Every module goes through a
> > "finishing" stage (scripts/modfinish) which looks for __module_table*
> > symbols and uses scripts/table2alias.c to add aliases such as
> > "usb:v0506p4601dl*dh*dc*dsc*dp*ic*isc*ip*" which hotplug can use to
> > probe for matching modules.
>
> How are you going to be able to handle hex values in that string? Or
> are you just going to rely on the fact that no valid hex value can be a
> identifier? You might want to add a ':' or something between fields to
> make it easier to parse, unless you've already written a bash and C
> parser that I can use :)

Yes, good idea. Of course, you shouldn't need to parse them, you just
need to create them, but clearer is better (I'd use , not : though).

> Hm, there goes my wonderful % decrease in size claims of diethotplug if
> we shink the original size of the module_table files...

Sorry. I'll try to be less efficient in the future.

> We're already including other kernel header files, why not include usb.h
> too? That way we don't have to remember to update the structures in two
> places.
>
> > + /* not matched against */
> > + kernel_long driver_info;
>
> Or is it because of "kernel_long"? I'm pretty sure this field is only
> used within the kernel, and userspace does not care at all about it.

It sucks to reproduce this, yes. But you need to know the size of the
structure to grab it out of the object file. At least this way it's
in the kernel source where we can change it.

> > +/* Looks like "usb:vNpNdlNdhNdcNdscNdpNicNiscNipN" */
> > +static int do_usb_table(void)
> > +{
> > + struct usb_device_id id;
> > + int r;
> > + char alias[200];
> > +
> > + while ((r = xread(STDIN_FILENO, &id, sizeof(id))) == sizeof(id)) {
> > + TO_NATIVE(id.match_flags);
> > + TO_NATIVE(id.idVendor);
> > + TO_NATIVE(id.idProduct);
> > + TO_NATIVE(id.bcdDevice_lo);
> > + TO_NATIVE(id.bcdDevice_hi);
> > +
> > + strcpy(alias, "usb:");
> > + ADD(alias, "v", id.match_flags&USB_DEVICE_ID_MATCH_VENDOR,
> > + id.idVendor);
>
> Why are you doing this "preprocessing" of the flags and the different
> fields? If you do this, you mess with the logic of the current
> /sbin/hotplug tools a lot, as they expect to have to do this.

The plan was that /sbin/hotplug will gather all the fields for
whatever device has been inserted and create the modprobe string, eg:

system("modprobe usb:v0506p4601dl01dh01dc01dsc01dp01ic01isc01ip01");

Which will simply match the alias such as:
usb:v0506p4601dl*dh*dc*dsc*dp*ic*isc*ip*

> Also realize that if you do this, you can't generate the existing
> modules.*map files from the exported values.

Hmm, I thought about enhancing this code to generate the .map
files as well (where it has all the information). BTW, did I break
the current depmod map-generating code?

> In summary, I like the idea, and removing userspace knowledge of kernel
> structures is very nice. Just a few tweaks are needed.

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

2002-11-25 21:36:42

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

On Mon, Nov 25, 2002 at 10:34:16AM +1100, Rusty Russell wrote:
> > > + /* not matched against */
> > > + kernel_long driver_info;
> >
> > Or is it because of "kernel_long"? I'm pretty sure this field is only
> > used within the kernel, and userspace does not care at all about it.
>
> It sucks to reproduce this, yes. But you need to know the size of the
> structure to grab it out of the object file. At least this way it's
> in the kernel source where we can change it.

But can't we still just use the structure from the kernel header and not
have to retype it here? If needed, we can change the type of
driver_info into something more portable than what it is today. That
would be much easier in the long run.

> > Why are you doing this "preprocessing" of the flags and the different
> > fields? If you do this, you mess with the logic of the current
> > /sbin/hotplug tools a lot, as they expect to have to do this.
>
> The plan was that /sbin/hotplug will gather all the fields for
> whatever device has been inserted and create the modprobe string, eg:
>
> system("modprobe usb:v0506p4601dl01dh01dc01dsc01dp01ic01isc01ip01");
>
> Which will simply match the alias such as:
> usb:v0506p4601dl*dh*dc*dsc*dp*ic*isc*ip*

Ah, you never told me about this plan :)

Yes, that would be very nice to have, pushing the logic out of the
/sbin/hotplug code and into modprobe doesn't bother me. That just saved
a _whole_ bunch of space in diethotplug, so you've made up for making
the size smaller.

How would modprobe know which driver to load based on the above line?
Are you going to scan all module files, or rely on something like the
modules.*map files of today?

> > Also realize that if you do this, you can't generate the existing
> > modules.*map files from the exported values.
>
> Hmm, I thought about enhancing this code to generate the .map
> files as well (where it has all the information). BTW, did I break
> the current depmod map-generating code?

I don't know, I haven't looked into that. Just realize that if you
pre-process the information like you are proposing to do, you can't get
it back later, which changes the way things work.

thanks,

greg k-h

2002-11-25 23:37:29

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

In message <[email protected]> you write:
> On Mon, Nov 25, 2002 at 10:34:16AM +1100, Rusty Russell wrote:
> > > > + /* not matched against */
> > > > + kernel_long driver_info;
> > >
> > > Or is it because of "kernel_long"? I'm pretty sure this field is only
> > > used within the kernel, and userspace does not care at all about it.
> >
> > It sucks to reproduce this, yes. But you need to know the size of the
> > structure to grab it out of the object file. At least this way it's
> > in the kernel source where we can change it.
>
> But can't we still just use the structure from the kernel header and not
> have to retype it here? If needed, we can change the type of
> driver_info into something more portable than what it is today. That
> would be much easier in the long run.

Absolutely. But I didn't want to mess with your headers.

> > Which will simply match the alias such as:
> > usb:v0506p4601dl*dh*dc*dsc*dp*ic*isc*ip*
>
> Ah, you never told me about this plan :)

Oh, so I didn't explain the "clever" part of my clever plan? Oops.

> Yes, that would be very nice to have, pushing the logic out of the
> /sbin/hotplug code and into modprobe doesn't bother me. That just saved
> a _whole_ bunch of space in diethotplug, so you've made up for making
> the size smaller.
>
> How would modprobe know which driver to load based on the above line?
> Are you going to scan all module files, or rely on something like the
> modules.*map files of today?

0.7 scans all the files (since it has no modules.dep and needs the
dependencies anyway). 0.8 reintroduces modules.dep (thanks Adam!)
which is where the alias info logically belongs ("stuff extracted from
modules"). Or maybe a separate file (implementation handwave).

> Just realize that if you pre-process the information like you are
> proposing to do, you can't get it back later, which changes the way
> things work.

Well, you could, by sucking out the aliases which start with "usb:"
(the post-processing just turns the tables into aliases, doesn't
delete them or anything). You could also use the old-fashioned way
and suck it from the tables directly, but then you're back in
"intimate knowledge of kernel internals" land.

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

2002-11-26 03:19:35

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

I really like Rusty's "module aliases" idea.

Its full potential has not been recognized in this discussion.

If we're going to use strings for device ID matching,
then we can consolidate all of the xxx_device_id types into one:

struct device_id {
const char *pattern;
/* In practice, many drivers want scalar driver data, many
want an integer, and a few could benefit from having both.
Alternatively, we could have no extra match data at all
and make drivers declare a parallel table, for it, but
most drivers only have a few ID's to match, so the cost of
providing these fields is small. */
int match_scalar;
void *match_ptr;
};

There would be a long period of backward compatability wrappers
and porting to use the interface directly, but eventually we would have:

- only one kind of module device table for generating module aliases,

- device ID matching consolidated into drivers/base,

- No need for user level programs to query devices to generate
hotplug information (goodbye pcimodules, usbmodules,
isapnpmodules),

- Zero changes to user or kernel needed to add a new hotplug
bus type (just drop the driver modules in /lib/modules/nnn/
and run depmod).

Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."

2002-11-26 03:49:53

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

On Mon, Nov 25, 2002 at 07:26:27PM -0800, Adam J. Richter wrote:
> I really like Rusty's "module aliases" idea.
>
> Its full potential has not been recognized in this discussion.
>
> If we're going to use strings for device ID matching,
> then we can consolidate all of the xxx_device_id types into one:
>
>
> struct device_id {
> const char *pattern;
> /* In practice, many drivers want scalar driver data, many
> want an integer, and a few could benefit from having both.
> Alternatively, we could have no extra match data at all
> and make drivers declare a parallel table, for it, but
> most drivers only have a few ID's to match, so the cost of
> providing these fields is small. */
> int match_scalar;
> void *match_ptr;
> };

Nice idea, but how are you going to get the pre-processor to generate a
string with the proper pattern, based on a bunch of flags and integers?
(not to say it can't be done, just tricky stuff...)

> There would be a long period of backward compatability wrappers
> and porting to use the interface directly, but eventually we would have:
>
> - only one kind of module device table for generating module aliases,

Very nice goal.

> - device ID matching consolidated into drivers/base,

Sorry, can't be fully done. A number of drivers really want to poke
around at the device before they say they really claim the device. So
we still need to call into them somehow.

> - No need for user level programs to query devices to generate
> hotplug information (goodbye pcimodules, usbmodules,
> isapnpmodules),

I think these can almost already go away now, with the info we have in
sysfs.

> - Zero changes to user or kernel needed to add a new hotplug
> bus type (just drop the driver modules in /lib/modules/nnn/
> and run depmod).

That is also a very nice goal.

Again, nice idea, have any idea how the code would look?

thanks,

greg k-h

2002-11-26 04:45:46

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

Greg Kroah wrote:
>On Mon, Nov 25, 2002 at 07:26:27PM -0800, Adam J. Richter wrote:
[...]
>> If we're going to use strings for device ID matching,
>> then we can consolidate all of the xxx_device_id types into one:
>>
>>
>> struct device_id {
>> const char *pattern;
>> /* In practice, many drivers want scalar driver data, many
>> want an integer, and a few could benefit from having both.
>> Alternatively, we could have no extra match data at all
>> and make drivers declare a parallel table, for it, but
>> most drivers only have a few ID's to match, so the cost of
>> providing these fields is small. */
>> int match_scalar;
>> void *match_ptr;
>> };

>Nice idea, but how are you going to get the pre-processor to generate a
>string with the proper pattern, based on a bunch of flags and integers?
>(not to say it can't be done, just tricky stuff...)

We won't. What I meant by "eventually" (below) is when
drivers are converted to do this natively. Initially, I would run a
hacked version of depmod to output appropriate string-based device_id
table declarations and append them to the corresponding .c files, at
least for pci, usb and maybe isapnp. The others can probably more
easily be done manually.

number of module_device_id tables in my linux-2.5.49 tree:
pci 206
usb 80
isapnp 21
input 8
parisc 7
parport 3 (a patch I submitted long ago)
ieee1394 1

Total 326

>> There would be a long period of backward compatability wrappers
>> and porting to use the interface directly, but eventually we would have:
>>
>> - only one kind of module device table for generating module aliases,

>Very nice goal.

>> - device ID matching consolidated into drivers/base,

>Sorry, can't be fully done. A number of drivers really want to poke
>around at the device before they say they really claim the device. So
>we still need to call into them somehow.

We already have a solution. A device is only attached if
device_driver->probe() returns zero. If ->probe() fails, device_attach
continues checking for other possible drivers.

>> - No need for user level programs to query devices to generate
>> hotplug information (goodbye pcimodules, usbmodules,
>> isapnpmodules),

>I think these can almost already go away now, with the info we have in
>sysfs.

>> - Zero changes to user or kernel needed to add a new hotplug
>> bus type (just drop the driver modules in /lib/modules/nnn/
>> and run depmod).

>That is also a very nice goal.

>Again, nice idea, have any idea how the code would look?

Yes. I envision replacing bus_type.match() with something
like this:

/* include/linux/device.h: */

typedef int (*dev_id_callback_t)(struct device *dev, const char *id,void *arg);
/* Return values:
> 1: Sucess. Do not call the function again. Return the result code.
0: Not matched. Call the function with the next device ID, if any.
If no more device ID's, return 0.
< 0: Error. Abort, returning the error code.
*/

struct bus_type {
...
void (*for_each_id)(struct device *dev, dev_id_callback_t callback,
void *arg);
}

/* drivers/base/bus.c: */

/* dev_id_callback functions */
static int try_this_driver(struct device *dev, const char *id, void *arg)
{
struct device_driver *drv = arg;

if (wildcard_match(drv->pattern, id))
return bus_match(dev, drv, id);
else
return 0;
}

static int try_every_driver_and_modprobe(struct device *dev, const char *id,
void *arg)
{
int result = 0;
int try;
try = 0;
for(;;) {
list_for_each(entry,&bus->drivers) {
struct device_driver * drv =
container_of(entry,struct device_driver,bus_list);
result = try_this_driver(dev, id, drv);
if (result)
return result;
}
if (try++ != 0)
return result;
request_module(id); /* or call hotplug or whatever */
}
BUG(); /* NOTREACHED */
return -1;
}

static int device_attach(struct device *dev)
{
...
error = (*bus->for_each_id)(dev, try_every_driver_and_modprobe, NULL);
}

static int driver_attach(struct device_driver *drv)
{
...
list_for_each(entry,&bus->devices) {
struct device *dev =container_of(entry,struct device,bus_list);
result = (*bus->for_each_id)(dev, try_this_driver, drv);
...
}
}

/* In drivers/pci/pci-driver.c: */

void pci_for_each_id(struct device *dev, dev_id_callback_t callback,
void *arg)
{
struct pci_dev *pcidev = to_pci_dev(dev);
char id[100];

sprintf(id, "pci-vendor=%04x-prod=%04x-...", pcidev->vendor,
pcidev->product, ...);
return (*callback)(dev, id, arg);
}

/* In drivers/usb/usb.c: */

void usb_for_each_id(struct device *dev, dev_id_callback_t callback,
void *arg)
{
/* Maybe do this on a per-interface basis as the current USB
code does, but assuming that it is per-device illustrates
the multiple callback functionality, different from PCI. */

struct usb_device *dev = to_usb_dev(dev);
char id[100];
int result;

for_each_configuration(config, dev) {
for_each_interface(intf, config, dev) {
sprintf(id, "usb-vendor=%04x-prod=%04x-...",
usbdev->vendor, usbdev->product,
configuration_data, inteface_data, etc. ...);
result = (*callback)(dev, id, arg);
if (result != 0)
return result;
}
}
}

Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."

2002-11-27 08:12:36

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

>>> = adam
>> = greg
> = adam

> [...]
>>> If we're going to use strings for device ID matching,
>>> then we can consolidate all of the xxx_device_id types into one:

This is going to end up rewriting every MODULE_DEVICE_TABLE in the
kernel, as well as hotplug and that depmod functionality, before
hotplugging handles module loading again, isn't it? Somehow I'd
rather not see us change so many things while we're "stabilizing".
("We had to destroy the village in order to save it.")

If you're really talking "strings", with arbitrary whitespace,
I rather like the idea of letting a bunch of key=value lines be
used as an ID. Easy to pass through hotplug, and it'd be lots
easier to work with from userland than the "numbers and spaces"
syntax of the 2.4 "modules.*map" files.

But the first string I'd like to see would be one composed of
several lines in current "modules.*map" format ... that'd be
good stuff to put back in 2.5.50 and its modutils even on a
temporary basis.

>>> - No need for user level programs to query devices to generate
>>> hotplug information (goodbye pcimodules, usbmodules,
>>> isapnpmodules),
>
>>I think these can almost already go away now, with the info we have in
>>sysfs.

The latest (cvs) hotplug scripts won't try to any of use
those on 2.5 systems, it expects /sys/bus/$type/devices/*/*
to expose all the necessary information (for coldplug, and
for per-interface hotplug).

PCI doesn't, USB does, I don't know about the rest. Getting
rid of 'usbmodules' was a good thing, mostly because of issues
related to usbfs, but the "coldplug" support does need to get
rewritten now. (Workaround: unplug/replug. After those work
at the sysfs and modutils level, that is.)

> static int try_every_driver_and_modprobe(struct device *dev, const char *id,
> void *arg)
> {
> ...
> request_module(id); /* or call hotplug or whatever */
> }
> BUG(); /* NOTREACHED */
> return -1;
> }

Why a BUG?

Hotplug is about more than just loading modules, and I'm not sure
that "try every driver and hotplug" would make sense.

- Dave

2002-11-27 20:49:39

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

On November 26, 2002, David Brownell wrote:

> >>> If we're going to use strings for device ID matching,
> >>> then we can consolidate all of the xxx_device_id types into one:

>This is going to end up rewriting every MODULE_DEVICE_TABLE in the
>kernel, as well as hotplug and that depmod functionality, before
>hotplugging handles module loading again, isn't it? Somehow I'd
>rather not see us change so many things while we're "stabilizing".
[...]

I already explained that this can be done by automated means,
(and it appears that Rusty Russell has already written much of the code
to do it):

| Initially, I would run a
| hacked version of depmod to output appropriate string-based device_id
| table declarations and append them to the corresponding .c files [...]

>If you're really talking "strings", with arbitrary whitespace,
>I rather like the idea of letting a bunch of key=value lines be
>used as an ID. [...]

That sounds sensible. That decision would be up to invidual
"bus types", but they'd probably mostly follow by example. By the
way, here are a few other features that I think might be desirable in
choosing an ID format:

- Unless a match pattern ends in "$", pattern matching should
return success if the ID has trailing garbage. That way
it will be easy to add additional detail to device ID's
later (for example, Jeff Garzik talks about adding a PCI
revision field).

- Maybe use your approach, or maybe avoid use of whitespace
and other characters with special meanings to the shell.
The question of what format is most "shell friendly" depends
on what shell script people have in mind to write for it.
For instance, without special characters, shell "case"
statements can correspond exactly to kernel pattern match text.
Examples of potential "real" uses would be appreciated.

- Where this is easily achievable, use a format small enough
so that the ID string can be allocated in the kernel stack
to eliminate an error branch.

- Prefer fixed-width hexadecimal, octal or binary numbers
to facilitate "bitmask" selections and because fixed-width
matching uses less stack space (and infintesmally less CPU).
"*" causes recursion in the match function, so use it
sparingly.

- For hexademical, use capitalization opposite that of any text
buit into the ID string to reduce the chance of accidentally
writing a pattern that has an unanticipated incorrect match.

- For devices that return an ASCII string (for example, ieee-1284
parallel port devices) maybe just use that.

> >>> - No need for user level programs to query devices to generate
> >>> hotplug information (goodbye pcimodules, usbmodules,
> >>> isapnpmodules),
> >
> >>I think these can almost already go away now, with the info we have in
> >>sysfs.

>The latest (cvs) hotplug scripts won't try to any of use
>those on 2.5 systems, it expects /sys/bus/$type/devices/*/*
>to expose all the necessary information (for coldplug, and
>for per-interface hotplug).

USB /sys information appears to be only for the currently
selected configuration, but we want to match drivers for all possible
device configurations, even though most USB devices only define one.
Also, although the usb driverfs code is very clean, I'd still like to
be able to configure out sysfs for systems that don't need it, and yet
I might still want modules on those systems precisely because I want
to mimize kernel memory footprint (by only loading certain drivers for
users or situations that actually use them).

> > static int try_every_driver_and_modprobe(struct device *dev, const char *id,
> > void *arg)
> > {
> > ...
> > request_module(id); /* or call hotplug or whatever */
> > }
> > BUG(); /* NOTREACHED */
> > return -1;
> > }

>Why a BUG?

Because that line should never be reached. However,
here is a cleaner (also untested) version of that routine:

static int try_every_driver_and_modprobe(struct device *dev, const char *id,
void *arg)
{
int try = 0;
do {
list_for_each(entry,&bus->drivers) {
struct device_driver * drv =
container_of(entry,struct device_driver,bus_list);
int result = try_this_driver(dev, id, drv);

if (result)
return result;
}
} while (try++ != 0 && request_module(id) == 0);
return result;
}

>Hotplug is about more than just loading modules, and I'm not sure
>that "try every driver and hotplug" would make sense.

The only activity being "tried" on each device is matching the
ID. The probe function is only called if the ID matches. The existing
code already works this way (with struct pci_device_id, usb_device_id,
etc.).

Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."

2002-11-27 21:42:38

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

>>This is going to end up rewriting every MODULE_DEVICE_TABLE in the
>>kernel, as well as hotplug and that depmod functionality, before
>>hotplugging handles module loading again, isn't it? Somehow I'd
>>rather not see us change so many things while we're "stabilizing".
>
> [...]
>
> I already explained that this can be done by automated means,
> (and it appears that Rusty Russell has already written much of the code
> to do it):
>
> | Initially, I would run a
> | hacked version of depmod to output appropriate string-based device_id
> | table declarations and append them to the corresponding .c files [...]

Hmm, "would" doesn't sound like "it's working now" ... and the latest
available modutils for 2.5 (version 0.7) certainly does none of this.

One of the points being that the breakage comes from changing the
format supported by modutils. Restoring current functionality should
IMO be high on the agenda .... USB has worked poorly in normal .configs
for a while now, because of this.

- Dave

2002-11-27 22:48:11

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

Adam J. Richter wrote:
> On Wed, Nov 27, 2002 at 01:53:58PM -0800, David Brownell wrote:
>
>>One of the points being that the breakage comes from changing the
>>format supported by modutils. Restoring current functionality should
>>IMO be high on the agenda .... USB has worked poorly in normal .configs
>>for a while now, because of this.
>
>
> I posted a patch a couple of days ago to revert 2.5.49/x86 to
> user level modules (no modversions or gplonly symbols in this version
> though). However, I can't find the patch on marc.theaimsgroup.com, so
> here it is again.

Thanks, but I was hoping for a less radical solution: just fixing
the "no device table support" bug fixed in the latest modutils ... I
do like the idea of forward motion in the module support, except that's
not what we've seen so far with modutils.

Seems like one of the issues is that there's really no maintainer
for modutils lately. And I'm not even sure where to get the latest
modutils (more recent than 0.7) even if I were ready to patch them.

- Dave

2002-11-28 00:17:45

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

David Brownell wrote:
>Adam J. Richter wrote:
>> On Wed, Nov 27, 2002 at 01:53:58PM -0800, David Brownell wrote:
>>
>>>One of the points being that the breakage comes from changing the
>>>format supported by modutils. Restoring current functionality should
>>>IMO be high on the agenda .... USB has worked poorly in normal .configs
>>>for a while now, because of this.
>>
>>
>> I posted a patch a couple of days ago to revert 2.5.49/x86 to
>> user level modules (no modversions or gplonly symbols in this version
>> though). However, I can't find the patch on marc.theaimsgroup.com, so
>> here it is again.

>Thanks, but I was hoping for a less radical solution: just fixing
>the "no device table support" bug fixed in the latest modutils ... I
>do like the idea of forward motion in the module support, except that's
>not what we've seen so far with modutils.

2.5.49 contains exports device ID tables again. I think the
existing depmod would work, except for some problem that Rusty found
where it tries to use the kernel module interface (it really shouldn't
care what operating system you're running as it should just read and
write files). Fix that depmod bug(?) and you should be happy, at
least until the next set of module patches (and if you don't use
"modprobe -c", module options, etc., all of which Rusty seems to be
working on).

>Seems like one of the issues is that there's really no maintainer
>for modutils lately. And I'm not even sure where to get the latest
>modutils (more recent than 0.7) even if I were ready to patch them.

ftp://ftp.kernel.org/pub/linux/utils/kernel/modutils/v2.4/

I am running modutils-2.4.21, although I see that there is
a 2.4.22.

Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."

2002-11-28 01:37:07

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

> 2.5.49 contains exports device ID tables again.

That's a start, but the 2.5 modutils don't handle them ...

>>Seems like one of the issues is that there's really no maintainer
>>for modutils lately. And I'm not even sure where to get the latest
>>modutils (more recent than 0.7) even if I were ready to patch them.
>
>
> ftp://ftp.kernel.org/pub/linux/utils/kernel/modutils/v2.4/

Those won't work with the current 2.5 module syscalls though ...
the latest 2.5-usable modutils seems to be

ftp://ftp.kernel.org/pub/linux/kernel/people/rusty/module-init-tools-0.7.tar.bz2

but I've seen comments on LKML referring to a "0.8" version with
essential bugfixes.

- Dave

2002-11-28 04:05:17

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

In message <[email protected]> you write:
> One of the points being that the breakage comes from changing the
> format supported by modutils. Restoring current functionality should
> IMO be high on the agenda .... USB has worked poorly in normal .configs
> for a while now, because of this.

Absolutely. I sent the patch to put the USB etc. tables back in
(merged in .48 IIRC).

Hopefully this is back together: the device-table-to-aliases stuff is
a separate step which can be argued on its own, and I think will
probably have to wait for 2.7 unless Greg is going to champion it.

The real win is simplicity and independence from the kernel
datastructures (which probably won't change during 2.6 anyway).

Hope that helps,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

2002-11-28 04:38:01

by Keith Owens

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

On Wed, 27 Nov 2002 16:22:25 -0800,
"Adam J. Richter" <[email protected]> wrote:
> 2.5.49 contains exports device ID tables again. I think the
>existing depmod would work, except for some problem that Rusty found
>where it tries to use the kernel module interface (it really shouldn't
>care what operating system you're running as it should just read and
>write files). Fix that depmod bug(?)

Not a bug. depmod on its own reads the current kernel symbols. depmod
-F map reads the symbosl from the mapfile and does not use the current
kernel at all.

2002-11-28 06:34:44

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

Rusty Russell wrote:
> In message <[email protected]> you write:
>
>>One of the points being that the breakage comes from changing the
>>format supported by modutils. Restoring current functionality should
>>IMO be high on the agenda .... USB has worked poorly in normal .configs
>>for a while now, because of this.
>
>
> Absolutely. I sent the patch to put the USB etc. tables back in
> (merged in .48 IIRC).

Hmm, with 2.5.50 and module-init-tools 0.8a two "modules.*map" files
are created -- but they're empty. That's with the latest 2.5 modutils

http://www.kernel.org/pub/linux/kernel/people/rusty/modules/

So that's not quite working yet. I should clarify that this hasn't
been the only stumbling block for usb in recent kernels ... looks
as if some of the others (like "re-plugging" issues seemingly in
driver model code) may be gone, but this modutils-based regression
is particularly visible.

- Dave

2002-11-28 21:50:23

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

--- modprobe.c-orig Thu Nov 28 13:18:37 2002
+++ modprobe.c Thu Nov 28 13:34:55 2002
@@ -53,6 +53,9 @@

#define MODULE_DIR "/lib/modules"

+int show_only = 0;
+int verbose = 0;
+
static void fatal(const char *fmt, ...)
__attribute__ ((noreturn, format (printf, 1, 2)));

@@ -384,7 +387,7 @@
for (dep = mod->dependencies; dep != NULL; dep = dep->next)
insmod(dep->module, options, NULL, 0, &call_history);

- /* If we fail to load after this piont, we abort the whole program. */
+ /* If we fail to load after this point, we abort the whole program. */
mod->state = LOADED;

/* Now, it may already be loaded: check /proc/modules */
@@ -428,13 +431,18 @@
if (newname)
rename_module(mod, map, st.st_size, newname);

- ret = syscall(__NR_init_module, map, st.st_size, options);
- if (ret != 0) {
- if (dont_fail)
- fatal("Error inserting %s (%s): %s\n",
- modname, mod->filename, moderror(errno));
- warn("Error inserting %s (%s): %s\n",
- modname, mod->filename, moderror(errno));
+ if (verbose)
+ printf ("insmod %s\n", mod->filename);
+
+ if (!show_only) {
+ ret = syscall(__NR_init_module, map, st.st_size, options);
+ if (ret != 0) {
+ if (dont_fail)
+ fatal("Error inserting %s (%s): %s\n",
+ modname, mod->filename, moderror(errno));
+ warn("Error inserting %s (%s): %s\n",
+ modname, mod->filename, moderror(errno));
+ }
}
free(map);
out_fd:
@@ -505,6 +513,7 @@
{ "showconfig", 0, NULL, 'c' },
{ "autoclean", 0, NULL, 'k' },
{ "quiet", 0, NULL, 'q' },
+ { "show", 0, NULL, 'n' },
{ "syslog", 0, NULL, 's' },
{ NULL, 0, NULL, 0 } };

@@ -515,7 +524,6 @@
struct utsname buf;
int opt;
int dump_only = 0;
- int verbose = 0;
const char *config = NULL;
char *dirname, *optstring = NOFAIL(strdup(""));
char *modname, *newname = NULL;
@@ -523,7 +531,7 @@

try_old_version("modprobe", argv);

- while ((opt = getopt_long(argc, argv, "vVC:o:kqsc", options, NULL)) != -1){
+ while ((opt = getopt_long(argc, argv, "vVC:o:knqsc", options, NULL)) != -1){
switch (opt) {
case 'v':
verbose = 1;
@@ -543,6 +551,9 @@
case 'k':
/* FIXME: This should actually do something */
break;
+ case 'n':
+ show_only = 1;
+ break;
case 'q':
/* FIXME: This should actually do something */
break;
@@ -550,7 +561,7 @@
/* FIXME: This should actually do something */
break;
default:
- fprintf(stderr, "Unknown option `%s'\n", argv[optind]);
+ fprintf(stderr, "Unknown option `%c'\n", opt);
print_usage(argv[0]);
}
}

Attachments:

modprobe.patch (2.52 kB)

2002-11-28 23:38:17

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

In message <[email protected]> you write:
> Thanks, but I was hoping for a less radical solution: just fixing
> the "no device table support" bug fixed in the latest modutils ... I
> do like the idea of forward motion in the module support, except that's
> not what we've seen so far with modutils.

Um, device table support went back in .49, at Adam's request (grand
plans are great and all that, but other maintainers are busy too, and
it'll take a while to get the new scheme sorted out). You just have to
run depmod -a to generate the .xxxmap files.

The patch included in the 0.8 NEWS file allows the new depmod to
generate the tables too.

> Seems like one of the issues is that there's really no maintainer
> for modutils lately. And I'm not even sure where to get the latest
> modutils (more recent than 0.7) even if I were ready to patch them.

Sorry, I thought I posted it a fair bit.
http://www.[COUNTRY].kernel.org/pub/linux/kernel/people/rusty/modules

Hope that helps!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

2002-11-29 01:24:08

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

In message <[email protected]> you write:
> Rusty Russell wrote:
> > In message <[email protected]> you write:
> >
> >>One of the points being that the breakage comes from changing the
> >>format supported by modutils. Restoring current functionality should
> >>IMO be high on the agenda .... USB has worked poorly in normal .configs
> >>for a while now, because of this.
> >
> >
> > Absolutely. I sent the patch to put the USB etc. tables back in
> > (merged in .48 IIRC).
>
> Hmm, with 2.5.50 and module-init-tools 0.8a two "modules.*map" files
> are created -- but they're empty. That's with the latest 2.5 modutils
>
> http://www.kernel.org/pub/linux/kernel/people/rusty/modules/
>
> So that's not quite working yet.

Yes, there's a patch in the NEWS file, which is referred to at the top
of the README file. Did you apply it? If not, apply the one below
(which doesn't delete the #include <linux/elf.h>, which broke
CONFIG_KALLSYMS=y).

Hope this helps!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Module Table Support for module-init-tools depmod
Author: Rusty Russell
Status: Tested on 2.5.50

D: This patch adds a "__mod_XXX_table" symbol which is an alias to the
D: module table, rather than a pointer. This makes it relatively
D: trivial to extract the table. Previously, it required a pointer
D: dereference, which means the relocations must be applied, which is
D: why the old depmod needs so much of modutils (ie. it basically
D: links the whole module in order to find the table).
D:
D: The old depmod can still be invoked using "-F System.map" to
D: generate the tables (there'll be lots of other warnings, and it
D: will generate a completely bogus modules.dep, but the tables should
D: be OK.)

diff -urNp --exclude TAGS -X /home/rusty/current-dontdiff --minimal linux-2.5.50/include/linux/module.h working-2.5.50-table/include/linux/module.h
--- linux-2.5.50/include/linux/module.h Mon Nov 25 08:44:18 2002
+++ working-2.5.50-table/include/linux/module.h Thu Nov 28 10:59:39 2002
@@ -14,6 +14,7 @@
#include <linux/cache.h>
#include <linux/kmod.h>
#include <linux/elf.h>
+#include <linux/stringify.h>

#include <asm/module.h>
#include <asm/uaccess.h> /* For struct exception_table_entry */
@@ -40,11 +40,14 @@ struct kernel_symbol

#ifdef MODULE

-#define MODULE_GENERIC_TABLE(gtype,name) \
-static const unsigned long __module_##gtype##_size \
- __attribute__ ((unused)) = sizeof(struct gtype##_id); \
-static const struct gtype##_id * __module_##gtype##_table \
- __attribute__ ((unused)) = name
+/* For replacement modutils, use an alias not a pointer. */
+#define MODULE_GENERIC_TABLE(gtype,name) \
+static const unsigned long __module_##gtype##_size \
+ __attribute__ ((unused)) = sizeof(struct gtype##_id); \
+static const struct gtype##_id * __module_##gtype##_table \
+ __attribute__ ((unused)) = name; \
+extern const struct gtype##_id __mod_##gtype##_table \
+ __attribute__ ((unused, alias(__stringify(name))))

/* This is magically filled in by the linker, but THIS_MODULE must be
a constant so it works in initializers. */

2002-11-29 03:41:06

[permalink] [raw]

Subject: Re: [PATCH] Module alias and table support

On Thu, Nov 28, 2002 at 02:14:29PM +1100, Rusty Russell wrote:
>
> Hopefully this is back together: the device-table-to-aliases stuff is
> a separate step which can be argued on its own, and I think will
> probably have to wait for 2.7 unless Greg is going to champion it.
>
> The real win is simplicity and independence from the kernel
> datastructures (which probably won't change during 2.6 anyway).

Which, imho, is worth championing! :)

I'm going to be in Austin next week, and I'll have some free time in the
evenings, so I'll try to take a look at your previous patch and see if I
can change it a bit to resolve the minor problems I had with it.

thanks,

greg k-h

2002-11-29 03:54:00