Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1715366 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack  callback Florian Fainelli <f.fainelli@gmail.com> - 2017-08-18 20:30 +0200
    Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback Måns Rullgård <mans@mansr.com> - 2017-08-19 18:10 +0200

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

On 08/07/2017 05:56 AM, Marc Zyngier wrote:
> On 28/07/17 15:06, Marc Gonzalez wrote:
>> On 27/07/2017 20:17, Florian Fainelli wrote:
>>
>>> On 07/26/2017 12:13 PM, Måns Rullgård wrote:
>>>
>>>> Florian Fainelli 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?
>>
>> Hello Florian,
>>
>> I am extremely grateful for you and Doug bringing the defect to
>> my attention, as it was indeed causing an issue which I had not
>> found the time to investigate.
>>
>> The reason I proposed an alternate patch is that
>> 1) Doug didn't seem to mind, 2) simpler code leads to fewer bugs
>> and less maintenance IME, and 3) I didn't see many drivers using
>> the irq_mask_ack() callback (9 out of 86) with a few misusing it,
>> by defining irq_mask = irq_mask_ack.
>>
>> As you point out, my patch might be slightly easier to backport
>> than Doug's (TBH, I hadn't considered that aspect until you
>> mentioned it).
>>
>> Has anyone ever quantified the performance improvement of
>> mask_ack over mask + ack?
> 
> Aren't you the one who is in position of measuring this effect on the
> actual HW that uses this?

What do we do with this patch series to move forward? Can we get Doug's
changes queued up for 4.14?
-- 
Florian

[toc] | [next] | [standalone]

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

Florian Fainelli <f.fainelli@gmail.com> writes:

> What do we do with this patch series to move forward? Can we get Doug's
> changes queued up for 4.14?

My opinion is that the correct combined function should be added and the
tango driver updated to use it.  Patches already exist, so what are we
waiting for?

-- 
Måns Rullgård

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web