more on opl3sa2

more tests with snd-opl3sa2. i have to use the paramers to modprobe since pnp doesn't work. the reason:
pnpc_register_driver sets driver.bus to its own static struct which doesn't have a device list. driver_attach tries to scan this device list and compare with the drivers device list. since there's no one for the bus it fails, the driver can't see it's devices.

Call stack:
driver_attach
bus_add_driver
driver_register
pnpc_register_driver
alsa_card_opl3sa_init

ok, tried by hand with the config data from bios/lspnp:

# modprobe snd-opl3sa2 isapnp=0 port=0x538 wss_port=0x530 sb_port=0x220
fm_port=0x388 midi_port=0x330 irq=5 dma1=0 dma2=1 Segmentation fault

Unable to handle kernel NULL pointer dereference at virtual address
00000144 printing eip:
d08aecfb
*pde = 00000000
Oops: 0002
CPU:    0
EIP:    0060:[<d08aecfb>]    Not tainted
EFLAGS: 00010246

EIP is at snd_opl3sa2_probe+0x3bb/0x3e4 [snd_opl3sa2]
eax: 00000000 ebx: cd9ace00 ecx: d089b734 edx: 00000000
esi: 00000000 edi: cd9ace6c ebp: c7e11f84 esp: c7e11f58
ds: 007b es: 007b ss: 0068
Process modprobe (pid: 1816, threadinfo=c7e10000 task=ce0527e0)

Stack: 00000000 00000000 00000000 00000000 cd9ace24 00000000 00000001
00000000 00000005 c82fec00 c9d5fcc4 c7e11fa8 d08b33c2 00000000 00000000
00000000 00000000 d08b0fe0 c027e304 c027e304 c7e11fbc c01291a2 0804ea38
0804e008 Call Trace:
 [<d08b33c2>] alsa_card_opl3sa2_init+0x2a/0xffffe3cc [snd_opl3sa2]
 [<d08b0fe0>] +0x0/0x100 [snd_opl3sa2]
 [<c01291a2>] sys_init_module+0x116/0x1ac
 [<c010a75b>] syscall_call+0x7/0xb

Code: 89 9a 44 01 00 00 8b 45 e8 89 98 60 0c 8b d0 31 c0 eb 0a 89

100% reproducible.

machine is a toshiba tecra 8000 laptop (p3-500), kernel is 2.5.59 + vmlinux.lds.h patch + sound-firmware-load-fix + jaroslav's driver patch + adam's pnp.h id len patch.

kernel config: no acpi, _all_ pnp options enabled. soundcore compiled in, alsa as module.

# lspci
00:00.0 Host bridge: Intel Corporation 440BX/ZX - 82443BX/ZX Host bridge
(AGP disabled) (rev 03) 00:04.0 VGA compatible controller: Neomagic
Corporation [MagicMedia 256AV] (rev 12) 00:05.0 Bridge: Intel Corporation
82371AB PIIX4 ISA (rev 02)
00:05.1 IDE interface: Intel Corporation 82371AB PIIX4 IDE (rev 01)
00:05.2 USB Controller: Intel Corporation 82371AB PIIX4 USB (rev 01)
00:05.3 Bridge: Intel Corporation 82371AB PIIX4 ACPI (rev 02)
00:09.0 Communication controller: Toshiba America Info Systems FIR Port
(rev 23) 00:0b.0 CardBus bridge: Toshiba America Info Systems ToPIC97 (rev
05) 00:0b.1 CardBus bridge: Toshiba America Info Systems ToPIC97 (rev 05)

# lspnp
01 PNP0c01 memory controller: RAM
02 PNP0200 system peripheral: DMA controller
03 PNP0000 system peripheral: programmable interrupt controller
04 PNP0100 system peripheral: system timer
05 PNP0800 system peripheral: other
06 PNP0c04 reserved: other
07 PNP0303 input device: keyboard
08 PNP0f13 input device: mouse
09 PNP0b00 system peripheral: real time clock
0a PNP0c02 system peripheral: other
0b PNP0700 mass storage device: floppy
0e PNP0501 communications device: RS-232
10 PNP0401 communications device: AT parallel port
11 PNP0a03 bridge controller: PCI
14 PNP0e03 bridge controller: PCMCIA
15 YMH0021 multimedia controller: audio

# lspnp -vv 15
15 YMH0021 multimedia controller: audio
    flags: [dynamic]
    allocated resources:
        io 0x0220-0x0233 [16-bit decode]
        io 0x0530-0x0537 [16-bit decode]
        io 0x0388-0x038f [16-bit decode]
        io 0x0330-0x0333 [16-bit decode]
        io 0x0538-0x0539 [16-bit decode]
        irq 5 [high edge]
        dma 0 [8 bit] [count byte] [compat]
        dma 1 [8 bit] [count byte] [compat]
    possible resources:
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0530-0x0537 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0538-0x0539 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma mask 0x000b [8 bit] [count byte] [compat]
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0530-0x0537 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0538-0x0539 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma disabled [8 bit] [count byte] [compat]
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0540-0x0547 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0548-0x0549 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma mask 0x000b [8 bit] [count byte] [compat]
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0540-0x0547 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0548-0x0549 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma disabled [8 bit] [count byte] [compat]
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0550-0x0557 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0558-0x0559 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma mask 0x000b [8 bit] [count byte] [compat]
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0550-0x0557 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0558-0x0559 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma disabled [8 bit] [count byte] [compat]
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0560-0x0567 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0568-0x0569 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma mask 0x000b [8 bit] [count byte] [compat]
        [start dep fn]
        io base 0x0220-0x0280 align 0x20 len 0x14 [16-bit decode]
        io 0x0560-0x0567 [16-bit decode]
        io base 0x0388-0x03b8 align 0x10 len 0x08 [16-bit decode]
        io base 0x0300-0x0330 align 0x10 len 0x04 [16-bit decode]
        io 0x0568-0x0569 [16-bit decode]
        irq mask 0x0ca0 [high edge]
        dma mask 0x000b [8 bit] [count byte] [compat]
        dma disabled [8 bit] [count byte] [compat]
        [end dep fn]
    compatible devices:
        identifier 'OPL3-SA3 Sound System'

if you need more info or if you want me to test patches...just ask

rgds,
-daniel

ok, found that one...very simple:
against 2.5.59 + jaroslav's driver patch...

sound/isa/opl3sa2.c renamed from sound/isa/opl3sa2.c~ +2 -1

@@ -830,7 +830,8 @@
 	if ((err = snd_card_register(card)) < 0)
 		goto __error;
 
-	pnpc_set_drvdata(pcard, card);
+	if (pcard)
+		pnpc_set_drvdata(pcard, card);
 	snd_opl3sa2_cards[dev] = card;
 	return 0;
 

the card is not detected by pnp, that problem stays. is that a problem of the pnp layer or is my toshiba laptop just so damn stupid??

Quoted 2 lines

the card is not detected by pnp, that problem stays. is that a problem of the pnp layer or is my toshiba laptop just so damn stupid??

Nope. It's fault of the driver. It scans for a card. Actually, the structure card -> devices is created only by the ISA PnP driver.

I don't see any reason to not group the PnP BIOS devices into one "card", too. Adam, do you have any comments?

Quoted 9 lines · Jaroslav Kysela

Quoted 2 lines

the card is not detected by pnp, that problem stays. is that a problem of the pnp layer or is my toshiba laptop just so damn stupid??

Nope. It's fault of the driver. It scans for a card. Actually, the structure card -> devices is created only by the ISA PnP driver.

I don't see any reason to not group the PnP BIOS devices into one "card", too. Adam, do you have any comments?

I have considered this approach several times. However, there are the following problems with representing the pnpbios devices under one card:

1.) If a driver attaches to the pnpbios card all other card-based drivers will be unable to use the pnpbios. One will attach and cause the others to fail. It is possible for the user to have more than one pnpbios sound card but with this approach such a user would only be able to use one sound device from the entire pnpbios.

2.) Doing so would misrepresent the pnpbios topology because it physically doesn't have any cards.

3.) The opl3sa2 driver doesn't need a card because it is only asking for one device anyway. Using the card interface puts unnecessary overhead on both the driver and the pnp layer.

Quoted 5 lines · Adam Belay

1.) If a driver attaches to the pnpbios card all other card-based drivers will be unable to use the pnpbios. One will attach and cause the others to fail. It is possible for the user to have more than one pnpbios sound card but with this approach such a user would only be able to use one sound device from the entire pnpbios.

I see. I think it's a design problem then. The rule card -> one driver is bad. We need something between card and device which will take care about drivers. Unfortunately, this information is dynamic (only driver knows which devices have to be attached).

I think that we need to discuss this thing very carefully.

Quoted 7 lines

2.) Doing so would misrepresent the pnpbios topology because it physically doesn't have any cards.

3.) The opl3sa2 driver doesn't need a card because it is only asking for one device anyway. Using the card interface puts unnecessary overhead on both the driver and the pnp layer.

Yes, but IT SHOULD WORK. Although it isn't an most efficient way. (I personally think that it's better to keep as much IDs as possible to avoid clashes in future).

How does this sound...
1.) detach pnp card service matching from the driver model, the driver model is what is imposing this one card per driver limit.
2.) create a special pnp_driver that handles cards and forwards driver model calls to the pnp card services, we can use attach_driver to avoid matching problems.

design goals for these changes should be as follows:
1.) multiple drivers can bind to one card
2.) pnp_attach, pnp_detach, and pnp status should be phased out and replaced with the special card driver, in other words the driver model can take care of this.

Quoted 10 lines · Adam Belay

How does this sound...
1.) detach pnp card service matching from the driver model, the driver model is what is imposing this one card per driver limit.
2.) create a special pnp_driver that handles cards and forwards driver model calls to the pnp card services, we can use attach_driver to avoid matching problems.

design goals for these changes should be as follows:
1.) multiple drivers can bind to one card
2.) pnp_attach, pnp_detach, and pnp status should be phased out and replaced with the special card driver, in other words the driver model can take care of this.

First of all I admit that I haven't been following closely, so I maybe way off.

Anyway, the old ISAPnP used, AFAIR, struct pci_bus for the card and struct pci_device for the devices. So what's wrong with using the basically the same abstraction with the driver model, which has buses and devices as well. That means each device can have its own driver, and I suppose that should be good enough (as opposed to only one driver per card).

But probably I'm missing something?

Quoted 7 lines · Jaroslav Kysela

Quoted 5 lines · Adam Belay

1.) If a driver attaches to the pnpbios card all other card-based drivers will be unable to use the pnpbios. One will attach and cause the others to fail. It is possible for the user to have more than one pnpbios sound card but with this approach such a user would only be able to use one sound device from the entire pnpbios.

Quoted 6 lines

I see. I think it's a design problem then. The rule card -> one driver is bad. We need something between card and device which will take care about drivers. Unfortunately, this information is dynamic (only driver knows which devices have to be attached).

I think that we need to discuss this thing very carefully.

I agree that we need to discuss this carefully and that there is a need for this change. Here are a few ideas to get a discussion started.

How does this sound...
1.) detach pnp card service matching from the driver model, the driver model is what is imposing this one card per driver limit.
2.) create a special pnp_driver that handles cards and forwards driver model calls to the pnp card services, we can use attach_driver to avoid matching problems.

design goals for these changes should be as follows:
1.) multiple drivers can bind to one card
2.) pnp_attach, pnp_detach, and pnp status should be phased out and replaced with the special card driver, in other words the driver model can take care of this.

This will solve the one device limit but I'm not sure how to handle the pnpbios stuff yet. I want it to be with as little overhead as possible and would prefer we don't use fake cards, any ideas? All that comes to mind for me is a unique pnpbios ID that the pnp layer will interpret and grant special exceptions.

Quoted 10 lines

Quoted 6 lines

2.) Doing so would misrepresent the pnpbios topology because it physically doesn't have any cards.

3.) The opl3sa2 driver doesn't need a card because it is only asking for one device anyway. Using the card interface puts unnecessary overhead on both the driver and the pnp layer.

Yes, but IT SHOULD WORK. Although it isn't an most efficient way. (I personally think that it's better to keep as much IDs as possible to avoid clashes in future).

Hmm, I'm not sure what you mean by clashes in the future. Nearly all of these isapnp devices are no longer manufactured, hence these devices will not have many future changes.

Still I do see value in describing as many ids as possible but I worry that the extra overhead may not be worth it. This also needs to be further discussed.