All of lore.kernel.org
 help / color / mirror / Atom feed
From: mans@mansr.com (Måns Rullgård)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Date: Wed, 26 Jul 2017 20:13:47 +0100	[thread overview]
Message-ID: <yw1x4ltzm2ok.fsf@mansr.com> (raw)
In-Reply-To: <3b858e14-0da1-d4aa-eb84-f136ece8c2a6@gmail.com> (Florian Fainelli's message of "Wed, 26 Jul 2017 11:20:05 -0700")

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.

-- 
M?ns Rullg?rd

WARNING: multiple messages have this Message-ID (diff)
From: "Måns Rullgård" <mans@mansr.com>
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>,
	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>,
	Mason <slash.tmp@free.fr>
Subject: Re: [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback
Date: Wed, 26 Jul 2017 20:13:47 +0100	[thread overview]
Message-ID: <yw1x4ltzm2ok.fsf@mansr.com> (raw)
In-Reply-To: <3b858e14-0da1-d4aa-eb84-f136ece8c2a6@gmail.com> (Florian Fainelli's message of "Wed, 26 Jul 2017 11:20:05 -0700")

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.

-- 
Måns Rullgård

  reply	other threads:[~2017-07-26 19:13 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-19 19:07 [PATCH v2 0/6] Add support for BCM7271 style interrupt controller Doug Berger
2017-07-19 19:07 ` Doug Berger
2017-07-19 19:07 ` Doug Berger
2017-07-19 19:07 ` [PATCH v2 1/6] genirq: generic chip: add irq_gc_mask_disable_and_ack_set() Doug Berger
2017-07-19 19:07   ` Doug Berger
2017-07-19 19:07 ` [PATCH v2 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set Doug Berger
2017-07-19 19:07   ` Doug Berger
2017-07-19 19:07   ` Doug Berger
2017-07-24 16:40   ` Marc Gonzalez
2017-07-24 16:40     ` Marc Gonzalez
2017-07-24 17:54     ` Doug Berger
2017-07-24 17:54       ` Doug Berger
2017-07-25 13:08       ` [PATCH v3] irqchip/tango: Don't use incorrect irq_mask_ack callback Marc Gonzalez
2017-07-25 13:08         ` Marc Gonzalez
2017-07-25 13:16         ` Måns Rullgård
2017-07-25 13:16           ` Måns Rullgård
2017-07-25 13:26           ` Marc Gonzalez
2017-07-25 13:26             ` Marc Gonzalez
2017-07-25 13:29             ` Måns Rullgård
2017-07-25 13:29               ` Måns Rullgård
2017-07-26 18:20               ` Florian Fainelli
2017-07-26 18:20                 ` Florian Fainelli
2017-07-26 19:13                 ` Måns Rullgård [this message]
2017-07-26 19:13                   ` Måns Rullgård
2017-07-27 18:17                   ` Florian Fainelli
2017-07-27 18:17                     ` Florian Fainelli
2017-07-28 14:06                     ` Marc Gonzalez
2017-07-28 14:06                       ` Marc Gonzalez
2017-08-07 12:56                       ` Marc Zyngier
2017-08-07 12:56                         ` Marc Zyngier
2017-08-18 18:24                         ` Florian Fainelli
2017-08-18 18:24                           ` Florian Fainelli
2017-08-19 16:05                           ` Måns Rullgård
2017-08-19 16:05                             ` Måns Rullgård
2017-08-21 13:25                         ` Marc Gonzalez
2017-08-21 13:25                           ` Marc Gonzalez
2017-09-18  8:49                           ` Marc Gonzalez
2017-09-18  8:49                             ` Marc Gonzalez
2017-07-25 14:15         ` Marc Gonzalez
2017-07-25 14:15           ` Marc Gonzalez
2017-07-19 19:07 ` [PATCH v2 3/6] genirq: generic chip: remove irq_gc_mask_disable_reg_and_ack() Doug Berger
2017-07-19 19:07   ` Doug Berger
2017-07-19 19:07 ` [PATCH v2 4/6] irqchip: brcmstb-l2: Remove some processing from the handler Doug Berger
2017-07-19 19:07   ` Doug Berger
2017-07-19 19:07   ` Doug Berger
2017-07-19 19:07 ` [PATCH v2 5/6] irqchip: brcmstb-l2: Abstract register accesses Doug Berger
2017-07-19 19:07   ` Doug Berger
2017-07-19 19:07 ` [PATCH v2 6/6] irqchip: brcmstb-l2: Add support for the BCM7271 L2 controller Doug Berger
2017-07-19 19:07   ` Doug Berger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=yw1x4ltzm2ok.fsf@mansr.com \
    --to=mans@mansr.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.