diff mbox

ALSA: via82xx: Use common error handling code in snd_via82xx_create()

Message ID 2332472c-61d2-8c21-bf46-5c80745d6380@users.sourceforge.net (mailing list archive)
State New, archived
Headers show

Commit Message

SF Markus Elfring Aug. 22, 2017, 5:43 p.m. UTC 
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(-)

Comments

Takashi Iwai Aug. 22, 2017, 5:57 p.m. UTC | #1 
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
Dan Carpenter Aug. 22, 2017, 7:07 p.m. UTC | #2 
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
SF Markus Elfring Aug. 22, 2017, 8:05 p.m. UTC | #3 
>> @@ -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
Dan Carpenter Aug. 22, 2017, 8:16 p.m. UTC | #4 
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
Takashi Iwai Aug. 22, 2017, 8:19 p.m. UTC | #5 
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
SF Markus Elfring Nov. 19, 2017, 6:37 a.m. UTC | #6 
>> @@ -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 mbox

Patch

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;
 }