Messages in this thread | | | From | Arnd Bergmann <> | Date | Tue, 3 Aug 2021 17:01:59 +0200 | Subject | Re: [PATCH V4 2/2] gpio: virtio: Add IRQ support |
| |
On Tue, Aug 3, 2021 at 1:36 PM Viresh Kumar <viresh.kumar@linaro.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@linaro.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
| |