Message ID | 20230126150657.367921-16-hverkuil-cisco@xs4all.nl (mailing list archive) |
---|---|
State | Obsoleted |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1pL3qn-00A5da-R2; Thu, 26 Jan 2023 15:07:42 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232207AbjAZPHi (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 26 Jan 2023 10:07:38 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41218 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232172AbjAZPH2 (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 26 Jan 2023 10:07:28 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 32E196BBEC for <linux-media@vger.kernel.org>; Thu, 26 Jan 2023 07:07:20 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id DE93EB81DEF for <linux-media@vger.kernel.org>; Thu, 26 Jan 2023 15:07:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D26EBC433A4; Thu, 26 Jan 2023 15:07:16 +0000 (UTC) From: Hans Verkuil <hverkuil-cisco@xs4all.nl> To: linux-media@vger.kernel.org Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>, Sakari Ailus <sakari.ailus@linux.intel.com> Subject: [PATCH 15/17] media: i2c: adp1653: introduce 'no_child' label Date: Thu, 26 Jan 2023 16:06:55 +0100 Message-Id: <20230126150657.367921-16-hverkuil-cisco@xs4all.nl> X-Mailer: git-send-email 2.39.0 In-Reply-To: <20230126150657.367921-1-hverkuil-cisco@xs4all.nl> References: <20230126150657.367921-1-hverkuil-cisco@xs4all.nl> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.4 (--) X-LSpam-Report: No, score=-2.4 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
media: sparse/smatch fixes
|
|
Commit Message
Hans Verkuil
Jan. 26, 2023, 3:06 p.m. UTC
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(-)
Comments
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; }