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 :)