On Thu, Aug 5, 2021 at 1:26 PM Viresh Kumar via Stratos-dev stratos-dev@op-lists.linaro.org wrote:
On 03-08-21, 17:01, Arnd Bergmann wrote:
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.
Based on discussion we had today (offline), I changed the design a bit and used handle_level_irq() instead, as it provides consistent calls to mask/unmask(), which simplified the whole thing a bit.
The new flow looks much nicer to me, without the workqueue, and doing the requeue directly in the unmask() operation.
I don't quite understand the purpose of the type_pending and mask_pending flags yet, can you explain what they actually do?
Also, I have no idea about whether using the handle_level_irq() function is actually correct here. I suppose if necessary, the driver could provide its own irq.handler callback in place of that.
Also I have broken the rule from specs, maybe we should update spec with that, where the specs said that the buffer must not be queued before enabling the interrupt. I just queue the buffer unconditionally now from unmask().
I am not sure but there may be some race around the "queued" flag and I wonder if we can land in a scenario where the buffer is left un-queued somehow, while an interrupt is enabled.
Can that be integrated with the "masked" state now? It looks like the two flags are always opposites now.
Arnd
On 05-08-21, 14:03, Arnd Bergmann wrote:
On Thu, Aug 5, 2021 at 1:26 PM Viresh Kumar via Stratos-dev
Based on discussion we had today (offline), I changed the design a bit and used handle_level_irq() instead, as it provides consistent calls to mask/unmask(), which simplified the whole thing a bit.
The new flow looks much nicer to me, without the workqueue, and doing the requeue directly in the unmask() operation.
I don't quite understand the purpose of the type_pending and mask_pending flags yet, can you explain what they actually do?
They are required to make sure we don't send unnecessary VIRTIO_GPIO_MSG_IRQ_TYPE events to the device, every time bus_unlock() is called.
mask_pending tracks if the masked state has changed since the time last bus_unlock() was called. So on an interrupt, both mask() and unmask() will get called by the irq-core now and mask_pending will change to true (in mask()} and then false (in unmask()). And eventually in bus_unlock() we won't send an extra VIRTIO_GPIO_MSG_IRQ_TYPE message.
Also, I have no idea about whether using the handle_level_irq() function is actually correct here. I suppose if necessary, the driver could provide its own irq.handler callback in place of that.
After looking at internals of these, I felt handle_level_irq() suits much better in our case as we need to queue the buffer only at unmask(). With handle_fasteoi_irq(), we would be required to do the same from multiple places, unmask(), eoi().
Also I have broken the rule from specs, maybe we should update spec with that, where the specs said that the buffer must not be queued before enabling the interrupt. I just queue the buffer unconditionally now from unmask().
I am not sure but there may be some race around the "queued" flag and I wonder if we can land in a scenario where the buffer is left un-queued somehow, while an interrupt is enabled.
Can that be integrated with the "masked" state now? It looks like the two flags are always opposites now.
Yeah, but then there can be races and keeping them separate is a better thing IMO.
I was thinking of something on these lines, disable_irq() followed by enable_irq():
CPU0 CPU1
disable_irq() -> irq_bus_lock() -> irq_mask() -> irq_bus_sync_unlock() -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE to disable interrupt Backend (at host) disables irq and returns the unused buffer.
enable_irq() -> irq_bus_lock() -> irq_unmask() -> Tries to queues buffer again Finds it already queued and returns. - virtio_gpio_event_vq() runs at guest - Finds VIRTIO_GPIO_IRQ_STATUS_INVALID in status - returns without doing anything -> irq_bus_sync_unlock() -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE to enable interrupt
So the irq is still enabled and the buffer isn't queued. Yes, need some locking here for sure, confirmed :)
On Thu, Aug 5, 2021 at 2:49 PM Viresh Kumar viresh.kumar@linaro.org wrote:
On 05-08-21, 14:03, Arnd Bergmann wrote:
On Thu, Aug 5, 2021 at 1:26 PM Viresh Kumar via Stratos-dev
Based on discussion we had today (offline), I changed the design a bit and used handle_level_irq() instead, as it provides consistent calls to mask/unmask(), which simplified the whole thing a bit.
The new flow looks much nicer to me, without the workqueue, and doing the requeue directly in the unmask() operation.
I don't quite understand the purpose of the type_pending and mask_pending flags yet, can you explain what they actually do?
They are required to make sure we don't send unnecessary VIRTIO_GPIO_MSG_IRQ_TYPE events to the device, every time bus_unlock() is called.
mask_pending tracks if the masked state has changed since the time last bus_unlock() was called. So on an interrupt, both mask() and unmask() will get called by the irq-core now and mask_pending will change to true (in mask()} and then false (in unmask()). And eventually in bus_unlock() we won't send an extra VIRTIO_GPIO_MSG_IRQ_TYPE message.
I hope this can still be simplified by working out better which state transitions are needed exactly. In particular, I would expect that we can get away with not sending a VIRTIO_GPIO_MSG_IRQ_TYPE for 'mask' state changes at all, but use that only for forcing 'enabled' state changes.
One part that I think is missing though is remembering the case when an eventq message came in after an interrupt got masked when the message was already armed. In this case, the virtio_gpio_event_vq() function would not call the irq handler, but the subsequent "unmask" callback would need to arrange having it called.
Arnd
On 05-08-21, 15:10, Arnd Bergmann wrote:
I hope this can still be simplified by working out better which state transitions are needed exactly. In particular, I would expect that we can get away with not sending a VIRTIO_GPIO_MSG_IRQ_TYPE for 'mask' state changes at all, but use that only for forcing 'enabled' state changes.
Something like this ?
struct vgpio_irq_line { u8 type; bool masked; bool update_pending; bool queued;
struct virtio_gpio_irq_request ireq ____cacheline_aligned; struct virtio_gpio_irq_response ires ____cacheline_aligned; };
static void virtio_gpio_irq_disable(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; }
static void virtio_gpio_irq_enable(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;
/* Queue the buffer unconditionally on enable */ virtio_gpio_irq_prepare(vgpio, d->hwirq); }
static void virtio_gpio_irq_mask(struct irq_data *d) { /* Nothing to do here */ }
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);
/* Queue the buffer unconditionally on unmask */ virtio_gpio_irq_prepare(vgpio, d->hwirq); }
static int virtio_gpio_irq_set_type(struct irq_data *d, unsigned int type) { 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];
switch (type) { case IRQ_TYPE_NONE: type = VIRTIO_GPIO_IRQ_TYPE_NONE; break; case IRQ_TYPE_EDGE_RISING: type = VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING; break; case IRQ_TYPE_EDGE_FALLING: type = VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING; break; case IRQ_TYPE_EDGE_BOTH: type = VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH; break; case IRQ_TYPE_LEVEL_LOW: type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW; break; case IRQ_TYPE_LEVEL_HIGH: type = VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH; break; default: dev_err(&vgpio->vdev->dev, "unsupported irq type: %u\n", type); return -EINVAL; }
irq_line->type = type; irq_line->update_pending = true;
return 0; }
static void update_irq_type(struct virtio_gpio *vgpio, u16 gpio, u8 type) { virtio_gpio_req(vgpio, VIRTIO_GPIO_MSG_IRQ_TYPE, gpio, type, NULL); }
static void virtio_gpio_irq_bus_lock(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct virtio_gpio *vgpio = gpiochip_get_data(gc);
mutex_lock(&vgpio->irq_lock); }
static void virtio_gpio_irq_bus_sync_unlock(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]; u8 type = irq_line->masked ? VIRTIO_GPIO_IRQ_TYPE_NONE : irq_line->type;
if (irq_line->update_pending) { irq_line->update_pending = false; update_irq_type(vgpio, d->hwirq, type); }
mutex_unlock(&vgpio->irq_lock); }
static struct irq_chip vgpio_irq_chip = { .name = "virtio-gpio", .irq_enable = virtio_gpio_irq_enable, .irq_disable = virtio_gpio_irq_disable, .irq_mask = virtio_gpio_irq_mask, .irq_unmask = virtio_gpio_irq_unmask, .irq_set_type = virtio_gpio_irq_set_type,
/* These are required to implement irqchip for slow busses */ .irq_bus_lock = virtio_gpio_irq_bus_lock, .irq_bus_sync_unlock = virtio_gpio_irq_bus_sync_unlock, };
One part that I think is missing though is remembering the case when an eventq message came in after an interrupt got masked when the message was already armed. In this case, the virtio_gpio_event_vq() function would not call the irq handler, but the subsequent "unmask" callback would need to arrange having it called.
I will come back to this.
stratos-dev@op-lists.linaro.org
-
Arnd Bergmann -
Viresh Kumar