| Message ID | 20230126150657.367921-1-hverkuil-cisco@xs4all.nl |
|---|---|
| Headers | show |
| Series | media: sparse/smatch fixes | expand |
Hi Dan, While trying to clean up smatch warnings in the media subsystem I came across a number of 'warn: missing unwind goto?' warnings that all had the same root cause as illustrated by this patch: the 'unwind' path just has a variation on printk(), it is not actually cleaning up anything. As Sakari suggested, is this something that smatch can be improved for? These false positives are a bit annoying. You can see the whole series here if you are interested: https://patchwork.linuxtv.org/project/linux-media/list/?series=9747 Regards, Hans On 26/01/2023 16:19, Sakari Ailus wrote: > Hi Hans, > > On Thu, Jan 26, 2023 at 04:06:55PM +0100, Hans Verkuil wrote: >> The code mixed gotos and returns, which confused smatch. Add a no_child >> label before the 'return -EINVAL;' to help smatch understand this. >> It's a bit more consistent as well. >> >> This fixes this smatch warning: >> >> adp1653.c:444 adp1653_of_init() warn: missing unwind goto? > > This is clearly a false positive. There's also no reason to just have a > label where you simply return a value. > > Would it be possible to just fix smatch instead? > >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> drivers/media/i2c/adp1653.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c >> index a61a77de6eee..136bca801db7 100644 >> --- a/drivers/media/i2c/adp1653.c >> +++ b/drivers/media/i2c/adp1653.c >> @@ -420,7 +420,7 @@ static int adp1653_of_init(struct i2c_client *client, >> >> child = of_get_child_by_name(node, "flash"); >> if (!child) >> - return -EINVAL; >> + goto no_child; >> >> if (of_property_read_u32(child, "flash-timeout-us", >> &pd->max_flash_timeout)) >> @@ -441,7 +441,7 @@ static int adp1653_of_init(struct i2c_client *client, >> >> child = of_get_child_by_name(node, "indicator"); >> if (!child) >> - return -EINVAL; >> + goto no_child; >> >> if (of_property_read_u32(child, "led-max-microamp", >> &pd->max_indicator_intensity)) >> @@ -459,6 +459,7 @@ static int adp1653_of_init(struct i2c_client *client, >> err: >> dev_err(&client->dev, "Required property not found\n"); >> of_node_put(child); >> +no_child: >> return -EINVAL; >> } >> >
On Fri, Jan 27, 2023 at 08:43:51AM +0100, Hans Verkuil wrote: > Hi Dan, > > While trying to clean up smatch warnings in the media subsystem I came > across a number of 'warn: missing unwind goto?' warnings that all had > the same root cause as illustrated by this patch: the 'unwind' path > just has a variation on printk(), it is not actually cleaning up anything. > > As Sakari suggested, is this something that smatch can be improved for? > These false positives are a bit annoying. > > You can see the whole series here if you are interested: > > https://patchwork.linuxtv.org/project/linux-media/list/?series=9747 > > Regards, > Oh wow. I really hate do nothing gotos. The canonical bug for do nothing gotos is forgetting to set the error code. I like that check because it finds a lot of error handling bugs. It's not just about the printk(). I could and probably should make the check ignore printks. There is also an of_node_put(child); The check doesn't look at what the error handling does, only that there is a direct returns surrounded by gotos. I could make the check so that it's only enabled when --spammy is turned on. I guess another option would be to only enable the warning if there were more than one label in the cleanup section at the end of the function. I can make that a warning and if there is only one label, then make that disable unless --spammy is used. regards, dan carpenter
On 27/01/2023 13:41, Dan Carpenter wrote: > On Fri, Jan 27, 2023 at 08:43:51AM +0100, Hans Verkuil wrote: >> Hi Dan, >> >> While trying to clean up smatch warnings in the media subsystem I came >> across a number of 'warn: missing unwind goto?' warnings that all had >> the same root cause as illustrated by this patch: the 'unwind' path >> just has a variation on printk(), it is not actually cleaning up anything. >> >> As Sakari suggested, is this something that smatch can be improved for? >> These false positives are a bit annoying. >> >> You can see the whole series here if you are interested: >> >> https://patchwork.linuxtv.org/project/linux-media/list/?series=9747 >> >> Regards, >> > > Oh wow. I really hate do nothing gotos. The canonical bug for do > nothing gotos is forgetting to set the error code. > > I like that check because it finds a lot of error handling bugs. I do too, I did find some real bugs with this as well. > > It's not just about the printk(). I could and probably should make the > check ignore printks. There is also an of_node_put(child); This are other examples in my patch series where there is only an error message: https://patchwork.linuxtv.org/project/linux-media/patch/20230126150657.367921-15-hverkuil-cisco@xs4all.nl/ https://patchwork.linuxtv.org/project/linux-media/patch/20230126150657.367921-17-hverkuil-cisco@xs4all.nl/ > > The check doesn't look at what the error handling does, only that there > is a direct returns surrounded by gotos. > > I could make the check so that it's only enabled when --spammy is turned > on. > > I guess another option would be to only enable the warning if there were > more than one label in the cleanup section at the end of the function. > I can make that a warning and if there is only one label, then make that > disable unless --spammy is used. That would cause us to miss a real bug like this: https://patchwork.linuxtv.org/project/linux-media/patch/20230126150657.367921-8-hverkuil-cisco@xs4all.nl/ I think that if you were able to check that all the code after a goto label consisted of variations on printk, then that would solve most of the false positives I've seen in the media subsystem. The few remaining ones we can work around ourselves. Regards, Hans > > regards, > dan carpenter >
On Fri, Jan 27, 2023 at 02:00:51PM +0100, Hans Verkuil wrote: > I think that if you were able to check that all the code after a goto label > consisted of variations on printk, then that would solve most of the > false positives I've seen in the media subsystem. Ok. That is doable. regards, dan carpenter
On 26.01.2023 17:06, Hans Verkuil wrote: > > The err_irq and err_clk labels just did a 'return ret'. So > drop these and replace them by just a return. > > This fixes a smatch warning: > > mxc-jpeg.c:2492 mxc_jpeg_probe() warn: missing unwind goto? > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Mirela Rabulea <mirela.rabulea@gmail.com> > Cc: Mirela Rabulea <mirela.rabulea@nxp.com> > --- > .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c > index 6cd015a35f7c..552d0900cb26 100644 > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c > @@ -2445,7 +2445,7 @@ static int mxc_jpeg_probe(struct platform_device *pdev) > ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > if (ret) { > dev_err(&pdev->dev, "No suitable DMA available.\n"); > - goto err_irq; > + return ret; > } > > jpeg->base_reg = devm_platform_ioremap_resource(pdev, 0); > @@ -2454,16 +2454,14 @@ static int mxc_jpeg_probe(struct platform_device *pdev) > > for (slot = 0; slot < MXC_MAX_SLOTS; slot++) { > dec_irq = platform_get_irq(pdev, slot); > - if (dec_irq < 0) { > - ret = dec_irq; > - goto err_irq; > - } > + if (dec_irq < 0) > + return dec_irq; > ret = devm_request_irq(&pdev->dev, dec_irq, mxc_jpeg_dec_irq, > 0, pdev->name, jpeg); > if (ret) { > dev_err(&pdev->dev, "Failed to request irq %d (%d)\n", > dec_irq, ret); > - goto err_irq; > + return ret; > } > } > > @@ -2475,15 +2473,13 @@ static int mxc_jpeg_probe(struct platform_device *pdev) > jpeg->clk_ipg = devm_clk_get(dev, "ipg"); > if (IS_ERR(jpeg->clk_ipg)) { > dev_err(dev, "failed to get clock: ipg\n"); > - ret = PTR_ERR(jpeg->clk_ipg); > - goto err_clk; > + return PTR_ERR(jpeg->clk_ipg); > } > > jpeg->clk_per = devm_clk_get(dev, "per"); > if (IS_ERR(jpeg->clk_per)) { > dev_err(dev, "failed to get clock: per\n"); > - ret = PTR_ERR(jpeg->clk_per); > - goto err_clk; > + return PTR_ERR(jpeg->clk_per); > } > > ret = mxc_jpeg_attach_pm_domains(jpeg); > @@ -2569,9 +2565,6 @@ static int mxc_jpeg_probe(struct platform_device *pdev) > > err_register: > mxc_jpeg_detach_pm_domains(jpeg); > - > -err_irq: > -err_clk: > return ret; > } > > -- > 2.39.0 >
Hi Hans, Thank you for the patch. On Thu, Jan 26, 2023 at 3:07 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > DEFINE_RES_IRQ_NAMED already casts to (struct resource), so no > need to do it again. > > This fixes a sparse warning: > > vpif.c:483:20: warning: cast to non-scalar > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Cc: "Lad, Prabhakar" <prabhakar.csengg@gmail.com> > --- > drivers/media/platform/ti/davinci/vpif.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Lad Prabhakar <prabhakar.csengg@gmail.com> Cheers, Prabhakar > diff --git a/drivers/media/platform/ti/davinci/vpif.c b/drivers/media/platform/ti/davinci/vpif.c > index da27da4c165a..832489822706 100644 > --- a/drivers/media/platform/ti/davinci/vpif.c > +++ b/drivers/media/platform/ti/davinci/vpif.c > @@ -480,7 +480,7 @@ static int vpif_probe(struct platform_device *pdev) > ret = irq; > goto err_put_rpm; > } > - res_irq = (struct resource)DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node)); > + res_irq = DEFINE_RES_IRQ_NAMED(irq, of_node_full_name(pdev->dev.of_node)); > res_irq.flags |= irq_get_trigger_type(irq); > > pdev_capture = kzalloc(sizeof(*pdev_capture), GFP_KERNEL); > -- > 2.39.0 >
We have way too many sparse/smatch warnings and errors today, making it hard to tell when new issues are introduced. This series will get rid of most of them. Regards, Hans Hans Verkuil (17): media: visl: make visl_qops static media: davinci/vpif.c: drop unnecessary cast media: i2c: s5c73m3: return 0 instead of 'ret'. media: dvb-frontends: cxd2880: return 0 instead of 'ret'. media: usb: dvb-usb-v2: af9015.c: return 0 instead of 'ret'. media: dvb-frontends: cxd2880: return 0 instead of 'ret'. media: marvell: change return to goto for proper unwind media: dvb-frontends: drx39xyj: replace return with goto for proper unwind media: nxp: imx-jpeg: replace dummy gotos by returns media: mediatek: mdp3: replace return by goto for proper unwind media: mediatek: vcodec/venc: return 0 instead of 'ret'. media: ti: davinci: vpbe_display.c: return 0 instead of 'ret'. media: i2c: ov7670: 0 instead of -EINVAL was returned media: usb: go7007: add second unwind label media: i2c: adp1653: introduce 'no_child' label media: st: delta: introduce 'err_too_many_comps' label media: dvb-frontends: mb86a16.c: always use the same error path .../dvb-frontends/cxd2880/cxd2880_tnrdmd.c | 4 ++-- .../cxd2880/cxd2880_tnrdmd_dvbt.c | 14 +++++++------- .../cxd2880/cxd2880_tnrdmd_dvbt2.c | 14 +++++++------- drivers/media/dvb-frontends/drx39xyj/drxj.c | 9 ++++++--- drivers/media/dvb-frontends/mb86a16.c | 9 ++++++--- drivers/media/i2c/adp1653.c | 5 +++-- drivers/media/i2c/ov7670.c | 2 +- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +- drivers/media/platform/marvell/mmp-driver.c | 2 +- .../platform/mediatek/mdp3/mtk-mdp3-comp.c | 3 ++- .../mediatek/vcodec/venc/venc_h264_if.c | 4 ++-- .../media/platform/nxp/imx-jpeg/mxc-jpeg.c | 19 ++++++------------- .../platform/st/sti/delta/delta-mjpeg-hdr.c | 16 +++++++++------- .../media/platform/ti/davinci/vpbe_display.c | 2 +- drivers/media/platform/ti/davinci/vpif.c | 2 +- drivers/media/test-drivers/visl/visl-video.c | 2 +- drivers/media/usb/dvb-usb-v2/af9015.c | 4 ++-- drivers/media/usb/go7007/go7007-usb.c | 11 +++++++---- 18 files changed, 65 insertions(+), 59 deletions(-)