[alsa-devel] [PATCH 0/10] fix error return code
These patches fix cases where the return code appears to be unintentially nonnegative.
The complete semantic match that finds the problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> @ok exists@ identifier f,ret,i; expression e; constant c; @@
f(...) { <+... ( return -c@i; | ret = -c@i; ... when != ret = e return ret; | if (ret < 0) { ... return ret; } ) ...+> }
@r exists@ identifier ret,l,ok.f; expression e1,e2,e3; statement S; position p1,p2,p3; @@
f(...) { ... when any ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret ( if (<+... ret = e3 ...+>) S | if (<+... &ret ...+>) S | if@p2(...) { ... when != ret = e2 when forall return@p3 ret; } ) ... when any }
@bad exists@ position r.p1,r.p2; statement S1,S2; identifier r.ret; expression e1; @@
( if@p1 ((ret < 0|ret != 0)) S1 | ret@p1 = 0 ) ... when any ret = e1 ... when any if@p2(...) S2
@bad2@ position r.p1,r.p2; identifier r.ret; expression e1; statement S2; @@
ret@p1 = 0 ... when != if (...) { ... ret = e1 ... return ret; } when any if@p2(...) S2
@script:python depends on !bad && !bad2@ p1 << r.p1; p2 << r.p2; p3 << r.p3; @@
cocci.print_main("",p1) cocci.print_secs("",p2) cocci.print_secs("",p3) // </smpl>
From: Julia Lawall Julia.Lawall@lip6.fr
Initialize ret on the second call to imx_audmux_v2_configure_port so that the subsequent test checks that result and not the previous one.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/soc/fsl/imx-sgtl5000.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index fb21b17..199408e 100644 --- a/sound/soc/fsl/imx-sgtl5000.c +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -94,7 +94,7 @@ static int __devinit imx_sgtl5000_probe(struct platform_device *pdev) dev_err(&pdev->dev, "audmux internal port setup failed\n"); return ret; } - imx_audmux_v2_configure_port(ext_port, + ret = imx_audmux_v2_configure_port(ext_port, IMX_AUDMUX_V2_PTCR_SYN, IMX_AUDMUX_V2_PDCR_RXDSEL(int_port)); if (ret) {
On Sun, Aug 19, 2012 at 09:02:52AM +0200, Julia Lawall wrote:
From: Julia Lawall Julia.Lawall@lip6.fr
Initialize ret on the second call to imx_audmux_v2_configure_port so that the subsequent test checks that result and not the previous one.
Applied all ASoC patches, thanks.
From: Julia Lawall Julia.Lawall@lip6.fr
Remove unnecessary calls to devm_kfree and replace iounmap by devm_iounmap (and use resource_size for the third argument). These changes make it possible to remove the error-handling code at the end of ux500_msp_i2s_init_msp, and all of the gotos become direct returns.
In the case of the second call to devm_kzalloc, the return variable ret was not initialized. Here it is changed to a direct return of -ENOMEM.
A simplified version of the semantic match that finds the second problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/soc/ux500/ux500_msp_i2s.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c index 36be11e..1b7c2f5 100644 --- a/sound/soc/ux500/ux500_msp_i2s.c +++ b/sound/soc/ux500/ux500_msp_i2s.c @@ -663,7 +663,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev, struct ux500_msp **msp_p, struct msp_i2s_platform_data *platform_data) { - int ret = 0; struct resource *res = NULL; struct i2s_controller *i2s_cont; struct ux500_msp *msp; @@ -687,15 +686,14 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev, if (res == NULL) { dev_err(&pdev->dev, "%s: ERROR: Unable to get resource!\n", __func__); - ret = -ENOMEM; - goto err_res; + return -ENOMEM; }
- msp->registers = ioremap(res->start, (res->end - res->start + 1)); + msp->registers = devm_ioremap(&pdev->dev, res->start, + resource_size(res)); if (msp->registers == NULL) { dev_err(&pdev->dev, "%s: ERROR: ioremap failed!\n", __func__); - ret = -ENOMEM; - goto err_res; + return -ENOMEM; }
msp->msp_state = MSP_STATE_IDLE; @@ -707,7 +705,7 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev, dev_err(&pdev->dev, "%s: ERROR: Failed to allocate I2S-controller!\n", __func__); - goto err_i2s_cont; + return -ENOMEM; } i2s_cont->dev.parent = &pdev->dev; i2s_cont->data = (void *)msp; @@ -718,14 +716,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev, msp->i2s_cont = i2s_cont;
return 0; - -err_i2s_cont: - iounmap(msp->registers); - -err_res: - devm_kfree(&pdev->dev, msp); - - return ret; }
void ux500_msp_i2s_cleanup_msp(struct platform_device *pdev, @@ -734,11 +724,6 @@ void ux500_msp_i2s_cleanup_msp(struct platform_device *pdev, dev_dbg(msp->dev, "%s: Enter (id = %d).\n", __func__, msp->id);
device_unregister(&msp->i2s_cont->dev); - devm_kfree(&pdev->dev, msp->i2s_cont); - - iounmap(msp->registers); - - devm_kfree(&pdev->dev, msp); }
MODULE_LICENSE("GPL v2");
From: Julia Lawall Julia.Lawall@lip6.fr
Convert a nonnegative error return code to a negative one, as returned elsewhere in the function.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/pci/rme9652/hdspm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index b8ac871..b12308b 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6585,7 +6585,7 @@ static int __devinit snd_hdspm_create(struct snd_card *card, snd_printk(KERN_ERR "HDSPM: " "unable to kmalloc Mixer memory of %d Bytes\n", (int)sizeof(struct hdspm_mixer)); - return err; + return -ENOMEM; }
hdspm->port_names_in = NULL;
From: Julia Lawall Julia.Lawall@lip6.fr
Initialize rc before returning on failure, as done elsewhere in the function.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/pci/sis7019.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/pci/sis7019.c b/sound/pci/sis7019.c index 535efe2..51e4340 100644 --- a/sound/pci/sis7019.c +++ b/sound/pci/sis7019.c @@ -1377,8 +1377,9 @@ static int __devinit sis_chip_create(struct snd_card *card, if (rc) goto error_out_cleanup;
- if (request_irq(pci->irq, sis_interrupt, IRQF_SHARED, KBUILD_MODNAME, - sis)) { + rc = request_irq(pci->irq, sis_interrupt, IRQF_SHARED, KBUILD_MODNAME, + sis); + if (rc) { dev_err(&pci->dev, "unable to allocate irq %d\n", sis->irq); goto error_out_cleanup; }
From: Julia Lawall Julia.Lawall@lip6.fr
Initialize err before returning on failure, as done elsewhere in the function.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/pci/ctxfi/ctatc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/pci/ctxfi/ctatc.c b/sound/pci/ctxfi/ctatc.c index 58b235c..a2f997a 100644 --- a/sound/pci/ctxfi/ctatc.c +++ b/sound/pci/ctxfi/ctatc.c @@ -1725,8 +1725,10 @@ int __devinit ct_atc_create(struct snd_card *card, struct pci_dev *pci, atc_connect_resources(atc);
atc->timer = ct_timer_new(atc); - if (!atc->timer) + if (!atc->timer) { + err = -ENOMEM; goto error1; + }
err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, atc, &ops); if (err < 0)
From: Julia Lawall Julia.Lawall@lip6.fr
In the first case, the second test of whether retval is negative is redundant. It is dropped and the previous and subsequent tests are combined.
In the second case, add an initialization of retval on failure of ioremap.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/atmel/ac97c.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c index 3c8d3ba..9052aff 100644 --- a/sound/atmel/ac97c.c +++ b/sound/atmel/ac97c.c @@ -278,14 +278,9 @@ static int atmel_ac97c_capture_hw_params(struct snd_pcm_substream *substream, if (retval < 0) return retval; /* snd_pcm_lib_malloc_pages returns 1 if buffer is changed. */ - if (cpu_is_at32ap7000()) { - if (retval < 0) - return retval; - /* snd_pcm_lib_malloc_pages returns 1 if buffer is changed. */ - if (retval == 1) - if (test_and_clear_bit(DMA_RX_READY, &chip->flags)) - dw_dma_cyclic_free(chip->dma.rx_chan); - } + if (cpu_is_at32ap7000() && retval == 1) + if (test_and_clear_bit(DMA_RX_READY, &chip->flags)) + dw_dma_cyclic_free(chip->dma.rx_chan);
/* Set restrictions to params. */ mutex_lock(&opened_mutex); @@ -980,6 +975,7 @@ static int __devinit atmel_ac97c_probe(struct platform_device *pdev)
if (!chip->regs) { dev_dbg(&pdev->dev, "could not remap register memory\n"); + retval = -ENOMEM; goto err_ioremap; }
From: Julia Lawall Julia.Lawall@lip6.fr
Initialize retval before returning from a failed call to ioremap.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/atmel/abdac.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/atmel/abdac.c b/sound/atmel/abdac.c index 98554f4..277ebce 100644 --- a/sound/atmel/abdac.c +++ b/sound/atmel/abdac.c @@ -452,6 +452,7 @@ static int __devinit atmel_abdac_probe(struct platform_device *pdev) dac->regs = ioremap(regs->start, resource_size(regs)); if (!dac->regs) { dev_dbg(&pdev->dev, "could not remap register memory\n"); + retval = -ENOMEM; goto out_free_card; }
From: Julia Lawall Julia.Lawall@lip6.fr
Initialize ret before returning on failure, as done elsewhere in the function.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/ppc/snd_ps3.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/ppc/snd_ps3.c b/sound/ppc/snd_ps3.c index 1aa52ef..9b18b52 100644 --- a/sound/ppc/snd_ps3.c +++ b/sound/ppc/snd_ps3.c @@ -1040,6 +1040,7 @@ static int __devinit snd_ps3_driver_probe(struct ps3_system_bus_device *dev) GFP_KERNEL); if (!the_card.null_buffer_start_vaddr) { pr_info("%s: nullbuffer alloc failed\n", __func__); + ret = -ENOMEM; goto clean_preallocate; } pr_debug("%s: null vaddr=%p dma=%#llx\n", __func__,
From: Julia Lawall Julia.Lawall@lip6.fr
It was forgotten to initialize ret to the result of calling snd_soc_dai_set_sysclk, unlike at the other calls in the same function.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
--- sound/soc/omap/am3517evm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/omap/am3517evm.c b/sound/soc/omap/am3517evm.c index 009533a..df65f98 100644 --- a/sound/soc/omap/am3517evm.c +++ b/sound/soc/omap/am3517evm.c @@ -59,7 +59,7 @@ static int am3517evm_hw_params(struct snd_pcm_substream *substream, return ret; }
- snd_soc_dai_set_sysclk(cpu_dai, OMAP_MCBSP_FSR_SRC_FSX, 0, + ret = snd_soc_dai_set_sysclk(cpu_dai, OMAP_MCBSP_FSR_SRC_FSX, 0, SND_SOC_CLOCK_IN); if (ret < 0) { printk(KERN_ERR "can't set CPU system clock OMAP_MCBSP_FSR_SRC_FSX\n");
On 08/19/2012 10:03 AM, Julia Lawall wrote:
From: Julia Lawall Julia.Lawall@lip6.fr
It was forgotten to initialize ret to the result of calling snd_soc_dai_set_sysclk, unlike at the other calls in the same function.
A simplified version of the semantic match that finds this problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret *if(...) { ... when != ret = e2 when forall return ret; }
// </smpl>
Signed-off-by: Julia Lawall Julia.Lawall@lip6.fr
sound/soc/omap/am3517evm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/omap/am3517evm.c b/sound/soc/omap/am3517evm.c index 009533a..df65f98 100644 --- a/sound/soc/omap/am3517evm.c +++ b/sound/soc/omap/am3517evm.c @@ -59,7 +59,7 @@ static int am3517evm_hw_params(struct snd_pcm_substream *substream, return ret; }
- snd_soc_dai_set_sysclk(cpu_dai, OMAP_MCBSP_FSR_SRC_FSX, 0,
- ret = snd_soc_dai_set_sysclk(cpu_dai, OMAP_MCBSP_FSR_SRC_FSX, 0, SND_SOC_CLOCK_IN); if (ret < 0) { printk(KERN_ERR "can't set CPU system clock OMAP_MCBSP_FSR_SRC_FSX\n");
Acked-by: Jarkko Nikula jarkko.nikula@bitmer.com
At Sun, 19 Aug 2012 09:02:51 +0200, Julia Lawall wrote:
These patches fix cases where the return code appears to be unintentially nonnegative.
The patch 10/10 seems missing. Or are all 9 patches? The patch number suddenly changes to X/9 from 4.
In anyway, the patches 3, 5, 6, 7, 8 and 9 are applied. I leave patches 1, 2 and 4 to Mark.
thanks,
Takashi
The complete semantic match that finds the problem is as follows: (http://coccinelle.lip6.fr/)
// <smpl> @ok exists@ identifier f,ret,i; expression e; constant c; @@
f(...) { <+... ( return -c@i; | ret = -c@i; ... when != ret = e return ret; | if (ret < 0) { ... return ret; } ) ...+> }
@r exists@ identifier ret,l,ok.f; expression e1,e2,e3; statement S; position p1,p2,p3; @@
f(...) { ... when any ( if@p1 ((ret < 0|ret != 0)) { ... return ret; } | ret@p1 = 0 ) ... when != ret = e1 when != &ret ( if (<+... ret = e3 ...+>) S | if (<+... &ret ...+>) S | if@p2(...) { ... when != ret = e2 when forall return@p3 ret; } ) ... when any }
@bad exists@ position r.p1,r.p2; statement S1,S2; identifier r.ret; expression e1; @@
( if@p1 ((ret < 0|ret != 0)) S1 | ret@p1 = 0 ) ... when any ret = e1 ... when any if@p2(...) S2
@bad2@ position r.p1,r.p2; identifier r.ret; expression e1; statement S2; @@
ret@p1 = 0 ... when != if (...) { ... ret = e1 ... return ret; } when any if@p2(...) S2
@script:python depends on !bad && !bad2@ p1 << r.p1; p2 << r.p2; p3 << r.p3; @@
cocci.print_main("",p1) cocci.print_secs("",p2) cocci.print_secs("",p3) // </smpl>
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 20 Aug 2012, Takashi Iwai wrote:
At Sun, 19 Aug 2012 09:02:51 +0200, Julia Lawall wrote:
These patches fix cases where the return code appears to be unintentially nonnegative.
The patch 10/10 seems missing. Or are all 9 patches? The patch number suddenly changes to X/9 from 4.
In anyway, the patches 3, 5, 6, 7, 8 and 9 are applied. I leave patches 1, 2 and 4 to Mark.
Sorry, I thought I fixed that, but I guess not enough. Patches 4 and 5 were the same originally. There is no patch 10.
julia
participants (4)
-
Jarkko Nikula -
Julia Lawall -
Mark Brown -
Takashi Iwai