On Fri, Jul 30, 2021 at 8:45 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.
Reviewed-by: Linus Walleij linus.walleij@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I think the flow you describe now addresses all the important concerns I had. I feel the text you use additional copy-editing, but I'm not a native English speaker myself and not qualified for that. Hope someone else can help out with that.
A few more comments on some details:
+If the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST +mask the interrupt for the GPIO line and discard any latched interrupt event +associated with it. The device MUST also 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}. If the driver +queues another pair of buffers, while the trigger type remains set as +\field{VIRTIO_GPIO_IRQ_TYPE_NONE}, the device MUST wait for another message of +type \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} before using the newly queued buffers.
We discussed this last part before, and you had already said you prefer this ordering, but after you explain it here, the interaction of VIRTIO_GPIO_MSG_SET_IRQ_TYPE with queued buffers still seems inconsistent to me: If setting the type to none cancels any outstanding eventq request, why do newly queued eventq request get added to the queue? If we change the order to always require the type to be enabled before queuing any events, this inconsistency would go away. With the behavior of the level triggered interrupts changed to fasteoi, do you still see a need to keep this ordering?
+\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}
+\begin{itemize} +\item The driver is requested by a client driver to enable interrupt for a GPIO
- line and configure it to a particular trigger type.
+\item The driver sends the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message over the
- \field{requestq} virtqueue and the device configures the GPIO line for the
- requested trigger type and unmasks the interrupt.
+\item The driver queues a pair of buffers, interrupt-request and
- interrupt-response, to the \field{eventq} virtqueue for the GPIO line.
+\item The driver notifies the device of the presence of new buffers on the
- \field{eventq} virtqueue.
+\item The interrupt is fully configured at this point.
+\item The device, on sensing an active interrupt on the GPIO line, finds the
- matching buffers (based on GPIO line number) from the \field{eventq}
- virtqueue and fills its \field{struct virtio_gpio_irq_response} buffer's
- \field{status} with \field{VIRTIO_GPIO_IRQ_STATUS_VALID} and returns the
- pair of buffers to the device.
+\item The device notifies the driver of the presence of returned buffers on the
- \field{eventq} virtqueue.
+\item If the GPIO line is configured for level interrupts, the device MUST
It would be nice to separate the normative text (using MUST etc) from the section describing the message flow.
+\item In a typical guest operating system kernel, the virtio-gpio driver
- notifies the client driver, that is associated with this gpio line, to
- process the event.
+\item In the case of a level triggered interrupt, the client driver must fully
- process and acknowledge the event at its source to return the line to its
- inactive state.
+\item The driver may again queue, same or new, pair of buffers for that GPIO
- line and notify the device.
I suggested this bit, but on second thought it still doesn't quite capture the important bit that the 'level' line has to be set to low /before/ the new buffer gets queued (to avoid a spurious notification). Otherwise there is no difference between edge and level interrupts, obviously in both cases the interrupt has to be processed eventually.
Arnd