linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* Possible race while masking IRQ on Allwinner A20
@ 2020-05-19  8:59 Jérôme Pouiller
  2020-05-21  7:26 ` Maxime Ripard
  0 siblings, 1 reply; 7+ messages in thread
From: Jérôme Pouiller @ 2020-05-19  8:59 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: Marc Dorval, Chen-Yu Tsai, Maxime Ripard

Hello arm developers,

I have some trouble with integration of the wfx driver[1] on Allwinner
A20 platform.

The chip WF200 is connected to the SDIO bus. At the beginning, I tried 
to use the IRQ provided by the SDIO bus. However, I have noticed I 
received some IRQs twice. Since the IRQ line is multiplexed with the 
data line, it is not very clear if it is a bug, or if the SDIO device 
has to support that.

The chip WF200 allows to use a dedicated line for the IRQ (aka
"Out-Of-Band" IRQ). So I have enabled this feature with a edge triggered 
IRQ. However, I missed some IRQs. Indeed, it seems that Allwinner use a 
32KHz clock to sample the IRQs. It is not fast enough for us. I think it 
explains why we miss some IRQs (using the attribute "input-debounce"[2], 
I tried to enable the 24Mhz clock, but without success).

Nevermind, I tried to use a level triggered IRQ (and my request is on 
this part). As you can see in the wfx driver (in  bus_sdio.c and bh.c), 
I use a threaded IRQ for that. Unfortunately, I receive some IRQs twice. 
I traced the problem, I get:

 QSGRenderThread-981   [000] d.h.   247.485524: irq_handler_entry: irq=80 name=wfx
 QSGRenderThread-981   [000] d.h.   247.485547: irq_handler_exit: irq=80 ret=handled
 QSGRenderThread-981   [000] d.h.   247.485600: irq_handler_entry: irq=80 name=wfx
 QSGRenderThread-981   [000] d.h.   247.485606: irq_handler_exit: irq=80 ret=handled
      irq/80-wfx-260   [001] ....   247.485828: io_read32: CONTROL: 0000f046
      irq/80-wfx-260   [001] ....   247.486072: io_read32: CONTROL: 0000f046
    kworker/1:1H-116   [001] ....   247.486214: io_read: QUEUE: 8b 00 84 18 00 00 00 00 01 00 15 82 2b 48 01 1e 88 42 30 00 08 6b d7 c3 53 e0 28 80 88 67 32 af ... (192 bytes)
    kworker/1:1H-116   [001] ....   247.493097: io_read: QUEUE: 00 00 00 00 00 00 00 00 06 06 00 6a 3f 95 00 60 00 00 00 00 08 62 00 00 01 00 5e 00 00 07 28 80 ... (192 bytes)
    [...]

On this trace, we can see:
  - the hard IRQ handler
  - the IRQ acknowledge from the thread irq/80-wfx-260
  - the access to the data from kworker/1:1H-116

As far as I understand, the first call to the IRQ handler (at 
247.485524) should mask the IRQ 80. So, the second IRQ (at 247.485600) 
should not happen and the thread irq/80 should be triggered only once.

Do you have any idea of what is going wrong with this IRQ?

Thank you,

[1] available in drivers/staging/wfx of the staging-next branch of the
Greg KH's repository. Be sure you get the up-to-date version.

[2] see commit 7c926492d38a3feef4b4b29c91b7c03eb1b8b546 for detail about 
the interactions between clock and debouncing.

-- 
Jérôme Pouiller



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Possible race while masking IRQ on Allwinner A20
  2020-05-19  8:59 Possible race while masking IRQ on Allwinner A20 Jérôme Pouiller
@ 2020-05-21  7:26 ` Maxime Ripard
  2020-05-21  8:02   ` Marc Zyngier
  2020-05-21 13:12   ` Jérôme Pouiller
  0 siblings, 2 replies; 7+ messages in thread
From: Maxime Ripard @ 2020-05-21  7:26 UTC (permalink / raw)
  To: Jérôme Pouiller
  Cc: Marc Zyngier, Marc Dorval, Thomas Gleixner, Chen-Yu Tsai,
	linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 3258 bytes --]

Hi!

Adding Thomas and Marc in Cc.

On Tue, May 19, 2020 at 10:59:26AM +0200, Jérôme Pouiller wrote:
> I have some trouble with integration of the wfx driver[1] on Allwinner
> A20 platform.
> 
> The chip WF200 is connected to the SDIO bus. At the beginning, I tried 
> to use the IRQ provided by the SDIO bus. However, I have noticed I 
> received some IRQs twice. Since the IRQ line is multiplexed with the 
> data line, it is not very clear if it is a bug, or if the SDIO device 
> has to support that.
> 
> The chip WF200 allows to use a dedicated line for the IRQ (aka
> "Out-Of-Band" IRQ). So I have enabled this feature with a edge triggered 
> IRQ. However, I missed some IRQs. Indeed, it seems that Allwinner use a 
> 32KHz clock to sample the IRQs. It is not fast enough for us. I think it 
> explains why we miss some IRQs (using the attribute "input-debounce"[2], 
> I tried to enable the 24Mhz clock, but without success).

Without success as in you couldn't make it use the 24MHz clock, or using it
didn't change anything?

But yeah, missing an edge interrupt is bound to happen at some point, and a
level interrupt is going to be more reliable (especially if you can't recover
from a missed interrupt).

> Nevermind, I tried to use a level triggered IRQ (and my request is on 
> this part). As you can see in the wfx driver (in  bus_sdio.c and bh.c), 
> I use a threaded IRQ for that. Unfortunately, I receive some IRQs twice. 
> I traced the problem, I get:
> 
>  QSGRenderThread-981   [000] d.h.   247.485524: irq_handler_entry: irq=80 name=wfx
>  QSGRenderThread-981   [000] d.h.   247.485547: irq_handler_exit: irq=80 ret=handled
>  QSGRenderThread-981   [000] d.h.   247.485600: irq_handler_entry: irq=80 name=wfx
>  QSGRenderThread-981   [000] d.h.   247.485606: irq_handler_exit: irq=80 ret=handled
>       irq/80-wfx-260   [001] ....   247.485828: io_read32: CONTROL: 0000f046
>       irq/80-wfx-260   [001] ....   247.486072: io_read32: CONTROL: 0000f046
>     kworker/1:1H-116   [001] ....   247.486214: io_read: QUEUE: 8b 00 84 18 00 00 00 00 01 00 15 82 2b 48 01 1e 88 42 30 00 08 6b d7 c3 53 e0 28 80 88 67 32 af ... (192 bytes)
>     kworker/1:1H-116   [001] ....   247.493097: io_read: QUEUE: 00 00 00 00 00 00 00 00 06 06 00 6a 3f 95 00 60 00 00 00 00 08 62 00 00 01 00 5e 00 00 07 28 80 ... (192 bytes)
>     [...]
> 
> On this trace, we can see:
>   - the hard IRQ handler
>   - the IRQ acknowledge from the thread irq/80-wfx-260
>   - the access to the data from kworker/1:1H-116
> 
> As far as I understand, the first call to the IRQ handler (at 
> 247.485524) should mask the IRQ 80. So, the second IRQ (at 247.485600) 
> should not happen and the thread irq/80 should be triggered only once.
> 
> Do you have any idea of what is going wrong with this IRQ?

That's pretty weird indeed. My first guess was that you weren't using
IRQF_ONESHOT, but it looks like you are. My next lead would be to see if the
mask / unmask hooks in the pinctrl driver are properly called (and actually do
what they are supposed to do). I'm not sure we have any in-tree user of a
threaded IRQ attached to the pinctrl driver, so it might have been broken for
quite some time.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Possible race while masking IRQ on Allwinner A20
  2020-05-21  7:26 ` Maxime Ripard
@ 2020-05-21  8:02   ` Marc Zyngier
  2020-05-21 13:28     ` Jérôme Pouiller
  2020-05-21 13:12   ` Jérôme Pouiller
  1 sibling, 1 reply; 7+ messages in thread
From: Marc Zyngier @ 2020-05-21  8:02 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Marc Dorval, Chen-Yu Tsai, Thomas Gleixner,
	Jérôme Pouiller, linux-arm-kernel

Hi Maxime, Jérôme,

On 2020-05-21 08:26, Maxime Ripard wrote:
> Hi!
> 
> Adding Thomas and Marc in Cc.
> 
> On Tue, May 19, 2020 at 10:59:26AM +0200, Jérôme Pouiller wrote:
>> I have some trouble with integration of the wfx driver[1] on Allwinner
>> A20 platform.
>> 
>> The chip WF200 is connected to the SDIO bus. At the beginning, I tried
>> to use the IRQ provided by the SDIO bus. However, I have noticed I
>> received some IRQs twice. Since the IRQ line is multiplexed with the
>> data line, it is not very clear if it is a bug, or if the SDIO device
>> has to support that.
>> 
>> The chip WF200 allows to use a dedicated line for the IRQ (aka
>> "Out-Of-Band" IRQ). So I have enabled this feature with a edge 
>> triggered
>> IRQ. However, I missed some IRQs. Indeed, it seems that Allwinner use 
>> a
>> 32KHz clock to sample the IRQs. It is not fast enough for us. I think 
>> it
>> explains why we miss some IRQs (using the attribute 
>> "input-debounce"[2],
>> I tried to enable the 24Mhz clock, but without success).
> 
> Without success as in you couldn't make it use the 24MHz clock, or 
> using it
> didn't change anything?
> 
> But yeah, missing an edge interrupt is bound to happen at some point, 
> and a
> level interrupt is going to be more reliable (especially if you can't 
> recover
> from a missed interrupt).
> 
>> Nevermind, I tried to use a level triggered IRQ (and my request is on
>> this part). As you can see in the wfx driver (in  bus_sdio.c and 
>> bh.c),
>> I use a threaded IRQ for that. Unfortunately, I receive some IRQs 
>> twice.
>> I traced the problem, I get:
>> 
>>  QSGRenderThread-981   [000] d.h.   247.485524: irq_handler_entry: 
>> irq=80 name=wfx
>>  QSGRenderThread-981   [000] d.h.   247.485547: irq_handler_exit: 
>> irq=80 ret=handled
>>  QSGRenderThread-981   [000] d.h.   247.485600: irq_handler_entry: 
>> irq=80 name=wfx
>>  QSGRenderThread-981   [000] d.h.   247.485606: irq_handler_exit: 
>> irq=80 ret=handled
>>       irq/80-wfx-260   [001] ....   247.485828: io_read32: CONTROL: 
>> 0000f046
>>       irq/80-wfx-260   [001] ....   247.486072: io_read32: CONTROL: 
>> 0000f046
>>     kworker/1:1H-116   [001] ....   247.486214: io_read: QUEUE: 8b 00 
>> 84 18 00 00 00 00 01 00 15 82 2b 48 01 1e 88 42 30 00 08 6b d7 c3 53 
>> e0 28 80 88 67 32 af ... (192 bytes)
>>     kworker/1:1H-116   [001] ....   247.493097: io_read: QUEUE: 00 00 
>> 00 00 00 00 00 00 06 06 00 6a 3f 95 00 60 00 00 00 00 08 62 00 00 01 
>> 00 5e 00 00 07 28 80 ... (192 bytes)
>>     [...]
>> 
>> On this trace, we can see:
>>   - the hard IRQ handler
>>   - the IRQ acknowledge from the thread irq/80-wfx-260
>>   - the access to the data from kworker/1:1H-116
>> 
>> As far as I understand, the first call to the IRQ handler (at
>> 247.485524) should mask the IRQ 80. So, the second IRQ (at 247.485600)
>> should not happen and the thread irq/80 should be triggered only once.
>> 
>> Do you have any idea of what is going wrong with this IRQ?
> 
> That's pretty weird indeed. My first guess was that you weren't using
> IRQF_ONESHOT, but it looks like you are. My next lead would be to see 
> if the
> mask / unmask hooks in the pinctrl driver are properly called (and 
> actually do
> what they are supposed to do). I'm not sure we have any in-tree user of 
> a
> threaded IRQ attached to the pinctrl driver, so it might have been 
> broken for
> quite some time.

What is certainly puzzling is that this driver doesn't seem to use
threaded IRQs at all. Instead, it uses its own workqueue that seems
to bypass the core IRQ subsystem altogether. So any guarantee we'd
expect goes at of the window.

It is also pretty unclear to me how whether the HW supports switch
from edge to level signalling. The request_irq() call definitely asks
for edge, and I don't know how you'd instruct the HW to change its
signalling method (in general, it isn't possible).

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Possible race while masking IRQ on Allwinner A20
  2020-05-21  7:26 ` Maxime Ripard
  2020-05-21  8:02   ` Marc Zyngier
@ 2020-05-21 13:12   ` Jérôme Pouiller
  1 sibling, 0 replies; 7+ messages in thread
From: Jérôme Pouiller @ 2020-05-21 13:12 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Marc Zyngier, Marc Dorval, Thomas Gleixner, Chen-Yu Tsai,
	linux-arm-kernel

On Thursday 21 May 2020 09:26:34 CEST Maxime Ripard wrote:
[...]
> On Tue, May 19, 2020 at 10:59:26AM +0200, Jérôme Pouiller wrote:
[...] 
> > The chip WF200 allows to use a dedicated line for the IRQ (aka
> > "Out-Of-Band" IRQ). So I have enabled this feature with a edge triggered 
> > IRQ. However, I missed some IRQs. Indeed, it seems that Allwinner use a 
> > 32KHz clock to sample the IRQs. It is not fast enough for us. I think it 
> > explains why we miss some IRQs (using the attribute "input-debounce"[2], 
> > I tried to enable the 24Mhz clock, but without success).
> 
> Without success as in you couldn't make it use the 24MHz clock, or using it
> didn't change anything?

I didn't make the change myself, but it seems that the board was unable
to run correctly when the attribute "input-debounce" was used. Since
"input-debounce" impacts the whole GPIO bank, I think it has impacted
other devices.

Finally, I didn't spend so much time on this and I don't know if it the
24Mhz clock has been finally correctly enabled.

-- 
Jérôme Pouiller



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Possible race while masking IRQ on Allwinner A20
  2020-05-21  8:02   ` Marc Zyngier
@ 2020-05-21 13:28     ` Jérôme Pouiller
  2020-05-21 13:39       ` Marc Zyngier
  0 siblings, 1 reply; 7+ messages in thread
From: Jérôme Pouiller @ 2020-05-21 13:28 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Marc Dorval, Chen-Yu Tsai, Thomas Gleixner, Maxime Ripard,
	linux-arm-kernel

On Thursday 21 May 2020 10:02:48 CEST Marc Zyngier wrote:
> On 2020-05-21 08:26, Maxime Ripard wrote:
> > On Tue, May 19, 2020 at 10:59:26AM +0200, Jérôme Pouiller wrote:
[...]
> >> Nevermind, I tried to use a level triggered IRQ (and my request is on
> >> this part). As you can see in the wfx driver (in  bus_sdio.c and
> >> bh.c), I use a threaded IRQ for that. Unfortunately, I receive some IRQs
> >> twice.
> >> I traced the problem, I get:
> >>
> >>  QSGRenderThread-981   [000] d.h.   247.485524: irq_handler_entry: irq=80 name=wfx
> >>  QSGRenderThread-981   [000] d.h.   247.485547: irq_handler_exit: irq=80 ret=handled
> >>  QSGRenderThread-981   [000] d.h.   247.485600: irq_handler_entry: irq=80 name=wfx
> >>  QSGRenderThread-981   [000] d.h.   247.485606: irq_handler_exit: irq=80 ret=handled
> >>       irq/80-wfx-260   [001] ....   247.485828: io_read32: CONTROL: 0000f046
> >>       irq/80-wfx-260   [001] ....   247.486072: io_read32: CONTROL: 0000f046
> >>     kworker/1:1H-116   [001] ....   247.486214: io_read: QUEUE: 8b 00 84 18 00 00 00 00 01 00 15 82 2b 48 01 1e 88 42 30 00 08 6b d7 c3 53 e0 28 80 88 67 32 af ... (192 bytes)
> >>     kworker/1:1H-116   [001] ....   247.493097: io_read: QUEUE: 00 00 00 00 00 00 00 00 06 06 00 6a 3f 95 00 60 00 00 00 00 08 62 00 00 01 00 5e 00 00 07 28 80 ... (192 bytes)
> >>     [...]
> >>
> >> On this trace, we can see:
> >>   - the hard IRQ handler
> >>   - the IRQ acknowledge from the thread irq/80-wfx-260
> >>   - the access to the data from kworker/1:1H-116
> >>
> >> As far as I understand, the first call to the IRQ handler (at
> >> 247.485524) should mask the IRQ 80. So, the second IRQ (at 247.485600)
> >> should not happen and the thread irq/80 should be triggered only once.
> >>
> >> Do you have any idea of what is going wrong with this IRQ?
> >
> > That's pretty weird indeed. My first guess was that you weren't using
> > IRQF_ONESHOT, but it looks like you are. My next lead would be to see
> > if the mask / unmask hooks in the pinctrl driver are properly called
> > (and actually do what they are supposed to do). I'm not sure we have
> > any in-tree user of a threaded IRQ attached to the pinctrl driver, so
> > it might have been broken for quite some time.
> 
> What is certainly puzzling is that this driver doesn't seem to use
> threaded IRQs at all. Instead, it uses its own workqueue that seems
> to bypass the core IRQ subsystem altogether. So any guarantee we'd
> expect goes at of the window.
>
> It is also pretty unclear to me how whether the HW supports switch
> from edge to level signalling. The request_irq() call definitely asks
> for edge, and I don't know how you'd instruct the HW to change its
> signalling method (in general, it isn't possible).

You are talking about the wfx driver? Be sure you read the right version
of the driver. The ability to use a level-triggered IRQ does not exist in
the stable tree. You have to check the "staging-next" tree from Greg[1].

[1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/staging/+/staging-next/drivers/staging/wfx/bus_sdio.c#109

-- 
Jérôme Pouiller



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Possible race while masking IRQ on Allwinner A20
  2020-05-21 13:28     ` Jérôme Pouiller
@ 2020-05-21 13:39       ` Marc Zyngier
  2020-05-21 14:08         ` Jérôme Pouiller
  0 siblings, 1 reply; 7+ messages in thread
From: Marc Zyngier @ 2020-05-21 13:39 UTC (permalink / raw)
  To: Jérôme Pouiller
  Cc: Marc Dorval, Chen-Yu Tsai, Thomas Gleixner, Maxime Ripard,
	linux-arm-kernel

On 2020-05-21 14:28, Jérôme Pouiller wrote:
> On Thursday 21 May 2020 10:02:48 CEST Marc Zyngier wrote:
>> On 2020-05-21 08:26, Maxime Ripard wrote:
>> > On Tue, May 19, 2020 at 10:59:26AM +0200, Jérôme Pouiller wrote:
> [...]
>> >> Nevermind, I tried to use a level triggered IRQ (and my request is on
>> >> this part). As you can see in the wfx driver (in  bus_sdio.c and
>> >> bh.c), I use a threaded IRQ for that. Unfortunately, I receive some IRQs
>> >> twice.
>> >> I traced the problem, I get:
>> >>
>> >>  QSGRenderThread-981   [000] d.h.   247.485524: irq_handler_entry: irq=80 name=wfx
>> >>  QSGRenderThread-981   [000] d.h.   247.485547: irq_handler_exit: irq=80 ret=handled
>> >>  QSGRenderThread-981   [000] d.h.   247.485600: irq_handler_entry: irq=80 name=wfx
>> >>  QSGRenderThread-981   [000] d.h.   247.485606: irq_handler_exit: irq=80 ret=handled
>> >>       irq/80-wfx-260   [001] ....   247.485828: io_read32: CONTROL: 0000f046
>> >>       irq/80-wfx-260   [001] ....   247.486072: io_read32: CONTROL: 0000f046
>> >>     kworker/1:1H-116   [001] ....   247.486214: io_read: QUEUE: 8b 00 84 18 00 00 00 00 01 00 15 82 2b 48 01 1e 88 42 30 00 08 6b d7 c3 53 e0 28 80 88 67 32 af ... (192 bytes)
>> >>     kworker/1:1H-116   [001] ....   247.493097: io_read: QUEUE: 00 00 00 00 00 00 00 00 06 06 00 6a 3f 95 00 60 00 00 00 00 08 62 00 00 01 00 5e 00 00 07 28 80 ... (192 bytes)
>> >>     [...]
>> >>
>> >> On this trace, we can see:
>> >>   - the hard IRQ handler
>> >>   - the IRQ acknowledge from the thread irq/80-wfx-260
>> >>   - the access to the data from kworker/1:1H-116
>> >>
>> >> As far as I understand, the first call to the IRQ handler (at
>> >> 247.485524) should mask the IRQ 80. So, the second IRQ (at 247.485600)
>> >> should not happen and the thread irq/80 should be triggered only once.
>> >>
>> >> Do you have any idea of what is going wrong with this IRQ?
>> >
>> > That's pretty weird indeed. My first guess was that you weren't using
>> > IRQF_ONESHOT, but it looks like you are. My next lead would be to see
>> > if the mask / unmask hooks in the pinctrl driver are properly called
>> > (and actually do what they are supposed to do). I'm not sure we have
>> > any in-tree user of a threaded IRQ attached to the pinctrl driver, so
>> > it might have been broken for quite some time.
>> 
>> What is certainly puzzling is that this driver doesn't seem to use
>> threaded IRQs at all. Instead, it uses its own workqueue that seems
>> to bypass the core IRQ subsystem altogether. So any guarantee we'd
>> expect goes at of the window.
>> 
>> It is also pretty unclear to me how whether the HW supports switch
>> from edge to level signalling. The request_irq() call definitely asks
>> for edge, and I don't know how you'd instruct the HW to change its
>> signalling method (in general, it isn't possible).
> 
> You are talking about the wfx driver? Be sure you read the right 
> version
> of the driver. The ability to use a level-triggered IRQ does not exist 
> in
> the stable tree. You have to check the "staging-next" tree from 
> Greg[1].

Right. It still remains that in this (new) code, the threaded handler
seems to kick a workqueue, and returns saying "I'm done". With a level
triggered interrupt, this is likely to result in an interrupt storm if
nothing masks the interrupt.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Possible race while masking IRQ on Allwinner A20
  2020-05-21 13:39       ` Marc Zyngier
@ 2020-05-21 14:08         ` Jérôme Pouiller
  0 siblings, 0 replies; 7+ messages in thread
From: Jérôme Pouiller @ 2020-05-21 14:08 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Marc Dorval, Chen-Yu Tsai, Thomas Gleixner, Maxime Ripard,
	linux-arm-kernel

On Thursday 21 May 2020 15:39:09 CEST Marc Zyngier wrote:
> On 2020-05-21 14:28, Jérôme Pouiller wrote:
> > On Thursday 21 May 2020 10:02:48 CEST Marc Zyngier wrote:
> >> On 2020-05-21 08:26, Maxime Ripard wrote:
> >> > On Tue, May 19, 2020 at 10:59:26AM +0200, Jérôme Pouiller wrote:
> > [...]
> >> >> Nevermind, I tried to use a level triggered IRQ (and my request is on
> >> >> this part). As you can see in the wfx driver (in  bus_sdio.c and
> >> >> bh.c), I use a threaded IRQ for that. Unfortunately, I receive some IRQs
> >> >> twice.
> >> >> I traced the problem, I get:
> >> >>
> >> >>  QSGRenderThread-981   [000] d.h.   247.485524: irq_handler_entry: irq=80 name=wfx
> >> >>  QSGRenderThread-981   [000] d.h.   247.485547: irq_handler_exit: irq=80 ret=handled
> >> >>  QSGRenderThread-981   [000] d.h.   247.485600: irq_handler_entry: irq=80 name=wfx
> >> >>  QSGRenderThread-981   [000] d.h.   247.485606: irq_handler_exit: irq=80 ret=handled
> >> >>       irq/80-wfx-260   [001] ....   247.485828: io_read32: CONTROL: 0000f046
> >> >>       irq/80-wfx-260   [001] ....   247.486072: io_read32: CONTROL: 0000f046
> >> >>     kworker/1:1H-116   [001] ....   247.486214: io_read: QUEUE: 8b 00 84 18 00 00 00 00 01 00 15 82 2b 48 01 1e 88 42 30 00 08 6b d7 c3 53 e0 28 80 88 67 32 af ... (192 bytes)
> >> >>     kworker/1:1H-116   [001] ....   247.493097: io_read: QUEUE: 00 00 00 00 00 00 00 00 06 06 00 6a 3f 95 00 60 00 00 00 00 08 62 00 00 01 00 5e 00 00 07 28 80 ... (192 bytes)
> >> >>     [...]
> >> >>
> >> >> On this trace, we can see:
> >> >>   - the hard IRQ handler
> >> >>   - the IRQ acknowledge from the thread irq/80-wfx-260
> >> >>   - the access to the data from kworker/1:1H-116
> >> >>
> >> >> As far as I understand, the first call to the IRQ handler (at
> >> >> 247.485524) should mask the IRQ 80. So, the second IRQ (at 247.485600)
> >> >> should not happen and the thread irq/80 should be triggered only once.
> >> >>
> >> >> Do you have any idea of what is going wrong with this IRQ?
> >> >
> >> > That's pretty weird indeed. My first guess was that you weren't using
> >> > IRQF_ONESHOT, but it looks like you are. My next lead would be to see
> >> > if the mask / unmask hooks in the pinctrl driver are properly called
> >> > (and actually do what they are supposed to do). I'm not sure we have
> >> > any in-tree user of a threaded IRQ attached to the pinctrl driver, so
> >> > it might have been broken for quite some time.
> >>
> >> What is certainly puzzling is that this driver doesn't seem to use
> >> threaded IRQs at all. Instead, it uses its own workqueue that seems
> >> to bypass the core IRQ subsystem altogether. So any guarantee we'd
> >> expect goes at of the window.
> >>
> >> It is also pretty unclear to me how whether the HW supports switch
> >> from edge to level signalling. The request_irq() call definitely asks
> >> for edge, and I don't know how you'd instruct the HW to change its
> >> signalling method (in general, it isn't possible).
> >
> > You are talking about the wfx driver? Be sure you read the right
> > version
> > of the driver. The ability to use a level-triggered IRQ does not exist
> > in
> > the stable tree. You have to check the "staging-next" tree from
> > Greg[1].
> 
> Right. It still remains that in this (new) code, the threaded handler
> seems to kick a workqueue, and returns saying "I'm done". With a level
> triggered interrupt, this is likely to result in an interrupt storm if
> nothing masks the interrupt.

The core the threaded IRQ handler is in bh.c/wfx_bh_request_rx():

	control_reg_read(wdev, &cur);
	prev = atomic_xchg(&wdev->hif.ctrl_reg, cur);
	complete(&wdev->hif.ctrl_ready);
	queue_work(system_highpri_wq, &wdev->hif.bh);

The call to control_reg_read() acknowledge the IRQ (and get the length of
data to read). After this function, the IRQ line is down (then, indeed the
read of data is done from a workqueue).


Concerning the hard IRQ handler, we use the default IRQ handler, that
indeed just return IRQ_WAKE_THREAD. Since we also specify IRQF_ONESHOT,
the IRQ should be masked until the threaded IRQ ends.


(You may wonder why the driver does not call wfx_bh_request_rx() from a
regular IRQ handler. It is because control_reg_read() is not atomic.)

-- 
Jérôme Pouiller



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-05-21 14:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19  8:59 Possible race while masking IRQ on Allwinner A20 Jérôme Pouiller
2020-05-21  7:26 ` Maxime Ripard
2020-05-21  8:02   ` Marc Zyngier
2020-05-21 13:28     ` Jérôme Pouiller
2020-05-21 13:39       ` Marc Zyngier
2020-05-21 14:08         ` Jérôme Pouiller
2020-05-21 13:12   ` Jérôme Pouiller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).