Message ID | 2332472c-61d2-8c21-bf46-5c80745d6380@users.sourceforge.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, 22 Aug 2017 19:43:08 +0200, SF Markus Elfring wrote: > > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Tue, 22 Aug 2017 19:37:26 +0200 > > Add jump targets so that a bit of exception handling can be better reused > at the end of this function. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > sound/pci/via82xx_modem.c | 33 +++++++++++++++++++-------------- > 1 file changed, 19 insertions(+), 14 deletions(-) > > diff --git a/sound/pci/via82xx_modem.c b/sound/pci/via82xx_modem.c > index 55f79b2599e7..8659776887b2 100644 > --- a/sound/pci/via82xx_modem.c > +++ b/sound/pci/via82xx_modem.c > @@ -1110,8 +1110,8 @@ static int snd_via82xx_create(struct snd_card *card, > return err; > > if ((chip = kzalloc(sizeof(*chip), GFP_KERNEL)) == NULL) { > - pci_disable_device(pci); > - return -ENOMEM; > + err = -ENOMEM; > + goto disable_device; > } > > spin_lock_init(&chip->reg_lock); > @@ -1121,30 +1121,27 @@ static int snd_via82xx_create(struct snd_card *card, > > if ((err = pci_request_regions(pci, card->driver)) < 0) { > kfree(chip); > - pci_disable_device(pci); > - return err; > + goto disable_device; > } > chip->port = pci_resource_start(pci, 0); > if (request_irq(pci->irq, snd_via82xx_interrupt, IRQF_SHARED, > KBUILD_MODNAME, chip)) { > dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); > - snd_via82xx_free(chip); > - return -EBUSY; > + err = -EBUSY; > + goto free_chip; > } > chip->irq = pci->irq; > if (ac97_clock >= 8000 && ac97_clock <= 48000) > chip->ac97_clock = ac97_clock; > synchronize_irq(chip->irq); > > - if ((err = snd_via82xx_chip_init(chip)) < 0) { > - snd_via82xx_free(chip); > - return err; > - } > + err = snd_via82xx_chip_init(chip); > + if (err < 0) > + goto free_chip; > > - if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { > - snd_via82xx_free(chip); > - return err; > - } > + err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops); > + if (err < 0) > + goto free_chip; > > /* The 8233 ac97 controller does not implement the master bit > * in the pci command register. IMHO this is a violation of the PCI spec. > @@ -1153,6 +1150,14 @@ static int snd_via82xx_create(struct snd_card *card, > > *r_via = chip; > return 0; > + > +disable_device: > + pci_disable_device(pci); > + goto exit; > +free_chip: > + snd_via82xx_free(chip); > +exit: > + return err; Doubly goto doesn't look like an improvement. Takashi
On Tue, Aug 22, 2017 at 07:43:08PM +0200, SF Markus Elfring wrote: > @@ -1121,30 +1121,27 @@ static int snd_via82xx_create(struct snd_card *card, > > if ((err = pci_request_regions(pci, card->driver)) < 0) { > kfree(chip); > - pci_disable_device(pci); > - return err; > + goto disable_device; > } [ snip ] > @@ -1153,6 +1150,14 @@ static int snd_via82xx_create(struct snd_card *card, > > *r_via = chip; > return 0; > + > +disable_device: > + pci_disable_device(pci); > + goto exit; Why is this "goto exit" here? It's leaking now. > +free_chip: > + snd_via82xx_free(chip); > +exit: > + return err; > } regards, dan carpenter
>> @@ -1121,30 +1121,27 @@ static int snd_via82xx_create(struct snd_card *card, >> >> if ((err = pci_request_regions(pci, card->driver)) < 0) { >> kfree(chip); >> - pci_disable_device(pci); >> - return err; >> + goto disable_device; >> } > > [ snip ] > >> @@ -1153,6 +1150,14 @@ static int snd_via82xx_create(struct snd_card *card, >> >> *r_via = chip; >> return 0; >> + >> +disable_device: >> + pci_disable_device(pci); >> + goto exit; > > Why is this "goto exit" here? Should the same return statement be reached after the proposed refactoring? Would you like to move such a function call to this source code place? > It's leaking now. How do you come to this conclusion? >> +free_chip: >> + snd_via82xx_free(chip); >> +exit: >> + return err; >> } Is this update suggestion worth for another look? Regards, Markus
On Tue, Aug 22, 2017 at 10:05:37PM +0200, SF Markus Elfring wrote: > >> @@ -1121,30 +1121,27 @@ static int snd_via82xx_create(struct snd_card *card, > >> > >> if ((err = pci_request_regions(pci, card->driver)) < 0) { > >> kfree(chip); > >> - pci_disable_device(pci); > >> - return err; > >> + goto disable_device; Ah, you're right. It's not leaking. I thought you deleted the kfree(chip);... regards, dan carpenter
On Tue, 22 Aug 2017 22:16:12 +0200, Dan Carpenter wrote: > > On Tue, Aug 22, 2017 at 10:05:37PM +0200, SF Markus Elfring wrote: > > >> @@ -1121,30 +1121,27 @@ static int snd_via82xx_create(struct snd_card *card, > > >> > > >> if ((err = pci_request_regions(pci, card->driver)) < 0) { > > >> kfree(chip); > > >> - pci_disable_device(pci); > > >> - return err; > > >> + goto disable_device; > > Ah, you're right. It's not leaking. I thought you deleted the > kfree(chip);... Well, this also suggests something wrong by the patch... If you can't achieve it cleanly, better not to touch it in a half-baked way. Takashi
>> @@ -1153,6 +1150,14 @@ static int snd_via82xx_create(struct snd_card *card, >> >> *r_via = chip; >> return 0; >> + >> +disable_device: >> + pci_disable_device(pci); >> + goto exit; >> +free_chip: >> + snd_via82xx_free(chip); >> +exit: >> + return err; > > Doubly goto doesn't look like an improvement. Would you like to integrate another software update with the statement “return err;” instead of “goto exit;” at this place? Regards, Markus
diff --git a/sound/pci/via82xx_modem.c b/sound/pci/via82xx_modem.c index 55f79b2599e7..8659776887b2 100644 --- a/sound/pci/via82xx_modem.c +++ b/sound/pci/via82xx_modem.c @@ -1110,8 +1110,8 @@ static int snd_via82xx_create(struct snd_card *card, return err; if ((chip = kzalloc(sizeof(*chip), GFP_KERNEL)) == NULL) { - pci_disable_device(pci); - return -ENOMEM; + err = -ENOMEM; + goto disable_device; } spin_lock_init(&chip->reg_lock); @@ -1121,30 +1121,27 @@ static int snd_via82xx_create(struct snd_card *card, if ((err = pci_request_regions(pci, card->driver)) < 0) { kfree(chip); - pci_disable_device(pci); - return err; + goto disable_device; } chip->port = pci_resource_start(pci, 0); if (request_irq(pci->irq, snd_via82xx_interrupt, IRQF_SHARED, KBUILD_MODNAME, chip)) { dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); - snd_via82xx_free(chip); - return -EBUSY; + err = -EBUSY; + goto free_chip; } chip->irq = pci->irq; if (ac97_clock >= 8000 && ac97_clock <= 48000) chip->ac97_clock = ac97_clock; synchronize_irq(chip->irq); - if ((err = snd_via82xx_chip_init(chip)) < 0) { - snd_via82xx_free(chip); - return err; - } + err = snd_via82xx_chip_init(chip); + if (err < 0) + goto free_chip; - if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { - snd_via82xx_free(chip); - return err; - } + err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops); + if (err < 0) + goto free_chip; /* The 8233 ac97 controller does not implement the master bit * in the pci command register. IMHO this is a violation of the PCI spec. @@ -1153,6 +1150,14 @@ static int snd_via82xx_create(struct snd_card *card, *r_via = chip; return 0; + +disable_device: + pci_disable_device(pci); + goto exit; +free_chip: + snd_via82xx_free(chip); +exit: + return err; }