Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ

[Maarten Brock <m.brock@vanmierlo.com>]

On 2020-05-17 22:44, Daniel Mack wrote:
> Hi Maarten,
> 
> Thanks for your review!
> 
> On 5/9/20 2:55 PM, Maarten Brock wrote:
>> On 2020-05-08 16:37, Daniel Mack wrote:
>>> Use a threaded IRQ handler to get rid of the irq_work kthread.
>>> This also allows for the driver to use interrupts generated by
>>> a threaded controller.
>>> 
>>> Signed-off-by: Daniel Mack <daniel@zonque.org>
>>> ---
>>>  drivers/tty/serial/sc16is7xx.c | 18 +++++-------------
>>>  1 file changed, 5 insertions(+), 13 deletions(-)
>>> 
>>> diff --git a/drivers/tty/serial/sc16is7xx.c
>>> b/drivers/tty/serial/sc16is7xx.c
> 
>>> @@ -1317,8 +1308,9 @@ static int sc16is7xx_probe(struct device *dev,
>>>      }
>>> 
>>>      /* Setup interrupt */
>>> -    ret = devm_request_irq(dev, irq, sc16is7xx_irq,
>>> -                   IRQF_TRIGGER_FALLING, dev_name(dev), s);
>>> +    ret = devm_request_threaded_irq(dev, irq, NULL, sc16is7xx_irq,
>>> +                    IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>>> +                    dev_name(dev), s);
>>>      if (!ret)
>>>          return 0;
>> 
>> Since UART0 is first handled completely in the for loop before UART1 
>> is
>> handled, a new interrupt may arise on UART0 while UART1 is being 
>> handled.
> 
> The code in the interrupt handling function loops forever until there 
> is
> no more interrupt bits pending. So if there is a new IRQ happening for
> UART0 while UART1 is being served, it will be handled in the same loop.

I'm sorry. I remembered this problem and didn't look well enough. It has
been fixed already for 20 months by the keep_polling flag.

> And just to be sure I understand correctly: this is unrelated to the
> switch to threaded IRQs, right? Falling edge triggers were always used
> for pdata probed devices.

The switch to threaded makes it possible to use IRQF_TRIGGER_LOW instead
of IRQF_TRIGGER_FALLING. The current implementation would keep on 
triggering
with IRQF_TRIGGER_LOW and the worker thread might not even get executed.

I don't have a clue what a pdata probed device is.

>> The result is a missed interrupt since the IRQ line might not *FALL* 
>> again.
> 
> It doesn't have to. We only exit the interrupt handler when there is
> nothing left to do, at which point the IRQ line ist back high. So it
> will fall again in case of new events.

Right, already fixed for a single device.

Different problem then: what if the interrupt is shared with another
device, say another sc16is7xx?

>> Therefor I suggest to change IRQF_TRIGGER_FALLING to IRQF_TRIGGER_LOW. 
>> This
>> way the thread will be retriggered after IRQ_HANDLED is returned.
> 
> This doesn't work in my setup unfortunately, as the interrupt 
> controller
> is incapable of handling level IRQs.

That sounds like a lousy interrupt controller to me. But still, I would
expect the interrupt controller driver to handle that problem, not the
device driver for the device hanging off the interrupt controller. Or
at least the interrupt controller driver should throw an error.

> Thanks,
> Daniel

Summerizing:
- After switching to a threaded IRQ, the trigger could be switched to
IRQF_TRIGGER_LOW and with that interrupt sharing can be enabled for
this device with IRQF_SHARED.

- Some (your) interrupt controllers do not support IRQF_TRIGGER_LOW.
For those only IRQF_TRIGGER_FALLING can be used for this device and
thus IRQF_SHARED cannot be used.

- The driver for your interrupt controller should be improved to support
level IRQs.

This makes me wonder if it would be better to let the device tree 
specify
the interrupt configuration. Which leads back to my earlier question: 
How
does one specify in the device tree that an interrupt is shared?

Kind regards,
Maarten