Re: [PATCH V4 2/2] gpio: virtio: Add IRQ support
From: Arnd Bergmann <arnd@kernel.org>
Date: 2021-08-03 15:02:19
Also in:
lkml
On Tue, Aug 3, 2021 at 1:36 PM Viresh Kumar [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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