From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751795AbdHaIXt (ORCPT ); Thu, 31 Aug 2017 04:23:49 -0400 Received: from mail2-relais-roc.national.inria.fr ([192.134.164.83]:63415 "EHLO mail2-relais-roc.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751572AbdHaIXr (ORCPT ); Thu, 31 Aug 2017 04:23:47 -0400 X-IronPort-AV: E=Sophos;i="5.41,451,1498514400"; d="scan'208";a="288982073" Date: Thu, 31 Aug 2017 10:23:19 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@hadrien To: Alexandre Belloni cc: Christophe JAILLET , perex@perex.cz, tiwai@suse.com, arvind.yadav.cs@gmail.com, nicolas.ferre@microchip.com, broonie@kernel.org, garsilva@embeddedor.com, andriy.shevchenko@linux.intel.com, bhumirks@gmail.com, alsa-devel@alsa-project.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [alsa-devel] [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()' In-Reply-To: <20170831081021.g4luo557ggtnyfyg@piout.net> Message-ID: References: <20170831044042.23306-1-christophe.jaillet@wanadoo.fr> <20170831081021.g4luo557ggtnyfyg@piout.net> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 31 Aug 2017, Alexandre Belloni wrote: > On 31/08/2017 at 06:40:42 +0200, Christophe JAILLET wrote: > > If 'clk_prepare_enable()' fails, we must release some resources before > > returning. Add a new label in the existing error handling path and 'goto' > > there. > > > > Fixes: 260ea95cc027 ("ASoC: atmel: ac97c: Handle return value of clk_prepare_enable.") > > Signed-off-by: Christophe JAILLET > > And here is the fallout of the stupid, brainless "fixing" of issues > reported by static analysis tools. > > This clk_prepare_enable will never fail. If it was going to fail, the > platform would never boot to a point were it is able to execute that > code. It is really annoying to have so much churn for absolutely 0 > benefit. Would it be more productive to put the code back like it was before, ie no return value and no check, and add a comment to the definition of clk_prepare_enable indicating that there are many case where the call cannot fail? Grepping through the code suggests that it is about 50-50 on checking the return value or not doing so, which might suggest that checking the value is often not required. julia > > Anyway, > Acked-by: Alexandre Belloni > > > --- > > sound/atmel/ac97c.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c > > index 30c64ab210d9..5ffefac2fa8f 100644 > > --- a/sound/atmel/ac97c.c > > +++ b/sound/atmel/ac97c.c > > @@ -785,7 +785,7 @@ static int atmel_ac97c_probe(struct platform_device *pdev) > > } > > retval = clk_prepare_enable(pclk); > > if (retval) > > - return retval; > > + goto err_prepare_enable; > > > > retval = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, > > SNDRV_DEFAULT_STR1, THIS_MODULE, > > @@ -881,6 +881,7 @@ static int atmel_ac97c_probe(struct platform_device *pdev) > > snd_card_free(card); > > err_snd_card_new: > > clk_disable_unprepare(pclk); > > +err_prepare_enable: > > clk_put(pclk); > > return retval; > > } > > -- > > 2.11.0 > > > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > -- > Alexandre Belloni, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julia Lawall Date: Thu, 31 Aug 2017 08:23:19 +0000 Subject: Re: [alsa-devel] [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()' Message-Id: List-Id: References: <20170831044042.23306-1-christophe.jaillet@wanadoo.fr> <20170831081021.g4luo557ggtnyfyg@piout.net> In-Reply-To: <20170831081021.g4luo557ggtnyfyg@piout.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alexandre Belloni Cc: alsa-devel@alsa-project.org, andriy.shevchenko@linux.intel.com, linux-kernel@vger.kernel.org, garsilva@embeddedor.com, kernel-janitors@vger.kernel.org, nicolas.ferre@microchip.com, tiwai@suse.com, broonie@kernel.org, Christophe JAILLET , arvind.yadav.cs@gmail.com, bhumirks@gmail.com On Thu, 31 Aug 2017, Alexandre Belloni wrote: > On 31/08/2017 at 06:40:42 +0200, Christophe JAILLET wrote: > > If 'clk_prepare_enable()' fails, we must release some resources before > > returning. Add a new label in the existing error handling path and 'goto' > > there. > > > > Fixes: 260ea95cc027 ("ASoC: atmel: ac97c: Handle return value of clk_prepare_enable.") > > Signed-off-by: Christophe JAILLET > > And here is the fallout of the stupid, brainless "fixing" of issues > reported by static analysis tools. > > This clk_prepare_enable will never fail. If it was going to fail, the > platform would never boot to a point were it is able to execute that > code. It is really annoying to have so much churn for absolutely 0 > benefit. Would it be more productive to put the code back like it was before, ie no return value and no check, and add a comment to the definition of clk_prepare_enable indicating that there are many case where the call cannot fail? Grepping through the code suggests that it is about 50-50 on checking the return value or not doing so, which might suggest that checking the value is often not required. julia > > Anyway, > Acked-by: Alexandre Belloni > > > --- > > sound/atmel/ac97c.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c > > index 30c64ab210d9..5ffefac2fa8f 100644 > > --- a/sound/atmel/ac97c.c > > +++ b/sound/atmel/ac97c.c > > @@ -785,7 +785,7 @@ static int atmel_ac97c_probe(struct platform_device *pdev) > > } > > retval = clk_prepare_enable(pclk); > > if (retval) > > - return retval; > > + goto err_prepare_enable; > > > > retval = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, > > SNDRV_DEFAULT_STR1, THIS_MODULE, > > @@ -881,6 +881,7 @@ static int atmel_ac97c_probe(struct platform_device *pdev) > > snd_card_free(card); > > err_snd_card_new: > > clk_disable_unprepare(pclk); > > +err_prepare_enable: > > clk_put(pclk); > > return retval; > > } > > -- > > 2.11.0 > > > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > -- > Alexandre Belloni, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julia Lawall Subject: Re: [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()' Date: Thu, 31 Aug 2017 10:23:19 +0200 (CEST) Message-ID: References: <20170831044042.23306-1-christophe.jaillet@wanadoo.fr> <20170831081021.g4luo557ggtnyfyg@piout.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail2-relais-roc.national.inria.fr (mail2-relais-roc.national.inria.fr [192.134.164.83]) by alsa0.perex.cz (Postfix) with ESMTP id CA985267256 for ; Thu, 31 Aug 2017 10:23:46 +0200 (CEST) In-Reply-To: <20170831081021.g4luo557ggtnyfyg@piout.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Alexandre Belloni Cc: alsa-devel@alsa-project.org, andriy.shevchenko@linux.intel.com, linux-kernel@vger.kernel.org, garsilva@embeddedor.com, kernel-janitors@vger.kernel.org, nicolas.ferre@microchip.com, tiwai@suse.com, broonie@kernel.org, Christophe JAILLET , arvind.yadav.cs@gmail.com, bhumirks@gmail.com List-Id: alsa-devel@alsa-project.org On Thu, 31 Aug 2017, Alexandre Belloni wrote: > On 31/08/2017 at 06:40:42 +0200, Christophe JAILLET wrote: > > If 'clk_prepare_enable()' fails, we must release some resources before > > returning. Add a new label in the existing error handling path and 'goto' > > there. > > > > Fixes: 260ea95cc027 ("ASoC: atmel: ac97c: Handle return value of clk_prepare_enable.") > > Signed-off-by: Christophe JAILLET > > And here is the fallout of the stupid, brainless "fixing" of issues > reported by static analysis tools. > > This clk_prepare_enable will never fail. If it was going to fail, the > platform would never boot to a point were it is able to execute that > code. It is really annoying to have so much churn for absolutely 0 > benefit. Would it be more productive to put the code back like it was before, ie no return value and no check, and add a comment to the definition of clk_prepare_enable indicating that there are many case where the call cannot fail? Grepping through the code suggests that it is about 50-50 on checking the return value or not doing so, which might suggest that checking the value is often not required. julia > > Anyway, > Acked-by: Alexandre Belloni > > > --- > > sound/atmel/ac97c.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sound/atmel/ac97c.c b/sound/atmel/ac97c.c > > index 30c64ab210d9..5ffefac2fa8f 100644 > > --- a/sound/atmel/ac97c.c > > +++ b/sound/atmel/ac97c.c > > @@ -785,7 +785,7 @@ static int atmel_ac97c_probe(struct platform_device *pdev) > > } > > retval = clk_prepare_enable(pclk); > > if (retval) > > - return retval; > > + goto err_prepare_enable; > > > > retval = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, > > SNDRV_DEFAULT_STR1, THIS_MODULE, > > @@ -881,6 +881,7 @@ static int atmel_ac97c_probe(struct platform_device *pdev) > > snd_card_free(card); > > err_snd_card_new: > > clk_disable_unprepare(pclk); > > +err_prepare_enable: > > clk_put(pclk); > > return retval; > > } > > -- > > 2.11.0 > > > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > -- > Alexandre Belloni, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >