ALSA: ymfpci: Fine-tuning for some function implementations

PATCH Applied SF Markus Elfring 3 participants 10 messages Sep 6, 2017 – Sep 7, 2017

Subsystems (2)

Sound Kernel janitors

lore

Share & tools

lore

SF Markus Elfring

author

2017-09-06 19:45 UTC #0 lore raw

cover ALSA: ymfpci: Fine-tuning for some function implementations

From: Markus Elfring <[email protected]>
Date: Wed, 6 Sep 2017 21:37:32 +0200

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
Use common error handling code in snd_card_ymfpci_probe() Use common error handling code in snd_ymfpci_create() Adjust 17 checks for null pointers

 sound/pci/ymfpci/ymfpci.c      | 62 ++++++++++++++++++------------------
 sound/pci/ymfpci/ymfpci_main.c | 72 ++++++++++++++++++++++--------------------
 2 files changed, 67 insertions(+), 67 deletions(-)

Signature · 1 line

2.14.1

SF Markus Elfring

author

2017-09-06 19:46 UTC #1 lore raw

PATCH 1/3 ALSA: ymfpci: Use common error handling code in snd_card_ymfpci_probe()

From: Markus Elfring <[email protected]>
Date: Wed, 6 Sep 2017 20:45:11 +0200

* Add a jump target 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.

* The script "checkpatch.pl" pointed information out like the following.

ERROR: do not use assignment in if condition

Thus fix a few source code places.

Signed-off-by Markus Elfring <[email protected]>

---
 sound/pci/ymfpci/ymfpci.c | 62 +++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 32 deletions(-)

sound/pci/ymfpci/ymfpci.c +30 ~~-32~~

index 4faf3e1ed06a..eafdee384059 100644
@@ -268,10 +268,9 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci,
 	if ((err = snd_ymfpci_create(card, pci,
 				     old_legacy_ctrl,
 			 	     &chip)) < 0) {
-		snd_card_free(card);
 		release_and_free_resource(mpu_res);
 		release_and_free_resource(fm_res);
-		return err;
+		goto free_card;
 	}
 	chip->fm_res = fm_res;
 	chip->mpu_res = mpu_res;

@@ -283,35 +282,31 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci,
 		card->shortname,
 		chip->reg_area_phys,
 		chip->irq);
-	if ((err = snd_ymfpci_pcm(chip, 0)) < 0) {
-		snd_card_free(card);
-		return err;
-	}
-	if ((err = snd_ymfpci_pcm_spdif(chip, 1)) < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	err = snd_ymfpci_pcm(chip, 0);
+	if (err < 0)
+		goto free_card;
+
+	err = snd_ymfpci_pcm_spdif(chip, 1);
+	if (err < 0)
+		goto free_card;
+
 	err = snd_ymfpci_mixer(chip, rear_switch[dev]);
-	if (err < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	if (err < 0)
+		goto free_card;
+
 	if (chip->ac97->ext_id & AC97_EI_SDAC) {
 		err = snd_ymfpci_pcm_4ch(chip, 2);
-		if (err < 0) {
-			snd_card_free(card);
-			return err;
-		}
+		if (err < 0)
+			goto free_card;
+
 		err = snd_ymfpci_pcm2(chip, 3);
-		if (err < 0) {
-			snd_card_free(card);
-			return err;
-		}
-	}
-	if ((err = snd_ymfpci_timer(chip, 0)) < 0) {
-		snd_card_free(card);
-		return err;
+		if (err < 0)
+			goto free_card;
 	}
+	err = snd_ymfpci_timer(chip, 0);
+	if (err < 0)
+		goto free_card;
+
 	if (chip->mpu_res) {
 		if ((err = snd_mpu401_uart_new(card, 0, MPU401_HW_YMFPCI,
 					       mpu_port[dev],

@@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci,
 			legacy_ctrl &= ~YMFPCI_LEGACY_FMEN;
 			pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl);
 		} else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) {
-			snd_card_free(card);
 			dev_err(card->dev, "cannot create opl3 hwdep\n");
-			return err;
+			goto free_card;
 		}
 	}
 
 	snd_ymfpci_create_gameport(chip, dev, legacy_ctrl, legacy_ctrl2);
 
-	if ((err = snd_card_register(card)) < 0) {
-		snd_card_free(card);
-		return err;
-	}
+	err = snd_card_register(card);
+	if (err < 0)
+		goto free_card;
+
 	pci_set_drvdata(pci, card);
 	dev++;
 	return 0;
+
+free_card:
+	snd_card_free(card);
+	return err;
 }
 
 static void snd_card_ymfpci_remove(struct pci_dev *pci)

Signature · 1 line

2.14.1

Dan Carpenter

2017-09-06 21:51 UTC #2 lore raw

Re: [PATCH 1/3] ALSA: ymfpci: Use common error handling code in snd_card_ymfpci_probe()

sound/pci/ymfpci/ymfpci.c static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 339

Quoted 5 lines · SF Markus Elfring

sound/pci/ymfpci/ymfpci.c +0 -1

@@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci,
 			legacy_ctrl &= ~YMFPCI_LEGACY_FMEN;
 			pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl);
 		} else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) {
-			snd_card_free(card);

                        ^^^^^^^^^^^^^^^^^^^

sound/pci/ymfpci/ymfpci.c static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 334

Quoted 1 line

sound/pci/ymfpci/ymfpci.c +0 -0

 			dev_err(card->dev, "cannot create opl3 hwdep\n");

                                ^^^^^^^^^

Review static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 335

Quoted 2 lines

(quoted diff) +1 -1

-			return err;
+			goto free_card;

Heh. I was worried that some of these re-orderings would introduce bugs but actually this one fixes a use after free.

regards,
dan carpenter

SF Markus Elfring

author

2017-09-07 07:41 UTC #3 lore raw

Re: [PATCH 1/3] ALSA: ymfpci: Use common error handling code in snd_card_ymfpci_probe()

Quoted 6 lines

sound/pci/ymfpci/ymfpci.c static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 339

Quoted 5 lines

sound/pci/ymfpci/ymfpci.c +0 -1

@@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci,
 			legacy_ctrl &= ~YMFPCI_LEGACY_FMEN;
 			pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl);
 		} else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) {
-			snd_card_free(card);

                        ^^^^^^^^^^^^^^^^^^^

Quoted 7 lines

sound/pci/ymfpci/ymfpci.c static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 334
Quoted 1 line
sound/pci/ymfpci/ymfpci.c +0 -0
 			dev_err(card->dev, "cannot create opl3 hwdep\n");
                                ^^^^^^^^^
Review static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 335
Quoted 2 lines
(quoted diff) +1 -1
-			return err;
+			goto free_card;
Heh. I was worried that some of these re-orderings would introduce bugs but actually this one fixes a use after free.

Thanks for your constructive feedback.

Does it mean that a special tag should be added to a commit message?

Regards,
Markus

Takashi Iwai

2017-09-07 08:09 UTC #4 lore raw

Re: [PATCH 1/3] ALSA: ymfpci: Use common error handling code in snd_card_ymfpci_probe()

Quoted 15 lines · SF Markus Elfring

Quoted 7 lines
sound/pci/ymfpci/ymfpci.c static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 339
Quoted 6 lines
sound/pci/ymfpci/ymfpci.c +0 -1
@@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci,
 			legacy_ctrl &= ~YMFPCI_LEGACY_FMEN;
 			pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl);
 		} else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) {
-			snd_card_free(card);
                        ^^^^^^^^^^^^^^^^^^^
Quoted 7 lines
sound/pci/ymfpci/ymfpci.c static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 334
Quoted 1 line
sound/pci/ymfpci/ymfpci.c +0 -0
 			dev_err(card->dev, "cannot create opl3 hwdep\n");
                                ^^^^^^^^^
Review static int snd_card_ymfpci_probe(struct pci_dev *pci, · line 335
Quoted 2 lines
(quoted diff) +1 -1
-			return err;
+			goto free_card;
Heh. I was worried that some of these re-orderings would introduce bugs but actually this one fixes a use after free.

Quoted 3 lines

Thanks for your constructive feedback.
Does it mean that a special tag should be added to a commit message?

No need for resend, I'll add some more notes at merging.

thanks,

Takashi

Takashi Iwai

2017-09-07 08:35 UTC #5 lore raw

Re: [PATCH 1/3] ALSA: ymfpci: Use common error handling code in snd_card_ymfpci_probe()

In reply to SF Markus Elfring

Quoted 9 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 6 Sep 2017 20:45:11 +0200
* Add a jump target 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.

Quoted 7 lines

* The script "checkpatch.pl" pointed information out like the following.
ERROR: do not use assignment in if condition
Thus fix a few source code places.
Signed-off-by Markus Elfring <[email protected]>

Applied now. Thanks.

Takashi

SF Markus Elfring

author

2017-09-06 19:48 UTC #6 lore raw

PATCH 2/3 ALSA: ymfpci: Use common error handling code in snd_ymfpci_create()

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 6 Sep 2017 21:12:51 +0200

* Add a jump target 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.

* The script "checkpatch.pl" pointed information out like the following.

ERROR: do not use assignment in if condition

Thus fix a few source code places.

Signed-off-by Markus Elfring <[email protected]>

---
 sound/pci/ymfpci/ymfpci_main.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

sound/pci/ymfpci/ymfpci_main.c +21 ~~-20~~

index edfd58248082..8ca2e41e5827 100644
@@ -2399,59 +2399,60 @@ int snd_ymfpci_create(struct snd_card *card,
 		dev_err(chip->card->dev,
 			"unable to grab memory region 0x%lx-0x%lx\n",
 			chip->reg_area_phys, chip->reg_area_phys + 0x8000 - 1);
-		snd_ymfpci_free(chip);
-		return -EBUSY;
+		err = -EBUSY;
+		goto free_chip;
 	}
 	if (request_irq(pci->irq, snd_ymfpci_interrupt, IRQF_SHARED,
 			KBUILD_MODNAME, chip)) {
 		dev_err(chip->card->dev, "unable to grab IRQ %d\n", pci->irq);
-		snd_ymfpci_free(chip);
-		return -EBUSY;
+		err = -EBUSY;
+		goto free_chip;
 	}
 	chip->irq = pci->irq;
 
 	snd_ymfpci_aclink_reset(pci);
 	if (snd_ymfpci_codec_ready(chip, 0) < 0) {
-		snd_ymfpci_free(chip);
-		return -EIO;
+		err = -EIO;
+		goto free_chip;
 	}
 
 	err = snd_ymfpci_request_firmware(chip);
 	if (err < 0) {
 		dev_err(chip->card->dev, "firmware request failed: %d\n", err);
-		snd_ymfpci_free(chip);
-		return err;
+		goto free_chip;
 	}
 	snd_ymfpci_download_image(chip);
 
 	udelay(100); /* seems we need a delay after downloading image.. */
 
 	if (snd_ymfpci_memalloc(chip) < 0) {
-		snd_ymfpci_free(chip);
-		return -EIO;
+		err = -EIO;
+		goto free_chip;
 	}
 
-	if ((err = snd_ymfpci_ac3_init(chip)) < 0) {
-		snd_ymfpci_free(chip);
-		return err;
-	}
+	err = snd_ymfpci_ac3_init(chip);
+	if (err < 0)
+		goto free_chip;
 
 #ifdef CONFIG_PM_SLEEP
 	chip->saved_regs = kmalloc(YDSXGR_NUM_SAVED_REGS * sizeof(u32),
 				   GFP_KERNEL);
 	if (chip->saved_regs = NULL) {
-		snd_ymfpci_free(chip);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto free_chip;
 	}
 #endif
 
-	if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) {
-		snd_ymfpci_free(chip);
-		return err;
-	}
+	err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops);
+	if (err < 0)
+		goto free_chip;
 
 	snd_ymfpci_proc_init(card, chip);
 
 	*rchip = chip;
 	return 0;
+
+free_chip:
+	snd_ymfpci_free(chip);
+	return err;
 }

Signature · 1 line

2.14.1

Takashi Iwai

Applied

2017-09-07 08:35 UTC #7 lore raw

Re: [PATCH 2/3] ALSA: ymfpci: Use common error handling code in snd_ymfpci_create()

Quoted 9 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 6 Sep 2017 21:12:51 +0200
* Add a jump target 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.

Quoted 7 lines

* The script "checkpatch.pl" pointed information out like the following.
ERROR: do not use assignment in if condition
Thus fix a few source code places.
Signed-off-by Markus Elfring <[email protected]>

Applied, thanks.

Takashi

SF Markus Elfring

author

2017-09-06 19:50 UTC #8 lore raw

PATCH 3/3 ALSA: ymfpci: Adjust 17 checks for null pointers

In reply to SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 6 Sep 2017 21:30:37 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written …

Thus fix affected source code places.

Signed-off-by Markus Elfring <[email protected]>

---
 sound/pci/ymfpci/ymfpci_main.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

sound/pci/ymfpci/ymfpci_main.c +16 ~~-15~~

index 8ca2e41e5827..4da6e43ad007 100644
@@ -308,7 +308,7 @@ static void snd_ymfpci_pcm_interrupt(struct snd_ymfpci *chip, struct snd_ymfpci_
 	
 	if ((ypcm = voice->ypcm) = NULL)
 		return;
-	if (ypcm->substream = NULL)
+	if (!ypcm->substream)
 		return;
 	spin_lock(&chip->reg_lock);
 	if (ypcm->running) {

@@ -396,7 +396,7 @@ static int snd_ymfpci_playback_trigger(struct snd_pcm_substream *substream,
 	int result = 0;
 
 	spin_lock(&chip->reg_lock);
-	if (ypcm->voices[0] = NULL) {
+	if (!ypcm->voices[0]) {
 		result = -EINVAL;
 		goto __unlock;
 	}

@@ -405,7 +405,7 @@ static int snd_ymfpci_playback_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 	case SNDRV_PCM_TRIGGER_RESUME:
 		chip->ctrl_playback[ypcm->voices[0]->number + 1] = cpu_to_le32(ypcm->voices[0]->bank_addr);
-		if (ypcm->voices[1] != NULL && !ypcm->use_441_slot)
+		if (ypcm->voices[1] && !ypcm->use_441_slot)
 			chip->ctrl_playback[ypcm->voices[1]->number + 1] = cpu_to_le32(ypcm->voices[1]->bank_addr);
 		ypcm->running = 1;
 		break;

@@ -418,7 +418,7 @@ static int snd_ymfpci_playback_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		chip->ctrl_playback[ypcm->voices[0]->number + 1] = 0;
-		if (ypcm->voices[1] != NULL && !ypcm->use_441_slot)
+		if (ypcm->voices[1] && !ypcm->use_441_slot)
 			chip->ctrl_playback[ypcm->voices[1]->number + 1] = 0;
 		ypcm->running = 0;
 		break;

@@ -468,16 +468,16 @@ static int snd_ymfpci_pcm_voice_alloc(struct snd_ymfpci_pcm *ypcm, int voices)
 {
 	int err;
 
-	if (ypcm->voices[1] != NULL && voices < 2) {
+	if (ypcm->voices[1] && voices < 2) {
 		snd_ymfpci_voice_free(ypcm->chip, ypcm->voices[1]);
 		ypcm->voices[1] = NULL;
 	}
-	if (voices = 1 && ypcm->voices[0] != NULL)
+	if (voices = 1 && ypcm->voices[0])
 		return 0;		/* already allocated */
-	if (voices = 2 && ypcm->voices[0] != NULL && ypcm->voices[1] != NULL)
+	if (voices = 2 && ypcm->voices[0] && ypcm->voices[1])
 		return 0;		/* already allocated */
 	if (voices > 1) {
-		if (ypcm->voices[0] != NULL && ypcm->voices[1] = NULL) {
+		if (ypcm->voices[0] && !ypcm->voices[1]) {
 			snd_ymfpci_voice_free(ypcm->chip, ypcm->voices[0]);
 			ypcm->voices[0] = NULL;
 		}

@@ -655,7 +655,7 @@ static int snd_ymfpci_playback_hw_free(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_ymfpci_pcm *ypcm;
 	
-	if (runtime->private_data = NULL)
+	if (!runtime->private_data)
 		return 0;
 	ypcm = runtime->private_data;

@@ -913,7 +913,7 @@ static int snd_ymfpci_playback_open_1(struct snd_pcm_substream *substream)
 		return err;
 
 	ypcm = kzalloc(sizeof(*ypcm), GFP_KERNEL);
-	if (ypcm = NULL)
+	if (!ypcm)
 		return -ENOMEM;
 	ypcm->chip = chip;
 	ypcm->type = PLAYBACK_VOICE;

@@ -1038,7 +1038,7 @@ static int snd_ymfpci_capture_open(struct snd_pcm_substream *substream,
 		return err;
 
 	ypcm = kzalloc(sizeof(*ypcm), GFP_KERNEL);
-	if (ypcm = NULL)
+	if (!ypcm)
 		return -ENOMEM;
 	ypcm->chip = chip;
 	ypcm->type = capture_bank_number + CAPTURE_REC;

@@ -1116,7 +1116,7 @@ static int snd_ymfpci_capture_close(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_ymfpci_pcm *ypcm = runtime->private_data;
 
-	if (ypcm != NULL) {
+	if (ypcm) {
 		chip->capture_substream[ypcm->capture_bank_number] = NULL;
 		snd_ymfpci_hw_stop(chip);
 	}

@@ -1310,7 +1310,8 @@ static int snd_ymfpci_spdif_default_put(struct snd_kcontrol *kcontrol,
 	spin_lock_irq(&chip->reg_lock);
 	change = chip->spdif_bits != val;
 	chip->spdif_bits = val;
-	if ((snd_ymfpci_readw(chip, YDSXGR_SPDIFOUTCTRL) & 1) && chip->pcm_spdif = NULL)
+	if ((snd_ymfpci_readw(chip, YDSXGR_SPDIFOUTCTRL) & 1) &&
+	    !chip->pcm_spdif)
 		snd_ymfpci_writew(chip, YDSXGR_SPDIFOUTSTATUS, chip->spdif_bits);
 	spin_unlock_irq(&chip->reg_lock);
 	return change;

@@ -2376,7 +2377,7 @@ int snd_ymfpci_create(struct snd_card *card,
 		return err;
 
 	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
-	if (chip = NULL) {
+	if (!chip) {
 		pci_disable_device(pci);
 		return -ENOMEM;
 	}

@@ -2437,7 +2438,7 @@ int snd_ymfpci_create(struct snd_card *card,
 #ifdef CONFIG_PM_SLEEP
 	chip->saved_regs = kmalloc(YDSXGR_NUM_SAVED_REGS * sizeof(u32),
 				   GFP_KERNEL);
-	if (chip->saved_regs = NULL) {
+	if (!chip->saved_regs) {
 		err = -ENOMEM;
 		goto free_chip;
 	}

Signature · 1 line

2.14.1

Takashi Iwai

2017-09-07 08:35 UTC #9 lore raw

Re: [PATCH 3/3] ALSA: ymfpci: Adjust 17 checks for null pointers

Quoted 7 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
Date: Wed, 6 Sep 2017 21:30:37 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Quoted 7 lines

The script “checkpatch.pl” pointed information out like the following.
Comparison to NULL could be written …
Thus fix affected source code places.
Signed-off-by Markus Elfring <[email protected]>

I don't find such a change is needed at all.
Skipped.

Takashi