[PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Mason]

From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"

Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/clocksource/tango_xtal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
index 2bcecafdeaea..0464c52e61f1 100644
--- a/drivers/clocksource/tango_xtal.c
+++ b/drivers/clocksource/tango_xtal.c
@@ -22,7 +22,7 @@ static u64 notrace read_sched_clock(void)
 static void __init tango_clocksource_init(struct device_node *np)
 {
 	struct clk *clk;
-	int xtal_freq, ret;
+	int xtal_freq, err;
 
 	xtal_in_cnt = of_iomap(np, 0);
 	if (xtal_in_cnt == NULL) {
@@ -40,9 +40,9 @@ static void __init tango_clocksource_init(struct device_node *np)
 	delay_timer.freq = xtal_freq;
 	delay_timer.read_current_timer = read_xtal_counter;
 
-	ret = clocksource_mmio_init(xtal_in_cnt, "tango-xtal", xtal_freq, 350,
+	err = clocksource_mmio_init(xtal_in_cnt, "tango-xtal", xtal_freq, 350,
 				    32, clocksource_mmio_readl_up);
-	if (!ret) {
+	if (err) {
 		pr_err("%s: registration failed\n", np->full_name);
 		return;
 	}
-- 
2.8.1

Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Daniel Lezcano]

On Tue, Apr 19, 2016 at 02:15:15PM +0200, Mason wrote:
> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> 
> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"
> 
> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---

Please resend the patch with the fix only, without s/ret/err/

Thanks

  -- Daniel

Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Mason]

On 19/04/2016 15:13, Daniel Lezcano wrote:

> On Tue, Apr 19, 2016 at 02:15:15PM +0200, Mason wrote:
>
>> From: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>
>> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"
>>
>> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
> 
> 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.)

How do others feel about this? Thomas?

Regards.

Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Daniel Lezcano]

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 <marc_gonzalez@sigmadesigns.com>
> >>
> >> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"
> >>
> >> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
> >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> >> ---
> > 
> > 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.

  -- Daniel

Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Mason]

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 <marc_gonzalez@sigmadesigns.com>
>>>>
>>>> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"
>>>>
>>>> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>
>>> 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

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

Do you agree that 2) would be a (minor) improvement?
If not, I will not bother with the patch.

Regards.

Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Daniel Lezcano]

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 <marc_gonzalez@sigmadesigns.com>
> >>>>
> >>>> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"
> >>>>
> >>>> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
> >>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> >>>> ---
> >>>
> >>> 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.
 
> 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 ...

Thanks.

  -- Daniel

Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Mason]

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 <marc_gonzalez@sigmadesigns.com>
>>>>>>
>>>>>> Commit 0881841f7e78 changed "if (ret != 0)" to "if (!ret)"
>>>>>>
>>>>>> Fixes: 0881841f7e78 ("Replace code by clocksource_mmio_init")
>>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>>> ---
>>>>>
>>>>> 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.

Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test

[Daniel Lezcano]

[ ... ]

> 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?

Actually, all the problem is coming from the CLOCKSOURCE_OF_DECLARE where 
the init_func is a void (*init_func)(struct device_node *) signature, thus 
not allowing to return a value.

Because of that, the init code path of the different drivers are somewhat 
fuzzy when an error occurs.

If the function could return an error code, then the drivers would be 
written in a way to catch and handle the errors gracefully. That implies the 
clocksource-probe() routine will be able to detect when the init_func() is 
failing, trace it and count if one clocksource/clockevent succeed at boot 
time.

So because the latter, it would make sense to give up registering and leave 
a clean place in the init function when something is going wrong.

  -- Daniel