Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious interrupts

[David Kozub <zub@linux.fjfi.cvut.cz>]

On Fri, 20 Oct 2017, Thomas Gleixner wrote:

> On Thu, 19 Oct 2017, David Kozub wrote:
>
>> This solves a BUG on ALIX 2c3 where mfgpt_tick is called before
>> clockevents_config_and_register returns. This caused mfgpt_tick to call a
>> null function pointer.
>>
>> Thanks to Daniel Lezcano and Thomas Gleixner for helping me analyze this
>> and suggesting a solution.
>
> Thanks for sending this! Though I have two minor issues with this:
>
> 1) The changelog.
>
> We try to structure changelogs by explaining the context, the problem and
> its consequences and the fix. If the fix is non obvious it wants an
> elaborate explanation. Let me give you an example:
>
>   The interrupt handler mfgpt_tick() is not robust versus spurious
>   interrupts which happen before the clock event device is registered and
>   fully initialized.
>
>   The reason is that the safe guard against spurious interrupts solely
>   checks for the clockevents shutdown state, but lacks a check for
>   detached state. If the interrupt hits while the device is in detached
>   state it passes the safe guard and dereferences the event handler call
>   back which is NULL.
>
>   Add the missing state check.
>
> See?

Thanks for the feedback. Yes, that is better.

Now that I'm neither the author of the code nor the author of the commit 
message it doesn't seem right that I should be submitting this.

>> -	if (clockevent_state_shutdown(&cs5535_clockevent))
>> +	if (clockevent_state_detached(&cs5535_clockevent) ||
>> +			clockevent_state_shutdown(&cs5535_clockevent))
>
> Please do not use random indentation for multiline conditionals
>
> 	if (clockevent_state_detached(&cs5535_clockevent) ||
> 	    clockevent_state_shutdown(&cs5535_clockevent))
>
> is the style we use through out the code.

OK. I noticed the four spaces. I was swayed by process/coding-style.rst:

Outside of comments, documentation and except in Kconfig, spaces are never 
used for indentation,

But now I think I just misunderstood that. The spaces on the second line 
of the "if" are not regarded as ident.


Best regards,
David