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