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

2017-09-18 Thread Marc Gonzalez 

On 21/08/2017 15:25, Marc Gonzalez wrote:


Using separate mask and ack functions (i.e. my patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 40868 connected to 172.27.64.110 port 5201
[ ID] Interval   Transfer Bandwidth   Retr  Cwnd
[  4]   0.00-1.00   sec   106 MBytes   888 Mbits/sec   18324 KBytes
[  4]   1.00-2.00   sec   106 MBytes   885 Mbits/sec0361 KBytes
[  4]   2.00-3.00   sec   105 MBytes   883 Mbits/sec4279 KBytes
[  4]   3.00-4.00   sec   106 MBytes   890 Mbits/sec0300 KBytes
[  4]   4.00-5.00   sec   106 MBytes   887 Mbits/sec0310 KBytes
[  4]   5.00-6.00   sec   105 MBytes   883 Mbits/sec0315 KBytes
[  4]   6.00-7.00   sec   105 MBytes   885 Mbits/sec0321 KBytes
[  4]   7.00-8.00   sec   105 MBytes   880 Mbits/sec0325 KBytes
[  4]   8.00-9.00   sec   106 MBytes   888 Mbits/sec0329 KBytes
[  4]   9.00-10.00  sec   106 MBytes   886 Mbits/sec0335 KBytes
[  4]  10.00-11.00  sec   105 MBytes   885 Mbits/sec0351 KBytes
[  4]  11.00-12.00  sec   106 MBytes   887 Mbits/sec1276 KBytes
[  4]  12.00-13.00  sec   106 MBytes   885 Mbits/sec0321 KBytes
[  4]  13.00-14.00  sec   105 MBytes   883 Mbits/sec0349 KBytes
[  4]  14.00-15.00  sec   106 MBytes   890 Mbits/sec0366 KBytes
[  4]  15.00-16.00  sec   106 MBytes   888 Mbits/sec2286 KBytes
[  4]  16.00-17.00  sec   105 MBytes   884 Mbits/sec0303 KBytes
[  4]  17.00-18.00  sec   105 MBytes   883 Mbits/sec0311 KBytes
[  4]  18.00-19.00  sec   105 MBytes   880 Mbits/sec0315 KBytes
[  4]  19.00-20.00  sec   106 MBytes   890 Mbits/sec0321 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   Retr
[  4]   0.00-20.00  sec  2.06 GBytes   885 Mbits/sec   25 sender


Using combined mask and ack functions (i.e. Doug's patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 41235 connected to 172.27.64.110 port 5201
[ ID] Interval   Transfer Bandwidth   Retr  Cwnd
[  4]   0.00-1.00   sec   107 MBytes   897 Mbits/sec   60324 KBytes
[  4]   1.00-2.00   sec   107 MBytes   898 Mbits/sec0361 KBytes
[  4]   2.00-3.00   sec   107 MBytes   898 Mbits/sec   39194 KBytes
[  4]   3.00-4.00   sec   107 MBytes   895 Mbits/sec0214 KBytes
[  4]   4.00-5.00   sec   107 MBytes   901 Mbits/sec0223 KBytes
[  4]   5.00-6.00   sec   108 MBytes   902 Mbits/sec0230 KBytes
[  4]   6.00-7.00   sec   107 MBytes   895 Mbits/sec0242 KBytes
[  4]   7.00-8.00   sec   107 MBytes   901 Mbits/sec0253 KBytes
[  4]   8.00-9.00   sec   107 MBytes   899 Mbits/sec0264 KBytes
[  4]   9.00-10.00  sec   108 MBytes   903 Mbits/sec0276 KBytes
[  4]  10.00-11.00  sec   108 MBytes   902 Mbits/sec0286 KBytes
[  4]  11.00-12.00  sec   107 MBytes   899 Mbits/sec0300 KBytes
[  4]  12.00-13.00  sec   107 MBytes   898 Mbits/sec   33247 KBytes
[  4]  13.00-14.00  sec   107 MBytes   900 Mbits/sec0294 KBytes
[  4]  14.00-15.00  sec   107 MBytes   900 Mbits/sec0325 KBytes
[  4]  15.00-16.00  sec   107 MBytes   899 Mbits/sec0342 KBytes
[  4]  16.00-17.00  sec   107 MBytes   898 Mbits/sec0351 KBytes
[  4]  17.00-18.00  sec   108 MBytes   902 Mbits/sec0355 KBytes
[  4]  18.00-19.00  sec   107 MBytes   901 Mbits/sec0359 KBytes
[  4]  19.00-20.00  sec   108 MBytes   903 Mbits/sec   32255 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   Retr
[  4]   0.00-20.00  sec  2.09 GBytes   900 Mbits/sec  164 sender


Ergo, it seems that the performance improvement of the combined
implementation is approximately 1.5% for a load generating ~80k
interrupts per second.


Hello irqchip maintainers,

As mentioned upthread, there is a bug in drivers/irqchip/irq-tango.c
caused by the unexpected implementation of irq_gc_mask_disable_reg_and_ack()

That bug can be fixed either by using an appropriate irq_mask_ack callback,
or by not defining an irq_mask_ack callback at all. The first option provides
~1.5% more throughput than the second, for a typical use-case.

Whichever option you favor, can we fix this bug in current and stable branches?
(The fix was submitted two months ago.)

Regards.

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

2017-09-18 Thread Marc Gonzalez 

On 21/08/2017 15:25, Marc Gonzalez wrote:


Using separate mask and ack functions (i.e. my patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 40868 connected to 172.27.64.110 port 5201
[ ID] Interval   Transfer Bandwidth   Retr  Cwnd
[  4]   0.00-1.00   sec   106 MBytes   888 Mbits/sec   18324 KBytes
[  4]   1.00-2.00   sec   106 MBytes   885 Mbits/sec0361 KBytes
[  4]   2.00-3.00   sec   105 MBytes   883 Mbits/sec4279 KBytes
[  4]   3.00-4.00   sec   106 MBytes   890 Mbits/sec0300 KBytes
[  4]   4.00-5.00   sec   106 MBytes   887 Mbits/sec0310 KBytes
[  4]   5.00-6.00   sec   105 MBytes   883 Mbits/sec0315 KBytes
[  4]   6.00-7.00   sec   105 MBytes   885 Mbits/sec0321 KBytes
[  4]   7.00-8.00   sec   105 MBytes   880 Mbits/sec0325 KBytes
[  4]   8.00-9.00   sec   106 MBytes   888 Mbits/sec0329 KBytes
[  4]   9.00-10.00  sec   106 MBytes   886 Mbits/sec0335 KBytes
[  4]  10.00-11.00  sec   105 MBytes   885 Mbits/sec0351 KBytes
[  4]  11.00-12.00  sec   106 MBytes   887 Mbits/sec1276 KBytes
[  4]  12.00-13.00  sec   106 MBytes   885 Mbits/sec0321 KBytes
[  4]  13.00-14.00  sec   105 MBytes   883 Mbits/sec0349 KBytes
[  4]  14.00-15.00  sec   106 MBytes   890 Mbits/sec0366 KBytes
[  4]  15.00-16.00  sec   106 MBytes   888 Mbits/sec2286 KBytes
[  4]  16.00-17.00  sec   105 MBytes   884 Mbits/sec0303 KBytes
[  4]  17.00-18.00  sec   105 MBytes   883 Mbits/sec0311 KBytes
[  4]  18.00-19.00  sec   105 MBytes   880 Mbits/sec0315 KBytes
[  4]  19.00-20.00  sec   106 MBytes   890 Mbits/sec0321 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   Retr
[  4]   0.00-20.00  sec  2.06 GBytes   885 Mbits/sec   25 sender


Using combined mask and ack functions (i.e. Doug's patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 41235 connected to 172.27.64.110 port 5201
[ ID] Interval   Transfer Bandwidth   Retr  Cwnd
[  4]   0.00-1.00   sec   107 MBytes   897 Mbits/sec   60324 KBytes
[  4]   1.00-2.00   sec   107 MBytes   898 Mbits/sec0361 KBytes
[  4]   2.00-3.00   sec   107 MBytes   898 Mbits/sec   39194 KBytes
[  4]   3.00-4.00   sec   107 MBytes   895 Mbits/sec0214 KBytes
[  4]   4.00-5.00   sec   107 MBytes   901 Mbits/sec0223 KBytes
[  4]   5.00-6.00   sec   108 MBytes   902 Mbits/sec0230 KBytes
[  4]   6.00-7.00   sec   107 MBytes   895 Mbits/sec0242 KBytes
[  4]   7.00-8.00   sec   107 MBytes   901 Mbits/sec0253 KBytes
[  4]   8.00-9.00   sec   107 MBytes   899 Mbits/sec0264 KBytes
[  4]   9.00-10.00  sec   108 MBytes   903 Mbits/sec0276 KBytes
[  4]  10.00-11.00  sec   108 MBytes   902 Mbits/sec0286 KBytes
[  4]  11.00-12.00  sec   107 MBytes   899 Mbits/sec0300 KBytes
[  4]  12.00-13.00  sec   107 MBytes   898 Mbits/sec   33247 KBytes
[  4]  13.00-14.00  sec   107 MBytes   900 Mbits/sec0294 KBytes
[  4]  14.00-15.00  sec   107 MBytes   900 Mbits/sec0325 KBytes
[  4]  15.00-16.00  sec   107 MBytes   899 Mbits/sec0342 KBytes
[  4]  16.00-17.00  sec   107 MBytes   898 Mbits/sec0351 KBytes
[  4]  17.00-18.00  sec   108 MBytes   902 Mbits/sec0355 KBytes
[  4]  18.00-19.00  sec   107 MBytes   901 Mbits/sec0359 KBytes
[  4]  19.00-20.00  sec   108 MBytes   903 Mbits/sec   32255 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bandwidth   Retr
[  4]   0.00-20.00  sec  2.09 GBytes   900 Mbits/sec  164 sender


Ergo, it seems that the performance improvement of the combined
implementation is approximately 1.5% for a load generating ~80k
interrupts per second.


Hello irqchip maintainers,

As mentioned upthread, there is a bug in drivers/irqchip/irq-tango.c
caused by the unexpected implementation of irq_gc_mask_disable_reg_and_ack()

That bug can be fixed either by using an appropriate irq_mask_ack callback,
or by not defining an irq_mask_ack callback at all. The first option provides
~1.5% more throughput than the second, for a typical use-case.

Whichever option you favor, can we fix this bug in current and stable branches?
(The fix was submitted two months ago.)

Regards.

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

2017-08-21 Thread Marc Gonzalez 
On 07/08/2017 14:56, 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 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?

Using separate mask and ack functions (i.e. my patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 40868 connected to 172.27.64.110 port 5201
[ ID] Interval   Transfer Bandwidth   Retr  Cwnd
[  4]   0.00-1.00   sec   106 MBytes   888 Mbits/sec   18324 KBytes   
[  4]   1.00-2.00   sec   106 MBytes   885 Mbits/sec0361 KBytes   
[  4]   2.00-3.00   sec   105 MBytes   883 Mbits/sec4279 KBytes   
[  4]   3.00-4.00   sec   106 MBytes   890 Mbits/sec0300 KBytes   
[  4]   4.00-5.00   sec   106 MBytes   887 Mbits/sec0310 KBytes   
[  4]   5.00-6.00   sec   105 MBytes   883 Mbits/sec0315 KBytes   
[  4]   6.00-7.00   sec   105 MBytes   885 Mbits/sec0321 KBytes   
[  4]   7.00-8.00   sec   105 MBytes   880 Mbits/sec0325 KBytes   
[  4]   8.00-9.00   sec   106 MBytes   888 Mbits/sec0329 KBytes   
[  4]   9.00-10.00  sec   106 MBytes   886 Mbits/sec0335 KBytes   
[  4]  10.00-11.00  sec   105 MBytes   885 Mbits/sec0351 KBytes   
[  4]  11.00-12.00  sec   106 MBytes   887 Mbits/sec1276 KBytes   
[  4]  12.00-13.00  sec   106 MBytes   885 Mbits/sec0321 KBytes   
[  4]  13.00-14.00  sec   105 MBytes   883 Mbits/sec0349 KBytes   
[  4]  14.00-15.00  sec   106 MBytes   890 Mbits/sec0366 KBytes   
[  4]  15.00-16.00  sec   106 MBytes   888 Mbits/sec2286 KBytes   
[  4]  16.00-17.00  sec   105 MBytes   884 Mbits/sec0303 KBytes   
[  4]

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

2017-08-21 Thread Marc Gonzalez 
On 07/08/2017 14:56, 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 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?

Using separate mask and ack functions (i.e. my patch)

# iperf3 -c 172.27.64.110 -t 20
Connecting to host 172.27.64.110, port 5201
[  4] local 172.27.64.1 port 40868 connected to 172.27.64.110 port 5201
[ ID] Interval   Transfer Bandwidth   Retr  Cwnd
[  4]   0.00-1.00   sec   106 MBytes   888 Mbits/sec   18324 KBytes   
[  4]   1.00-2.00   sec   106 MBytes   885 Mbits/sec0361 KBytes   
[  4]   2.00-3.00   sec   105 MBytes   883 Mbits/sec4279 KBytes   
[  4]   3.00-4.00   sec   106 MBytes   890 Mbits/sec0300 KBytes   
[  4]   4.00-5.00   sec   106 MBytes   887 Mbits/sec0310 KBytes   
[  4]   5.00-6.00   sec   105 MBytes   883 Mbits/sec0315 KBytes   
[  4]   6.00-7.00   sec   105 MBytes   885 Mbits/sec0321 KBytes   
[  4]   7.00-8.00   sec   105 MBytes   880 Mbits/sec0325 KBytes   
[  4]   8.00-9.00   sec   106 MBytes   888 Mbits/sec0329 KBytes   
[  4]   9.00-10.00  sec   106 MBytes   886 Mbits/sec0335 KBytes   
[  4]  10.00-11.00  sec   105 MBytes   885 Mbits/sec0351 KBytes   
[  4]  11.00-12.00  sec   106 MBytes   887 Mbits/sec1276 KBytes   
[  4]  12.00-13.00  sec   106 MBytes   885 Mbits/sec0321 KBytes   
[  4]  13.00-14.00  sec   105 MBytes   883 Mbits/sec0349 KBytes   
[  4]  14.00-15.00  sec   106 MBytes   890 Mbits/sec0366 KBytes   
[  4]  15.00-16.00  sec   106 MBytes   888 Mbits/sec2286 KBytes   
[  4]  16.00-17.00  sec   105 MBytes   884 Mbits/sec0303 KBytes   
[  4]

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

2017-08-19 Thread Måns Rullgård 
Florian Fainelli  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

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

2017-08-19 Thread Måns Rullgård 
Florian Fainelli  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

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

2017-08-18 Thread Florian Fainelli 
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  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

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

2017-08-18 Thread Florian Fainelli 
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  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

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

2017-08-07 Thread Marc Zyngier 
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  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?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

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

2017-08-07 Thread Marc Zyngier 
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  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?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

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

2017-07-28 Thread Marc Gonzalez 
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  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?

Regards.

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

2017-07-28 Thread Marc Gonzalez 
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  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?

Regards.

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

2017-07-27 Thread Florian Fainelli 
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  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

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

2017-07-27 Thread Florian Fainelli 
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  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

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

2017-07-26 Thread Måns Rullgård 
Florian Fainelli  writes:

> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>> Marc Gonzalez  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

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

2017-07-26 Thread Måns Rullgård 
Florian Fainelli  writes:

> On 07/25/2017 06:29 AM, Måns Rullgård wrote:
>> Marc Gonzalez  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

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

2017-07-26 Thread Florian Fainelli 
On 07/25/2017 06:29 AM, Måns Rullgård wrote:
> Marc Gonzalez  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?
-- 
Florian

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

2017-07-26 Thread Florian Fainelli 
On 07/25/2017 06:29 AM, Måns Rullgård wrote:
> Marc Gonzalez  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?
-- 
Florian

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

2017-07-25 Thread Marc Gonzalez 
On 25/07/2017 15:08, Marc Gonzalez wrote:

> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
> 
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
> 
> Reported-by: Doug Berger 
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs 
> SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez 
> Cc: sta...@vger.kernel.org

FWIW, the lockup reported in the thread below disappears
once the patch is applied.

https://lkml.org/lkml/2016/10/21/709

Regards.

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

2017-07-25 Thread Marc Gonzalez 
On 25/07/2017 15:08, Marc Gonzalez wrote:

> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
> 
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
> 
> Reported-by: Doug Berger 
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs 
> SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez 
> Cc: sta...@vger.kernel.org

FWIW, the lockup reported in the thread below disappears
once the patch is applied.

https://lkml.org/lkml/2016/10/21/709

Regards.

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

2017-07-25 Thread Måns Rullgård 
Marc Gonzalez  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?

-- 
Måns Rullgård

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

2017-07-25 Thread Måns Rullgård 
Marc Gonzalez  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?

-- 
Måns Rullgård

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

2017-07-25 Thread Marc Gonzalez 
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.

Regards.

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

2017-07-25 Thread Marc Gonzalez 
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.

Regards.

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

2017-07-25 Thread Måns Rullgård 
Marc Gonzalez  writes:

> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
>
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
>
> Reported-by: Doug Berger 
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs 
> SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez 
> Cc: sta...@vger.kernel.org
> ---
> As discussed previously, it is acceptable for tango to rely
> on mask_ack_irq() doing the right thing(TM).
> ---
>  drivers/irqchip/irq-tango.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index bdbb5c0ff7fe..825085cdab99 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -141,7 +141,6 @@ static void __init tangox_irq_init_chip(struct 
> irq_chip_generic *gc,
>   for (i = 0; i < 2; i++) {
>   ct[i].chip.irq_ack = irq_gc_ack_set_bit;
>   ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
> - ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
>   ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
>   ct[i].chip.irq_set_type = tangox_irq_set_type;
>   ct[i].chip.name = gc->domain->name;
> -- 

What happened to the patch adding the proper combined function?

-- 
Måns Rullgård

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

2017-07-25 Thread Måns Rullgård 
Marc Gonzalez  writes:

> irq_gc_mask_disable_reg_and_ack() is not equivalent to
> irq_gc_mask_disable_reg() and irq_gc_ack_set_bit().
>
> Leave the irq_mask_ack callback undefined, and let the irqchip
> framework use irq_mask and irq_ack instead.
>
> Reported-by: Doug Berger 
> Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs 
> SMP86xx/SMP87xx interrupt controller")
> Signed-off-by: Marc Gonzalez 
> Cc: sta...@vger.kernel.org
> ---
> As discussed previously, it is acceptable for tango to rely
> on mask_ack_irq() doing the right thing(TM).
> ---
>  drivers/irqchip/irq-tango.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c
> index bdbb5c0ff7fe..825085cdab99 100644
> --- a/drivers/irqchip/irq-tango.c
> +++ b/drivers/irqchip/irq-tango.c
> @@ -141,7 +141,6 @@ static void __init tangox_irq_init_chip(struct 
> irq_chip_generic *gc,
>   for (i = 0; i < 2; i++) {
>   ct[i].chip.irq_ack = irq_gc_ack_set_bit;
>   ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
> - ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
>   ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
>   ct[i].chip.irq_set_type = tangox_irq_set_type;
>   ct[i].chip.name = gc->domain->name;
> -- 

What happened to the patch adding the proper combined function?

-- 
Måns Rullgård