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=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 CDD0CC48BD1 for ; Thu, 10 Jun 2021 21:30:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B0504613E3 for ; Thu, 10 Jun 2021 21:30:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230436AbhFJVc1 (ORCPT ); Thu, 10 Jun 2021 17:32:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54710 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230083AbhFJVc1 (ORCPT ); Thu, 10 Jun 2021 17:32:27 -0400 Received: from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com [IPv6:2a00:1450:4864:20::22a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B61F7C0617A6 for ; Thu, 10 Jun 2021 14:30:20 -0700 (PDT) Received: by mail-lj1-x22a.google.com with SMTP id d2so6818403ljj.11 for ; Thu, 10 Jun 2021 14:30:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Mc0t7dqVDGu20ZDgp1J77BZzzR8pf6dBx6PMN3sIo8c=; b=aOEIplv0pyoO+2D1enYzTH7rRMUM5qf5y/qVUZMMn97grwASNtzDCe6NqISGYiZMo/ ukksvEVMwqEiLXlOk7Xqes3HVR1NjBClrE1UmSQ+glzsnFi3+RbYzGF78dSlKpLFU0JZ fdTr/eSTmYpuSXZulL5tGXRJG/I0jBiZbkhkcpdSROdkYtLEck/4uALoj6p0onaq315m JtShyX4vbRX1kL2eW4pFO5ne3PKIMIIoI3VPqWTXEo3+d3M4W6uXg7lVNw/lt/VjhUrE 6A8O8GSKAnoiX+g/msiwrr/WSJCOVPblDBUfbeD3OI0es+O8CDxB3CEA+scJZ1LWc7dI gSOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Mc0t7dqVDGu20ZDgp1J77BZzzR8pf6dBx6PMN3sIo8c=; b=CI4mmUJU+zuB4jSqt0prVqmsbYVFHDupqu7Qh5bF5fW82v6LuyTNKN/UVCbTVCMKDS 86l/57y+d10IsqWBMdX04fnaCJwzFIhCq7ANOxabwKaW1Emf8grYCcQT7+YqY61XCwHY wSXcM8Ah9XUx777idQiHLYXj9OD82pUuiiEFFn9q8kKC8ghPXLv+FfV0bvOrCn/Ash38 pDWy4rcVnLFBnDlNQl6BW334J4f66POw2bsFSSTU8MmYwduFo2cpimN8LDu1AfGsMNxk rfBgw/vcQh5NtII3D/QlFLCw/4qSXDDlXAdliYDGOzHAYkCMivW0PfP1nku3oNYcRBmJ VWNA== X-Gm-Message-State: AOAM531cGvBYESEOTK/5uQ/XQtDJiJo1pp0LgztJBqE5F4EKjLj1w/0g lvdhBPn5URritgqyKZIuDSao1gYcgMFGjaZh8OjGjw== X-Google-Smtp-Source: ABdhPJwLHL8jOR3WGl2QJnT+GPWUrRqLDv/5Ut16hTcM8MfJaU0MmKihHfxqkHVnSdiiDFsReGTpITxPhKmBolVuFZo= X-Received: by 2002:a2e:22c3:: with SMTP id i186mr433222lji.273.1623360618946; Thu, 10 Jun 2021 14:30:18 -0700 (PDT) MIME-Version: 1.0 References: <911941d4bf19f18abdc9700abca9f26b3c04c343.1623326176.git.viresh.kumar@linaro.org> In-Reply-To: <911941d4bf19f18abdc9700abca9f26b3c04c343.1623326176.git.viresh.kumar@linaro.org> From: Linus Walleij Date: Thu, 10 Jun 2021 23:30:07 +0200 Message-ID: Subject: Re: [PATCH V3 2/3] gpio: virtio: Add IRQ support To: Viresh Kumar , Marc Zyngier , Thomas Gleixner Cc: Bartosz Golaszewski , "Enrico Weigelt, metux IT consult" , Viresh Kumar , "Michael S. Tsirkin" , Jason Wang , Vincent Guittot , Bill Mills , =?UTF-8?B?QWxleCBCZW5uw6ll?= , stratos-dev@op-lists.linaro.org, "open list:GPIO SUBSYSTEM" , linux-kernel , Stefan Hajnoczi , "Stefano Garzarella --cc virtualization @ lists . linux-foundation . org" , virtualization@lists.linux-foundation.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org Hi Viresh! thanks for this interesting patch! On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar wrote: > This patch adds IRQ support for the virtio GPIO driver. Note that this > uses the irq_bus_lock/unlock() callbacks since the operations over > virtio can sleep. > > Signed-off-by: Viresh Kumar > drivers/gpio/gpio-virtio.c | 256 ++++++++++++++++++++++++++++++- > include/uapi/linux/virtio_gpio.h | 15 ++ You also need to select GPIOLIB_IRQCHIP in the Kconfig entry for the gpio-virtio driver, because you make use of it. > +static void virtio_gpio_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_to_gpio_chip(d); > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + struct vgpio_line *line = &vgpio->lines[d->hwirq]; > + > + line->masked = true; > + line->masked_pending = true; > +} > + > +static void virtio_gpio_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_to_gpio_chip(d); > + struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc); > + struct vgpio_line *line = &vgpio->lines[d->hwirq]; > + > + line->masked = false; > + line->masked_pending = true; > +} This looks dangerous in combination with this: > +static void virtio_gpio_interrupt(struct virtqueue *vq) > +{ (...) > + local_irq_disable(); > + ret = generic_handle_irq(irq); > + local_irq_enable(); Nominally slow IRQs like those being marshalled over virtio should be nested, handle_nested_irq(irq); but are they? Or are they just quite slow not super slow? If a threaded IRQF_ONESHOT was requested the IRQ core will kick the thread and *MASK* this IRQ, which means it will call back to your .irq_mask() function and expect it to be masked from this point. But the IRQ will not actually be masked until you issue your callbacks in the .irq_bus_sync_unlock() callback right? So from this point until .irq_bus_sync_unlock() get called and actually mask the IRQ, it could be fired again? I suppose the IRQ handler is reentrant? This would violate the API. I would say that from this point and until you sync you need a spinlock or other locking primitive to stop this IRQ from fireing again, and a spinlock will imply local_irq_disable() so this gets really complex. I would say only using nesting IRQs or guarantee this some other way, one way would be to specify that whatever is at the other side of virtio cannot send another GPIO IRQ message before the last one is handled, so you would need to send a specific (new) VIRTIO_GPIO_REQ_IRQ_ACK after all other messages have been sent in .irq_bus_sync_unlock() so that the next GPIO IRQ can be dispatched after that. (Is this how messaged signalled interrupts work? No idea. When in doubt ask the IRQ maintainers.) Thanks, Linus Walleij