From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()' Date: Thu, 31 Aug 2017 12:04:03 +0300 Message-ID: <1504170243.25945.170.camel@linux.intel.com> References: <20170831044042.23306-1-christophe.jaillet@wanadoo.fr> <20170831081021.g4luo557ggtnyfyg@piout.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by alsa0.perex.cz (Postfix) with ESMTP id E3031267256 for ; Thu, 31 Aug 2017 11:04:11 +0200 (CEST) In-Reply-To: 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: Julia Lawall , Alexandre Belloni Cc: alsa-devel@alsa-project.org, 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 T24gVGh1LCAyMDE3LTA4LTMxIGF0IDEwOjIzICswMjAwLCBKdWxpYSBMYXdhbGwgd3JvdGU6Cj4g Cj4gT24gVGh1LCAzMSBBdWcgMjAxNywgQWxleGFuZHJlIEJlbGxvbmkgd3JvdGU6Cj4gCj4gPiBP biAzMS8wOC8yMDE3IGF0IDA2OjQwOjQyICswMjAwLCBDaHJpc3RvcGhlIEpBSUxMRVQgd3JvdGU6 Cj4gPiA+IElmICdjbGtfcHJlcGFyZV9lbmFibGUoKScgZmFpbHMsIHdlIG11c3QgcmVsZWFzZSBz b21lIHJlc291cmNlcwo+ID4gPiBiZWZvcmUKPiA+ID4gcmV0dXJuaW5nLiBBZGQgYSBuZXcgbGFi ZWwgaW4gdGhlIGV4aXN0aW5nIGVycm9yIGhhbmRsaW5nIHBhdGggYW5kCj4gPiA+ICdnb3RvJwo+ ID4gPiB0aGVyZS4KPiA+ID4gCj4gPiA+IEZpeGVzOiAyNjBlYTk1Y2MwMjcgKCJBU29DOiBhdG1l bDogYWM5N2M6IEhhbmRsZSByZXR1cm4gdmFsdWUgb2YKPiA+ID4gY2xrX3ByZXBhcmVfZW5hYmxl LiIpCj4gPiA+IFNpZ25lZC1vZmYtYnk6IENocmlzdG9waGUgSkFJTExFVCA8Y2hyaXN0b3BoZS5q YWlsbGV0QHdhbmFkb28uZnI+Cj4gPiAKPiA+IEFuZCBoZXJlIGlzIHRoZSBmYWxsb3V0IG9mIHRo ZSBzdHVwaWQsIGJyYWlubGVzcyAiZml4aW5nIiBvZiBpc3N1ZXMKPiA+IHJlcG9ydGVkIGJ5IHN0 YXRpYyBhbmFseXNpcyB0b29scy4KPiA+IAo+ID4gVGhpcyBjbGtfcHJlcGFyZV9lbmFibGUgd2ls bCBuZXZlciBmYWlsLiBJZiBpdCB3YXMgZ29pbmcgdG8gZmFpbCwKPiA+IHRoZQo+ID4gcGxhdGZv cm0gd291bGQgbmV2ZXIgYm9vdCB0byBhIHBvaW50IHdlcmUgaXQgaXMgYWJsZSB0byBleGVjdXRl IHRoYXQKPiA+IGNvZGUuIEl0IGlzIHJlYWxseSBhbm5veWluZyB0byBoYXZlIHNvIG11Y2ggY2h1 cm4gZm9yIGFic29sdXRlbHkgMAo+ID4gYmVuZWZpdC4KPiAKPiBXb3VsZCBpdCBiZSBtb3JlIHBy b2R1Y3RpdmUgdG8gcHV0IHRoZSBjb2RlIGJhY2sgbGlrZSBpdCB3YXMgYmVmb3JlLAo+IGllIG5v Cj4gcmV0dXJuIHZhbHVlIGFuZCBubyBjaGVjaywgYW5kIGFkZCBhIGNvbW1lbnQgdG8gdGhlIGRl ZmluaXRpb24gb2YKPiBjbGtfcHJlcGFyZV9lbmFibGUgaW5kaWNhdGluZyB0aGF0IHRoZXJlIGFy ZSBtYW55IGNhc2Ugd2hlcmUgdGhlIGNhbGwKPiBjYW5ub3QgZmFpbD/CoMKgR3JlcHBpbmcgdGhy b3VnaCB0aGUgY29kZSBzdWdnZXN0cyB0aGF0IGl0IGlzIGFib3V0IDUwLQo+IDUwIG9uCj4gY2hl Y2tpbmcgdGhlIHJldHVybiB2YWx1ZSBvciBub3QgZG9pbmcgc28sIHdoaWNoIG1pZ2h0IHN1Z2dl c3QgdGhhdAo+IGNoZWNraW5nIHRoZSB2YWx1ZSBpcyBvZnRlbiBub3QgcmVxdWlyZWQuCgpJIGRp ZG4ndCBsb29rIGludG8gdGhlIGNvZGUsIHRob3VnaCBzcGVjdWxhdGluZyBpdCBtaWdodCBiZSB0 aGUgY2FzZQp3aGVuIENMSyBmcmFtZXdvcmsgaXMgbm90IGVuYWJsZWQsIHRob3VnaCBtYW55IGRy aXZlcnMgYXJlIGRlcGVuZGVudCB0bwppdCwgc28sIGl0IHdvdWxkIG5ldmVyIGZhaWwgaW4gc3Vj aCBjYXNlcy4gTmV2ZXJ0aGVsZXNzIHRoZXJlIG1pZ2h0IGJlCm90aGVyIGNhc2VzIGZvciBDTEsg QVBJIHRvIGZhaWwuCgotLSAKQW5keSBTaGV2Y2hlbmtvIDxhbmRyaXkuc2hldmNoZW5rb0BsaW51 eC5pbnRlbC5jb20+CkludGVsIEZpbmxhbmQgT3kKX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX18KQWxzYS1kZXZlbCBtYWlsaW5nIGxpc3QKQWxzYS1kZXZlbEBh bHNhLXByb2plY3Qub3JnCmh0dHA6Ly9tYWlsbWFuLmFsc2EtcHJvamVjdC5vcmcvbWFpbG1hbi9s aXN0aW5mby9hbHNhLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Date: Thu, 31 Aug 2017 09:04:03 +0000 Subject: Re: [alsa-devel] [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()' Message-Id: <1504170243.25945.170.camel@linux.intel.com> List-Id: References: <20170831044042.23306-1-christophe.jaillet@wanadoo.fr> <20170831081021.g4luo557ggtnyfyg@piout.net> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Julia Lawall , Alexandre Belloni Cc: alsa-devel@alsa-project.org, 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, 2017-08-31 at 10:23 +0200, Julia Lawall wrote: > > 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. I didn't look into the code, though speculating it might be the case when CLK framework is not enabled, though many drivers are dependent to it, so, it would never fail in such cases. Nevertheless there might be other cases for CLK API to fail. -- Andy Shevchenko Intel Finland Oy From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750922AbdHaJEM (ORCPT ); Thu, 31 Aug 2017 05:04:12 -0400 Received: from mga11.intel.com ([192.55.52.93]:55767 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750772AbdHaJEK (ORCPT ); Thu, 31 Aug 2017 05:04:10 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,451,1498546800"; d="scan'208";a="130252903" Message-ID: <1504170243.25945.170.camel@linux.intel.com> Subject: Re: [alsa-devel] [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()' From: Andy Shevchenko To: Julia Lawall , 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, bhumirks@gmail.com, alsa-devel@alsa-project.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org Date: Thu, 31 Aug 2017 12:04:03 +0300 In-Reply-To: References: <20170831044042.23306-1-christophe.jaillet@wanadoo.fr> <20170831081021.g4luo557ggtnyfyg@piout.net> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-08-31 at 10:23 +0200, Julia Lawall wrote: > > 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. I didn't look into the code, though speculating it might be the case when CLK framework is not enabled, though many drivers are dependent to it, so, it would never fail in such cases. Nevertheless there might be other cases for CLK API to fail. -- Andy Shevchenko Intel Finland Oy