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

[Thomas Gleixner]

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@xxxxxxxxxxxxx>
> Signed-off-by: David Kozub <zub@xxxxxxxxxxxxxxxxxx>
> ---
>  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