| Message ID | 20230126150657.367921-16-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: sparse/smatch fixes | expand |
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; > } >
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
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; }
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? 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(-)