From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 729D0C433DF for ; Mon, 18 May 2020 11:15:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 575D3207F5 for ; Mon, 18 May 2020 11:15:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726428AbgERLPY (ORCPT ); Mon, 18 May 2020 07:15:24 -0400 Received: from fieber.vanmierlo.com ([84.243.197.177]:45828 "EHLO kerio9.vanmierlo.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726726AbgERLPX (ORCPT ); Mon, 18 May 2020 07:15:23 -0400 X-Footer: dmFubWllcmxvLmNvbQ== Received: from roundcube.vanmierlo.com ([192.168.37.37]) (authenticated user m.brock@vanmierlo.com) by kerio9.vanmierlo.com (Kerio Connect 9.2.12 patch 1) with ESMTPA; Mon, 18 May 2020 13:14:49 +0200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Mon, 18 May 2020 13:14:49 +0200 From: Maarten Brock To: Daniel Mack Cc: devicetree@vger.kernel.org, linux-serial@vger.kernel.org, gregkh@linuxfoundation.org, robh+dt@kernel.org, jslaby@suse.com, pascal.huerst@gmail.com, linux-serial-owner@vger.kernel.org Subject: Re: [PATCH 4/4] sc16is7xx: Use threaded IRQ In-Reply-To: <584de876-e675-0172-97ed-0c9534eb9526@zonque.org> References: <20200508143757.2609740-1-daniel@zonque.org> <20200508143757.2609740-5-daniel@zonque.org> <61fdcf12976c924fd86c5203aba673a7@vanmierlo.com> <584de876-e675-0172-97ed-0c9534eb9526@zonque.org> Message-ID: X-Sender: m.brock@vanmierlo.com User-Agent: Roundcube Webmail/1.3.3 Sender: linux-serial-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-serial@vger.kernel.org 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 >>> --- >>>  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