[alsa-devel] [PATCH] ALSA: via82xx: Use common error handling code in snd_via82xx_create()
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; }
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;
snd_via82xx_free(chip);return -EBUSY;
err = -EBUSY;goto free_chip;
- 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
participants (3)
-
Dan Carpenter -
SF Markus Elfring -
Takashi Iwai