[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; } -- 2.14.0
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
@@ -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
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
participants (3)
-
Dan Carpenter -
SF Markus Elfring -
Takashi Iwai