ISAPNP/ALSA hasn't been working for a while now (cards didn't get
detected), here is a patch to update OPL3SA2 to the new PnP layer.
ALSA device list:
#0: Yamaha OPL3-SA23 at 0x100, irq 5, dma 1&3
Driver tested in kernel and modular, i have tried to maintain
detection/resource override semantics and general detection code
paths.
Regards,
Zwane Mwaikambo
Index: linux-2.5.50/sound/isa/opl3sa2.c
===================================================================
RCS file: /build/cvsroot/linux-2.5.50/sound/isa/opl3sa2.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 opl3sa2.c
--- linux-2.5.50/sound/isa/opl3sa2.c 28 Nov 2002 01:36:04 -0000 1.1.1.1
+++ linux-2.5.50/sound/isa/opl3sa2.c 30 Nov 2002 11:51:30 -0000
@@ -24,6 +24,7 @@
#include <linux/interrupt.h>
#include <linux/pm.h>
#include <linux/slab.h>
+#include <linux/pnp.h>
#ifndef LINUX_ISAPNP_H
#include <linux/isapnp.h>
#define isapnp_card pci_bus
@@ -135,6 +136,7 @@
typedef struct snd_opl3sa2 opl3sa2_t;
#define chip_t opl3sa2_t
+int nr_cards;
struct snd_opl3sa2 {
snd_card_t *card;
@@ -148,7 +150,7 @@
snd_rawmidi_t *rmidi;
cs4231_t *cs4231;
#ifdef __ISAPNP__
- struct isapnp_dev *dev;
+ struct pnp_dev *dev;
#endif
unsigned char ctlregs[0x20];
int ymode; /* SL added */
@@ -164,30 +166,28 @@
static snd_card_t *snd_opl3sa2_cards[SNDRV_CARDS] = SNDRV_DEFAULT_PTR;
#ifdef __ISAPNP__
+static struct pnp_dev *snd_opl3sa2_isapnp_devs[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
-static struct isapnp_card *snd_opl3sa2_isapnp_cards[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
-static const struct isapnp_card_id *snd_opl3sa2_isapnp_id[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
-
-#define ISAPNP_OPL3SA2(_va, _vb, _vc, _device, _function) \
- { \
- ISAPNP_CARD_ID(_va, _vb, _vc, _device), \
- devs : { ISAPNP_DEVICE_ID(_va, _vb, _vc, _function), } \
- }
-
-static struct isapnp_card_id snd_opl3sa2_pnpids[] __devinitdata = {
+/* Please try and keep the 1:1 mapping with the dev listing below */
+static const struct pnp_id pnp_opl3sa2_cards[] = {
/* Yamaha YMF719E-S (Genius Sound Maker 3DX) */
- ISAPNP_OPL3SA2('Y','M','H',0x0020,0x0021),
+ {.id = "YMH0020", .driver_data = 0},
/* Yamaha OPL3-SA3 (integrated on Intel's Pentium II AL440LX motherboard) */
- ISAPNP_OPL3SA2('Y','M','H',0x0030,0x0021),
+ {.id = "YMH0030", .driver_data = 0},
/* Yamaha OPL3-SA2 */
- ISAPNP_OPL3SA2('Y','M','H',0x0800,0x0021),
+ {.id = "YMH0800", .driver_data = 0},
/* NeoMagic MagicWave 3DX */
- ISAPNP_OPL3SA2('N','M','X',0x2200,0x2210),
- /* --- */
- { ISAPNP_CARD_END, } /* end */
+ {.id = "NMX2200", .driver_data = 0},
+ {"", 0}
};
-ISAPNP_CARD_TABLE(snd_opl3sa2_pnpids);
+static const struct pnp_id pnp_opl3sa2_devs[] = {
+ {.id = "YMH0021", .driver_data = 0},
+ {.id = "YMH0021", .driver_data = 0},
+ {.id = "YMH0021", .driver_data = 0},
+ {.id = "NMX2210", .driver_data = 0},
+ {"", 0}
+};
#endif /* __ISAPNP__ */
@@ -634,22 +634,42 @@
#endif /* CONFIG_PM */
+static int snd_opl3sa2_free(opl3sa2_t *chip)
+{
+#ifdef __ISAPNP__
+ pnp_disable_dev(chip->dev);
+ pnp_remove_device(chip->dev);
+#endif
+#ifdef CONFIG_PM
+ if (chip->pm_dev)
+ pm_unregister(chip->pm_dev);
+#endif
+ if (chip->irq >= 0)
+ free_irq(chip->irq, (void *)chip);
+ if (chip->res_port) {
+ release_resource(chip->res_port);
+ kfree_nocheck(chip->res_port);
+ }
+
+ snd_magic_kfree(chip);
+ return 0;
+}
+
+static int snd_opl3sa2_dev_free(snd_device_t *device)
+{
+ opl3sa2_t *chip = snd_magic_cast(opl3sa2_t, device->device_data, return -ENXIO);
+ return snd_opl3sa2_free(chip);
+}
+
#ifdef __ISAPNP__
static int __init snd_opl3sa2_isapnp(int dev, opl3sa2_t *chip)
{
- const struct isapnp_card_id *id = snd_opl3sa2_isapnp_id[dev];
- struct isapnp_card *card = snd_opl3sa2_isapnp_cards[dev];
- struct isapnp_dev *pdev;
+ int res = 0;
+ struct pnp_dev *pdev = snd_opl3sa2_isapnp_devs[dev];
+
+ if (!enable[dev])
+ return -ENODEV;
- chip->dev = isapnp_find_dev(card, id->devs[0].vendor, id->devs[0].function, NULL);
- if (chip->dev->active) {
- chip->dev = NULL;
- return -EBUSY;
- }
- /* PnP initialization */
- pdev = chip->dev;
- if (pdev->prepare(pdev)<0)
- return -EAGAIN;
if (sb_port[dev] != SNDRV_AUTO_PORT)
isapnp_resource_change(&pdev->resource[0], sb_port[dev], 16);
if (wss_port[dev] != SNDRV_AUTO_PORT)
@@ -666,59 +686,27 @@
isapnp_resource_change(&pdev->dma_resource[1], dma2[dev], 1);
if (irq[dev] != SNDRV_AUTO_IRQ)
isapnp_resource_change(&pdev->irq_resource[0], irq[dev], 1);
- if (pdev->activate(pdev)<0) {
- snd_printk("isapnp configure failure (out of resources?)\n");
- return -EBUSY;
- }
- sb_port[dev] = pdev->resource[0].start;
- wss_port[dev] = pdev->resource[1].start;
- fm_port[dev] = pdev->resource[2].start;
- midi_port[dev] = pdev->resource[3].start;
- port[dev] = pdev->resource[4].start;
+
+ sb_port[dev] = pci_resource_start(pdev, 0);
+ wss_port[dev] = pci_resource_start(pdev, 1);
+ fm_port[dev] = pci_resource_start(pdev, 2);
+ midi_port[dev] = pci_resource_start(pdev, 3);
+ port[dev] = pci_resource_start(pdev, 4);
dma1[dev] = pdev->dma_resource[0].start;
dma2[dev] = pdev->dma_resource[1].start;
irq[dev] = pdev->irq_resource[0].start;
+
snd_printdd("isapnp OPL3-SA: sb port=0x%lx, wss port=0x%lx, fm port=0x%lx, midi port=0x%lx\n",
sb_port[dev], wss_port[dev], fm_port[dev], midi_port[dev]);
snd_printdd("isapnp OPL3-SA: control port=0x%lx, dma1=%i, dma2=%i, irq=%i\n",
port[dev], dma1[dev], dma2[dev], irq[dev]);
- return 0;
-}
-
-static void snd_opl3sa2_deactivate(opl3sa2_t *chip)
-{
- if (chip->dev) {
- chip->dev->deactivate(chip->dev);
- chip->dev = NULL;
- }
+ chip->dev = pdev;
+
+out_error:
+ return res;
}
#endif /* __ISAPNP__ */
-static int snd_opl3sa2_free(opl3sa2_t *chip)
-{
-#ifdef __ISAPNP__
- snd_opl3sa2_deactivate(chip);
-#endif
-#ifdef CONFIG_PM
- if (chip->pm_dev)
- pm_unregister(chip->pm_dev);
-#endif
- if (chip->irq >= 0)
- free_irq(chip->irq, (void *)chip);
- if (chip->res_port) {
- release_resource(chip->res_port);
- kfree_nocheck(chip->res_port);
- }
- snd_magic_kfree(chip);
- return 0;
-}
-
-static int snd_opl3sa2_dev_free(snd_device_t *device)
-{
- opl3sa2_t *chip = snd_magic_cast(opl3sa2_t, device->device_data, return -ENXIO);
- return snd_opl3sa2_free(chip);
-}
-
static int __init snd_opl3sa2_probe(int dev)
{
int xirq, xdma1, xdma2;
@@ -763,7 +751,9 @@
err = -ENOMEM;
goto __error;
}
+ spin_lock_init(&chip->reg_lock);
chip->irq = -1;
+
if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0)
goto __error;
#ifdef __ISAPNP__
@@ -855,30 +845,47 @@
}
#ifdef __ISAPNP__
-static int __init snd_opl3sa2_isapnp_detect(struct isapnp_card *card,
- const struct isapnp_card_id *id)
+static int pnp_opl3sa2_probe(struct pnp_dev *pdev, const struct pnp_id *card_id,
+ const struct pnp_id *dev_id)
{
- static int dev;
+ static int dev;
int res;
for ( ; dev < SNDRV_CARDS; dev++) {
if (!enable[dev])
continue;
- snd_opl3sa2_isapnp_cards[dev] = card;
- snd_opl3sa2_isapnp_id[dev] = id;
+ snd_opl3sa2_isapnp_devs[dev] = pdev;
res = snd_opl3sa2_probe(dev);
if (res < 0)
return res;
dev++;
+ nr_cards++;
return 0;
}
return -ENODEV;
}
+
+static void pnp_opl3sa2_remove(struct pnp_dev *pdev)
+{
+ opl3sa2_t *chip = pdev->driver_data;
+ if (chip)
+ chip->dev = NULL;
+ nr_cards--;
+}
+
+static struct pnp_driver snd_opl3sa2_pnp_driver = {
+ .name = "opl3sa2",
+ .card_id_table = pnp_opl3sa2_cards,
+ .id_table = pnp_opl3sa2_devs,
+ .probe = pnp_opl3sa2_probe,
+ .remove = pnp_opl3sa2_remove,
+};
+
#endif /* __ISAPNP__ */
static int __init alsa_card_opl3sa2_init(void)
{
- int dev, cards = 0;
+ int dev;
for (dev = 0; dev < SNDRV_CARDS; dev++) {
if (!enable[dev])
@@ -888,15 +895,19 @@
continue;
#endif
if (snd_opl3sa2_probe(dev) >= 0)
- cards++;
+ nr_cards++;
}
+
#ifdef __ISAPNP__
- cards += isapnp_probe_cards(snd_opl3sa2_pnpids, snd_opl3sa2_isapnp_detect);
+ /* upon registration the probe function will increment nr_cards */
+ pnp_register_driver(&snd_opl3sa2_pnp_driver);
#endif
- if (!cards) {
+
+ if (nr_cards == 0) {
#ifdef MODULE
printk(KERN_ERR "Yamaha OPL3-SA soundcard not found or device busy\n");
#endif
+ pnp_unregister_driver(&snd_opl3sa2_pnp_driver);
return -ENODEV;
}
return 0;
--
function.linuxpower.ca
On Sat, Nov 30, 2002 at 07:00:13AM -0500, Zwane Mwaikambo wrote:
> ISAPNP/ALSA hasn't been working for a while now (cards didn't get
> detected), here is a patch to update OPL3SA2 to the new PnP layer.
>
> ALSA device list:
> #0: Yamaha OPL3-SA23 at 0x100, irq 5, dma 1&3
>
> Driver tested in kernel and modular, i have tried to maintain
> detection/resource override semantics and general detection code
> paths.
>
> Regards,
> Zwane Mwaikambo
Hi Zwane,
Many changes are pending that will allow you to manage entire pnp
cards. For devices like opl3sa2, with only one device, you don't
have to use this, though I would prefer you do for consistency with
the other alsa drivers and because it has better card-specific
matching. I'll send a big patch out with all of these changes.
They should be in the bk tree soon as well. I apologize for any
difficulty this may have caused.
Much of this driver has already been converted by me and has been
submitted publicly on the lkml. What remains, however is the
http://marc.theaimsgroup.com/?l=linux-kernel&m=103844524222955&w=2
remove code. I like how you did yours in this patch but it doesn't
seem to release anything. I included comments to help you better
understand how to convert pnp drivers. I hope they help.
Thanks for your effort. I'd love to see more pnp drivers converted.
If you have any questions feel free to ask.
Regards,
Adam
>
> Index: linux-2.5.50/sound/isa/opl3sa2.c
> ===================================================================
> RCS file: /build/cvsroot/linux-2.5.50/sound/isa/opl3sa2.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 opl3sa2.c
> --- linux-2.5.50/sound/isa/opl3sa2.c 28 Nov 2002 01:36:04 -0000 1.1.1.1
> +++ linux-2.5.50/sound/isa/opl3sa2.c 30 Nov 2002 11:51:30 -0000
> @@ -24,6 +24,7 @@
> #include <linux/interrupt.h>
> #include <linux/pm.h>
> #include <linux/slab.h>
> +#include <linux/pnp.h>
> #ifndef LINUX_ISAPNP_H
> #include <linux/isapnp.h>
isapnp.h isn't even needed. Please don't use it.
instead only use pnp.h.
> #define isapnp_card pci_bus
this is wrong, it should be pnp_card. I actually
got rid of this definition entirely in my patch
and used pnp_card.
> @@ -135,6 +136,7 @@
>
> typedef struct snd_opl3sa2 opl3sa2_t;
> #define chip_t opl3sa2_t
> +int nr_cards;
>
> struct snd_opl3sa2 {
> snd_card_t *card;
> @@ -148,7 +150,7 @@
> snd_rawmidi_t *rmidi;
> cs4231_t *cs4231;
> #ifdef __ISAPNP__
the definition __ISAPNP__ should not be used, use
CONFIG_PNP or CONFIG_PNP_CARD instead. I've only
mentioned this for this instance but it is used
throughout your patch. Actually, now that I think
about it I need to change my patch to use
CONFIG_PNP_CARD isntead of CONFIG_PNP.
> - struct isapnp_dev *dev;
> + struct pnp_dev *dev;
> #endif
> unsigned char ctlregs[0x20];
> int ymode; /* SL added */
> @@ -164,30 +166,28 @@
> static snd_card_t *snd_opl3sa2_cards[SNDRV_CARDS] = SNDRV_DEFAULT_PTR;
>
> #ifdef __ISAPNP__
> +static struct pnp_dev *snd_opl3sa2_isapnp_devs[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
>
> -static struct isapnp_card *snd_opl3sa2_isapnp_cards[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
> -static const struct isapnp_card_id *snd_opl3sa2_isapnp_id[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
> -
> -#define ISAPNP_OPL3SA2(_va, _vb, _vc, _device, _function) \
> - { \
> - ISAPNP_CARD_ID(_va, _vb, _vc, _device), \
> - devs : { ISAPNP_DEVICE_ID(_va, _vb, _vc, _function), } \
> - }
> -
> -static struct isapnp_card_id snd_opl3sa2_pnpids[] __devinitdata = {
> +/* Please try and keep the 1:1 mapping with the dev listing below */
> +static const struct pnp_id pnp_opl3sa2_cards[] = {
> /* Yamaha YMF719E-S (Genius Sound Maker 3DX) */
> - ISAPNP_OPL3SA2('Y','M','H',0x0020,0x0021),
> + {.id = "YMH0020", .driver_data = 0},
> /* Yamaha OPL3-SA3 (integrated on Intel's Pentium II AL440LX motherboard) */
> - ISAPNP_OPL3SA2('Y','M','H',0x0030,0x0021),
> + {.id = "YMH0030", .driver_data = 0},
> /* Yamaha OPL3-SA2 */
> - ISAPNP_OPL3SA2('Y','M','H',0x0800,0x0021),
> + {.id = "YMH0800", .driver_data = 0},
> /* NeoMagic MagicWave 3DX */
> - ISAPNP_OPL3SA2('N','M','X',0x2200,0x2210),
> - /* --- */
> - { ISAPNP_CARD_END, } /* end */
> + {.id = "NMX2200", .driver_data = 0},
> + {"", 0}
> };
>
> -ISAPNP_CARD_TABLE(snd_opl3sa2_pnpids);
> +static const struct pnp_id pnp_opl3sa2_devs[] = {
> + {.id = "YMH0021", .driver_data = 0},
> + {.id = "YMH0021", .driver_data = 0},
> + {.id = "YMH0021", .driver_data = 0},
> + {.id = "NMX2210", .driver_data = 0},
> + {"", 0}
> +};
>
this id table is fine, however, I would prefer
you use the new pnp_card_id structure instead.
> #endif /* __ISAPNP__ */
>
> @@ -634,22 +634,42 @@
>
> #endif /* CONFIG_PM */
>
> +static int snd_opl3sa2_free(opl3sa2_t *chip)
> +{
> +#ifdef __ISAPNP__
> + pnp_disable_dev(chip->dev);
Please __NEVER__ do this if the driver has been matched with
this device, the pnp layer will automatically take care of this.
> + pnp_remove_device(chip->dev);
This would create a big problem, if you remove the device, other
drivers can't use it. This should only be called by protocols
when the device is physically removed.
> +#endif
> +#ifdef CONFIG_PM
> + if (chip->pm_dev)
> + pm_unregister(chip->pm_dev);
> +#endif
> + if (chip->irq >= 0)
> + free_irq(chip->irq, (void *)chip);
> + if (chip->res_port) {
> + release_resource(chip->res_port);
> + kfree_nocheck(chip->res_port);
> + }
> +
> + snd_magic_kfree(chip);
> + return 0;
> +}
> +
> +static int snd_opl3sa2_dev_free(snd_device_t *device)
> +{
> + opl3sa2_t *chip = snd_magic_cast(opl3sa2_t, device->device_data, return -ENXIO);
> + return snd_opl3sa2_free(chip);
> +}
> +
> #ifdef __ISAPNP__
> static int __init snd_opl3sa2_isapnp(int dev, opl3sa2_t *chip)
> {
> - const struct isapnp_card_id *id = snd_opl3sa2_isapnp_id[dev];
> - struct isapnp_card *card = snd_opl3sa2_isapnp_cards[dev];
> - struct isapnp_dev *pdev;
> + int res = 0;
> + struct pnp_dev *pdev = snd_opl3sa2_isapnp_devs[dev];
> +
> + if (!enable[dev])
> + return -ENODEV;
once again, the pnp layer will keep track of this.
>
> - chip->dev = isapnp_find_dev(card, id->devs[0].vendor, id->devs[0].function, NULL);
> - if (chip->dev->active) {
> - chip->dev = NULL;
> - return -EBUSY;
> - }
> - /* PnP initialization */
> - pdev = chip->dev;
> - if (pdev->prepare(pdev)<0)
> - return -EAGAIN;
> if (sb_port[dev] != SNDRV_AUTO_PORT)
> isapnp_resource_change(&pdev->resource[0], sb_port[dev], 16);
> if (wss_port[dev] != SNDRV_AUTO_PORT)
> @@ -666,59 +686,27 @@
> isapnp_resource_change(&pdev->dma_resource[1], dma2[dev], 1);
> if (irq[dev] != SNDRV_AUTO_IRQ)
> isapnp_resource_change(&pdev->irq_resource[0], irq[dev], 1);
I understand that you're trying to preserve these but there's
really no reason to. isapnp_resource_change doesn't work. If
someone does bring up a reason to use these then I'll add
these functions.
> - if (pdev->activate(pdev)<0) {
> - snd_printk("isapnp configure failure (out of resources?)\n");
> - return -EBUSY;
> - }
The pnp layer won't call the probe function if the resource
config fails, therefore this is unnecessary.
> - sb_port[dev] = pdev->resource[0].start;
> - wss_port[dev] = pdev->resource[1].start;
> - fm_port[dev] = pdev->resource[2].start;
> - midi_port[dev] = pdev->resource[3].start;
> - port[dev] = pdev->resource[4].start;
> +
> + sb_port[dev] = pci_resource_start(pdev, 0);
> + wss_port[dev] = pci_resource_start(pdev, 1);
> + fm_port[dev] = pci_resource_start(pdev, 2);
> + midi_port[dev] = pci_resource_start(pdev, 3);
> + port[dev] = pci_resource_start(pdev, 4);
> dma1[dev] = pdev->dma_resource[0].start;
> dma2[dev] = pdev->dma_resource[1].start;
> irq[dev] = pdev->irq_resource[0].start;
New macros are available for these. Please use them instead.
Never use pci_resource_start on a pnp_dev structure.
> +
> snd_printdd("isapnp OPL3-SA: sb port=0x%lx, wss port=0x%lx, fm port=0x%lx, midi port=0x%lx\n",
> sb_port[dev], wss_port[dev], fm_port[dev], midi_port[dev]);
> snd_printdd("isapnp OPL3-SA: control port=0x%lx, dma1=%i, dma2=%i, irq=%i\n",
> port[dev], dma1[dev], dma2[dev], irq[dev]);
> - return 0;
> -}
> -
> -static void snd_opl3sa2_deactivate(opl3sa2_t *chip)
> -{
> - if (chip->dev) {
> - chip->dev->deactivate(chip->dev);
> - chip->dev = NULL;
> - }
> + chip->dev = pdev;
> +
> +out_error:
> + return res;
> }
> #endif /* __ISAPNP__ */
>
> -static int snd_opl3sa2_free(opl3sa2_t *chip)
> -{
> -#ifdef __ISAPNP__
> - snd_opl3sa2_deactivate(chip);
> -#endif
> -#ifdef CONFIG_PM
> - if (chip->pm_dev)
> - pm_unregister(chip->pm_dev);
> -#endif
> - if (chip->irq >= 0)
> - free_irq(chip->irq, (void *)chip);
> - if (chip->res_port) {
> - release_resource(chip->res_port);
> - kfree_nocheck(chip->res_port);
> - }
> - snd_magic_kfree(chip);
> - return 0;
> -}
> -
> -static int snd_opl3sa2_dev_free(snd_device_t *device)
> -{
> - opl3sa2_t *chip = snd_magic_cast(opl3sa2_t, device->device_data, return -ENXIO);
> - return snd_opl3sa2_free(chip);
> -}
> -
> static int __init snd_opl3sa2_probe(int dev)
> {
> int xirq, xdma1, xdma2;
> @@ -763,7 +751,9 @@
> err = -ENOMEM;
> goto __error;
> }
> + spin_lock_init(&chip->reg_lock);
> chip->irq = -1;
> +
> if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0)
> goto __error;
> #ifdef __ISAPNP__
> @@ -855,30 +845,47 @@
> }
>
> #ifdef __ISAPNP__
> -static int __init snd_opl3sa2_isapnp_detect(struct isapnp_card *card,
> - const struct isapnp_card_id *id)
> +static int pnp_opl3sa2_probe(struct pnp_dev *pdev, const struct pnp_id *card_id,
> + const struct pnp_id *dev_id)
> {
> - static int dev;
> + static int dev;
> int res;
>
> for ( ; dev < SNDRV_CARDS; dev++) {
> if (!enable[dev])
> continue;
> - snd_opl3sa2_isapnp_cards[dev] = card;
> - snd_opl3sa2_isapnp_id[dev] = id;
> + snd_opl3sa2_isapnp_devs[dev] = pdev;
> res = snd_opl3sa2_probe(dev);
> if (res < 0)
> return res;
> dev++;
> + nr_cards++;
> return 0;
> }
> return -ENODEV;
> }
> +
> +static void pnp_opl3sa2_remove(struct pnp_dev *pdev)
> +{
> + opl3sa2_t *chip = pdev->driver_data;
> + if (chip)
> + chip->dev = NULL;
> + nr_cards--;
> +}
This doesn't free anything, am I missing something?
In other words shouldn't it call snd_opl3sa2_dev_free
or something?
> +
> +static struct pnp_driver snd_opl3sa2_pnp_driver = {
> + .name = "opl3sa2",
> + .card_id_table = pnp_opl3sa2_cards,
> + .id_table = pnp_opl3sa2_devs,
> + .probe = pnp_opl3sa2_probe,
> + .remove = pnp_opl3sa2_remove,
> +};
> +
this is good, but you may want to make this a pnpc_driver
instead when pnp card support is included. Actually,
in the most recent release pnp v0.93 card_id_table
doesn't exist in the pnp_driver structure.
> #endif /* __ISAPNP__ */
>
> static int __init alsa_card_opl3sa2_init(void)
> {
> - int dev, cards = 0;
> + int dev;
>
> for (dev = 0; dev < SNDRV_CARDS; dev++) {
> if (!enable[dev])
> @@ -888,15 +895,19 @@
> continue;
> #endif
> if (snd_opl3sa2_probe(dev) >= 0)
> - cards++;
> + nr_cards++;
> }
> +
> #ifdef __ISAPNP__
> - cards += isapnp_probe_cards(snd_opl3sa2_pnpids, snd_opl3sa2_isapnp_detect);
> + /* upon registration the probe function will increment nr_cards */
> + pnp_register_driver(&snd_opl3sa2_pnp_driver);\
this is fine to do that but please be aware that this
function will return the number of matched devices.
> #endif
> - if (!cards) {
> +
> + if (nr_cards == 0) {
> #ifdef MODULE
> printk(KERN_ERR "Yamaha OPL3-SA soundcard not found or device busy\n");
> #endif
> + pnp_unregister_driver(&snd_opl3sa2_pnp_driver);
You may not want to unregister the driver just becuase
it fails. An isapnp protocol module could be loaded
after this driver and the device needed could be added,
in which case it would then attach to this driver if
you didn't unregister it.
> return -ENODEV;
Therefore this didn't fail becuase the driver was still
registered.
> }
> return 0;
> --
On Sat, 30 Nov 2002, Zwane Mwaikambo wrote:
> ISAPNP/ALSA hasn't been working for a while now (cards didn't get
> detected), here is a patch to update OPL3SA2 to the new PnP layer.
>
> ALSA device list:
> #0: Yamaha OPL3-SA23 at 0x100, irq 5, dma 1&3
>
> Driver tested in kernel and modular, i have tried to maintain
> detection/resource override semantics and general detection code
> paths.
Updated to fix broken module unload
Index: linux-2.5.50/sound/isa/opl3sa2.c
===================================================================
RCS file: /build/cvsroot/linux-2.5.50/sound/isa/opl3sa2.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 opl3sa2.c
--- linux-2.5.50/sound/isa/opl3sa2.c 28 Nov 2002 01:36:04 -0000 1.1.1.1
+++ linux-2.5.50/sound/isa/opl3sa2.c 1 Dec 2002 05:17:24 -0000
@@ -24,6 +24,7 @@
#include <linux/interrupt.h>
#include <linux/pm.h>
#include <linux/slab.h>
+#include <linux/pnp.h>
#ifndef LINUX_ISAPNP_H
#include <linux/isapnp.h>
#define isapnp_card pci_bus
@@ -135,6 +136,7 @@
typedef struct snd_opl3sa2 opl3sa2_t;
#define chip_t opl3sa2_t
+int nr_cards;
struct snd_opl3sa2 {
snd_card_t *card;
@@ -148,7 +150,7 @@
snd_rawmidi_t *rmidi;
cs4231_t *cs4231;
#ifdef __ISAPNP__
- struct isapnp_dev *dev;
+ struct pnp_dev *dev;
#endif
unsigned char ctlregs[0x20];
int ymode; /* SL added */
@@ -164,30 +166,28 @@
static snd_card_t *snd_opl3sa2_cards[SNDRV_CARDS] = SNDRV_DEFAULT_PTR;
#ifdef __ISAPNP__
+static struct pnp_dev *snd_opl3sa2_isapnp_devs[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
-static struct isapnp_card *snd_opl3sa2_isapnp_cards[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
-static const struct isapnp_card_id *snd_opl3sa2_isapnp_id[SNDRV_CARDS] __devinitdata = SNDRV_DEFAULT_PTR;
-
-#define ISAPNP_OPL3SA2(_va, _vb, _vc, _device, _function) \
- { \
- ISAPNP_CARD_ID(_va, _vb, _vc, _device), \
- devs : { ISAPNP_DEVICE_ID(_va, _vb, _vc, _function), } \
- }
-
-static struct isapnp_card_id snd_opl3sa2_pnpids[] __devinitdata = {
+/* Please try and keep the 1:1 mapping with the dev listing below */
+static const struct pnp_id pnp_opl3sa2_cards[] = {
/* Yamaha YMF719E-S (Genius Sound Maker 3DX) */
- ISAPNP_OPL3SA2('Y','M','H',0x0020,0x0021),
+ {.id = "YMH0020", .driver_data = 0},
/* Yamaha OPL3-SA3 (integrated on Intel's Pentium II AL440LX motherboard) */
- ISAPNP_OPL3SA2('Y','M','H',0x0030,0x0021),
+ {.id = "YMH0030", .driver_data = 0},
/* Yamaha OPL3-SA2 */
- ISAPNP_OPL3SA2('Y','M','H',0x0800,0x0021),
+ {.id = "YMH0800", .driver_data = 0},
/* NeoMagic MagicWave 3DX */
- ISAPNP_OPL3SA2('N','M','X',0x2200,0x2210),
- /* --- */
- { ISAPNP_CARD_END, } /* end */
+ {.id = "NMX2200", .driver_data = 0},
+ {"", 0}
};
-ISAPNP_CARD_TABLE(snd_opl3sa2_pnpids);
+static const struct pnp_id pnp_opl3sa2_devs[] = {
+ {.id = "YMH0021", .driver_data = 0},
+ {.id = "YMH0021", .driver_data = 0},
+ {.id = "YMH0021", .driver_data = 0},
+ {.id = "NMX2210", .driver_data = 0},
+ {"", 0}
+};
#endif /* __ISAPNP__ */
@@ -246,6 +246,7 @@
unsigned long port;
unsigned char tmp, tmp1;
char str[2];
+ int ret;
card = chip->card;
port = chip->port;
@@ -256,7 +257,8 @@
tmp = snd_opl3sa2_read(chip, OPL3SA2_MISC);
if (tmp == 0xff) {
snd_printd("OPL3-SA [0x%lx] detect = 0x%x\n", port, tmp);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_fail;
}
switch (tmp & 0x07) {
case 0x01:
@@ -276,14 +278,16 @@
snd_opl3sa2_write(chip, OPL3SA2_MISC, tmp ^ 7);
if ((tmp1 = snd_opl3sa2_read(chip, OPL3SA2_MISC)) != tmp) {
snd_printd("OPL3-SA [0x%lx] detect (1) = 0x%x (0x%x)\n", port, tmp, tmp1);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_fail;
}
/* try if the MIC register is accesible */
tmp = snd_opl3sa2_read(chip, OPL3SA2_MIC);
snd_opl3sa2_write(chip, OPL3SA2_MIC, 0x8a);
if (((tmp1 = snd_opl3sa2_read(chip, OPL3SA2_MIC)) & 0x9f) != 0x8a) {
snd_printd("OPL3-SA [0x%lx] detect (2) = 0x%x (0x%x)\n", port, tmp, tmp1);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_fail;
}
snd_opl3sa2_write(chip, OPL3SA2_MIC, 0x9f);
/* initialization */
@@ -308,6 +312,10 @@
snd_opl3sa2_write(chip, OPL3SA3_ANLG_DOWN, 0x00); /* Analog Block Partial Power Down - default */
}
return 0;
+
+out_fail:
+ release_resource(chip->res_port);
+ return ret;
}
static void snd_opl3sa2_interrupt(int irq, void *dev_id, struct pt_regs *regs)
@@ -634,22 +642,38 @@
#endif /* CONFIG_PM */
+static int snd_opl3sa2_free(opl3sa2_t *chip)
+{
+#ifdef CONFIG_PM
+ if (chip->pm_dev)
+ pm_unregister(chip->pm_dev);
+#endif
+ if (chip->irq >= 0)
+ free_irq(chip->irq, (void *)chip);
+ if (chip->res_port) {
+ release_resource(chip->res_port);
+ kfree_nocheck(chip->res_port);
+ }
+
+ snd_magic_kfree(chip);
+ return 0;
+}
+
+static int snd_opl3sa2_dev_free(snd_device_t *device)
+{
+ opl3sa2_t *chip = snd_magic_cast(opl3sa2_t, device->device_data, return -ENXIO);
+ return snd_opl3sa2_free(chip);
+}
+
#ifdef __ISAPNP__
static int __init snd_opl3sa2_isapnp(int dev, opl3sa2_t *chip)
{
- const struct isapnp_card_id *id = snd_opl3sa2_isapnp_id[dev];
- struct isapnp_card *card = snd_opl3sa2_isapnp_cards[dev];
- struct isapnp_dev *pdev;
+ int res = 0;
+ struct pnp_dev *pdev = snd_opl3sa2_isapnp_devs[dev];
+
+ if (!enable[dev])
+ return -ENODEV;
- chip->dev = isapnp_find_dev(card, id->devs[0].vendor, id->devs[0].function, NULL);
- if (chip->dev->active) {
- chip->dev = NULL;
- return -EBUSY;
- }
- /* PnP initialization */
- pdev = chip->dev;
- if (pdev->prepare(pdev)<0)
- return -EAGAIN;
if (sb_port[dev] != SNDRV_AUTO_PORT)
isapnp_resource_change(&pdev->resource[0], sb_port[dev], 16);
if (wss_port[dev] != SNDRV_AUTO_PORT)
@@ -666,59 +690,26 @@
isapnp_resource_change(&pdev->dma_resource[1], dma2[dev], 1);
if (irq[dev] != SNDRV_AUTO_IRQ)
isapnp_resource_change(&pdev->irq_resource[0], irq[dev], 1);
- if (pdev->activate(pdev)<0) {
- snd_printk("isapnp configure failure (out of resources?)\n");
- return -EBUSY;
- }
- sb_port[dev] = pdev->resource[0].start;
- wss_port[dev] = pdev->resource[1].start;
- fm_port[dev] = pdev->resource[2].start;
- midi_port[dev] = pdev->resource[3].start;
- port[dev] = pdev->resource[4].start;
+
+ sb_port[dev] = pci_resource_start(pdev, 0);
+ wss_port[dev] = pci_resource_start(pdev, 1);
+ fm_port[dev] = pci_resource_start(pdev, 2);
+ midi_port[dev] = pci_resource_start(pdev, 3);
+ port[dev] = pci_resource_start(pdev, 4);
dma1[dev] = pdev->dma_resource[0].start;
dma2[dev] = pdev->dma_resource[1].start;
irq[dev] = pdev->irq_resource[0].start;
+
snd_printdd("isapnp OPL3-SA: sb port=0x%lx, wss port=0x%lx, fm port=0x%lx, midi port=0x%lx\n",
sb_port[dev], wss_port[dev], fm_port[dev], midi_port[dev]);
snd_printdd("isapnp OPL3-SA: control port=0x%lx, dma1=%i, dma2=%i, irq=%i\n",
port[dev], dma1[dev], dma2[dev], irq[dev]);
- return 0;
-}
-
-static void snd_opl3sa2_deactivate(opl3sa2_t *chip)
-{
- if (chip->dev) {
- chip->dev->deactivate(chip->dev);
- chip->dev = NULL;
- }
+ chip->dev = pdev;
+
+ return res;
}
#endif /* __ISAPNP__ */
-static int snd_opl3sa2_free(opl3sa2_t *chip)
-{
-#ifdef __ISAPNP__
- snd_opl3sa2_deactivate(chip);
-#endif
-#ifdef CONFIG_PM
- if (chip->pm_dev)
- pm_unregister(chip->pm_dev);
-#endif
- if (chip->irq >= 0)
- free_irq(chip->irq, (void *)chip);
- if (chip->res_port) {
- release_resource(chip->res_port);
- kfree_nocheck(chip->res_port);
- }
- snd_magic_kfree(chip);
- return 0;
-}
-
-static int snd_opl3sa2_dev_free(snd_device_t *device)
-{
- opl3sa2_t *chip = snd_magic_cast(opl3sa2_t, device->device_data, return -ENXIO);
- return snd_opl3sa2_free(chip);
-}
-
static int __init snd_opl3sa2_probe(int dev)
{
int xirq, xdma1, xdma2;
@@ -763,7 +754,9 @@
err = -ENOMEM;
goto __error;
}
+ spin_lock_init(&chip->reg_lock);
chip->irq = -1;
+
if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0)
goto __error;
#ifdef __ISAPNP__
@@ -855,30 +848,47 @@
}
#ifdef __ISAPNP__
-static int __init snd_opl3sa2_isapnp_detect(struct isapnp_card *card,
- const struct isapnp_card_id *id)
+static int pnp_opl3sa2_probe(struct pnp_dev *pdev, const struct pnp_id *card_id,
+ const struct pnp_id *dev_id)
{
- static int dev;
+ static int dev;
int res;
for ( ; dev < SNDRV_CARDS; dev++) {
if (!enable[dev])
continue;
- snd_opl3sa2_isapnp_cards[dev] = card;
- snd_opl3sa2_isapnp_id[dev] = id;
+ snd_opl3sa2_isapnp_devs[dev] = pdev;
res = snd_opl3sa2_probe(dev);
if (res < 0)
return res;
dev++;
+ nr_cards++;
return 0;
}
return -ENODEV;
}
+
+static void pnp_opl3sa2_remove(struct pnp_dev *pdev)
+{
+ opl3sa2_t *chip = pdev->driver_data;
+ if (chip)
+ chip->dev = NULL;
+ nr_cards--;
+}
+
+static struct pnp_driver snd_opl3sa2_pnp_driver = {
+ .name = "opl3sa2",
+ .card_id_table = pnp_opl3sa2_cards,
+ .id_table = pnp_opl3sa2_devs,
+ .probe = pnp_opl3sa2_probe,
+ .remove = pnp_opl3sa2_remove,
+};
+
#endif /* __ISAPNP__ */
static int __init alsa_card_opl3sa2_init(void)
{
- int dev, cards = 0;
+ int dev;
for (dev = 0; dev < SNDRV_CARDS; dev++) {
if (!enable[dev])
@@ -888,15 +898,21 @@
continue;
#endif
if (snd_opl3sa2_probe(dev) >= 0)
- cards++;
+ nr_cards++;
}
+
#ifdef __ISAPNP__
- cards += isapnp_probe_cards(snd_opl3sa2_pnpids, snd_opl3sa2_isapnp_detect);
+ /* upon registration the probe function will increment nr_cards */
+ pnp_register_driver(&snd_opl3sa2_pnp_driver);
#endif
- if (!cards) {
+
+ if (nr_cards == 0) {
#ifdef MODULE
printk(KERN_ERR "Yamaha OPL3-SA soundcard not found or device busy\n");
#endif
+#ifdef __ISAPNP__
+ pnp_unregister_driver(&snd_opl3sa2_pnp_driver);
+#endif
return -ENODEV;
}
return 0;
@@ -905,9 +921,11 @@
static void __exit alsa_card_opl3sa2_exit(void)
{
int idx;
-
for (idx = 0; idx < SNDRV_CARDS; idx++)
snd_card_free(snd_opl3sa2_cards[idx]);
+#ifdef __ISAPNP__
+ pnp_unregister_driver(&snd_opl3sa2_pnp_driver);
+#endif
}
module_init(alsa_card_opl3sa2_init)
--
function.linuxpower.ca
On Sun, 1 Dec 2002, Adam Belay wrote:
> Many changes are pending that will allow you to manage entire pnp
> cards. For devices like opl3sa2, with only one device, you don't
> have to use this, though I would prefer you do for consistency with
> the other alsa drivers and because it has better card-specific
> matching. I'll send a big patch out with all of these changes.
> They should be in the bk tree soon as well. I apologize for any
> difficulty this may have caused.
>
> Much of this driver has already been converted by me and has been
> submitted publicly on the lkml. What remains, however is the
>
> http://marc.theaimsgroup.com/?l=linux-kernel&m=103844524222955&w=2
Cool, i think a more interesting device would be the AWE32 portion of the
soundblaster driver since that has the wavetable device on it.
> remove code. I like how you did yours in this patch but it doesn't
> seem to release anything. I included comments to help you better
> understand how to convert pnp drivers. I hope they help.
>
> Thanks for your effort. I'd love to see more pnp drivers converted.
> If you have any questions feel free to ask.
>
> > #define isapnp_card pci_bus
>
> this is wrong, it should be pnp_card. I actually
> got rid of this definition entirely in my patch
> and used pnp_card.
That would be a remnant from the driver before the conversion, i didn't
actually used it and removed all references in code.
> > @@ -135,6 +136,7 @@
> >
> > typedef struct snd_opl3sa2 opl3sa2_t;
> > #define chip_t opl3sa2_t
> > +int nr_cards;
> >
> > struct snd_opl3sa2 {
> > snd_card_t *card;
> > @@ -148,7 +150,7 @@
> > snd_rawmidi_t *rmidi;
> > cs4231_t *cs4231;
> > #ifdef __ISAPNP__
>
> the definition __ISAPNP__ should not be used, use
> CONFIG_PNP or CONFIG_PNP_CARD instead. I've only
> mentioned this for this instance but it is used
> throughout your patch. Actually, now that I think
> about it I need to change my patch to use
> CONFIG_PNP_CARD isntead of CONFIG_PNP.
Noted
> > #endif /* __ISAPNP__ */
> >
> > @@ -634,22 +634,42 @@
> >
> > #endif /* CONFIG_PM */
> >
> > +static int snd_opl3sa2_free(opl3sa2_t *chip)
> > +{
> > +#ifdef __ISAPNP__
> > + pnp_disable_dev(chip->dev);
>
> Please __NEVER__ do this if the driver has been matched with
> this device, the pnp layer will automatically take care of this.
I actually removed that just before because it causes an oops on module
unload.
> > + pnp_remove_device(chip->dev);
>
> This would create a big problem, if you remove the device, other
> drivers can't use it. This should only be called by protocols
> when the device is physically removed.
>
> I understand that you're trying to preserve these but there's
> really no reason to. isapnp_resource_change doesn't work. If
> someone does bring up a reason to use these then I'll add
> these functions.
Left to avoid too much change in the first round.
> > - if (pdev->activate(pdev)<0) {
> > - snd_printk("isapnp configure failure (out of resources?)\n");
> > - return -EBUSY;
> > - }
>
> The pnp layer won't call the probe function if the resource
> config fails, therefore this is unnecessary.
Yep, thats why i removed them, i would think that would be the same for
the former pdev->prepare() function?
> > dma2[dev] = pdev->dma_resource[1].start;
> > irq[dev] = pdev->irq_resource[0].start;
>
> New macros are available for these. Please use them instead.
> Never use pci_resource_start on a pnp_dev structure.
gotcha.
> > +static void pnp_opl3sa2_remove(struct pnp_dev *pdev)
> > +{
> > + opl3sa2_t *chip = pdev->driver_data;
> > + if (chip)
> > + chip->dev = NULL;
> > + nr_cards--;
> > +}
>
> This doesn't free anything, am I missing something?
> In other words shouldn't it call snd_opl3sa2_dev_free
> or something?
That is called in a different path, the ALSA freeing/unloading paths can
get a bit confusing.
> > #ifdef __ISAPNP__
> > - cards += isapnp_probe_cards(snd_opl3sa2_pnpids, snd_opl3sa2_isapnp_detect);
> > + /* upon registration the probe function will increment nr_cards */
> > + pnp_register_driver(&snd_opl3sa2_pnp_driver);\
>
> this is fine to do that but please be aware that this
> function will return the number of matched devices.
Thats ok because we're incrementing nr_cards in the ->probe function and
using that instead of 'cards'.
> > - if (!cards) {
> > +
> > + if (nr_cards == 0) {
> > #ifdef MODULE
> > printk(KERN_ERR "Yamaha OPL3-SA soundcard not found or device busy\n");
> > #endif
> > + pnp_unregister_driver(&snd_opl3sa2_pnp_driver);
>
> You may not want to unregister the driver just becuase
> it fails. An isapnp protocol module could be loaded
> after this driver and the device needed could be added,
> in which case it would then attach to this driver if
> you didn't unregister it.
Yep had fixed that when i testing module failures.
Cheers,
Zwane
--
function.linuxpower.ca
On Sun, Dec 01, 2002 at 02:02:48AM -0500, Zwane Mwaikambo wrote:
> On Sun, 1 Dec 2002, Adam Belay wrote:
>
> > Many changes are pending that will allow you to manage entire pnp
> > cards. For devices like opl3sa2, with only one device, you don't
> > have to use this, though I would prefer you do for consistency with
> > the other alsa drivers and because it has better card-specific
> > matching. I'll send a big patch out with all of these changes.
> > They should be in the bk tree soon as well. I apologize for any
> > difficulty this may have caused.
> >
> > Much of this driver has already been converted by me and has been
> > submitted publicly on the lkml. What remains, however is the
> >
> > http://marc.theaimsgroup.com/?l=linux-kernel&m=103844524222955&w=2
>
> Cool, i think a more interesting device would be the AWE32 portion of the
> soundblaster driver since that has the wavetable device on it.
Agreed. Pual is currently working on the OSS driver. I'd love to see the
ALSA sb driver working with the new card API. There are some ALSA drivers
that have three and perhaps even four devices. They should be interesting
:).
> > > #endif /* __ISAPNP__ */
> > >
> > > @@ -634,22 +634,42 @@
> > >
> > > #endif /* CONFIG_PM */
> > >
> > > +static int snd_opl3sa2_free(opl3sa2_t *chip)
> > > +{
> > > +#ifdef __ISAPNP__
> > > + pnp_disable_dev(chip->dev);
> >
> > Please __NEVER__ do this if the driver has been matched with
> > this device, the pnp layer will automatically take care of this.
>
> I actually removed that just before because it causes an oops on module
> unload.
It caused an oops? I'll bet the pnp layer got confused by it. I'll add
some busy flags to prevent drivers from calling this when the device is
being used by the driver through the conventional driver-model style.
Thanks for pointing this out.
>
> > > + pnp_remove_device(chip->dev);
> >
> > This would create a big problem, if you remove the device, other
> > drivers can't use it. This should only be called by protocols
> > when the device is physically removed.
> >
> > I understand that you're trying to preserve these but there's
> > really no reason to. isapnp_resource_change doesn't work. If
> > someone does bring up a reason to use these then I'll add
> > these functions.
>
> Left to avoid too much change in the first round.
I think it's better just to remove these becuase they may be
dependent on isapnp.h. Eventually I'll remove all of these from
isapnp.h.
> > > +static void pnp_opl3sa2_remove(struct pnp_dev *pdev)
> > > +{
> > > + opl3sa2_t *chip = pdev->driver_data;
> > > + if (chip)
> > > + chip->dev = NULL;
> > > + nr_cards--;
> > > +}
> >
> > This doesn't free anything, am I missing something?
> > In other words shouldn't it call snd_opl3sa2_dev_free
> > or something?
>
> That is called in a different path, the ALSA freeing/unloading paths can
> get a bit confusing.
I see now. The problem is that when the remove function is called, the pnp
layer expects the device's resources to be freed and not in use. I should
add some checks for this as well. The pnp layer will disable the device
and this could cause big problems if the driver is used in between the time
of the ALSA remove code path and this driver removal. Furthermore, if there
is more than one sound card and the driver-model wants to remove one, a
problem would occur. Perhaps this aspect of ALSA needs to be changed.
Any ideas?
Thanks,
Adam
On Sun, 1 Dec 2002, Adam Belay wrote:
> It caused an oops? I'll bet the pnp layer got confused by it. I'll add
> some busy flags to prevent drivers from calling this when the device is
> being used by the driver through the conventional driver-model style.
> Thanks for pointing this out.
Here is what the stack looked like;
EIP is at kfree+0xcd/0xe0
[<c024228d>] pnp_free_ids+0x1d/0x30
[<c0241beb>] pnp_release_device+0x1b/0x30
[<c02555a5>] device_release+0x15/0x20
[<c02390e3>] kobject_cleanup+0x73/0x80
[<c0241dac>] pnp_remove_device+0x1c/0xcd
We were releasing an already freed block of memory (this one was slab
poisoned).
> I see now. The problem is that when the remove function is called, the pnp
> layer expects the device's resources to be freed and not in use. I should
> add some checks for this as well. The pnp layer will disable the device
> and this could cause big problems if the driver is used in between the time
> of the ALSA remove code path and this driver removal. Furthermore, if there
> is more than one sound card and the driver-model wants to remove one, a
> problem would occur. Perhaps this aspect of ALSA needs to be changed.
> Any ideas?
Hmm i thought ->remove was per device or do you iterate internally over
all registered driver devices? Thats why i originally did the
pnp_remove_device in the driver's card removal path. How about only
disabling registered devices on final driver unregister?
As an aside where does this fit in with the whole pci_dev business?
Cheers,
Zwane
--
function.linuxpower.ca
On Sun, Dec 01, 2002 at 02:04:45PM -0500, Zwane Mwaikambo wrote:
> On Sun, 1 Dec 2002, Adam Belay wrote:
>
> > It caused an oops? I'll bet the pnp layer got confused by it. I'll add
> > some busy flags to prevent drivers from calling this when the device is
> > being used by the driver through the conventional driver-model style.
> > Thanks for pointing this out.
>
> Here is what the stack looked like;
>
> EIP is at kfree+0xcd/0xe0
> [<c024228d>] pnp_free_ids+0x1d/0x30
> [<c0241beb>] pnp_release_device+0x1b/0x30
> [<c02555a5>] device_release+0x15/0x20
> [<c02390e3>] kobject_cleanup+0x73/0x80
> [<c0241dac>] pnp_remove_device+0x1c/0xcd
>
> We were releasing an already freed block of memory (this one was slab
> poisoned).
Maybe there's something wrong with the release code, I'll take a look at it.
here's a copy of those functions from the latest version but they're
essentially the same as they were in the version you're using.
void __pnp_remove_device(struct pnp_dev *dev)
{
spin_lock(&pnp_lock);
list_del_init(&dev->global_list);
list_del_init(&dev->protocol_list);
spin_unlock(&pnp_lock);
device_unregister(&dev->dev);
}
static void pnp_free_ids(struct pnp_dev *dev)
{
struct pnp_id * id;
struct pnp_id * next;
if (!dev)
return;
id = dev->id;
while (id) {
next = id->next;
kfree(id);
id = next;
}
}
static void pnp_release_device(struct device *dmdev)
{
struct pnp_dev * dev = to_pnp_dev(dmdev);
if (dev->res)
pnp_free_resources(dev->res);
pnp_free_ids(dev);
kfree(dev);
}
>
> > I see now. The problem is that when the remove function is called, the pnp
> > layer expects the device's resources to be freed and not in use. I should
> > add some checks for this as well. The pnp layer will disable the device
> > and this could cause big problems if the driver is used in between the time
> > of the ALSA remove code path and this driver removal. Furthermore, if there
> > is more than one sound card and the driver-model wants to remove one, a
> > problem would occur. Perhaps this aspect of ALSA needs to be changed.
> > Any ideas?
>
> Hmm i thought ->remove was per device or do you iterate internally over
You are right, ->remove is per device.
> all registered driver devices? Thats why i originally did the
> pnp_remove_device in the driver's card removal path. How about only
You'll see why not to call pnp_remove_device below.
> disabling registered devices on final driver unregister?
If the driver isn't attached to them they should remain disabled so that other
pnp devices can use their resources. Things can get very tight if you have a
lot of pnp devices.
This may clear some things up for you.
The PnP Device Life cycle.
1.) A PnP protocol detects the device
2.) the PnP protocol creates a pnp_dev structure and sets the values in it
3.) the PnP protocol calls pnp_add_device
4.) pnp_add_device names the device, calls any quirks, and sets aditional
pnp specific values
5.) pnp_add_device registers the device with the driver model
6.) it then adds it to the global device lists and other device management
lists
7.) finally it attaches the sysfs interface to the device
8.) from that point, the driver model takes over the device
9.) it searches through the pool of existing drivers for a match.
(uses pnp_match_device)
10.) if a match isn't found it waits for a driver to be registered
11.) after a match is found it calls the probe function
12.) the pnp layer activates the device, does some housekeeping and then
passes the device to the driver's probe function.
13.) the driver's probe function then reads the resource configuration and
passes the information to the rest of the driver
14.) the device rests happily until the driver model requests that it is to
be unbound from the driver
15.) the driver model then tells the pnp_layer about this and the pnp_layer
informs the driver that it must stop using the device. (->remove)
16.) the driver releases all resources (io ports, irqs, etc) and informs the
device that it is no longer needed. This cannot fail and thus returns void.
16.) the pnp layer then disables the device so its resource can be used by
other devices.
17.) as drivers are loaded and unloaded steps 8 - 16 repeat
18.) finally the protocol calls pnp_remove device when it discovers that the
device has been physically removed or when the protocol itself is removed.
19.) pnp_remove_device prevents any new devices from matching with it
20.) the driver model calls release when it's ready and all memory resources
(kfree) are freed.
* if the driver is unloaded it calls pnp_unregister driver. The driver model
then iterates through all devices registered through the driver and calls
->remove (step 15)
> As an aside where does this fit in with the whole pci_dev business?
>
Before the driver model and the new pnp layer pci_dev was used as a universal
structure for resource using devices. Now that the driver model exists, every
type of device can have its own structure. If you look, pci_dev still has old
isapnp code in it. In fact, it's quite a mess becuase of it. It needs to be
removed but I haven't yet becuase it will cause all of these old pnp drivers
to not be able to compile. When enough drivers are converted I'll remove
this stuff.
Thanks,
Adam