Messages in this thread | | | Date | Fri, 20 Oct 2017 09:38:57 +0200 (CEST) | From | David Kozub <> | Subject | Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious interrupts |
| |
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
| |