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

[Arnd Bergmann <arnd@xxxxxxxxxx>]

On Tue, Aug 3, 2021 at 1:36 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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