On Tue, Oct 12, 2021 at 9:16 AM Viresh Kumar viresh.kumar@linaro.org wrote:
This patch adds support for interrupts to the virtio-gpio specification. This uses the feature bit 0 for the same.
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/114 Cc: Marc Zyngier maz@kernel.org Cc: Thomas Gleixner tglx@linutronix.de Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
This looks good to me overall, I find it very clear in describing the protocol, and I see no remaining technical issues.
Marc had a number of concerns with the older versions, so I think he needs to see if you have addressed those as well.
I think we need a little bit of copy-editing on some of the sentences, but I'm not commenting on those as I'm not a native English speaker.
There is one bit that I would change, and I think we have discussed it in the past, but I don't remember the reasoning here:
+When the interrupt is disabled by the driver, by setting the trigger type to +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device should return any unused pair of +buffers for the GPIO line, over the \field{eventq} virtqueue, after setting the +\field{status} field to \field{VIRTIO_GPIO_IRQ_STATUS_INVALID}. This also masks +the interrupt.
+The driver can enable and unmask the interrupt in any order, i.e. it can enable +the interrupt first and then queue the buffers or queue the buffers first and +then enable the interrupt.
This should work fine, but it seems odd that this is asymmetric between enable/unmask and disable/mask. My feeling is that it would be more consistent if you also require the "enable" to happen before "unmask" to limit the number possible states that the irq line can be in.
Do you have a particular use case in mind that would benefit from first queuing the event request and then enabling the line? I see you listed this as one of the things you changed in v9, but I don't remember for what reason. Your "eventq Operation: Message Flow" section only describes one of the two allowed cases here, and I think it's the one we want anyway.
Arnd