Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754233AbcDSSXX (ORCPT ); Tue, 19 Apr 2016 14:23:23 -0400 Received: from smtp4-g21.free.fr ([212.27.42.4]:59559 "EHLO smtp4-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752834AbcDSSXW (ORCPT ); Tue, 19 Apr 2016 14:23:22 -0400 Subject: Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test To: Daniel Lezcano Cc: Thomas Gleixner , Sebastian Frias , Mans Rullgard , LKML References: <57162153.4070707@free.fr> <20160419131330.GA419@linaro.org> <57163B1F.7050402@free.fr> <20160419145946.GB419@linaro.org> <57166910.2080703@free.fr> <20160419173104.GA5862@linaro.org> From: Mason Message-ID: <57167780.5040506@free.fr> Date: Tue, 19 Apr 2016 20:22:56 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 SeaMonkey/2.39 MIME-Version: 1.0 In-Reply-To: <20160419173104.GA5862@linaro.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2737 Lines: 85 On 19/04/2016 19:31, Daniel Lezcano wrote: > On Tue, Apr 19, 2016 at 07:21:20PM +0200, Mason wrote: >> On 19/04/2016 16:59, Daniel Lezcano wrote: >>> On Tue, Apr 19, 2016 at 04:05:19PM +0200, Mason wrote: >>>> On 19/04/2016 15:13, Daniel Lezcano wrote: >>>> >>>>> On Tue, Apr 19, 2016 at 02:15:15PM +0200, Mason wrote: >>>>> >>>>>> From: Marc Gonzalez >>>>>> >>>>>> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)" >>>>>> >>>>>> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init") >>>>>> Signed-off-by: Marc Gonzalez >>>>>> --- >>>>> >>>>> Please resend the patch with the fix only, without s/ret/err/ >>>> >>>> As I wrote on IRC, I think it is misguided to consider variable >>>> renaming as not part of the fix. A properly named variable helps >>>> reviewers by communicating intent. >>>> >>>> Had I named the variable 'err' in the first place, would you have >>>> introduced the bug by writing >>>> >>>> if (!err) { >>>> pr_err("registration failed"); >>>> } >>>> >>>> or would if (!err) have jumped out for an error path? >>>> (Not a rhetorical question; if you say it would not have helped, >>>> then I guess my mental workflow is different.) >>> >>> Ok I won't argue for a stupid variable name. >>> >>> The point is we are at v4.6-rc4 and even if the change is obvious, it is a >>> good practice to do a simple change: >>> >>> - if (!ret) { >>> + if (ret) { >>> >>> Why ? Because maintainers have a lot of code to review, and removing the >>> noise as much as possible helps them to make their life easier especially >>> when they have to pay double attention for fixes at RC. >>> >>> If the 'ret' name is a problem for you, just send another patch for v4.7 to >>> change the name. >> >> I want to be sure I understand, please correct me if I'm wrong. >> >> 1) you have already committed the minimal fix above (changing only >> the test, and keeping the original variable name) and this will be >> pushed to linux-next for the upcoming v4.6-rc > > yes. Thanks for the quick turn around. >> 2) if I want to change the variable name, I can send another patch, >> to be pushed in the next merge window, for v4.7 > > yes. > >> Do you agree that 2) would be a (minor) improvement? >> If not, I will not bother with the patch. > > Usually people are using 'ret' > > grep -r "ret =" drivers/ | wc -l > 76754 > > or are using 'err' > > grep -r "err =" drivers/ | wc -l > 27940 > > Up to you ... About the error handling... you advised against panic() because there might be other clock sources. Does it makes sense to give up registering sched_clock and delay_timer when the clocksource registration fails? Regards.