2009-11-22 21:02:05

by Ondrej Zary

[permalink] [raw]
Subject: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

Hello,
just found that Linux does not automatically load "ne" module for ISA PnP
NE2000 cards (RTL8019AS). ne.c contains this:

static struct isapnp_device_id isapnp_clone_list[] __initdata = {
{ ISAPNP_CARD_ID('A','X','E',0x2011),
ISAPNP_VENDOR('A','X','E'), ISAPNP_FUNCTION(0x2011),
(long) "NetGear EA201" },
{ ISAPNP_ANY_ID, ISAPNP_ANY_ID,
ISAPNP_VENDOR('E','D','I'), ISAPNP_FUNCTION(0x0216),
(long) "NN NE2000" },
{ ISAPNP_ANY_ID, ISAPNP_ANY_ID,
ISAPNP_VENDOR('P','N','P'), ISAPNP_FUNCTION(0x80d6),
(long) "Generic PNP" },
{ } /* terminate list */
};

MODULE_DEVICE_TABLE(isapnp, isapnp_clone_list);

but "modinfo ne" does not show any aliases. The ne.mod.c file produced during
build does not contain aliases too. The problem is that
scripts/mod/file2alias.c simply ignores isapnp.

ne.c is not the only file using MODULE_DEVICE_TABLE(isapnp, ...):
drivers/pcmcia/i82365.c
drivers/net/3c515.c
drivers/net/ne.c
drivers/net/smc-ultra.c
drivers/isdn/hisax/hisax_fspcipnp.c
drivers/media/radio/radio-sf16fmi.c
drivers/scsi/aha1542.c
drivers/scsi/aha152x.c
drivers/scsi/sym53c416.c
drivers/scsi/g_NCR5380.c
sound/oss/ad1848.c

How is (was) this supposed to work?

--
Ondrej Zary


2009-11-23 03:29:54

by Rusty Russell

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Mon, 23 Nov 2009 07:31:57 am Ondrej Zary wrote:
> The problem is that
> scripts/mod/file2alias.c simply ignores isapnp.

AFAICT it always has, and noone has complained until now. Perhaps something
was still reading /lib/modules/`uname -r`/modules.isapnpmap?

Two things are required:
1) Modify file2alias to add aliases for isapnp. This is pretty easy.
2) Expose the isapnp devices in sysfs where udev will match them up
(/sys/bus/isa/devices/<dev>/modalias I guess?)

The first patches would look something like these. We could mangle the
PNP names a-la ISAPNP_VENDOR/ISAPNP_DEVICE, but as long as the published
modalias matches the file2alias result, it doesn't matter.

Jaroslav? Greg? Patches for sysfs welcome...

Thanks,
Rusty.
PS. Jon, perhaps the new decade is a good time to kill modules.*map?

isapnp: move definitions to mod_devicetable.h so file2alias can reach them.

Signed-off-by: Rusty Russell <[email protected]>

diff --git a/include/linux/isapnp.h b/include/linux/isapnp.h
--- a/include/linux/isapnp.h
+++ b/include/linux/isapnp.h
@@ -43,10 +43,10 @@
*/

#ifdef __KERNEL__
+#include <linux/mod_devicetable.h>

#define DEVICE_COUNT_COMPATIBLE 4

-#define ISAPNP_ANY_ID 0xffff
#define ISAPNP_CARD_DEVS 8

#define ISAPNP_CARD_ID(_va, _vb, _vc, _device) \
@@ -74,12 +74,6 @@ struct isapnp_card_id {
#define ISAPNP_DEVICE_SINGLE_END \
.card_vendor = 0, .card_device = 0

-struct isapnp_device_id {
- unsigned short card_vendor, card_device;
- unsigned short vendor, function;
- unsigned long driver_data; /* data private to the driver */
-};
-
#if defined(CONFIG_ISAPNP) || (defined(CONFIG_ISAPNP_MODULE) && defined(MODULE))

#define __ISAPNP__
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -474,4 +474,11 @@ struct platform_device_id {
__attribute__((aligned(sizeof(kernel_ulong_t))));
};

+#define ISAPNP_ANY_ID 0xffff
+struct isapnp_device_id {
+ unsigned short card_vendor, card_device;
+ unsigned short vendor, function;
+ kernel_ulong_t driver_data; /* data private to the driver */
+};
+
#endif /* LINUX_MOD_DEVICETABLE_H */


isapnp: put aliases in .modinfo so modinfo can find them.

Once isa devices publish a modalias field, I think udev should "Just Work".

Signed-off-by: Rusty Russell <[email protected]>

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -727,6 +727,19 @@ static int do_platform_entry(const char
return 1;
}

+/* Format is: isa:cvNcdNvNfN */
+static int do_isapnp_entry(const char *filename,
+ struct isapnp_device_id *id, char *alias)
+{
+ strcpy(alias, "isa:");
+ ADD(alias, "cv", id->card_vendor != ISAPNP_ANY_ID, id->card_vendor);
+ ADD(alias, "cd", id->card_device != ISAPNP_ANY_ID, id->card_device);
+ ADD(alias, "v", id->vendor != ISAPNP_ANY_ID, id->vendor);
+ ADD(alias, "f", id->function, id->function);
+ add_wildcard(alias);
+ return 1;
+}
+
/* Ignore any prefix, eg. some architectures prepend _ */
static inline int sym_is(const char *symbol, const char *name)
{
@@ -874,6 +887,10 @@ void handle_moddevtable(struct module *m
do_table(symval, sym->st_size,
sizeof(struct platform_device_id), "platform",
do_platform_entry, mod);
+ else if (sym_is(symname, "__mod_isapnp_device_table"))
+ do_table(symval, sym->st_size,
+ sizeof(struct isapnp_device_id), "isa",
+ do_isapnp_entry, mod);
free(zeros);
}

2009-11-23 08:40:19

by Takashi Iwai

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

At Mon, 23 Nov 2009 13:59:53 +1030,
Rusty Russell wrote:
>
> On Mon, 23 Nov 2009 07:31:57 am Ondrej Zary wrote:
> > The problem is that
> > scripts/mod/file2alias.c simply ignores isapnp.
>
> AFAICT it always has, and noone has complained until now. Perhaps something
> was still reading /lib/modules/`uname -r`/modules.isapnpmap?

I don't think modules.isapnpmap is needed again.
The question came from the fact that the isapnp device wasn't loaded
automatically.

But, I thought Kay already added isapnp support sometime ago, but it
didn't seem to get in... Kay?


> Two things are required:
> 1) Modify file2alias to add aliases for isapnp. This is pretty easy.
> 2) Expose the isapnp devices in sysfs where udev will match them up
> (/sys/bus/isa/devices/<dev>/modalias I guess?)

There is non-pnp ISA bus, so I'm afraid "isa" may conflict.
I suppose "isapnp" would be a safer choice.


thanks,

Takashi

> The first patches would look something like these. We could mangle the
> PNP names a-la ISAPNP_VENDOR/ISAPNP_DEVICE, but as long as the published
> modalias matches the file2alias result, it doesn't matter.
>
> Jaroslav? Greg? Patches for sysfs welcome...
>
> Thanks,
> Rusty.
> PS. Jon, perhaps the new decade is a good time to kill modules.*map?
>
> isapnp: move definitions to mod_devicetable.h so file2alias can reach them.
>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/include/linux/isapnp.h b/include/linux/isapnp.h
> --- a/include/linux/isapnp.h
> +++ b/include/linux/isapnp.h
> @@ -43,10 +43,10 @@
> */
>
> #ifdef __KERNEL__
> +#include <linux/mod_devicetable.h>
>
> #define DEVICE_COUNT_COMPATIBLE 4
>
> -#define ISAPNP_ANY_ID 0xffff
> #define ISAPNP_CARD_DEVS 8
>
> #define ISAPNP_CARD_ID(_va, _vb, _vc, _device) \
> @@ -74,12 +74,6 @@ struct isapnp_card_id {
> #define ISAPNP_DEVICE_SINGLE_END \
> .card_vendor = 0, .card_device = 0
>
> -struct isapnp_device_id {
> - unsigned short card_vendor, card_device;
> - unsigned short vendor, function;
> - unsigned long driver_data; /* data private to the driver */
> -};
> -
> #if defined(CONFIG_ISAPNP) || (defined(CONFIG_ISAPNP_MODULE) && defined(MODULE))
>
> #define __ISAPNP__
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -474,4 +474,11 @@ struct platform_device_id {
> __attribute__((aligned(sizeof(kernel_ulong_t))));
> };
>
> +#define ISAPNP_ANY_ID 0xffff
> +struct isapnp_device_id {
> + unsigned short card_vendor, card_device;
> + unsigned short vendor, function;
> + kernel_ulong_t driver_data; /* data private to the driver */
> +};
> +
> #endif /* LINUX_MOD_DEVICETABLE_H */
>
>
> isapnp: put aliases in .modinfo so modinfo can find them.
>
> Once isa devices publish a modalias field, I think udev should "Just Work".
>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -727,6 +727,19 @@ static int do_platform_entry(const char
> return 1;
> }
>
> +/* Format is: isa:cvNcdNvNfN */
> +static int do_isapnp_entry(const char *filename,
> + struct isapnp_device_id *id, char *alias)
> +{
> + strcpy(alias, "isa:");
> + ADD(alias, "cv", id->card_vendor != ISAPNP_ANY_ID, id->card_vendor);
> + ADD(alias, "cd", id->card_device != ISAPNP_ANY_ID, id->card_device);
> + ADD(alias, "v", id->vendor != ISAPNP_ANY_ID, id->vendor);
> + ADD(alias, "f", id->function, id->function);
> + add_wildcard(alias);
> + return 1;
> +}
> +
> /* Ignore any prefix, eg. some architectures prepend _ */
> static inline int sym_is(const char *symbol, const char *name)
> {
> @@ -874,6 +887,10 @@ void handle_moddevtable(struct module *m
> do_table(symval, sym->st_size,
> sizeof(struct platform_device_id), "platform",
> do_platform_entry, mod);
> + else if (sym_is(symname, "__mod_isapnp_device_table"))
> + do_table(symval, sym->st_size,
> + sizeof(struct isapnp_device_id), "isa",
> + do_isapnp_entry, mod);
> free(zeros);
> }
>
>

2009-11-23 14:21:20

by Kay Sievers

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Mon, Nov 23, 2009 at 09:40, Takashi Iwai <[email protected]> wrote:
> At Mon, 23 Nov 2009 13:59:53 +1030,
> Rusty Russell wrote:
>>
>> On Mon, 23 Nov 2009 07:31:57 am Ondrej Zary wrote:
>> > The problem is that
>> > scripts/mod/file2alias.c simply ignores isapnp.
>>
>> AFAICT it always has, and noone has complained until now.  Perhaps something
>> was still reading /lib/modules/`uname -r`/modules.isapnpmap?
>
> I don't think modules.isapnpmap is needed again.
> The question came from the fact that the isapnp device wasn't loaded
> automatically.

All the map files are not uses for anything these days. Ideally,
depmod should just stop creating these dead files.

> But, I thought Kay already added isapnp support sometime ago, but it
> didn't seem to get in...  Kay?

No, we don't even have proper modaliases for the pnp bus
(/sys/bus/pnp) and the alias string for pnp in the modules have a
broken and unfixable format. PNP autoloading is all handled by the
acpi modaliases these days. This is the first time in years I hear
anybody asking for hotplug setups of plain old pnp devices. :)

Thanks,
Kay

2009-11-23 21:51:40

by Rusty Russell

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Mon, 23 Nov 2009 07:10:22 pm Takashi Iwai wrote:
> At Mon, 23 Nov 2009 13:59:53 +1030, Rusty Russell wrote:
> > Two things are required:
> > 1) Modify file2alias to add aliases for isapnp. This is pretty easy.
> > 2) Expose the isapnp devices in sysfs where udev will match them up
> > (/sys/bus/isa/devices/<dev>/modalias I guess?)
>
> There is non-pnp ISA bus, so I'm afraid "isa" may conflict.
> I suppose "isapnp" would be a safer choice.

Without pnp, I don't think you can enumerate the ISA bus. Hence I chose
"isa:" in the hope that udev would "just work" if isapnp created the
modalias under /sys/bus/isa...

But Kay says there's a pnp bus (who knew?). I don't know what he means
by unfixable aliases tho, since it's all in the kernel source so we can
change it at any time?

Thanks,
Rusty.

2009-11-24 07:29:23

by Ondrej Zary

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Monday 23 November 2009, Kay Sievers wrote:
> On Mon, Nov 23, 2009 at 09:40, Takashi Iwai <[email protected]> wrote:
> > At Mon, 23 Nov 2009 13:59:53 +1030,
> >
> > Rusty Russell wrote:
> >> On Mon, 23 Nov 2009 07:31:57 am Ondrej Zary wrote:
> >> > The problem is that
> >> > scripts/mod/file2alias.c simply ignores isapnp.
> >>
> >> AFAICT it always has, and noone has complained until now.  Perhaps
> >> something was still reading /lib/modules/`uname -r`/modules.isapnpmap?
> >
> > I don't think modules.isapnpmap is needed again.
> > The question came from the fact that the isapnp device wasn't loaded
> > automatically.
>
> All the map files are not uses for anything these days. Ideally,
> depmod should just stop creating these dead files.
>
> > But, I thought Kay already added isapnp support sometime ago, but it
> > didn't seem to get in...  Kay?
>
> No, we don't even have proper modaliases for the pnp bus
> (/sys/bus/pnp) and the alias string for pnp in the modules have a
> broken and unfixable format. PNP autoloading is all handled by the
> acpi modaliases these days. This is the first time in years I hear
> anybody asking for hotplug setups of plain old pnp devices. :)


Added these two lines to modules.alias (and deleted modules.alias.bin):
alias acpi*:RTL8019:* ne
alias pnp:dRTL8019* ne

and it works. This works too:
alias acpi*:PNP80d6:* ne
alias pnp:dPNP80d6* ne

The card has two IDs (as some PnP devices do) and both work.

--
Ondrej Zary

2009-11-24 08:54:02

by Kay Sievers

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Mon, Nov 23, 2009 at 22:51, Rusty Russell <[email protected]> wrote:
> On Mon, 23 Nov 2009 07:10:22 pm Takashi Iwai wrote:
>> At Mon, 23 Nov 2009 13:59:53 +1030, Rusty Russell wrote:
>> > Two things are required:
>> > 1) Modify file2alias to add aliases for isapnp.  This is pretty easy.
>> > 2) Expose the isapnp devices in sysfs where udev will match them up
>> >    (/sys/bus/isa/devices/<dev>/modalias I guess?)
>>
>> There is non-pnp ISA bus, so I'm afraid "isa" may conflict.
>> I suppose "isapnp" would be a safer choice.
>
> Without pnp, I don't think you can enumerate the ISA bus. Hence I chose
> "isa:" in the hope that udev would "just work" if isapnp created the
> modalias under /sys/bus/isa...
>
> But Kay says there's a pnp bus (who knew?).  I don't know what he means
> by unfixable aliases tho, since it's all in the kernel source so we can
> change it at any time?

Here's a bit of the background:

The aliases in the modules can only match a single value, but PNP
devices often have vendor specific IDs and usual IDs describing the
function.

Like here, where the interesting ID is only the second one of the two
for a single device:
$ grep . /sys/bus/pnp/devices/*/id
/sys/bus/pnp/devices/00:09/id:LEN0006
/sys/bus/pnp/devices/00:09/id:PNP0f13

So the kernel device would need to compose a "modalias" string with
all IDs in a single line. But the in-module aliases can unfortunately
match only on a single value, like:
alias pnp:dPNP0700* floppy

For that reason, a while ago udev switched to acpi aliases entirely,
and dropped all pnp: usage. The acpi aliases can handle multi-values
just fine with single strings like:
$ cat /sys/bus/acpi/devices/LEN0006:00/modalias
acpi:LEN0006:PNP0F13:

alias acpi*:PNP0700:* floppy

Udev does not use any pnp: or /sys/bus/pnp/ values since a while. But
there might be still distros who use the (broken) pnp: aliases and
ship the (also broken) shell scripts to iterate over the sysfs devices
and call modprobe for all they find.

Thanks,
Kay

2009-11-24 08:57:41

by Kay Sievers

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Tue, Nov 24, 2009 at 08:29, Ondrej Zary <[email protected]> wrote:
> On Monday 23 November 2009, Kay Sievers wrote:
>> On Mon, Nov 23, 2009 at 09:40, Takashi Iwai <[email protected]> wrote:
>> > At Mon, 23 Nov 2009 13:59:53 +1030,
>> >
>> > Rusty Russell wrote:
>> >> On Mon, 23 Nov 2009 07:31:57 am Ondrej Zary wrote:
>> >> > The problem is that
>> >> > scripts/mod/file2alias.c simply ignores isapnp.
>> >>
>> >> AFAICT it always has, and noone has complained until now.  Perhaps
>> >> something was still reading /lib/modules/`uname -r`/modules.isapnpmap?
>> >
>> > I don't think modules.isapnpmap is needed again.
>> > The question came from the fact that the isapnp device wasn't loaded
>> > automatically.
>>
>> All the map files are not uses for anything these days. Ideally,
>> depmod should just stop creating these dead files.
>>
>> > But, I thought Kay already added isapnp support sometime ago, but it
>> > didn't seem to get in...  Kay?
>>
>> No, we don't even have proper modaliases for the pnp bus
>> (/sys/bus/pnp) and the alias string for pnp in the modules have a
>> broken and unfixable format. PNP autoloading is all handled by the
>> acpi modaliases these days. This is the first time in years I hear
>> anybody asking for hotplug setups of plain old pnp devices. :)
>
>
> Added these two lines to modules.alias (and deleted modules.alias.bin):
> alias acpi*:RTL8019:* ne
> alias pnp:dRTL8019* ne
>
> and it works. This works too:
> alias acpi*:PNP80d6:* ne
> alias pnp:dPNP80d6* ne

Oh, your box has acpi mapped pnp aliases for this device? You can see
your ID string somewhere in this list?
grep . /sys/bus/acpi/devices/*/modalias

If not, is it in the pnp bus list?
grep . /sys/bus/pnp/devices/*/id

Thanks,
Kay

2009-11-24 21:23:40

by Ondrej Zary

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Tuesday 24 November 2009 09:57:28 Kay Sievers wrote:
> On Tue, Nov 24, 2009 at 08:29, Ondrej Zary <[email protected]>
wrote:
> > On Monday 23 November 2009, Kay Sievers wrote:
> >> On Mon, Nov 23, 2009 at 09:40, Takashi Iwai <[email protected]> wrote:
> >> > At Mon, 23 Nov 2009 13:59:53 +1030,
> >> >
> >> > Rusty Russell wrote:
> >> >> On Mon, 23 Nov 2009 07:31:57 am Ondrej Zary wrote:
> >> >> > The problem is that
> >> >> > scripts/mod/file2alias.c simply ignores isapnp.
> >> >>
> >> >> AFAICT it always has, and noone has complained until now.  Perhaps
> >> >> something was still reading /lib/modules/`uname
> >> >> -r`/modules.isapnpmap?
> >> >
> >> > I don't think modules.isapnpmap is needed again.
> >> > The question came from the fact that the isapnp device wasn't loaded
> >> > automatically.
> >>
> >> All the map files are not uses for anything these days. Ideally,
> >> depmod should just stop creating these dead files.
> >>
> >> > But, I thought Kay already added isapnp support sometime ago, but it
> >> > didn't seem to get in...  Kay?
> >>
> >> No, we don't even have proper modaliases for the pnp bus
> >> (/sys/bus/pnp) and the alias string for pnp in the modules have a
> >> broken and unfixable format. PNP autoloading is all handled by the
> >> acpi modaliases these days. This is the first time in years I hear
> >> anybody asking for hotplug setups of plain old pnp devices. :)
> >
> > Added these two lines to modules.alias (and deleted modules.alias.bin):
> > alias acpi*:RTL8019:* ne
> > alias pnp:dRTL8019* ne
> >
> > and it works. This works too:
> > alias acpi*:PNP80d6:* ne
> > alias pnp:dPNP80d6* ne
>
> Oh, your box has acpi mapped pnp aliases for this device? You can see
> your ID string somewhere in this list?
> grep . /sys/bus/acpi/devices/*/modalias
>
> If not, is it in the pnp bus list?
> grep . /sys/bus/pnp/devices/*/id

It's listen in the pnp bus only:

$ grep -i 80d6 /sys/bus/acpi/devices/*/modalias
$ grep -i 80d6 /sys/bus/pnp/devices/*/id
/sys/bus/pnp/devices/01:01.00/id:PNP80d6
$ cat /sys/bus/pnp/devices/01\:01.00/id
RTL8019
PNP80d6

So the acpi alias was useless (I just copied another entry). This alone works:
alias pnp:dPNP80d6* ne

--
Ondrej Zary

2009-11-24 23:51:41

by Rusty Russell

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Tue, 24 Nov 2009 07:23:51 pm Kay Sievers wrote:
> On Mon, Nov 23, 2009 at 22:51, Rusty Russell <[email protected]> wrote:
> > On Mon, 23 Nov 2009 07:10:22 pm Takashi Iwai wrote:
> >> At Mon, 23 Nov 2009 13:59:53 +1030, Rusty Russell wrote:
> >> > Two things are required:
> >> > 1) Modify file2alias to add aliases for isapnp. This is pretty easy.
> >> > 2) Expose the isapnp devices in sysfs where udev will match them up
> >> > (/sys/bus/isa/devices/<dev>/modalias I guess?)
> >>
> >> There is non-pnp ISA bus, so I'm afraid "isa" may conflict.
> >> I suppose "isapnp" would be a safer choice.
> >
> > Without pnp, I don't think you can enumerate the ISA bus. Hence I chose
> > "isa:" in the hope that udev would "just work" if isapnp created the
> > modalias under /sys/bus/isa...
> >
> > But Kay says there's a pnp bus (who knew?). I don't know what he means
> > by unfixable aliases tho, since it's all in the kernel source so we can
> > change it at any time?
>
> Here's a bit of the background:
>
> The aliases in the modules can only match a single value, but PNP
> devices often have vendor specific IDs and usual IDs describing the
> function.

I'm confused. You can have multiple aliases in a module. Many do.

> Like here, where the interesting ID is only the second one of the two
> for a single device:
> $ grep . /sys/bus/pnp/devices/*/id
> /sys/bus/pnp/devices/00:09/id:LEN0006
> /sys/bus/pnp/devices/00:09/id:PNP0f13
>
> So the kernel device would need to compose a "modalias" string with
> all IDs in a single line. But the in-module aliases can unfortunately
> match only on a single value, like:
> alias pnp:dPNP0700* floppy
>
> For that reason, a while ago udev switched to acpi aliases entirely,
> and dropped all pnp: usage. The acpi aliases can handle multi-values
> just fine with single strings like:
> $ cat /sys/bus/acpi/devices/LEN0006:00/modalias
> acpi:LEN0006:PNP0F13:
>
> alias acpi*:PNP0700:* floppy

Then why didn't you change the pnp modalias format to be more like the ACPI
one?

(I know nothing about ACPI or PNP, so maybe I'm missing something fundamental).

Thanks,
Rusty.

2009-11-25 11:39:54

by Kay Sievers

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Wed, Nov 25, 2009 at 00:51, Rusty Russell <[email protected]> wrote:
> On Tue, 24 Nov 2009 07:23:51 pm Kay Sievers wrote:
>> On Mon, Nov 23, 2009 at 22:51, Rusty Russell <[email protected]> wrote:
>> > On Mon, 23 Nov 2009 07:10:22 pm Takashi Iwai wrote:
>> >> At Mon, 23 Nov 2009 13:59:53 +1030, Rusty Russell wrote:
>> >> > Two things are required:
>> >> > 1) Modify file2alias to add aliases for isapnp.  This is pretty easy.
>> >> > 2) Expose the isapnp devices in sysfs where udev will match them up
>> >> >    (/sys/bus/isa/devices/<dev>/modalias I guess?)
>> >>
>> >> There is non-pnp ISA bus, so I'm afraid "isa" may conflict.
>> >> I suppose "isapnp" would be a safer choice.
>> >
>> > Without pnp, I don't think you can enumerate the ISA bus. Hence I chose
>> > "isa:" in the hope that udev would "just work" if isapnp created the
>> > modalias under /sys/bus/isa...
>> >
>> > But Kay says there's a pnp bus (who knew?).  I don't know what he means
>> > by unfixable aliases tho, since it's all in the kernel source so we can
>> > change it at any time?
>>
>> Here's a bit of the background:
>>
>> The aliases in the modules can only match a single value, but PNP
>> devices often have vendor specific IDs and usual IDs describing the
>> function.
>
> I'm confused.  You can have multiple aliases in a module.  Many do.

Sure, but we have only a single modalias string to match against. The
aliases need to be in a form where they can match the modalias string
containing more than a single entry. The current ones can't do that,
but they were known to be used by some systems with hacky shell
scripts or custom modprobe configs to load pnp device modules.

>> Like here, where the interesting ID is only the second one of the two
>> for a single device:
>>   $ grep . /sys/bus/pnp/devices/*/id
>>   /sys/bus/pnp/devices/00:09/id:LEN0006
>>   /sys/bus/pnp/devices/00:09/id:PNP0f13
>>
>> So the kernel device would need to compose a "modalias" string with
>> all IDs in a single line. But the in-module aliases can unfortunately
>> match only on a single value, like:
>>   alias pnp:dPNP0700* floppy
>>
>> For that reason, a while ago udev switched to acpi aliases entirely,
>> and dropped all pnp: usage. The acpi aliases can handle multi-values
>> just fine with single strings like:
>>   $ cat /sys/bus/acpi/devices/LEN0006:00/modalias
>>   acpi:LEN0006:PNP0F13:
>>
>>   alias acpi*:PNP0700:* floppy
>
> Then why didn't you change the pnp modalias format to be more like the ACPI
> one?

I'm all for doing that, but the current ones are used in old shell
script hacks and custom modprobe configs. Not sure, if that is still
something we need to care about. As mentioned, for that reason,
upstream udev dropped all that a while ago and supports only acpi
aliases today. At the moment we decide not to care about the possible
old users anymore, we could change the pnp aliases to a sane format.

> (I know nothing about ACPI or PNP, so maybe I'm missing something fundamental).

I think you don't miss anything, it's just a bit hard sometimes to get
rid of old users, even when they use the stuff in a wrong way. :)

Thanks,
Kay

2009-11-25 12:05:51

by Kay Sievers

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Tue, Nov 24, 2009 at 22:23, Ondrej Zary <[email protected]> wrote:
> On Tuesday 24 November 2009 09:57:28 Kay Sievers wrote:

>> Oh, your box has acpi mapped pnp aliases for this device? You can see
>> your ID string somewhere in this list?
>>   grep . /sys/bus/acpi/devices/*/modalias
>>
>> If not, is it in the pnp bus list?
>>   grep . /sys/bus/pnp/devices/*/id
>
> It's listen in the pnp bus only:
>
> $ grep -i 80d6 /sys/bus/acpi/devices/*/modalias
> $ grep -i 80d6 /sys/bus/pnp/devices/*/id
> /sys/bus/pnp/devices/01:01.00/id:PNP80d6
> $ cat /sys/bus/pnp/devices/01\:01.00/id
> RTL8019
> PNP80d6
>
> So the acpi alias was useless (I just copied another entry). This alone works:
> alias pnp:dPNP80d6* ne

I see. Plain upstream udev would not load anything here, as the pnp
bus has no modalias support because of the non-working aliases. We
rely on acpi mapping all the pnp entries. Your system probably runs
the shell script which iterates over the "id" file and calls modprobe
for all it finds in there.

Kay

2009-11-25 13:21:38

by Ondrej Zary

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Wednesday 25 November 2009, Kay Sievers wrote:
> On Tue, Nov 24, 2009 at 22:23, Ondrej Zary <[email protected]>
wrote:
> > On Tuesday 24 November 2009 09:57:28 Kay Sievers wrote:
> >> Oh, your box has acpi mapped pnp aliases for this device? You can see
> >> your ID string somewhere in this list?
> >>   grep . /sys/bus/acpi/devices/*/modalias
> >>
> >> If not, is it in the pnp bus list?
> >>   grep . /sys/bus/pnp/devices/*/id
> >
> > It's listen in the pnp bus only:
> >
> > $ grep -i 80d6 /sys/bus/acpi/devices/*/modalias
> > $ grep -i 80d6 /sys/bus/pnp/devices/*/id
> > /sys/bus/pnp/devices/01:01.00/id:PNP80d6
> > $ cat /sys/bus/pnp/devices/01\:01.00/id
> > RTL8019
> > PNP80d6
> >
> > So the acpi alias was useless (I just copied another entry). This alone
> > works: alias pnp:dPNP80d6* ne
>
> I see. Plain upstream udev would not load anything here, as the pnp
> bus has no modalias support because of the non-working aliases. We
> rely on acpi mapping all the pnp entries. Your system probably runs
> the shell script which iterates over the "id" file and calls modprobe
> for all it finds in there.

It's Debian Squeeze.

--
Ondrej Zary

2009-11-25 14:43:00

by matthieu castet

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

Hi,

Kay Sievers <kay.sievers <at> vrfy.org> writes:
>
> No, we don't even have proper modaliases for the pnp bus
> (/sys/bus/pnp) and the alias string for pnp in the modules have a
> broken and unfixable format. PNP autoloading is all handled by the
> acpi modaliases these days. This is the first time in years I hear
> anybody asking for hotplug setups of plain old pnp devices. :)
>
The naming is really bad. This as nothing to do with acpi : pnp and pnp_cards
entries are duplicated to another naming. And this should work on system
without
acpi.
This should have been named to pnp2 or something like that.

Now I don't see why isapnp_device_id and pnp_card_device_id can't be merged
together.

For example most of alsa isa card use pnp_card_device_id and not
isapnp_device_id.

Matthieu


2009-11-25 15:25:36

by Kay Sievers

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Wed, Nov 25, 2009 at 15:42, Matthieu CASTET <[email protected]> wrote:
> Kay Sievers <kay.sievers <at> vrfy.org> writes:
>>
>> No, we don't even have proper modaliases for the pnp bus
>> (/sys/bus/pnp) and the alias string for pnp in the modules have a
>> broken and unfixable format. PNP autoloading is all handled by the
>> acpi modaliases these days. This is the first time in years I hear
>> anybody asking for hotplug setups of plain old pnp devices. :)
>>
> The naming is really bad. This as nothing to do with acpi : pnp and pnp_cards
> entries are duplicated to another naming. And this should work on system
> without
> acpi.
> This should have been named to pnp2 or something like that.

I don' think there is anything like a second version, it's just that
acpi handles the same pnp ids natively, so we added (non-broken)
matches for it, which works properly on almost all current boxes. If
someone wants to finally fix pnp and get proper modalias working, the
acpi alias generation in the pnp modules can be reverted without
trouble.

> Now I don't see why isapnp_device_id and pnp_card_device_id can't be merged
> together.
>
> For example most of alsa isa card use pnp_card_device_id and not
> isapnp_device_id.

It's just that nobody really cared, I guess. I wouldn't be surprised,
if this is just another round of "discussion" like we had so many
about this topic the past years, and nothing will happen to
pnp/isapnp. :)

Kay

2009-11-25 17:18:01

by Ondrej Zary

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Tuesday 24 November 2009 09:53:51 Kay Sievers wrote:
> On Mon, Nov 23, 2009 at 22:51, Rusty Russell <[email protected]> wrote:
> > On Mon, 23 Nov 2009 07:10:22 pm Takashi Iwai wrote:
> >> At Mon, 23 Nov 2009 13:59:53 +1030, Rusty Russell wrote:
> >> > Two things are required:
> >> > 1) Modify file2alias to add aliases for isapnp.  This is pretty easy.
> >> > 2) Expose the isapnp devices in sysfs where udev will match them up
> >> >    (/sys/bus/isa/devices/<dev>/modalias I guess?)
> >>
> >> There is non-pnp ISA bus, so I'm afraid "isa" may conflict.
> >> I suppose "isapnp" would be a safer choice.
> >
> > Without pnp, I don't think you can enumerate the ISA bus. Hence I chose
> > "isa:" in the hope that udev would "just work" if isapnp created the
> > modalias under /sys/bus/isa...
> >
> > But Kay says there's a pnp bus (who knew?).  I don't know what he means
> > by unfixable aliases tho, since it's all in the kernel source so we can
> > change it at any time?
>
> Here's a bit of the background:
>
> The aliases in the modules can only match a single value, but PNP
> devices often have vendor specific IDs and usual IDs describing the
> function.
>
> Like here, where the interesting ID is only the second one of the two
> for a single device:
> $ grep . /sys/bus/pnp/devices/*/id
> /sys/bus/pnp/devices/00:09/id:LEN0006
> /sys/bus/pnp/devices/00:09/id:PNP0f13

It's the same for my RTL8019 card:
$ grep . /sys/bus/pnp/devices/*/id
/sys/bus/pnp/devices/01:01.00/id:RTL8019
/sys/bus/pnp/devices/01:01.00/id:PNP80d6

and it works with this (which could be easily generated from the current ne
module, I think):
alias pnp:dPNP80d6* ne

This works fine too:
alias pnp:dRTL8019* ne
alias pnp:dPNP80d6* ne

> So the kernel device would need to compose a "modalias" string with
> all IDs in a single line. But the in-module aliases can unfortunately
> match only on a single value, like:
> alias pnp:dPNP0700* floppy
>
> For that reason, a while ago udev switched to acpi aliases entirely,
> and dropped all pnp: usage. The acpi aliases can handle multi-values
> just fine with single strings like:
> $ cat /sys/bus/acpi/devices/LEN0006:00/modalias
> acpi:LEN0006:PNP0F13:
>
> alias acpi*:PNP0700:* floppy
>
> Udev does not use any pnp: or /sys/bus/pnp/ values since a while. But
> there might be still distros who use the (broken) pnp: aliases and
> ship the (also broken) shell scripts to iterate over the sysfs devices
> and call modprobe for all they find.
>
> Thanks,
> Kay



--
Ondrej Zary

2009-12-18 19:52:58

by Ondrej Zary

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Monday 23 November 2009 04:29:53 Rusty Russell wrote:
> On Mon, 23 Nov 2009 07:31:57 am Ondrej Zary wrote:
> > The problem is that
> > scripts/mod/file2alias.c simply ignores isapnp.
>
> AFAICT it always has, and noone has complained until now. Perhaps
> something was still reading /lib/modules/`uname -r`/modules.isapnpmap?
>
> Two things are required:
> 1) Modify file2alias to add aliases for isapnp. This is pretty easy.
> 2) Expose the isapnp devices in sysfs where udev will match them up
> (/sys/bus/isa/devices/<dev>/modalias I guess?)
>
> The first patches would look something like these. We could mangle the
> PNP names a-la ISAPNP_VENDOR/ISAPNP_DEVICE, but as long as the published
> modalias matches the file2alias result, it doesn't matter.
>
> Jaroslav? Greg? Patches for sysfs welcome...
>
> Thanks,
> Rusty.
> PS. Jon, perhaps the new decade is a good time to kill modules.*map?
>
> isapnp: move definitions to mod_devicetable.h so file2alias can reach them.
>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/include/linux/isapnp.h b/include/linux/isapnp.h
> --- a/include/linux/isapnp.h
> +++ b/include/linux/isapnp.h
> @@ -43,10 +43,10 @@
> */
>
> #ifdef __KERNEL__
> +#include <linux/mod_devicetable.h>
>
> #define DEVICE_COUNT_COMPATIBLE 4
>
> -#define ISAPNP_ANY_ID 0xffff
> #define ISAPNP_CARD_DEVS 8
>
> #define ISAPNP_CARD_ID(_va, _vb, _vc, _device) \
> @@ -74,12 +74,6 @@ struct isapnp_card_id {
> #define ISAPNP_DEVICE_SINGLE_END \
> .card_vendor = 0, .card_device = 0
>
> -struct isapnp_device_id {
> - unsigned short card_vendor, card_device;
> - unsigned short vendor, function;
> - unsigned long driver_data; /* data private to the driver */
> -};
> -
> #if defined(CONFIG_ISAPNP) || (defined(CONFIG_ISAPNP_MODULE) &&
> defined(MODULE))
>
> #define __ISAPNP__
> diff --git a/include/linux/mod_devicetable.h
> b/include/linux/mod_devicetable.h --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -474,4 +474,11 @@ struct platform_device_id {
> __attribute__((aligned(sizeof(kernel_ulong_t))));
> };
>
> +#define ISAPNP_ANY_ID 0xffff
> +struct isapnp_device_id {
> + unsigned short card_vendor, card_device;
> + unsigned short vendor, function;
> + kernel_ulong_t driver_data; /* data private to the driver */
> +};
> +
> #endif /* LINUX_MOD_DEVICETABLE_H */
>
>
> isapnp: put aliases in .modinfo so modinfo can find them.
>
> Once isa devices publish a modalias field, I think udev should "Just Work".
>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -727,6 +727,19 @@ static int do_platform_entry(const char
> return 1;
> }
>
> +/* Format is: isa:cvNcdNvNfN */
> +static int do_isapnp_entry(const char *filename,
> + struct isapnp_device_id *id, char *alias)
> +{
> + strcpy(alias, "isa:");
> + ADD(alias, "cv", id->card_vendor != ISAPNP_ANY_ID, id->card_vendor);
> + ADD(alias, "cd", id->card_device != ISAPNP_ANY_ID, id->card_device);
> + ADD(alias, "v", id->vendor != ISAPNP_ANY_ID, id->vendor);
> + ADD(alias, "f", id->function, id->function);
> + add_wildcard(alias);
> + return 1;
> +}
> +
> /* Ignore any prefix, eg. some architectures prepend _ */
> static inline int sym_is(const char *symbol, const char *name)
> {
> @@ -874,6 +887,10 @@ void handle_moddevtable(struct module *m
> do_table(symval, sym->st_size,
> sizeof(struct platform_device_id), "platform",
> do_platform_entry, mod);
> + else if (sym_is(symname, "__mod_isapnp_device_table"))
> + do_table(symval, sym->st_size,
> + sizeof(struct isapnp_device_id), "isa",
> + do_isapnp_entry, mod);
> free(zeros);
> }

The above patch did not work. However, the patch below works fine (at least
with Debian). It needs your first patch that moves the definitions to
mod_devicetable.h. Verified that aliases for these modules are generated
correctly:
drivers/media/radio/radio-sf16fmi.c
drivers/net/ne.c
drivers/net/3c515.c
drivers/net/smc-ultra.c
drivers/pcmcia/i82365.c
drivers/scsi/aha1542.c
drivers/scsi/aha152x.c
drivers/scsi/sym53c416.c
drivers/scsi/g_NCR5380.c

Tested with RTL8019AS (ne), AVA-1505AE (aha152x) and dtc436e (g_NCR5380)
cards - they now work automatically.

Found that drivers/isdn/hisax/hisax_fcpcipnp.c has broken pnp device table -
wrong type (isapnp instead of pnp) and also ending record missing.


Generate pnp:d aliases for isapnp_device_tables. This allows udev to load
these modules automatically.

Signed-off-by: Ondrej Zary <[email protected]>

--- linux-source-2.6.32-orig/scripts/mod/file2alias.c 2009-12-03 04:51:21.000000000 +0100
+++ linux-source-2.6.32/scripts/mod/file2alias.c 2009-12-18 20:20:57.000000000 +0100
@@ -727,6 +727,19 @@ static int do_platform_entry(const char
return 1;
}

+/* looks like: "pnp:dD" */
+static int do_isapnp_entry(const char *filename,
+ struct isapnp_device_id *id, char *alias)
+{
+ sprintf(alias, "pnp:d%c%c%c%x%x%x%x*",
+ 'A' + ((id->vendor >> 2) & 0x3f) - 1,
+ 'A' + (((id->vendor & 3) << 3) | ((id->vendor >> 13) & 7)) - 1,
+ 'A' + ((id->vendor >> 8) & 0x1f) - 1,
+ (id->function >> 4) & 0x0f, id->function & 0x0f,
+ (id->function >> 12) & 0x0f, (id->function >> 8) & 0x0f);
+ return 1;
+}
+
/* Ignore any prefix, eg. some architectures prepend _ */
static inline int sym_is(const char *symbol, const char *name)
{
@@ -874,6 +887,10 @@ void handle_moddevtable(struct module *m
do_table(symval, sym->st_size,
sizeof(struct platform_device_id), "platform",
do_platform_entry, mod);
+ else if (sym_is(symname, "__mod_isapnp_device_table"))
+ do_table(symval, sym->st_size,
+ sizeof(struct isapnp_device_id), "isa",
+ do_isapnp_entry, mod);
free(zeros);
}



hisax_fcpcipnp: fix broken isapnp device table.

--- linux-source-2.6.32-orig/drivers/isdn/hisax/hisax_fcpcipnp.c 2009-12-03 04:51:21.000000000 +0100
+++ linux-source-2.6.32/drivers/isdn/hisax/hisax_fcpcipnp.c 2009-12-17 23:55:08.000000000 +0100
@@ -74,9 +74,10 @@ static struct pnp_device_id fcpnp_ids[]
.id = "AVM0900",
.driver_data = (unsigned long) "Fritz!Card PnP",
},
+ { .id = "" }
};

-MODULE_DEVICE_TABLE(isapnp, fcpnp_ids);
+MODULE_DEVICE_TABLE(pnp, fcpnp_ids);
#endif

static int protocol = 2; /* EURO-ISDN Default */


--
Ondrej Zary

2009-12-21 10:24:27

by Rusty Russell

[permalink] [raw]
Subject: Re: MODULE_DEVICE_TABLE(isapnp, ...) does nothing

On Sat, 19 Dec 2009 06:22:39 am Ondrej Zary wrote:
> Generate pnp:d aliases for isapnp_device_tables. This allows udev to load
> these modules automatically.
>
> Signed-off-by: Ondrej Zary <[email protected]>

I'm happy with this, so I've separated and taken them. Too late for the
merge window I'm afraid, but it'll be in next one.

Thanks,
Rusty.