Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback

From: Florian Fainelli <f.fainelli@gmail.com>
To: "Måns Rullgård" <mans@mansr.com>,
	"Marc Gonzalez" <marc_gonzalez@sigmadesigns.com>,
	Mason <slash.tmp@free.fr>
Cc: Doug Berger <opendmb@gmail.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Jason Cooper <jason@lakedaemon.net>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Date: Thu, 27 Jul 2017 11:17:18 -0700	[thread overview]
Message-ID: <48734beb-0e6b-3a8f-ebf4-b1cec63322e5@gmail.com> (raw)
In-Reply-To: <yw1x4ltzm2ok.fsf@mansr.com>

On 07/26/2017 12:13 PM, Måns Rullgård wrote:
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>
>>>> On 25/07/2017 15:16, Måns Rullgård wrote:
>>>>
>>>>> What happened to the patch adding the proper combined function?
>>>>
>>>> It appears you're not CCed on v2.
>>>>
>>>> https://patchwork.kernel.org/patch/9859799/
>>>>
>>>> Doug wrote:
>>>>> Yes, you understand correctly.  The irq_mask_ack method is entirely
>>>>> optional and I assume that is why this issue went undetected for so
>>>>> long; however, it is slightly more efficient to combine the functions
>>>>> (even if the ack is unnecessary) which is why I chose to do so for my
>>>>> changes to the irqchip-brcmstb-l2 driver where I first discovered this
>>>>> issue.  How much value the improved efficiency has is certainly
>>>>> debatable, but interrupt handling is one area where people might care
>>>>> about such a small difference.  As the irqchip-tango driver maintainer
>>>>> you are welcome to decide whether or not the irq_mask_ack method makes
>>>>> sense to you.
>>>>
>>>> My preference goes to leaving the irq_mask_ack callback undefined,
>>>> and let the irqchip framework use irq_mask and irq_ack instead.
>>>
>>> Why would you prefer the less efficient way?
>>>
>>
>> Same question here, that does not really make sense to me.
>>
>> The whole point of this patch series is to have a set of efficient and
>> bugfree (or nearly) helper functions that drivers can rely on, are you
>> saying that somehow using irq_mask_and_ack is exposing a bug in the
>> tango irqchip driver and using the separate functions does not expose
>> this bug?
> 
> There is currently a bug in that the function used doesn't do what its
> name implies which can't be good.  Using the separate mask and ack
> functions obviously works, but combining them saves a lock/unlock
> sequence.  The correct combined function has already been written, so I
> see no reason not to use it.

Marc/Mason, are you intending to get this patch accepted in order to
provide a quick bugfix targeting earlier kernels with the tango irqchip
driver or is this how you think the correct fix for the tango irqchip
driver is as opposed to using Doug's fix?
-- 
Florian