Messages in this thread | | | Date | Fri, 20 Oct 2017 00:21:37 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious interrupts |
| |
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?
> Suggested-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz> > --- > drivers/clocksource/cs5535-clockevt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/clocksource/cs5535-clockevt.c b/drivers/clocksource/cs5535-clockevt.c > index a1df588343f2..56100506b933 100644 > --- a/drivers/clocksource/cs5535-clockevt.c > +++ b/drivers/clocksource/cs5535-clockevt.c > @@ -117,7 +117,8 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id) > /* Turn off the clock (and clear the event) */ > disable_timer(cs5535_event_clock); > > - 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.
Thanks,
tglx
| |