From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752488AbdJTHjF (ORCPT <rfc822;w@1wt.eu>);
        Fri, 20 Oct 2017 03:39:05 -0400
Received: from mailgw1.fjfi.cvut.cz ([147.32.9.3]:55212 "EHLO
        mailgw1.fjfi.cvut.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752170AbdJTHjC (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 20 Oct 2017 03:39:02 -0400
X-CTU-FNSPE-Virus-Scanned: amavisd-new at fjfi.cvut.cz
DKIM-Filter: OpenDKIM Filter v2.11.0 mailgw1.fjfi.cvut.cz 882C2A04D8
Date: Fri, 20 Oct 2017 09:38:57 +0200 (CEST)
From: David Kozub <zub@linux.fjfi.cvut.cz>
To: Thomas Gleixner <tglx@linutronix.de>
cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious
 interrupts
In-Reply-To: <alpine.DEB.2.20.1710200011111.2054@nanos>
Message-ID: <alpine.LRH.2.21.1710200930370.3927@linux.fjfi.cvut.cz>
References: <20171019211651.039346004D@linux.fjfi.cvut.cz> <alpine.DEB.2.20.1710200011111.2054@nanos>
User-Agent: Alpine 2.21 (LRH 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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