linux-kernel - Re: [PATCH V4 2/2] gpio: virtio: Add IRQ support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK8P3a29NfFWwtGHhqos1P8f_SmzPJTXvEY5BZJAEMbV2SKe-Q@mail.gmail.com>
Date:   Tue, 3 Aug 2021 17:01:59 +0200
From:   Arnd Bergmann <arnd@...nel.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        "Enrico Weigelt, metux IT consult" <info@...ux.net>,
        Viresh Kumar <vireshk@...nel.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Bill Mills <bill.mills@...aro.org>,
        Alex Bennée <alex.bennee@...aro.org>,
        Cornelia Huck <cohuck@...hat.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Stratos Mailing List <stratos-dev@...lists.linaro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE" 
        <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH V4 2/2] gpio: virtio: Add IRQ support

On Tue, Aug 3, 2021 at 1:36 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> This patch adds IRQ support for the virtio GPIO driver. Note that this
> uses the irq_bus_lock/unlock() callbacks, since those operations over
> virtio may sleep. Also the notifications for the eventq are processed
> using a work item to allow sleep-able operations.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/gpio/Kconfig             |   1 +
>  drivers/gpio/gpio-virtio.c       | 281 ++++++++++++++++++++++++++++++-
>  include/uapi/linux/virtio_gpio.h |  25 +++
>  3 files changed, 303 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index e5993d6864fb..222f4ae98a35 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1672,6 +1672,7 @@ config GPIO_MOCKUP
>  config GPIO_VIRTIO
>         tristate "VirtIO GPIO support"
>         depends on VIRTIO
> +       select GPIOLIB_IRQCHIP
>         help
>           Say Y here to enable guest support for virtio-based GPIO controllers.
>

> +struct vgpio_irq_line {
> +       u8 type;
> +       bool masked;
> +       bool update_pending;
> +       bool queued;
> +
> +       struct virtio_gpio_irq_request ireq;
> +       struct virtio_gpio_irq_response ires;
> +};

I think the last two members should be marked as __cacheline_aligned,
since they are transferred separately over DMA.

> +static void virtio_gpio_irq_eoi(struct irq_data *d)
> +{
> +       /*
> +        * Queue buffers, by calling virtio_gpio_irq_prepare(), from
> +        * virtio_gpio_event_vq() itself, after taking into consideration the
> +        * masking status of the interrupt.
> +        */
> +}

Shouldn't this just requeue the interrupt? There is no reason to
defer this, and I think we want the normal operation to not have
to involve any scheduling.

> +static void virtio_gpio_irq_unmask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct virtio_gpio *vgpio = gpiochip_get_data(gc);
> +       struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];
> +
> +       irq_line->masked = false;
> +       irq_line->update_pending = true;
> +}

Same here. unmask is probably less important, but it's the
same operation: if you want interrupts to become active
again when they are not, just queue the request

> +static void virtio_gpio_irq_mask(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct virtio_gpio *vgpio = gpiochip_get_data(gc);
> +       struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];
> +
> +       irq_line->masked = true;
> +       irq_line->update_pending = true;
> +}

This is of course the tricky bit, I was hoping you had found a solution
based on what I wrote above for eio() and unmask().

> +static void vgpio_work_handler(struct work_struct *work)
> +{
> +       struct virtio_gpio *vgpio = container_of(work, struct virtio_gpio,
> +                                                work);
> +       struct device *dev = &vgpio->vdev->dev;
> +       struct vgpio_irq_line *irq_line;
> +       int irq, gpio, ret;
> +       unsigned int len;
> +
> +       mutex_lock(&vgpio->irq_lock);
> +
> +       while (true) {
> +               irq_line = virtqueue_get_buf(vgpio->event_vq, &len);
> +               if (!irq_line)
> +                       break;

Related to above, I think all the eventq handling should be moved into the
virtio_gpio_event_vq() function directly.

> +               /* The interrupt may have been disabled by now */
> +               if (irq_line->update_pending && irq_line->masked)
> +                       update_irq_type(vgpio, gpio, VIRTIO_GPIO_IRQ_TYPE_NONE);

This is a part I'm not sure about, and I suppose it's the same part that
Marc was also confused by.

As far as I can tell, the update_irq_type() message would lead to the
interrupt getting delivered when it was armed and is now getting disabled,
but I don't see why we would call update_irq_type() as a result of the
eventq notification.

      Arnd