[alsa-devel] [PATCH 0/3] ALSA: ymfpci: Fine-tuning for some function implementations
From: Markus Elfring elfring@users.sourceforge.net 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(-)
From: Markus Elfring elfring@users.sourceforge.net 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 elfring@users.sourceforge.net --- sound/pci/ymfpci/ymfpci.c | 62 +++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 32 deletions(-)
diff --git a/sound/pci/ymfpci/ymfpci.c b/sound/pci/ymfpci/ymfpci.c index 4faf3e1ed06a..eafdee384059 100644 --- a/sound/pci/ymfpci/ymfpci.c +++ b/sound/pci/ymfpci/ymfpci.c @@ -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)
On Wed, Sep 06, 2017 at 09:46:52PM +0200, SF Markus Elfring wrote:
@@ -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;
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
@@ -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;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
On Thu, 07 Sep 2017 09:41:39 +0200, SF Markus Elfring wrote:
@@ -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;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?
No need for resend, I'll add some more notes at merging.
thanks,
Takashi
On Wed, 06 Sep 2017 21:46:52 +0200, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net 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 elfring@users.sourceforge.net
Applied now. Thanks.
Takashi
From: Markus Elfring elfring@users.sourceforge.net 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 elfring@users.sourceforge.net --- sound/pci/ymfpci/ymfpci_main.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/sound/pci/ymfpci/ymfpci_main.c b/sound/pci/ymfpci/ymfpci_main.c index edfd58248082..8ca2e41e5827 100644 --- a/sound/pci/ymfpci/ymfpci_main.c +++ b/sound/pci/ymfpci/ymfpci_main.c @@ -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; }
On Wed, 06 Sep 2017 21:48:11 +0200, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net 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 elfring@users.sourceforge.net
Applied, thanks.
Takashi
From: Markus Elfring elfring@users.sourceforge.net 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 elfring@users.sourceforge.net --- sound/pci/ymfpci/ymfpci_main.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/sound/pci/ymfpci/ymfpci_main.c b/sound/pci/ymfpci/ymfpci_main.c index 8ca2e41e5827..4da6e43ad007 100644 --- a/sound/pci/ymfpci/ymfpci_main.c +++ b/sound/pci/ymfpci/ymfpci_main.c @@ -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; }
On Wed, 06 Sep 2017 21:50:09 +0200, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net 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 elfring@users.sourceforge.net
I don't find such a change is needed at all. Skipped.
Takashi
participants (3)
-
Dan Carpenter -
SF Markus Elfring -
Takashi Iwai