| Message ID | 1420587617-28521-1-git-send-email-andy.shevchenko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Wed, 7 Jan 2015 01:40:17 +0200, Andy Shevchenko wrote: > > The managed functions allow to get ->probe() and ->remove() simplier. > This patch converts code to use managed functions. This doesn't work well because of the order of release calls in device_release(). devres_release_all() is called at first before anything else. Thus the card's free callback would be called after it, and snd_fm801_free() touches the hardware before disabling. Takashi > While here remove the dead code and fix the value printed in error > message. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > sound/pci/fm801.c | 29 +++++++---------------------- > 1 file changed, 7 insertions(+), 22 deletions(-) > > diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c > index dcda3c1..2708500 100644 > --- a/sound/pci/fm801.c > +++ b/sound/pci/fm801.c > @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) > v4l2_device_unregister(&chip->v4l2_dev); > } > #endif > - if (chip->irq >= 0) > - free_irq(chip->irq, chip); > pci_release_regions(chip->pci); > - pci_disable_device(chip->pci); > > - kfree(chip); > return 0; > } > > @@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, > }; > > *rchip = NULL; > - if ((err = pci_enable_device(pci)) < 0) > + if ((err = pcim_enable_device(pci)) < 0) > return err; > - chip = kzalloc(sizeof(*chip), GFP_KERNEL); > - if (chip == NULL) { > - pci_disable_device(pci); > + chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL); > + if (chip == NULL) > return -ENOMEM; > - } > spin_lock_init(&chip->reg_lock); > chip->card = card; > chip->pci = pci; > chip->irq = -1; > chip->tea575x_tuner = tea575x_tuner; > - if ((err = pci_request_regions(pci, "FM801")) < 0) { > - kfree(chip); > - pci_disable_device(pci); > + if ((err = pci_request_regions(pci, "FM801")) < 0) > return err; > - } > chip->port = pci_resource_start(pci, 0); > if ((tea575x_tuner & TUNER_ONLY) == 0) { > - if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED, > - KBUILD_MODNAME, chip)) { > - dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq); > + if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt, > + IRQF_SHARED, KBUILD_MODNAME, chip)) { > + dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); > snd_fm801_free(chip); > return -EBUSY; > } > @@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, > /* init might set tuner access method */ > tea575x_tuner = chip->tea575x_tuner; > > - if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) { > - pci_clear_master(pci); > - free_irq(chip->irq, chip); > - chip->irq = -1; > - } > - > if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { > snd_fm801_free(chip); > return err; > -- > 1.8.3.101.g727a46b >
On Wed, Jan 7, 2015 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 7 Jan 2015 01:40:17 +0200, > Andy Shevchenko wrote: >> >> The managed functions allow to get ->probe() and ->remove() simplier. >> This patch converts code to use managed functions. > > This doesn't work well because of the order of release calls in > device_release(). devres_release_all() is called at first before > anything else. Thus the card's free callback would be called after > it, and snd_fm801_free() touches the hardware before disabling. > Hmm… I didn't get what make those calls. Sound core? The driver is a normal PCI driver, so resources will be freed *after* ->remove() of driver was called. Please, elaborate. > > Takashi > >> While here remove the dead code and fix the value printed in error >> message. >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> --- >> sound/pci/fm801.c | 29 +++++++---------------------- >> 1 file changed, 7 insertions(+), 22 deletions(-) >> >> diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c >> index dcda3c1..2708500 100644 >> --- a/sound/pci/fm801.c >> +++ b/sound/pci/fm801.c >> @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) >> v4l2_device_unregister(&chip->v4l2_dev); >> } >> #endif >> - if (chip->irq >= 0) >> - free_irq(chip->irq, chip); >> pci_release_regions(chip->pci); >> - pci_disable_device(chip->pci); >> >> - kfree(chip); >> return 0; >> } >> >> @@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, >> }; >> >> *rchip = NULL; >> - if ((err = pci_enable_device(pci)) < 0) >> + if ((err = pcim_enable_device(pci)) < 0) >> return err; >> - chip = kzalloc(sizeof(*chip), GFP_KERNEL); >> - if (chip == NULL) { >> - pci_disable_device(pci); >> + chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL); >> + if (chip == NULL) >> return -ENOMEM; >> - } >> spin_lock_init(&chip->reg_lock); >> chip->card = card; >> chip->pci = pci; >> chip->irq = -1; >> chip->tea575x_tuner = tea575x_tuner; >> - if ((err = pci_request_regions(pci, "FM801")) < 0) { >> - kfree(chip); >> - pci_disable_device(pci); >> + if ((err = pci_request_regions(pci, "FM801")) < 0) >> return err; >> - } >> chip->port = pci_resource_start(pci, 0); >> if ((tea575x_tuner & TUNER_ONLY) == 0) { >> - if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED, >> - KBUILD_MODNAME, chip)) { >> - dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq); >> + if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt, >> + IRQF_SHARED, KBUILD_MODNAME, chip)) { >> + dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); >> snd_fm801_free(chip); >> return -EBUSY; >> } >> @@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, >> /* init might set tuner access method */ >> tea575x_tuner = chip->tea575x_tuner; >> >> - if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) { >> - pci_clear_master(pci); >> - free_irq(chip->irq, chip); >> - chip->irq = -1; >> - } >> - >> if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { >> snd_fm801_free(chip); >> return err; >> -- >> 1.8.3.101.g727a46b >>
At Wed, 7 Jan 2015 12:54:40 +0200, Andy Shevchenko wrote: > > On Wed, Jan 7, 2015 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote: > > At Wed, 7 Jan 2015 01:40:17 +0200, > > Andy Shevchenko wrote: > >> > >> The managed functions allow to get ->probe() and ->remove() simplier. > >> This patch converts code to use managed functions. > > > > This doesn't work well because of the order of release calls in > > device_release(). devres_release_all() is called at first before > > anything else. Thus the card's free callback would be called after > > it, and snd_fm801_free() touches the hardware before disabling. > > > > Hmm… I didn't get what make those calls. Sound core? The driver is a > normal PCI driver, so resources will be freed *after* ->remove() of > driver was called. > Please, elaborate. Ah, OK, this seems working, as this is the managed *pci* device that is a parent of the card device, thus it shall be released after the children. But any reason not to use pcim_iomap_regions_request_all()? thanks, Takashi > > > > > Takashi > > > >> While here remove the dead code and fix the value printed in error > >> message. > >> > >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> --- > >> sound/pci/fm801.c | 29 +++++++---------------------- > >> 1 file changed, 7 insertions(+), 22 deletions(-) > >> > >> diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c > >> index dcda3c1..2708500 100644 > >> --- a/sound/pci/fm801.c > >> +++ b/sound/pci/fm801.c > >> @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) > >> v4l2_device_unregister(&chip->v4l2_dev); > >> } > >> #endif > >> - if (chip->irq >= 0) > >> - free_irq(chip->irq, chip); > >> pci_release_regions(chip->pci); > >> - pci_disable_device(chip->pci); > >> > >> - kfree(chip); > >> return 0; > >> } > >> > >> @@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, > >> }; > >> > >> *rchip = NULL; > >> - if ((err = pci_enable_device(pci)) < 0) > >> + if ((err = pcim_enable_device(pci)) < 0) > >> return err; > >> - chip = kzalloc(sizeof(*chip), GFP_KERNEL); > >> - if (chip == NULL) { > >> - pci_disable_device(pci); > >> + chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL); > >> + if (chip == NULL) > >> return -ENOMEM; > >> - } > >> spin_lock_init(&chip->reg_lock); > >> chip->card = card; > >> chip->pci = pci; > >> chip->irq = -1; > >> chip->tea575x_tuner = tea575x_tuner; > >> - if ((err = pci_request_regions(pci, "FM801")) < 0) { > >> - kfree(chip); > >> - pci_disable_device(pci); > >> + if ((err = pci_request_regions(pci, "FM801")) < 0) > >> return err; > >> - } > >> chip->port = pci_resource_start(pci, 0); > >> if ((tea575x_tuner & TUNER_ONLY) == 0) { > >> - if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED, > >> - KBUILD_MODNAME, chip)) { > >> - dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq); > >> + if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt, > >> + IRQF_SHARED, KBUILD_MODNAME, chip)) { > >> + dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); > >> snd_fm801_free(chip); > >> return -EBUSY; > >> } > >> @@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, > >> /* init might set tuner access method */ > >> tea575x_tuner = chip->tea575x_tuner; > >> > >> - if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) { > >> - pci_clear_master(pci); > >> - free_irq(chip->irq, chip); > >> - chip->irq = -1; > >> - } > >> - > >> if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { > >> snd_fm801_free(chip); > >> return err; > >> -- > >> 1.8.3.101.g727a46b > >> > > > > -- > With Best Regards, > Andy Shevchenko >
On Wed, Jan 7, 2015 at 1:27 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 7 Jan 2015 12:54:40 +0200, > Andy Shevchenko wrote: >> >> On Wed, Jan 7, 2015 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote: >> > At Wed, 7 Jan 2015 01:40:17 +0200, >> > Andy Shevchenko wrote: >> >> >> >> The managed functions allow to get ->probe() and ->remove() simplier. >> >> This patch converts code to use managed functions. >> > >> > This doesn't work well because of the order of release calls in >> > device_release(). devres_release_all() is called at first before >> > anything else. Thus the card's free callback would be called after >> > it, and snd_fm801_free() touches the hardware before disabling. >> > >> >> Hmm… I didn't get what make those calls. Sound core? The driver is a >> normal PCI driver, so resources will be freed *after* ->remove() of >> driver was called. >> Please, elaborate. > > Ah, OK, this seems working, as this is the managed *pci* device that > is a parent of the card device, thus it shall be released after the > children. > > But any reason not to use pcim_iomap_regions_request_all()? Yes. I was doubt to put a comment about this. So, the initial idea is to move to MMIO instead of IO access. That's why you may see previous patch about fm801_{read,write}w() macros. I would like to do that later. > > > thanks, > > Takashi > >> >> > >> > Takashi >> > >> >> While here remove the dead code and fix the value printed in error >> >> message. >> >> >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >> --- >> >> sound/pci/fm801.c | 29 +++++++---------------------- >> >> 1 file changed, 7 insertions(+), 22 deletions(-) >> >> >> >> diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c >> >> index dcda3c1..2708500 100644 >> >> --- a/sound/pci/fm801.c >> >> +++ b/sound/pci/fm801.c >> >> @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) >> >> v4l2_device_unregister(&chip->v4l2_dev); >> >> } >> >> #endif >> >> - if (chip->irq >= 0) >> >> - free_irq(chip->irq, chip); >> >> pci_release_regions(chip->pci); >> >> - pci_disable_device(chip->pci); >> >> >> >> - kfree(chip); >> >> return 0; >> >> } >> >> >> >> @@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, >> >> }; >> >> >> >> *rchip = NULL; >> >> - if ((err = pci_enable_device(pci)) < 0) >> >> + if ((err = pcim_enable_device(pci)) < 0) >> >> return err; >> >> - chip = kzalloc(sizeof(*chip), GFP_KERNEL); >> >> - if (chip == NULL) { >> >> - pci_disable_device(pci); >> >> + chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL); >> >> + if (chip == NULL) >> >> return -ENOMEM; >> >> - } >> >> spin_lock_init(&chip->reg_lock); >> >> chip->card = card; >> >> chip->pci = pci; >> >> chip->irq = -1; >> >> chip->tea575x_tuner = tea575x_tuner; >> >> - if ((err = pci_request_regions(pci, "FM801")) < 0) { >> >> - kfree(chip); >> >> - pci_disable_device(pci); >> >> + if ((err = pci_request_regions(pci, "FM801")) < 0) >> >> return err; >> >> - } >> >> chip->port = pci_resource_start(pci, 0); >> >> if ((tea575x_tuner & TUNER_ONLY) == 0) { >> >> - if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED, >> >> - KBUILD_MODNAME, chip)) { >> >> - dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq); >> >> + if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt, >> >> + IRQF_SHARED, KBUILD_MODNAME, chip)) { >> >> + dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); >> >> snd_fm801_free(chip); >> >> return -EBUSY; >> >> } >> >> @@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, >> >> /* init might set tuner access method */ >> >> tea575x_tuner = chip->tea575x_tuner; >> >> >> >> - if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) { >> >> - pci_clear_master(pci); >> >> - free_irq(chip->irq, chip); >> >> - chip->irq = -1; >> >> - } >> >> - >> >> if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { >> >> snd_fm801_free(chip); >> >> return err; >> >> -- >> >> 1.8.3.101.g727a46b >> >> >> >> >> >> -- >> With Best Regards, >> Andy Shevchenko >>
At Wed, 7 Jan 2015 14:20:26 +0200, Andy Shevchenko wrote: > > On Wed, Jan 7, 2015 at 1:27 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Wed, 7 Jan 2015 12:54:40 +0200, > > Andy Shevchenko wrote: > >> > >> On Wed, Jan 7, 2015 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote: > >> > At Wed, 7 Jan 2015 01:40:17 +0200, > >> > Andy Shevchenko wrote: > >> >> > >> >> The managed functions allow to get ->probe() and ->remove() simplier. > >> >> This patch converts code to use managed functions. > >> > > >> > This doesn't work well because of the order of release calls in > >> > device_release(). devres_release_all() is called at first before > >> > anything else. Thus the card's free callback would be called after > >> > it, and snd_fm801_free() touches the hardware before disabling. > >> > > >> > >> Hmm… I didn't get what make those calls. Sound core? The driver is a > >> normal PCI driver, so resources will be freed *after* ->remove() of > >> driver was called. > >> Please, elaborate. > > > > Ah, OK, this seems working, as this is the managed *pci* device that > > is a parent of the card device, thus it shall be released after the > > children. > > > > But any reason not to use pcim_iomap_regions_request_all()? > > Yes. I was doubt to put a comment about this. Well, pcim_iomap_regions_request_all() can just request regions without actually iomapping via passing 0 to mask. But, now looking at the code again, I couldn't find the place releasing the non-mmio regions. Hmm... it might be a leak? > So, the initial idea is > to move to MMIO instead of IO access. That's why you may see previous > patch about fm801_{read,write}w() macros. > I would like to do that later. Yeah, of course, rewriting to MMIO would be nicer. I guess you're testing with the real hardware, so I'm for this rewrite. thanks, Takashi > > > > > > > thanks, > > > > Takashi > > > >> > >> > > >> > Takashi > >> > > >> >> While here remove the dead code and fix the value printed in error > >> >> message. > >> >> > >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> >> --- > >> >> sound/pci/fm801.c | 29 +++++++---------------------- > >> >> 1 file changed, 7 insertions(+), 22 deletions(-) > >> >> > >> >> diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c > >> >> index dcda3c1..2708500 100644 > >> >> --- a/sound/pci/fm801.c > >> >> +++ b/sound/pci/fm801.c > >> >> @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) > >> >> v4l2_device_unregister(&chip->v4l2_dev); > >> >> } > >> >> #endif > >> >> - if (chip->irq >= 0) > >> >> - free_irq(chip->irq, chip); > >> >> pci_release_regions(chip->pci); > >> >> - pci_disable_device(chip->pci); > >> >> > >> >> - kfree(chip); > >> >> return 0; > >> >> } > >> >> > >> >> @@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, > >> >> }; > >> >> > >> >> *rchip = NULL; > >> >> - if ((err = pci_enable_device(pci)) < 0) > >> >> + if ((err = pcim_enable_device(pci)) < 0) > >> >> return err; > >> >> - chip = kzalloc(sizeof(*chip), GFP_KERNEL); > >> >> - if (chip == NULL) { > >> >> - pci_disable_device(pci); > >> >> + chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL); > >> >> + if (chip == NULL) > >> >> return -ENOMEM; > >> >> - } > >> >> spin_lock_init(&chip->reg_lock); > >> >> chip->card = card; > >> >> chip->pci = pci; > >> >> chip->irq = -1; > >> >> chip->tea575x_tuner = tea575x_tuner; > >> >> - if ((err = pci_request_regions(pci, "FM801")) < 0) { > >> >> - kfree(chip); > >> >> - pci_disable_device(pci); > >> >> + if ((err = pci_request_regions(pci, "FM801")) < 0) > >> >> return err; > >> >> - } > >> >> chip->port = pci_resource_start(pci, 0); > >> >> if ((tea575x_tuner & TUNER_ONLY) == 0) { > >> >> - if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED, > >> >> - KBUILD_MODNAME, chip)) { > >> >> - dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq); > >> >> + if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt, > >> >> + IRQF_SHARED, KBUILD_MODNAME, chip)) { > >> >> + dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); > >> >> snd_fm801_free(chip); > >> >> return -EBUSY; > >> >> } > >> >> @@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, > >> >> /* init might set tuner access method */ > >> >> tea575x_tuner = chip->tea575x_tuner; > >> >> > >> >> - if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) { > >> >> - pci_clear_master(pci); > >> >> - free_irq(chip->irq, chip); > >> >> - chip->irq = -1; > >> >> - } > >> >> - > >> >> if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { > >> >> snd_fm801_free(chip); > >> >> return err; > >> >> -- > >> >> 1.8.3.101.g727a46b > >> >> > >> > >> > >> > >> -- > >> With Best Regards, > >> Andy Shevchenko > >> > > > > -- > With Best Regards, > Andy Shevchenko >
On Wed, Jan 7, 2015 at 3:12 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 7 Jan 2015 14:20:26 +0200, > Andy Shevchenko wrote: >> >> On Wed, Jan 7, 2015 at 1:27 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > At Wed, 7 Jan 2015 12:54:40 +0200, >> > Andy Shevchenko wrote: >> >> >> >> On Wed, Jan 7, 2015 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote: >> >> > At Wed, 7 Jan 2015 01:40:17 +0200, >> >> > Andy Shevchenko wrote: >> >> >> >> >> >> The managed functions allow to get ->probe() and ->remove() simplier. >> >> >> This patch converts code to use managed functions. >> >> > >> >> > This doesn't work well because of the order of release calls in >> >> > device_release(). devres_release_all() is called at first before >> >> > anything else. Thus the card's free callback would be called after >> >> > it, and snd_fm801_free() touches the hardware before disabling. >> >> > >> >> >> >> Hmm… I didn't get what make those calls. Sound core? The driver is a >> >> normal PCI driver, so resources will be freed *after* ->remove() of >> >> driver was called. >> >> Please, elaborate. >> > >> > Ah, OK, this seems working, as this is the managed *pci* device that >> > is a parent of the card device, thus it shall be released after the >> > children. >> > >> > But any reason not to use pcim_iomap_regions_request_all()? >> >> Yes. I was doubt to put a comment about this. > > Well, pcim_iomap_regions_request_all() can just request regions > without actually iomapping via passing 0 to mask. But, now looking at > the code again, I couldn't find the place releasing the non-mmio > regions. Hmm... it might be a leak? It's in snd_fm801_free(). pci_release_regions(chip->pci); > >> So, the initial idea is >> to move to MMIO instead of IO access. That's why you may see previous >> patch about fm801_{read,write}w() macros. >> I would like to do that later. > > Yeah, of course, rewriting to MMIO would be nicer. I guess you're > testing with the real hardware, so I'm for this rewrite. > > > thanks, > > Takashi > >> >> > >> > >> > thanks, >> > >> > Takashi >> > >> >> >> >> > >> >> > Takashi >> >> > >> >> >> While here remove the dead code and fix the value printed in error >> >> >> message. >> >> >> >> >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >> >> --- >> >> >> sound/pci/fm801.c | 29 +++++++---------------------- >> >> >> 1 file changed, 7 insertions(+), 22 deletions(-) >> >> >> >> >> >> diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c >> >> >> index dcda3c1..2708500 100644 >> >> >> --- a/sound/pci/fm801.c >> >> >> +++ b/sound/pci/fm801.c >> >> >> @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) >> >> >> v4l2_device_unregister(&chip->v4l2_dev); >> >> >> } >> >> >> #endif >> >> >> - if (chip->irq >= 0) >> >> >> - free_irq(chip->irq, chip); >> >> >> pci_release_regions(chip->pci); >> >> >> - pci_disable_device(chip->pci); >> >> >> >> >> >> - kfree(chip); >> >> >> return 0; >> >> >> } >> >> >> >> >> >> @@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, >> >> >> }; >> >> >> >> >> >> *rchip = NULL; >> >> >> - if ((err = pci_enable_device(pci)) < 0) >> >> >> + if ((err = pcim_enable_device(pci)) < 0) >> >> >> return err; >> >> >> - chip = kzalloc(sizeof(*chip), GFP_KERNEL); >> >> >> - if (chip == NULL) { >> >> >> - pci_disable_device(pci); >> >> >> + chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL); >> >> >> + if (chip == NULL) >> >> >> return -ENOMEM; >> >> >> - } >> >> >> spin_lock_init(&chip->reg_lock); >> >> >> chip->card = card; >> >> >> chip->pci = pci; >> >> >> chip->irq = -1; >> >> >> chip->tea575x_tuner = tea575x_tuner; >> >> >> - if ((err = pci_request_regions(pci, "FM801")) < 0) { >> >> >> - kfree(chip); >> >> >> - pci_disable_device(pci); >> >> >> + if ((err = pci_request_regions(pci, "FM801")) < 0) >> >> >> return err; >> >> >> - } >> >> >> chip->port = pci_resource_start(pci, 0); >> >> >> if ((tea575x_tuner & TUNER_ONLY) == 0) { >> >> >> - if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED, >> >> >> - KBUILD_MODNAME, chip)) { >> >> >> - dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq); >> >> >> + if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt, >> >> >> + IRQF_SHARED, KBUILD_MODNAME, chip)) { >> >> >> + dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); >> >> >> snd_fm801_free(chip); >> >> >> return -EBUSY; >> >> >> } >> >> >> @@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, >> >> >> /* init might set tuner access method */ >> >> >> tea575x_tuner = chip->tea575x_tuner; >> >> >> >> >> >> - if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) { >> >> >> - pci_clear_master(pci); >> >> >> - free_irq(chip->irq, chip); >> >> >> - chip->irq = -1; >> >> >> - } >> >> >> - >> >> >> if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { >> >> >> snd_fm801_free(chip); >> >> >> return err; >> >> >> -- >> >> >> 1.8.3.101.g727a46b >> >> >> >> >> >> >> >> >> >> >> -- >> >> With Best Regards, >> >> Andy Shevchenko >> >> >> >> >> >> -- >> With Best Regards, >> Andy Shevchenko >>
At Wed, 7 Jan 2015 15:27:35 +0200, Andy Shevchenko wrote: > > On Wed, Jan 7, 2015 at 3:12 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Wed, 7 Jan 2015 14:20:26 +0200, > > Andy Shevchenko wrote: > >> > >> On Wed, Jan 7, 2015 at 1:27 PM, Takashi Iwai <tiwai@suse.de> wrote: > >> > At Wed, 7 Jan 2015 12:54:40 +0200, > >> > Andy Shevchenko wrote: > >> >> > >> >> On Wed, Jan 7, 2015 at 9:11 AM, Takashi Iwai <tiwai@suse.de> wrote: > >> >> > At Wed, 7 Jan 2015 01:40:17 +0200, > >> >> > Andy Shevchenko wrote: > >> >> >> > >> >> >> The managed functions allow to get ->probe() and ->remove() simplier. > >> >> >> This patch converts code to use managed functions. > >> >> > > >> >> > This doesn't work well because of the order of release calls in > >> >> > device_release(). devres_release_all() is called at first before > >> >> > anything else. Thus the card's free callback would be called after > >> >> > it, and snd_fm801_free() touches the hardware before disabling. > >> >> > > >> >> > >> >> Hmm… I didn't get what make those calls. Sound core? The driver is a > >> >> normal PCI driver, so resources will be freed *after* ->remove() of > >> >> driver was called. > >> >> Please, elaborate. > >> > > >> > Ah, OK, this seems working, as this is the managed *pci* device that > >> > is a parent of the card device, thus it shall be released after the > >> > children. > >> > > >> > But any reason not to use pcim_iomap_regions_request_all()? > >> > >> Yes. I was doubt to put a comment about this. > > > > Well, pcim_iomap_regions_request_all() can just request regions > > without actually iomapping via passing 0 to mask. But, now looking at > > the code again, I couldn't find the place releasing the non-mmio > > regions. Hmm... it might be a leak? > > It's in snd_fm801_free(). > pci_release_regions(chip->pci); I meant the release of regions allocated in pcim_iomap_regions_request_all(). The function requests all regions, not only for MMIO ones: int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, const char *name) { int request_mask = ((1 << 6) - 1) & ~mask; int rc; rc = pci_request_selected_regions(pdev, request_mask, name); if (rc) return rc; rc = pcim_iomap_regions(pdev, mask, name); if (rc) pci_release_selected_regions(pdev, request_mask); return rc; } The regions allocated via pcim_iomap_regions() are released via pcim_iomap_release() properly. But what about the former one, the regions allocated via pci_request_selected_regions()...? Takashi
On Wed, Jan 7, 2015 at 3:31 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 7 Jan 2015 15:27:35 +0200, > Andy Shevchenko wrote: Regarding to the original patch, is it okay now to apply? [] > I meant the release of regions allocated in > pcim_iomap_regions_request_all(). The function requests all regions, > not only for MMIO ones: > > int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, > const char *name) > { > int request_mask = ((1 << 6) - 1) & ~mask; > int rc; > > rc = pci_request_selected_regions(pdev, request_mask, name); > if (rc) > return rc; > > rc = pcim_iomap_regions(pdev, mask, name); > if (rc) > pci_release_selected_regions(pdev, request_mask); > return rc; > } > > The regions allocated via pcim_iomap_regions() are released via > pcim_iomap_release() properly. But what about the former one, the > regions allocated via pci_request_selected_regions()...? Looks like it requires it's own release function to be implemented, yes. Good catch.
At Wed, 7 Jan 2015 15:40:26 +0200, Andy Shevchenko wrote: > > On Wed, Jan 7, 2015 at 3:31 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Wed, 7 Jan 2015 15:27:35 +0200, > > Andy Shevchenko wrote: > > Regarding to the original patch, is it okay now to apply? Yes. If you prefer, I can apply it first alone. Takashi
At Wed, 07 Jan 2015 14:43:41 +0100, Takashi Iwai wrote: > > At Wed, 7 Jan 2015 15:40:26 +0200, > Andy Shevchenko wrote: > > > > On Wed, Jan 7, 2015 at 3:31 PM, Takashi Iwai <tiwai@suse.de> wrote: > > > At Wed, 7 Jan 2015 15:27:35 +0200, > > > Andy Shevchenko wrote: > > > > Regarding to the original patch, is it okay now to apply? > > Yes. If you prefer, I can apply it first alone. OK, I found that the regions are released in pcim_release(). So, there is no leak. Meanwhile it means that pci_release_regions() is superfluous, too, once when you enabled the managed mode via pcim_enable_device(). Could you resubmit a v2 patch with that removal? thanks, Takashi
On Wed, Jan 7, 2015 at 3:57 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 07 Jan 2015 14:43:41 +0100, > Takashi Iwai wrote: >> >> At Wed, 7 Jan 2015 15:40:26 +0200, >> Andy Shevchenko wrote: >> > >> > On Wed, Jan 7, 2015 at 3:31 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > > At Wed, 7 Jan 2015 15:27:35 +0200, >> > > Andy Shevchenko wrote: >> > >> > Regarding to the original patch, is it okay now to apply? >> >> Yes. If you prefer, I can apply it first alone. Why not? :) > > OK, I found that the regions are released in pcim_release(). So, > there is no leak. > > Meanwhile it means that pci_release_regions() is superfluous, too, > once when you enabled the managed mode via pcim_enable_device(). > Could you resubmit a v2 patch with that removal? Within few hours together with the second patch. But if you can do this as a fixup it would be nice.
At Wed, 7 Jan 2015 16:25:22 +0200, Andy Shevchenko wrote: > > On Wed, Jan 7, 2015 at 3:57 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Wed, 07 Jan 2015 14:43:41 +0100, > > Takashi Iwai wrote: > >> > >> At Wed, 7 Jan 2015 15:40:26 +0200, > >> Andy Shevchenko wrote: > >> > > >> > On Wed, Jan 7, 2015 at 3:31 PM, Takashi Iwai <tiwai@suse.de> wrote: > >> > > At Wed, 7 Jan 2015 15:27:35 +0200, > >> > > Andy Shevchenko wrote: > >> > > >> > Regarding to the original patch, is it okay now to apply? > >> > >> Yes. If you prefer, I can apply it first alone. > > Why not? :) > > > > > OK, I found that the regions are released in pcim_release(). So, > > there is no leak. > > > > Meanwhile it means that pci_release_regions() is superfluous, too, > > once when you enabled the managed mode via pcim_enable_device(). > > Could you resubmit a v2 patch with that removal? > > Within few hours together with the second patch. But if you can do > this as a fixup it would be nice. OK, now applied this patch with a modification (removal of pci_release_regions()) to for-next branch. thanks, Takashi
At Wed, 07 Jan 2015 15:59:35 +0100, Takashi Iwai wrote: > > At Wed, 7 Jan 2015 16:25:22 +0200, > Andy Shevchenko wrote: > > > > On Wed, Jan 7, 2015 at 3:57 PM, Takashi Iwai <tiwai@suse.de> wrote: > > > At Wed, 07 Jan 2015 14:43:41 +0100, > > > Takashi Iwai wrote: > > >> > > >> At Wed, 7 Jan 2015 15:40:26 +0200, > > >> Andy Shevchenko wrote: > > >> > > > >> > On Wed, Jan 7, 2015 at 3:31 PM, Takashi Iwai <tiwai@suse.de> wrote: > > >> > > At Wed, 7 Jan 2015 15:27:35 +0200, > > >> > > Andy Shevchenko wrote: > > >> > > > >> > Regarding to the original patch, is it okay now to apply? > > >> > > >> Yes. If you prefer, I can apply it first alone. > > > > Why not? :) > > > > > > > > OK, I found that the regions are released in pcim_release(). So, > > > there is no leak. > > > > > > Meanwhile it means that pci_release_regions() is superfluous, too, > > > once when you enabled the managed mode via pcim_enable_device(). > > > Could you resubmit a v2 patch with that removal? > > > > Within few hours together with the second patch. But if you can do > > this as a fixup it would be nice. > > OK, now applied this patch with a modification (removal of > pci_release_regions()) to for-next branch. I recalled finally why I didn't want this sort of changes. Namely, devm_request_irq() can't be used safely for the shared PCI interrupts. There is a small open window between the driver's remove call (i.e. card's private_free or device free calls) and the call of devres_release_all(). The registered irq handler still remains during this window. When an irq is triggered from another shared device during this, it goes to the remaining irq handler and accesses the hardware unexpectedly. In the case of snd_fm801_interrupt(), it's not too bad, though. Takashi
On Tue, Jan 20, 2015 at 2:42 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 07 Jan 2015 15:59:35 +0100, > Takashi Iwai wrote: [] > I recalled finally why I didn't want this sort of changes. Namely, > devm_request_irq() can't be used safely for the shared PCI > interrupts. > > There is a small open window between the driver's remove call > (i.e. card's private_free or device free calls) and the call of > devres_release_all(). The registered irq handler still remains during > this window. When an irq is triggered from another shared device > during this, it goes to the remaining irq handler and accesses the > hardware unexpectedly. In the case of snd_fm801_interrupt(), it's not > too bad, though. Don't we have interrupts disabled on ->remove() stage?
At Tue, 20 Jan 2015 15:46:13 +0200, Andy Shevchenko wrote: > > On Tue, Jan 20, 2015 at 2:42 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Wed, 07 Jan 2015 15:59:35 +0100, > > Takashi Iwai wrote: > > [] > > > > I recalled finally why I didn't want this sort of changes. Namely, > > devm_request_irq() can't be used safely for the shared PCI > > interrupts. > > > > There is a small open window between the driver's remove call > > (i.e. card's private_free or device free calls) and the call of > > devres_release_all(). The registered irq handler still remains during > > this window. When an irq is triggered from another shared device > > during this, it goes to the remaining irq handler and accesses the > > hardware unexpectedly. In the case of snd_fm801_interrupt(), it's not > > too bad, though. > > Don't we have interrupts disabled on ->remove() stage? It's a shared irq, so it doesn't help even if your device disables the irq. The other device can trigger the irq line at any time. Takashi
On Tue, Jan 20, 2015 at 3:48 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Tue, 20 Jan 2015 15:46:13 +0200, > Andy Shevchenko wrote: >> >> On Tue, Jan 20, 2015 at 2:42 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > At Wed, 07 Jan 2015 15:59:35 +0100, >> > Takashi Iwai wrote: >> >> [] >> >> >> > I recalled finally why I didn't want this sort of changes. Namely, >> > devm_request_irq() can't be used safely for the shared PCI >> > interrupts. >> > >> > There is a small open window between the driver's remove call >> > (i.e. card's private_free or device free calls) and the call of >> > devres_release_all(). The registered irq handler still remains during >> > this window. When an irq is triggered from another shared device >> > during this, it goes to the remaining irq handler and accesses the >> > hardware unexpectedly. In the case of snd_fm801_interrupt(), it's not >> > too bad, though. >> >> Don't we have interrupts disabled on ->remove() stage? > > It's a shared irq, so it doesn't help even if your device disables the > irq. The other device can trigger the irq line at any time. Got it. We can call devm_free_irq() explicitly in the ->remove() then.
At Tue, 20 Jan 2015 15:51:05 +0200, Andy Shevchenko wrote: > > On Tue, Jan 20, 2015 at 3:48 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Tue, 20 Jan 2015 15:46:13 +0200, > > Andy Shevchenko wrote: > >> > >> On Tue, Jan 20, 2015 at 2:42 PM, Takashi Iwai <tiwai@suse.de> wrote: > >> > At Wed, 07 Jan 2015 15:59:35 +0100, > >> > Takashi Iwai wrote: > >> > >> [] > >> > >> > >> > I recalled finally why I didn't want this sort of changes. Namely, > >> > devm_request_irq() can't be used safely for the shared PCI > >> > interrupts. > >> > > >> > There is a small open window between the driver's remove call > >> > (i.e. card's private_free or device free calls) and the call of > >> > devres_release_all(). The registered irq handler still remains during > >> > this window. When an irq is triggered from another shared device > >> > during this, it goes to the remaining irq handler and accesses the > >> > hardware unexpectedly. In the case of snd_fm801_interrupt(), it's not > >> > too bad, though. > >> > >> Don't we have interrupts disabled on ->remove() stage? > > > > It's a shared irq, so it doesn't help even if your device disables the > > irq. The other device can trigger the irq line at any time. > > Got it. We can call devm_free_irq() explicitly in the ->remove() then. Right, but then there is no much merit to use devm_request_irq(), too :) As mentioned, in the case of fm801, it's not too bad. It's just a single register read, should read zero, and skip the rest. So we can live as is. My point is that we can't take this approach blindly "just because others do". Takashi
diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c index dcda3c1..2708500 100644 --- a/sound/pci/fm801.c +++ b/sound/pci/fm801.c @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) v4l2_device_unregister(&chip->v4l2_dev); } #endif - if (chip->irq >= 0) - free_irq(chip->irq, chip); pci_release_regions(chip->pci); - pci_disable_device(chip->pci); - kfree(chip); return 0; } @@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, }; *rchip = NULL; - if ((err = pci_enable_device(pci)) < 0) + if ((err = pcim_enable_device(pci)) < 0) return err; - chip = kzalloc(sizeof(*chip), GFP_KERNEL); - if (chip == NULL) { - pci_disable_device(pci); + chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL); + if (chip == NULL) return -ENOMEM; - } spin_lock_init(&chip->reg_lock); chip->card = card; chip->pci = pci; chip->irq = -1; chip->tea575x_tuner = tea575x_tuner; - if ((err = pci_request_regions(pci, "FM801")) < 0) { - kfree(chip); - pci_disable_device(pci); + if ((err = pci_request_regions(pci, "FM801")) < 0) return err; - } chip->port = pci_resource_start(pci, 0); if ((tea575x_tuner & TUNER_ONLY) == 0) { - if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED, - KBUILD_MODNAME, chip)) { - dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq); + if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt, + IRQF_SHARED, KBUILD_MODNAME, chip)) { + dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); snd_fm801_free(chip); return -EBUSY; } @@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, /* init might set tuner access method */ tea575x_tuner = chip->tea575x_tuner; - if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) { - pci_clear_master(pci); - free_irq(chip->irq, chip); - chip->irq = -1; - } - if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { snd_fm801_free(chip); return err;
The managed functions allow to get ->probe() and ->remove() simplier. This patch converts code to use managed functions. While here remove the dead code and fix the value printed in error message. Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> --- sound/pci/fm801.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)