[PATCH V7 0/2] virtio: Add specification for virtio-gpio

[PATCH V7 0/2] virtio: Add specification for virtio-gpio

[Viresh Kumar]

Hello,

This patchset adds virtio specification for GPIO devices. It supports basic GPIO
line operations, along with optional interrupts on them (in a separate patch).

This is an *alternative implementation* of the virtio-gpio specification, a
different version of it was earlier posted by Enrico [1].

I took back V4 of the specification I posted earlier (on June 17th), to allow
Enrico to come up with something that is robust and works for everyone (as he
started this thing last year), but it didn't go as expected. I couldn't
see any of my review comments being incorporated (or any intentions of them
getting in ever) in the three versions Enrico posted.

And so I am trying to present an alternative approach here to solve the problem,
which I believe is more clear and robust. I am open to suggestions to improve it
further.

I will let the virtio/gpio maintainers decide on its fate.

Key differences from Enrico's approach [1]:

- config structure is rearranged to remove everything apart from number of gpios
  and size of the gpio_names_field.

- Request/free is merged with set-direction itself.

- Interrupt implementation handled with feature bit 0. Either the interrupts are
  fully supported or not at all.

- All non-interrupt traffic happens on a single virtqueue, requestq. Interrupt
  traffic goes over eventq.

- Doesn't add any ordering restrictions on the device for different GPIO lines,
  it can respond earlier to the second request, while still processing the first
  one.


Version history of the specification I sent:

V6 -> V7:
- Drop Activate/Deactivate requests and merge them with set-direction one.
- Dropped separate calls to set direction to input or output (with a value),
  with a single call to set-direction (input, output, or none). Note that the
  driver needs to call set-value before calling set-direction-out now.
- Allow multiple messages for a GPIO line to be sent together, they must be
  processed in order though.
- The gpio-line names aren't required to be set for all the lines now, it is
  optional and can be present only for a subset of lines. Provided example as
  well.
- Merge irq-set/mask/unmask to a single set-irq-type message.
- We have only 6 message types now instead of 11 in v6.
- Mentioned about non-atomic nature of the these messages in commit log for
  patch 1/2.
- Improved spec content overall.

V5 -> V6:
- All non-interrupt traffic happens on a single virtqueue, requestq. Interrupt
  traffic goes over eventq now.
- Many fields dropped from the config structure.
- Separate message type to get gpio-names, added more description about how the
  names should be.
- Much clearer message flows, both non-irq messages and irq-messages.
- Parallelism supported for all type of messages, for different GPIO pins.
- All references to POSIX errors dropped, just reply pass or fail.
- request/free renamed to activate/deactivate, as that's what we will end up
  doing there, activate or deactivate a GPIO line.
- General purpose IO as I/O or Input/Output instead.
- Hopefully I didn't miss any review comments.

V4 -> V5:
- Split into two patches, irq stuff in patch 2.
- Use separate virtqueue for sending data from device/driver.
- Allow parallel processing of requests for different GPIOs, only one request at
  a time for the same GPIO line.
- Same goes for interrupt side, only one interrupt request per GPIO line.
- Improved formatting in general.
- Add new sections explaining message flow sequence.

V3 -> V4:
- The GPIO line names must be unique within a device.
- The gpio_names[0] field is dropped and gpio_names_offset field is
  added in place of the 2 bytes of padding.
- New interrupts must not be initiated by the device, without a response
  for the previous one.

V2 -> V3:
- Unused char in name string should be marked 0 now.
- s/host/device/ and s/guest/driver/
- Added a new feature for IRQ mode, VIRTIO_GPIO_F_IRQ.
- A new feature should be added for Version information if required
  later on.

V1 -> V2:
- gpio_names_size is 32 bit.
- gpio field is 16 bit.
- padding added 16 bit.
- Added packed attribute to few structures
- Add the missing 'type' field to the request
- Dropped to _nodata request/responses to simplify a bit, updated
  related text.

--
Viresh

[1] https://lists.oasis-open.org/archives/virtio-dev/202106/msg00030.html

Viresh Kumar (2):
  virtio-gpio: Add the device specification
  virtio-gpio: Add support for interrupts

 conformance.tex |  30 ++-
 content.tex     |   1 +
 virtio-gpio.tex | 528 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 555 insertions(+), 4 deletions(-)
 create mode 100644 virtio-gpio.tex

-- 
2.31.1.272.g89b43f80a514

[virtio-dev] [PATCH V7 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

virtio-gpio is a virtual GPIO controller. It provides a way to flexibly
communicate with the host GPIO controllers from the guest.

Note that the current implementation doesn't provide atomic APIs for
GPIO configurations. i.e. the driver (guest) would need to implement
sleep-able versions of the APIs as the guest will respond asynchronously
over the virtqueue.

This patch adds the specification for it.

Based on the initial work posted by:
"Enrico Weigelt, metux IT consult" <lkml@metux.net>.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 conformance.tex |  28 +++-
 content.tex     |   1 +
 virtio-gpio.tex | 339 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 364 insertions(+), 4 deletions(-)
 create mode 100644 virtio-gpio.tex

diff --git a/conformance.tex b/conformance.tex
index 94d7a06db899..c52f1a40be2d 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -30,8 +30,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Driver Conformance / IOMMU Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / Sound Driver Conformance},
 \ref{sec:Conformance / Driver Conformance / Memory Driver Conformance},
-\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance} or
-\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance}.
+\ref{sec:Conformance / Driver Conformance / I2C Adapter Driver Conformance},
+\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance} or
+\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance}.
 
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
@@ -54,8 +55,9 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \ref{sec:Conformance / Device Conformance / IOMMU Device Conformance},
 \ref{sec:Conformance / Device Conformance / Sound Device Conformance},
 \ref{sec:Conformance / Device Conformance / Memory Device Conformance},
-\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance} or
-\ref{sec:Conformance / Device Conformance / SCMI Device Conformance}.
+\ref{sec:Conformance / Device Conformance / I2C Adapter Device Conformance},
+\ref{sec:Conformance / Device Conformance / SCMI Device Conformance} or
+\ref{sec:Conformance / Device Conformance / GPIO Device Conformance}.
 
     \item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}.
   \end{itemize}
@@ -301,6 +303,15 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Device Types / SCMI Device / Device Operation / Setting Up eventq Buffers}
 \end{itemize}
 
+\conformance{\subsection}{GPIO Driver Conformance}\label{sec:Conformance / Driver Conformance / GPIO Driver Conformance}
+
+A General Purpose Input/Output (GPIO) driver MUST conform to the following
+normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / GPIO Device / requestq Operation}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -550,6 +561,15 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / SCMI Device / Device Operation / Shared Memory Operation}
 \end{itemize}
 
+\conformance{\subsection}{GPIO Device Conformance}\label{sec:Conformance / Device Conformance / GPIO Device Conformance}
+
+A General Purpose Input/Output (GPIO) device MUST conform to the following
+normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / GPIO Device / requestq Operation}
+\end{itemize}
+
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
 A conformant implementation MUST be either transitional or
 non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index 31b02e1dca0e..0008727a80df 100644
--- a/content.tex
+++ b/content.tex
@@ -6583,6 +6583,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-mem.tex}
 \input{virtio-i2c.tex}
 \input{virtio-scmi.tex}
+\input{virtio-gpio.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/virtio-gpio.tex b/virtio-gpio.tex
new file mode 100644
index 000000000000..67f1e04e8047
--- /dev/null
+++ b/virtio-gpio.tex
@@ -0,0 +1,339 @@
+\section{GPIO Device}\label{sec:Device Types / GPIO Device}
+
+The Virtio GPIO device is a virtual General Purpose Input/Output device that
+supports a variable number of named I/O lines, which can be configured in input
+mode or in output mode with logical level low (0) or high (1).
+
+\subsection{Device ID}\label{sec:Device Types / GPIO Device / Device ID}
+41
+
+\subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
+
+None currently defined.
+
+\subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
+
+GPIO device uses the following configuration structure layout:
+
+\begin{lstlisting}
+struct virtio_gpio_config {
+    le16 ngpio;
+    u8 padding[2];
+    le32 gpio_names_size;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{ngpio}] is the total number of GPIO lines supported by the device.
+
+\item[\field{padding}] has no meaning and is reserved for future use. This
+    MUST be set to zero by the device.
+
+\item[\field{gpio_names_size}] is the size of the gpio-names memory block in
+    bytes, which can be fetched by the driver using the
+    \field{VIRTIO_GPIO_MSG_GET_LINE_NAMES} message. The device MUST set this to
+    0 if it doesn't support names for the GPIO lines.
+\end{description}
+
+
+\subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device Initialization}
+
+\begin{itemize}
+\item The driver MUST configure and initialize the \field{requestq} virtqueue.
+\end{itemize}
+
+\subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation}
+
+The driver uses the \field{requestq} virtqueue to send messages to the device.
+The driver sends a pair of buffers, request (filled by driver) and response (to
+be filled by device later), to the device. The device in turn fills the response
+buffer and sends it back to the driver.
+
+\begin{lstlisting}
+struct virtio_gpio_request {
+    le16 type;
+    le16 gpio;
+    le32 value;
+};
+\end{lstlisting}
+
+All the fields of this structure are set by the driver and read by the device.
+
+\begin{description}
+\item[\field{type}] is the GPIO message type, i.e. one of
+    \field{VIRTIO_GPIO_MSG_*} values.
+
+\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
+    \field{ngpio}.
+
+\item[\field{value}] is a message specific value.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_gpio_response {
+    u8 status;
+    u8 value[N];
+};
+
+/* Possible values of the status field */
+#define VIRTIO_GPIO_STATUS_OK                   0x0
+#define VIRTIO_GPIO_STATUS_ERR                  0x1
+\end{lstlisting}
+
+All the fields of this structure are set by the device and read by the driver.
+
+\begin{description}
+\item[\field{status}] of the GPIO message,
+    \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR}
+    on failure.
+
+\item[\field{value}] is a message specific defined value. The value of N (i.e.
+    size of the \field{value} buffer in bytes) is 1 for most of the messages,
+    except \field{VIRTIO_GPIO_MSG_GET_LINE_NAMES}, where it is set to the value
+    of \field{gpio_names_size} field.
+\end{description}
+
+Following is the list of messages supported by the virtio gpio specification.
+
+\begin{lstlisting}
+/* GPIO message types */
+#define VIRTIO_GPIO_MSG_GET_LINE_NAMES          0x0001
+#define VIRTIO_GPIO_MSG_GET_DIRECTION           0x0002
+#define VIRTIO_GPIO_MSG_SET_DIRECTION           0x0003
+#define VIRTIO_GPIO_MSG_GET_VALUE               0x0004
+#define VIRTIO_GPIO_MSG_SET_VALUE               0x0005
+
+/* GPIO Direction types */
+#define VIRTIO_GPIO_DIRECTION_NONE              0x00
+#define VIRTIO_GPIO_DIRECTION_OUT               0x01
+#define VIRTIO_GPIO_DIRECTION_IN                0x02
+\end{lstlisting}
+
+\subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names}
+
+The driver sends this message to receive a stream of zero-terminated strings,
+where each string represents the name of a GPIO line, present in increasing
+order of the GPIO line numbers. The names of the GPIO lines are optional and may
+be present only for a subset of GPIO lines. If missing, then a zero-byte must be
+present for the GPIO line. If present, the name string must be zero-terminated
+and the name must be unique within a GPIO Device.
+
+These names of the GPIO lines should be most meaningful producer names for the
+system, such as name indicating the usage. For example "MMC-CD", "Red LED Vdd"
+and "ethernet reset" are reasonable line names as they describe what the line is
+used for, while "GPIO0" is not a good name to give to a GPIO line.
+
+Here is an example of how the gpio names memory block may look like for a GPIO
+device with 10 GPIO lines, where line names are provided only for lines 0
+("MMC-CD"), 5 ("Red LED Vdd") and 7 ("ethernet reset").
+
+\begin{lstlisting}
+u8 gpio_names[] = {
+    'M', 'M', 'C', '-', 'C', 'D', 0,
+    0,
+    0,
+    0,
+    0,
+    'R', 'e', 'd', ' ', 'L', 'E', 'D', ' ', 'V', 'd', 'd', 0,
+    0,
+    'E', 't', 'h', 'e', 'r', 'n', 'e', 't', ' ', 'r', 'e', 's', 'e', 't', 0,
+    0,
+    0
+};
+\end{lstlisting}
+
+The device MUST set the \field{gpio_names_size} to a non-zero value if this
+message is supported by the device, else it must be set to zero.
+
+The driver must allocate the \field{value[N]} buffer in the \field{struct
+virtio_gpio_response} for N bytes, where N = \field{gpio_names_size}.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_LINE_NAMES} & 0 & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & gpio-names & \field{gpio_names_size} \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}
+
+The driver sends this message to request the device to return a line's
+configured direction.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+    & \field{VIRTIO_GPIO_STATUS_*} & \field{VIRTIO_GPIO_DIRECTION_*} & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Set Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Set Direction}
+
+The driver sends this message to request the device to configure a line's
+direction. The driver can either set the direction to
+\field{VIRTIO_GPIO_DIRECTION_IN} or \field{VIRTIO_GPIO_DIRECTION_OUT}, which
+also activates the line, or to \field{VIRTIO_GPIO_DIRECTION_NONE}, which
+deactivates the line.
+
+The driver MUST set the value of the GPIO line, using the
+\field{VIRTIO_GPIO_MSG_SET_VALUE} message, before setting the direction of the
+line to output.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_SET_DIRECTION} & line number & \field{VIRTIO_GPIO_DIRECTION_*} \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Get Value}\label{sec:Device Types / GPIO Device / requestq Operation / Get Value}
+
+The driver sends this message to request the device to return current value
+sensed on a line.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_GET_VALUE} & line number & 0 \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 = low, 1 = high & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Device / requestq Operation / Set Value}
+
+The driver sends this message to request the device to set the value of a line.
+The line may already be configured for output or may get configured to output
+later, at which point this output value must be used by the device for the line.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_SET_VALUE} & line number & 0 = low, 1 = high \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
+\subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow}
+
+\begin{itemize}
+\item The driver queues \field{struct virtio_gpio_request} and
+    \field{virtio_gpio_response} buffers to the \field{requestq} virtqueue,
+    after filling all fields of the \field{struct virtio_gpio_request} buffer as
+    defined by the specific message type.
+
+\item The driver notifies the device of the presence of buffers on the
+    \field{requestq} virtqueue.
+
+\item The device, after receiving the message from the driver, processes it and
+    fills all the fields of the \field{struct virtio_gpio_response} buffer
+    (received from the driver). The \field{status} must be set to
+    \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR}
+    on failure.
+
+\item The device puts the buffers back on the \field{requestq} virtqueue and
+    notifies the driver of the same.
+
+\item The driver fetches the buffers and processes the response received in the
+    \field{virtio_gpio_response} buffer.
+
+\item The driver can send multiple messages in parallel for same or different
+    GPIO line.
+\end{itemize}
+
+\drivernormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation}
+
+\begin{itemize}
+\item The driver MUST send messages on the \field{requestq} virtqueue.
+
+\item The driver MUST queue both \field{struct virtio_gpio_request} and
+    \field{virtio_gpio_response} for every message sent to the device.
+
+\item The \field{struct virtio_gpio_request} buffer MUST be filled by the driver
+    and MUST be read-only for the device.
+
+\item The \field{struct virtio_gpio_response} buffer MUST be filled by the
+    device and MUST be writable by the device.
+
+\item The driver MAY send multiple messages for same or different GPIO lines in
+    parallel.
+\end{itemize}
+
+\devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation}
+
+\begin{itemize}
+\item The device MUST set all the fields of the \field{struct
+    virtio_gpio_response} before sending it back to the driver.
+
+\item The device MUST set all the fields of the \field{struct
+    virtio_gpio_config} on receiving a configuration request from the driver.
+
+\item The device MUST set the \field{gpio_names_size} field as zero in the
+    \field{struct virtio_gpio_config}, if it doesn't implement names for
+    individual GPIO lines.
+
+\item The device MUST set the \field{gpio_names_size} field, in the
+    \field{struct virtio_gpio_config}, with the size of gpio-names memory block
+    in bytes, if the device implements names for individual GPIO lines. The
+    strings MUST be zero-terminated and an unique (if available) within the GPIO
+    device.
+
+\item The device MUST process multiple messages, for the same GPIO line,
+    sequentially and respond to them in the order they were received on the
+    virtqueue.
+
+\item The device MAY process messages, for different GPIO lines, out of order
+    and in parallel, and MAY send message's response to the driver out of order.
+
+\item The device MUST discard all state information corresponding to a GPIO
+    line, once the driver has requested to set its direction to
+    \field{VIRTIO_GPIO_DIRECTION_NONE}.
+\end{itemize}
-- 
2.31.1.272.g89b43f80a514


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

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/110
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 conformance.tex |   2 +
 virtio-gpio.tex | 191 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 192 insertions(+), 1 deletion(-)

diff --git a/conformance.tex b/conformance.tex
index c52f1a40be2d..64bcc12d1199 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -310,6 +310,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 
 \begin{itemize}
 \item \ref{drivernormative:Device Types / GPIO Device / requestq Operation}
+\item \ref{drivernormative:Device Types / GPIO Device / eventq Operation}
 \end{itemize}
 
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
@@ -568,6 +569,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 
 \begin{itemize}
 \item \ref{devicenormative:Device Types / GPIO Device / requestq Operation}
+\item \ref{devicenormative:Device Types / GPIO Device / eventq Operation}
 \end{itemize}
 
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
diff --git a/virtio-gpio.tex b/virtio-gpio.tex
index 67f1e04e8047..ffc89b98f34e 100644
--- a/virtio-gpio.tex
+++ b/virtio-gpio.tex
@@ -11,11 +11,17 @@ \subsection{Virtqueues}\label{sec:Device Types / GPIO Device / Virtqueues}
 
 \begin{description}
 \item[0] requestq
+\item[1] eventq
 \end{description}
 
+The \field{eventq} virtqueue is available only if the \field{VIRTIO_GPIO_F_IRQ}
+feature is enabled by the device.
+
 \subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
 
-None currently defined.
+\begin{description}
+\item[VIRTIO_GPIO_F_IRQ (0)] The device supports interrupts on GPIO lines.
+\end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / GPIO Device / Device configuration layout}
 
@@ -46,6 +52,15 @@ \subsection{Device Initialization}\label{sec:Device Types / GPIO Device / Device
 
 \begin{itemize}
 \item The driver MUST configure and initialize the \field{requestq} virtqueue.
+
+\item The driver MUST check the presence of \field{VIRTIO_GPIO_F_IRQ} feature
+    before initiating any IRQ messages.
+
+\item The driver MUST configure and initialize the \field{eventq} virtqueue if
+    the \field{VIRTIO_GPIO_F_IRQ} feature is enabled by the device.
+
+\item If the \field{VIRTIO_GPIO_F_IRQ} feature is supported, then the interrupt
+    for all GPIO lines must be in \field{VIRTIO_GPIO_IRQ_TYPE_NONE} state.
 \end{itemize}
 
 \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / requestq Operation}
@@ -108,11 +123,20 @@ \subsection{Device Operation: requestq}\label{sec:Device Types / GPIO Device / r
 #define VIRTIO_GPIO_MSG_SET_DIRECTION           0x0003
 #define VIRTIO_GPIO_MSG_GET_VALUE               0x0004
 #define VIRTIO_GPIO_MSG_SET_VALUE               0x0005
+#define VIRTIO_GPIO_MSG_SET_IRQ_TYPE            0x0006
 
 /* GPIO Direction types */
 #define VIRTIO_GPIO_DIRECTION_NONE              0x00
 #define VIRTIO_GPIO_DIRECTION_OUT               0x01
 #define VIRTIO_GPIO_DIRECTION_IN                0x02
+
+/* GPIO interrupt types */
+#define VIRTIO_GPIO_IRQ_TYPE_NONE               0x00
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_RISING        0x01
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_FALLING       0x02
+#define VIRTIO_GPIO_IRQ_TYPE_EDGE_BOTH          0x03
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_HIGH         0x04
+#define VIRTIO_GPIO_IRQ_TYPE_LEVEL_LOW          0x08
 \end{lstlisting}
 
 \subsubsection{requestq Operation: Get Line Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Line Names}
@@ -262,6 +286,36 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi
 \hline
 \end{tabularx}
 
+\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type}
+
+The driver sends this message to request the device to set the IRQ trigger type,
+to one of the \field{VIRTIO_GPIO_IRQ_TYPE_*} values, for a line configured for
+input.
+
+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
+enabled by the device.
+
+The device MUST mask the interrupt for a GPIO line, if the trigger type is set
+to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
+status associated with the line. The device MUST unmask the interrupt for any
+other value of the trigger type.
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
+\hline
+& \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} & line number & one of \field{VIRTIO_GPIO_IRQ_TYPE_*} \\
+\hline
+\end{tabularx}
+
+\begin{tabularx}{\textwidth}{ |l||X|X|X| }
+\hline
+\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
+\hline
+& \field{VIRTIO_GPIO_STATUS_*} & 0 & 1 \\
+\hline
+\end{tabularx}
+
 \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO Device / requestq Operation / Message Flow}
 
 \begin{itemize}
@@ -305,6 +359,16 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
 
 \item The driver MAY send multiple messages for same or different GPIO lines in
     parallel.
+
+\item The driver MUST NOT send IRQ messages for a GPIO line configured for
+    output.
+
+\item The driver MUST NOT send IRQ messages if the \field{VIRTIO_GPIO_F_IRQ}
+    feature is not enabled by the device.
+
+\item The driver MUST set the IRQ trigger type to
+    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
+    previously configured for a different trigger type.
 \end{itemize}
 
 \devicenormative{\subsubsection}{requestq Operation}{Device Types / GPIO Device / requestq Operation}
@@ -337,3 +401,128 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
     line, once the driver has requested to set its direction to
     \field{VIRTIO_GPIO_DIRECTION_NONE}.
 \end{itemize}
+
+\subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eventq Operation}
+
+The \field{eventq} virtqueue is used for sending interrupt events from the
+device to the driver. The driver queues a separate pair of buffers,
+interrupt-request (filled by driver) and interrupt-response (to be filled by
+device later), to the \field{eventq} virtqueue for each GPIO line. The device,
+on sensing an interrupt, returns the pair of buffers of the respective GPIO line
+for which the interrupt is sensed.
+
+\begin{lstlisting}
+struct virtio_gpio_irq_request {
+    le16 gpio;
+};
+\end{lstlisting}
+
+This structure is filled by the driver and read by the device.
+
+\begin{description}
+\item[\field{gpio}] is the GPIO line number, i.e. 0 <= \field{gpio} <
+    \field{ngpio}.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_gpio_irq_response {
+    u8 status;
+};
+
+/* Possible values of the interrupt status field */
+#define VIRTIO_GPIO_IRQ_STATUS_INVALID          0x0
+#define VIRTIO_GPIO_IRQ_STATUS_VALID            0x1
+\end{lstlisting}
+
+This structure is filled by the device and read by the driver.
+
+\begin{description}
+\item[\field{status}] of the interrupt event, \field{VIRTIO_GPIO_IRQ_VALID} on
+    valid interrupt and \field{VIRTIO_GPIO_IRQ_INVALID} otherwise on returning
+    the buffer back to the driver without an interrupt.
+\end{description}
+
+\subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}
+
+\begin{itemize}
+\item The driver is requested by a user to enable interrupt for a GPIO and
+    configure it for a particular trigger type.
+
+\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 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 interrupt is fully configured at this point.
+
+\item The device, on sensing an interrupt on a 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_VALID} and returns the pair of buffers to the
+    device.
+
+\item The device notifies the driver of the presence of new buffers on the
+    \field{eventq} virtqueue.
+
+\item If the GPIO line is configured for Level interrupts, the device MUST mask
+    the interrupts for this GPIO line, until the time the buffers are made
+    available again by the driver.
+
+\item If the GPIO line is configured for Edge interrupts, the device MUST latch
+    the latest interrupt received for this GPIO line, until the time the buffers
+    are made available again by the driver. At that point, the device can again
+    return the buffers for the line if an interrupt was received while the
+    device was waiting for the buffers to be made available by the driver.
+
+\item The driver on receiving the notification from the device, processes the
+    interrupt. The driver may try to update the trigger-type of the interrupt
+    for the GPIO line over the \field{requestq} virtqueue.
+
+\item The driver may again queue, same or new, pair of buffers for that GPIO
+    line and notify the device.
+
+\item The driver may send the \field{VIRTIO_GPIO_MSG_SET_IRQ_TYPE} message, with
+    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} trigger type, over the \field{requestq}
+    virtqueue, once it no longer wants to receive the interrupts for a GPIO
+    line.
+
+\item The device must return the unused pair of buffers for that GPIO line, over
+    the \field{eventq} virtqueue, by setting the \field{status} field with
+    \field{VIRTIO_GPIO_IRQ_INVALID}.
+
+\item The driver can then free the associated buffer and remove it from the
+    \field{eventq} virtqueue.
+\end{itemize}
+
+\drivernormative{\subsubsection}{eventq Operation}{Device Types / GPIO Device / eventq Operation}
+
+\begin{itemize}
+\item The driver MUST queue a separate pair of buffers, interrupt-request and
+    interrupt-response, to the \field{eventq} virtqueue for each GPIO line for
+    which it is expecting an interrupt from the device.
+
+\item The driver MUST not queue a pair of buffers for a GPIO line which it is
+    not going to use as interrupt source, otherwise the buffer may never get
+    freed by the device (as no set trigger type request will follow from the
+    driver).
+
+\item The driver MUST NOT add multiple pairs of buffers for the same GPIO line
+    on the \field{eventq} virtqueue. So there can only be one interrupt event
+    for each GPIO line at any point of time.
+
+\item The pair of buffers for any GPIO line can either be owned by the device or
+    the driver at any particular point of time, but not both.
+\end{itemize}
+
+\devicenormative{\subsubsection}{eventq Operation}{Device Types / GPIO Device / eventq Operation}
+
+\begin{itemize}
+\item The device can send an interrupt event for a GPIO line to the driver, only
+    if a buffer for that GPIO line is provided by the driver in the first place
+    on the \field{eventq} virtqueue.
+\end{itemize}
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V7 0/2] virtio: Add specification for virtio-gpio

[Arnd Bergmann]

On Wed, Jul 28, 2021 at 1:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Version history of the specification I sent:
>
> V6 -> V7:
> - Drop Activate/Deactivate requests and merge them with set-direction one.
> - Dropped separate calls to set direction to input or output (with a value),
>   with a single call to set-direction (input, output, or none). Note that the
>   driver needs to call set-value before calling set-direction-out now.
> - Allow multiple messages for a GPIO line to be sent together, they must be
>   processed in order though.
> - The gpio-line names aren't required to be set for all the lines now, it is
>   optional and can be present only for a subset of lines. Provided example as
>   well.
> - Merge irq-set/mask/unmask to a single set-irq-type message.
> - We have only 6 message types now instead of 11 in v6.
> - Mentioned about non-atomic nature of the these messages in commit log for
>   patch 1/2.
> - Improved spec content overall.

Nice update, this all sounds good to me. I'll go through the two patches
in more detail next, but don't expect to find much then.

       Arnd

Re: [PATCH V7 1/2] virtio-gpio: Add the device specification

[Arnd Bergmann]

On Wed, Jul 28, 2021 at 1:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> virtio-gpio is a virtual GPIO controller. It provides a way to flexibly
> communicate with the host GPIO controllers from the guest.
>
> Note that the current implementation doesn't provide atomic APIs for
> GPIO configurations. i.e. the driver (guest) would need to implement
> sleep-able versions of the APIs as the guest will respond asynchronously
> over the virtqueue.
>
> This patch adds the specification for it.
>
> Based on the initial work posted by:
> "Enrico Weigelt, metux IT consult" <lkml@metux.net>.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/110
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

I found one detail that could be improved if you can come up with a better
way, or you just leave it:

> +\begin{lstlisting}
> +struct virtio_gpio_response {
> +    u8 status;
> +    u8 value[N];
> +};
> +
> +/* Possible values of the status field */
> +#define VIRTIO_GPIO_STATUS_OK                   0x0
> +#define VIRTIO_GPIO_STATUS_ERR                  0x1
> +\end{lstlisting}
> +
> +All the fields of this structure are set by the device and read by the driver.
> +
> +\begin{description}
> +\item[\field{status}] of the GPIO message,
> +    \field{VIRTIO_GPIO_STATUS_OK} on success and \field{VIRTIO_GPIO_STATUS_ERR}
> +    on failure.
> +
> +\item[\field{value}] is a message specific defined value. The value of N (i.e.
> +    size of the \field{value} buffer in bytes) is 1 for most of the messages,
> +    except \field{VIRTIO_GPIO_MSG_GET_LINE_NAMES}, where it is set to the value
> +    of \field{gpio_names_size} field.
> +\end{description}

I think the structure layout here is good, but I wonder if the
description would be clearer
if you define two separate structures, with a fixed-length structure for normal
requests, and the variable-length structure for VIRTIO_GPIO_MSG_GET_LINE_NAMES.

         Arnd

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Wed, Jul 28, 2021 at 1:11 PM 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/110
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

This also looks good to me overall, I have a few idea for clarifications below,
but none of them are likely to change the actual behavior.

More importantly though, I would like to see a review by the linux irqchip
maintainers (Marc Zyngier and/or Thomas Gleixner, both added to Cc here)
before this gets merged into the virtio specification. I have a basic
understanding
of the subsystem but probably got something wrong myself.

That review is probably easier once you post the driver code for it but if
Thomas or Marc want to comment on the spec before seeing the driver,
the patch is available at [1], along with the base virtio-gpio spec.

> +\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type}
> +
> +The driver sends this message to request the device to set the IRQ trigger type,
> +to one of the \field{VIRTIO_GPIO_IRQ_TYPE_*} values, for a line configured for
> +input.
> +
> +This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
> +enabled by the device.
> +
> +The device MUST mask the interrupt for a GPIO line, if the trigger type is set
> +to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
> +status associated with the line. The device MUST unmask the interrupt for any
> +other value of the trigger type.

I think the last sentence here is somewhat ambiguous, I'm not sure
what it actually
means to unmask a level triggered line if the driver has not queued an
event request
for that line.

For an edge triggered interrupt, I think it makes sense to unmask the
interrupt at
the device level and latch any incoming events until a request gets
queued on the
eventq.

> +\item The driver MUST set the IRQ trigger type to
> +    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
> +    previously configured for a different trigger type.
>  \end{itemize}

I'm not entirely sure why this "MUST" be done. Can't the device clean up the
irq when the  line is set back to direction=none or the queues are shut down?

> +\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 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.

Ah, I guess that explains the flow. I was expecting the reverse order (first
set-irq-type, then queue event request), but I suppose it works either way.

> +\item The driver on receiving the notification from the device, processes the
> +    interrupt. The driver may try to update the trigger-type of the interrupt
> +    for the GPIO line over the \field{requestq} virtqueue.
> +
> +\item The driver may again queue, same or new, pair of buffers for that GPIO
> +    line and notify the device.

I feel there needs to be another step between these two, as this is where
some of the magic happens that guarantees we don't get too many or
not enough interrupts. How about:

\item In a typical guest operating system kernel, the virtio-gpio
    driver notifies another device driver that is associated with this gpio
    line to process the event.

\item In the case of a level triggered interrupt, the secondary device
    driver must fully process and acknowledge the event at its source
    to return the line to its inactive state.

       Arnd

[1] https://lists.oasis-open.org/archives/virtio-dev/202107/msg00206.html

Re: [PATCH V7 1/2] virtio-gpio: Add the device specification

[Linus Walleij]

Hi Viresh,

overall this looks good, with the minor nits pointed out:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

On Wed, Jul 28, 2021 at 1:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> +\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}
> +
> +The driver sends this message to request the device to return a line's
> +configured direction.
> +
> +\begin{tabularx}{\textwidth}{ |l||X|X|X| }
> +\hline
> +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
> +\hline
> +& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
> +\hline
> +\end{tabularx}
> +
> +\begin{tabularx}{\textwidth}{ |l||X|X|X| }
> +\hline
> +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
> +\hline
> +    & \field{VIRTIO_GPIO_STATUS_*} & \field{VIRTIO_GPIO_DIRECTION_*} & 1 \\
> +\hline
> +\end{tabularx}

Is it clearly defined which value of VIRTIO_GPIO_DIRECTION_[IN|OUT]
mean "input" and which value means "output"? Else add some text
clarifying this.

> +\begin{tabularx}{\textwidth}{ |l||X|X|X| }
> +\hline
> +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
> +\hline
> +& \field{VIRTIO_GPIO_STATUS_*} & 0 = low, 1 = high & 1 \\
> +\hline
> +\end{tabularx}

Here, for example it is clearly defined.

Yours,
Linus Walleij

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Linus Walleij]

On Wed, Jul 28, 2021 at 1:11 PM 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/110
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

From a GPIO point of view this looks good, so FWIW
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

It's best if you get feedback from the irqchip people too.

Yours,
Linus Walleij

Re: [PATCH V7 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On 28-07-21, 13:26, Arnd Bergmann wrote:
> I think the structure layout here is good, but I wonder if the
> description would be clearer
> if you define two separate structures, with a fixed-length structure for normal
> requests, and the variable-length structure for VIRTIO_GPIO_MSG_GET_LINE_NAMES.

Sure.

-- 
viresh

Re: [PATCH V7 1/2] virtio-gpio: Add the device specification

[Viresh Kumar]

On 28-07-21, 14:56, Linus Walleij wrote:
> overall this looks good, with the minor nits pointed out:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks.

> On Wed, Jul 28, 2021 at 1:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > +\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}
> > +
> > +The driver sends this message to request the device to return a line's
> > +configured direction.
> > +
> > +\begin{tabularx}{\textwidth}{ |l||X|X|X| }
> > +\hline
> > +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
> > +\hline
> > +& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
> > +\hline
> > +\end{tabularx}
> > +
> > +\begin{tabularx}{\textwidth}{ |l||X|X|X| }
> > +\hline
> > +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
> > +\hline
> > +    & \field{VIRTIO_GPIO_STATUS_*} & \field{VIRTIO_GPIO_DIRECTION_*} & 1 \\
> > +\hline
> > +\end{tabularx}
> 
> Is it clearly defined which value of VIRTIO_GPIO_DIRECTION_[IN|OUT]
> mean "input" and which value means "output"? Else add some text
> clarifying this.

+/* GPIO Direction types */
+#define VIRTIO_GPIO_DIRECTION_NONE              0x00
+#define VIRTIO_GPIO_DIRECTION_OUT               0x01
+#define VIRTIO_GPIO_DIRECTION_IN                0x02

Not sure if you missed this.

Were you looking for this or some text saying
"VIRTIO_GPIO_DIRECTION_IN means direction-input", which looked self
explanatory to me at least, though I can surely add it anyway.

-- 
viresh

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 28-07-21, 14:05, Arnd Bergmann wrote:
> On Wed, Jul 28, 2021 at 1:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
 
> This also looks good to me overall, I have a few idea for clarifications below,
> but none of them are likely to change the actual behavior.
> 
> More importantly though, I would like to see a review by the linux irqchip
> maintainers (Marc Zyngier and/or Thomas Gleixner, both added to Cc here)
> before this gets merged into the virtio specification. I have a basic
> understanding
> of the subsystem but probably got something wrong myself.

Thanks for adding them.

> That review is probably easier once you post the driver code for it but if
> Thomas or Marc want to comment on the spec before seeing the driver,
> the patch is available at [1], along with the base virtio-gpio spec.
> 
> > +\subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type}
> > +
> > +The driver sends this message to request the device to set the IRQ trigger type,
> > +to one of the \field{VIRTIO_GPIO_IRQ_TYPE_*} values, for a line configured for
> > +input.
> > +
> > +This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
> > +enabled by the device.
> > +
> > +The device MUST mask the interrupt for a GPIO line, if the trigger type is set
> > +to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
> > +status associated with the line. The device MUST unmask the interrupt for any
> > +other value of the trigger type.
> 
> I think the last sentence here is somewhat ambiguous, I'm not sure
> what it actually
> means to unmask a level triggered line if the driver has not queued an
> event request
> for that line.

(For everyone to follow, device == host, driver == guest)

There are two different situation where the interrupt can come and I
believe the behavior should be different.

- The driver has requested for the trigger-type to be set and
  unmasking of the interrupt. The device MUST unmask the interrupt
  even if the buffer isn't queued over eventq, and so it must latch
  the interrupt value and report it as soon as the buffer is made
  available.

  This behavior should be same for both edge or level interrupts.

- Once the interrupt is reported by the device, by returning the
  buffer over the eventq, the _level_ interrupt must be masked by the
  device until the time the buffer is re-queued by the driver, else it
  will end up reporting the same interrupt again as the line would
  have stayed at the same level until the interrupt handler had a
  chance to run and service the interrupt.

  This is not the case for edge interrupt of course, where we need to
  latch value as that counts for a new interrupt.

> For an edge triggered interrupt, I think it makes sense to unmask the
> interrupt at
> the device level and latch any incoming events until a request gets
> queued on the
> eventq.

I think the same holds true for level interrupts too.

> > +\item The driver MUST set the IRQ trigger type to
> > +    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
> > +    previously configured for a different trigger type.
> >  \end{itemize}
> 
> I'm not entirely sure why this "MUST" be done. Can't the device clean up the
> irq when the  line is set back to direction=none or the queues are shut down?

Yes we can infer that from direction=none as well, but even then the
device needs to return the unused buffers over the eventq. And I
thought it maybe better to explicitly mark this to be done and not mix
with other operations like direction=none.

For the queues being shut case, I see that as a crash or hard stop.
The device may want to handle things in that case by its own way, but
the spec must try to avoid that case entirely and not encourage people
to do things that way :)

> > +\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 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.
> 
> Ah, I guess that explains the flow. I was expecting the reverse order (first
> set-irq-type, then queue event request), but I suppose it works either way.

Whatever the flow is here at the driver, the device may observe these
differently as these are two different virtqueues here. The device
should always provide the buffer before requesting the device to
enable to interrupt IMO.

> > +\item The driver on receiving the notification from the device, processes the
> > +    interrupt. The driver may try to update the trigger-type of the interrupt
> > +    for the GPIO line over the \field{requestq} virtqueue.
> > +
> > +\item The driver may again queue, same or new, pair of buffers for that GPIO
> > +    line and notify the device.
> 
> I feel there needs to be another step between these two, as this is where
> some of the magic happens that guarantees we don't get too many or
> not enough interrupts. How about:
> 
> \item In a typical guest operating system kernel, the virtio-gpio
>     driver notifies another device driver that is associated with this gpio
>     line to process the event.
> 
> \item In the case of a level triggered interrupt, the secondary device
>     driver must fully process and acknowledge the event at its source
>     to return the line to its inactive state.

Sure, they can be added. Will do.

-- 
viresh

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Thu, Jul 29, 2021 at 7:40 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 28-07-21, 14:05, Arnd Bergmann wrote:
>
> > > +The device MUST mask the interrupt for a GPIO line, if the trigger type is set
> > > +to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
> > > +status associated with the line. The device MUST unmask the interrupt for any
> > > +other value of the trigger type.
> >
> > I think the last sentence here is somewhat ambiguous, I'm not sure
> > what it actually means to unmask a level triggered line if the driver
> > has not queued an event request for that line.
>
> (For everyone to follow, device == host, driver == guest)
>
> There are two different situation where the interrupt can come and I
> believe the behavior should be different.
>
> - The driver has requested for the trigger-type to be set and
>   unmasking of the interrupt. The device MUST unmask the interrupt
>   even if the buffer isn't queued over eventq, and so it must latch
>   the interrupt value and report it as soon as the buffer is made
>   available.
>
>   This behavior should be same for both edge or level interrupts.

I think they are fundamentally different here, see below:

> - Once the interrupt is reported by the device, by returning the
>   buffer over the eventq, the _level_ interrupt must be masked by the
>   device until the time the buffer is re-queued by the driver, else it
>   will end up reporting the same interrupt again as the line would
>   have stayed at the same level until the interrupt handler had a
>   chance to run and service the interrupt.
>
>   This is not the case for edge interrupt of course, where we need to
>   latch value as that counts for a new interrupt.
>
> > For an edge triggered interrupt, I think it makes sense to unmask the
> > interrupt at the device level and latch any incoming events until a
> > request gets queued on the eventq.
>
> I think the same holds true for level interrupts too.

Wouldn't that cause the virtio-gpio device to send a second spurious
interrupt reply for every hardware event?

During probe time, the level may be active if the device connected to the
gpio has latched an event itself. When that device gets initialized, its
driver would clear the status and bring the line to 'inactive' state before
enabling interrupts. If the virtio-gpio device latches the state of the line
having been active in the past, you get a first spurious reply. The driver
will know how to deal with it, but it can be avoided.

For the later operation, the gpio client device signals an event by
making the line active again, at this point the virtio-gpio device (in
the host) gets signaled and queues the reply. If it then looks at the
line, it is still active, so with your model it would have to latch another
event. The gpio slave driver in the guest then processes the event
and deactivates the line in order to not get woken up again until
something new happens.

If the virtio-gpio device has already latched another event, this
logic fails and you get woken up again directly at the end-of-interrupt
(i.e. requeueing the eventq message) even if the line is currently
inactive.

> > > +\item The driver MUST set the IRQ trigger type to
> > > +    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
> > > +    previously configured for a different trigger type.
> > >  \end{itemize}
> >
> > I'm not entirely sure why this "MUST" be done. Can't the device clean up the
> > irq when the  line is set back to direction=none or the queues are shut down?
>
> Yes we can infer that from direction=none as well, but even then the
> device needs to return the unused buffers over the eventq. And I
> thought it maybe better to explicitly mark this to be done and not mix
> with other operations like direction=none.
>
> For the queues being shut case, I see that as a crash or hard stop.
> The device may want to handle things in that case by its own way, but
> the spec must try to avoid that case entirely and not encourage people
> to do things that way :)

This still sounds like a "SHOULD" instead of "MUST" to me though,
but I don't remember the exact definitions of these.

> > > +\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 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.
> >
> > Ah, I guess that explains the flow. I was expecting the reverse order (first
> > set-irq-type, then queue event request), but I suppose it works either way.
>
> Whatever the flow is here at the driver, the device may observe these
> differently as these are two different virtqueues here. The device
> should always provide the buffer before requesting the device to
> enable to interrupt IMO.

Fair enough, I don't care too much about this one, I was just thinking we'd
expect it to do it the other way round since that is more consistent with the
continued operation after the interrupts remain enabled and we go back and
forth requeing new eventq requests without turning them off again.

       Arnd

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 29-07-21, 09:45, Arnd Bergmann wrote:
> Wouldn't that cause the virtio-gpio device to send a second spurious
> interrupt reply for every hardware event?
> 
> During probe time, the level may be active if the device connected to the
> gpio has latched an event itself.

At this time the trigger type should be set to NONE by default and no
latching of the line's value must be performed by the device. The
device should look for interrupts only after the interrupt is
explicitly unmasked (irq-type set to non-NONE value) by the driver.

> When that device gets initialized, its
> driver would clear the status and bring the line to 'inactive' state before
> enabling interrupts. If the virtio-gpio device latches the state of the line
> having been active in the past, you get a first spurious reply.

And so this shouldn't happen.

> The driver
> will know how to deal with it, but it can be avoided.
> 
> For the later operation, the gpio client device signals an event by
> making the line active again, at this point the virtio-gpio device (in
> the host) gets signaled and queues the reply. If it then looks at the
> line, it is still active, so with your model it would have to latch another
> event.

No, since interrupt-handling is being performed at this time, the
device MUST NOT latch a value for trigger type _level_. This is very
explicitly stated in the current version of specs.

> The gpio slave driver in the guest then processes the event
> and deactivates the line in order to not get woken up again until
> something new happens.
> 
> If the virtio-gpio device has already latched another event, this
> logic fails and you get woken up again directly at the end-of-interrupt
> (i.e. requeueing the eventq message) even if the line is currently
> inactive.

This shouldn't happen, but I now find the reliance on the request
buffer being available or not a bit confusing with respect to masking
or unmasking of the interrupt. More on this later..

> > > > +\item The driver MUST set the IRQ trigger type to
> > > > +    \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
> > > > +    previously configured for a different trigger type.
> > > >  \end{itemize}
> > >
> > > I'm not entirely sure why this "MUST" be done. Can't the device clean up the
> > > irq when the  line is set back to direction=none or the queues are shut down?
> >
> > Yes we can infer that from direction=none as well, but even then the
> > device needs to return the unused buffers over the eventq. And I
> > thought it maybe better to explicitly mark this to be done and not mix
> > with other operations like direction=none.
> >
> > For the queues being shut case, I see that as a crash or hard stop.
> > The device may want to handle things in that case by its own way, but
> > the spec must try to avoid that case entirely and not encourage people
> > to do things that way :)
> 
> This still sounds like a "SHOULD" instead of "MUST" to me though,
> but I don't remember the exact definitions of these.

Sure, I can make that a SHOULD :)

> Fair enough, I don't care too much about this one, I was just thinking we'd
> expect it to do it the other way round since that is more consistent with the
> continued operation after the interrupts remain enabled and we go back and
> forth requeing new eventq requests without turning them off again.

Coming back to the confusing part here. I think we should make the
interrupt controller (device) dumb here and leave all intelligence at
the users (driver).

What about defining the sequence of events like this:

For edge interrupt:

- driver sets trigger type to EDGE (falling/rising).
- device enables the interrupt line, latches a value only after this
  point.
- driver queues a buffer over eventq (here or before setting trigger type).
- Once buffer is available over eventq, device returns it back and
  latches the next interrupt as soon as it happens.
- driver on receiving the buffer, re-queues the buffer and handles the
  interrupt by calling client's handler. Though queuing the buffer it
  after handling the request won't make a huge difference (and may be
  better for simpler driver code) as the next interrupt is latched
  anyway by the device and can be processed only after this one is
  done at the driver (some locking will be there to make sure only
  one interrupt gets processed per line at a time).
- Note that in this sequence we don't mask/unmask the interrupt at
  all. That's what kernel will also do in handle_edge_irq() if I
  understood it correctly.


For level interrupt:

- driver sets trigger type to LEVEL (high/low).
- device enables the interrupt line, latches a value only after this
  point.
- driver queues a buffer over eventq (here or before setting trigger type).
- Once buffer is available over eventq, device returns it back and
  latches the next interrupt as soon as it happens. Its okay.
- The driver on receiving the buffer, passes control to irq-core which
  masks the interrupt by calling driver to set the trigger type to
  NONE.
- The device masks the interrupt and discards any latched value.
- The irq-core handles the interrupt by calling client's handler.
- The irq-core unmasks the interrupt by calling the driver to set the
  trigger type to LEVEL.
- The device unmasks the interrupt and latches any new value from here
  on.
- The driver queues the buffer again.

How does that sound ? This removes any logical dependency between
availability of the buffer and the interrupt being masked or unmasked.

One thing I am not very clear about is where do
.irq_bus_lock()/unlock() come in this picture. Are they called during
interrupt handling time too or just during setting of the interrupt
(i.e. request_irq()).

-- 
viresh

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Thu, Jul 29, 2021 at 10:45 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > Fair enough, I don't care too much about this one, I was just thinking we'd
> > expect it to do it the other way round since that is more consistent with the
> > continued operation after the interrupts remain enabled and we go back and
> > forth requeing new eventq requests without turning them off again.
>
> Coming back to the confusing part here. I think we should make the
> interrupt controller (device) dumb here and leave all intelligence at
> the users (driver).
>
> What about defining the sequence of events like this:
>
> For edge interrupt:
>
> - driver sets trigger type to EDGE (falling/rising).
> - device enables the interrupt line, latches a value only after this
>   point.
> - driver queues a buffer over eventq (here or before setting trigger type).
> - Once buffer is available over eventq, device returns it back and
>   latches the next interrupt as soon as it happens.
> - driver on receiving the buffer, re-queues the buffer and handles the
>   interrupt by calling client's handler. Though queuing the buffer it
>   after handling the request won't make a huge difference (and may be
>   better for simpler driver code) as the next interrupt is latched
>   anyway by the device and can be processed only after this one is
>   done at the driver (some locking will be there to make sure only
>   one interrupt gets processed per line at a time).
> - Note that in this sequence we don't mask/unmask the interrupt at
>   all. That's what kernel will also do in handle_edge_irq() if I
>   understood it correctly.

Maybe this is where we misunderstood each other: I was thinking
of it in terms of handle_fasteoi_irq() control flow, not handle_edge_irq().

For the edge interrupt, it doesn't make much of a difference, but
for the level case, I think we really want handle_fasteoi_irq(),
and as I understand it, the driver side should work the same way
here.

> For level interrupt:
>
> - driver sets trigger type to LEVEL (high/low).
> - device enables the interrupt line, latches a value only after this
>   point.
> - driver queues a buffer over eventq (here or before setting trigger type).
> - Once buffer is available over eventq, device returns it back and
>   latches the next interrupt as soon as it happens. Its okay.
> - The driver on receiving the buffer, passes control to irq-core which
>   masks the interrupt by calling driver to set the trigger type to
>   NONE.
> - The device masks the interrupt and discards any latched value.
> - The irq-core handles the interrupt by calling client's handler.
> - The irq-core unmasks the interrupt by calling the driver to set the
>   trigger type to LEVEL.
> - The device unmasks the interrupt and latches any new value from here
>   on.
> - The driver queues the buffer again.
>
> How does that sound ? This removes any logical dependency between
> availability of the buffer and the interrupt being masked or unmasked.

I think having to mask/unmask the level interrupt explicitly adds
way too much complexity here, in particular when these are both
blocking operations. With the fasteoi flow, the irq core never has
to mask it, but only atomically requeue the buffer at the end.

       Arnd

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 28-07-21, 14:05, Arnd Bergmann wrote:
> I think the last sentence here is somewhat ambiguous, I'm not sure
> what it actually
> means to unmask a level triggered line if the driver has not queued an
> event request
> for that line.

I think I now understand what you meant by this, I wasn't thinking of
fasteoi earlier :)

What about following diff over this patch:

diff --git a/virtio-gpio.tex b/virtio-gpio.tex
index ffc89b98f34e..0085708151d7 100644
--- a/virtio-gpio.tex
+++ b/virtio-gpio.tex
@@ -288,17 +288,31 @@ \subsubsection{requestq Operation: Set Value}\label{sec:Device Types / GPIO Devi

 \subsubsection{requestq Operation: Set IRQ Type}\label{sec:Device Types / GPIO Device / requestq Operation / Set IRQ Type}

+This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is enabled
+by the device.
+
 The driver sends this message to request the device to set the IRQ trigger type,
 to one of the \field{VIRTIO_GPIO_IRQ_TYPE_*} values, for a line configured for
 input.

-This request is only allowed if the \field{VIRTIO_GPIO_F_IRQ} feature is
-enabled by the device.
+The device notifies the driver of an interrupt event using the per GPIO line
+buffers provided by the driver on the \field{eventq} virtqueue.

 The device MUST mask the interrupt for a GPIO line, if the trigger type is set
 to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
-status associated with the line. The device MUST unmask the interrupt for any
-other value of the trigger type.
+event associated with the line. The device MUST NOT notify the driver of an
+interrupt event if the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}
+for the GPIO line.
+
+The device MUST unmask the interrupt for any other value of the trigger type and
+notify the driver of an interrupt event once the corresponding buffers are
+available.
+
+For edge trigger type, the device MUST latch the interrupt event from this point
+onward and report it to the driver as soon as a buffers are available.
+
+For level trigger type, the device only reports an interrupt if the
+corresponding interrupt level was sensed when the buffers are received.

 \begin{tabularx}{\textwidth}{ |l||X|X|X| }
 \hline
@@ -366,7 +380,7 @@ \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D
 \item The driver MUST NOT send IRQ messages if the \field{VIRTIO_GPIO_F_IRQ}
     feature is not enabled by the device.

-\item The driver MUST set the IRQ trigger type to
+\item The driver SHOULD set the IRQ trigger type to
     \field{VIRTIO_GPIO_IRQ_TYPE_NONE} once it is done using the GPIO line,
     previously configured for a different trigger type.
 \end{itemize}
@@ -445,19 +459,19 @@ \subsection{Device Operation: eventq}\label{sec:Device Types / GPIO Device / eve
 \subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Device / eventq Operation / Message Flow}

 \begin{itemize}
-\item The driver is requested by a user to enable interrupt for a GPIO and
+\item The driver is requested by a client to enable interrupt for a GPIO and
     configure it for 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 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 interrupt is fully configured at this point.

 \item The device, on sensing an interrupt on a GPIO line, finds the matching
@@ -469,19 +483,29 @@ \subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Dev
 \item The device notifies the driver of the presence of new buffers on the
     \field{eventq} virtqueue.

-\item If the GPIO line is configured for Level interrupts, the device MUST mask
-    the interrupts for this GPIO line, until the time the buffers are made
-    available again by the driver.
+\item If the GPIO line is configured for level interrupts, the device MUST
+    ignore the interrupts for this GPIO line, until the time the buffers are
+    made available again by the driver. Once the buffers are available again,
+    and the interrupt on the line is still active, the device can again notify
+    the driver of an interrupt event.

-\item If the GPIO line is configured for Edge interrupts, the device MUST latch
+\item If the GPIO line is configured for edge interrupts, the device MUST latch
     the latest interrupt received for this GPIO line, until the time the buffers
     are made available again by the driver. At that point, the device can again
-    return the buffers for the line if an interrupt was received while the
-    device was waiting for the buffers to be made available by the driver.
+    notify the driver if an interrupt was received while the device was waiting
+    for the buffers to be made available by the driver.

 \item The driver on receiving the notification from the device, processes the
     interrupt. The driver may try to update the trigger-type of the interrupt
-    for the GPIO line over the \field{requestq} virtqueue.
+    for the GPIO line over the \field{requestq} virtqueue at this point.
+
+\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.

Re: [PATCH V7 1/2] virtio-gpio: Add the device specification

[Linus Walleij]

On Thu, Jul 29, 2021 at 5:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 28-07-21, 14:56, Linus Walleij wrote:
> > overall this looks good, with the minor nits pointed out:
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Thanks.
>
> > On Wed, Jul 28, 2021 at 1:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > +\subsubsection{requestq Operation: Get Direction}\label{sec:Device Types / GPIO Device / requestq Operation / Get Direction}
> > > +
> > > +The driver sends this message to request the device to return a line's
> > > +configured direction.
> > > +
> > > +\begin{tabularx}{\textwidth}{ |l||X|X|X| }
> > > +\hline
> > > +\textbf{Request} & \field{type} & \field{gpio} & \field{value} \\
> > > +\hline
> > > +& \field{VIRTIO_GPIO_MSG_GET_DIRECTION} & line number & 0 \\
> > > +\hline
> > > +\end{tabularx}
> > > +
> > > +\begin{tabularx}{\textwidth}{ |l||X|X|X| }
> > > +\hline
> > > +\textbf{Response} & \field{status} & \field{value[N]} & \field{Where N is} \\
> > > +\hline
> > > +    & \field{VIRTIO_GPIO_STATUS_*} & \field{VIRTIO_GPIO_DIRECTION_*} & 1 \\
> > > +\hline
> > > +\end{tabularx}
> >
> > Is it clearly defined which value of VIRTIO_GPIO_DIRECTION_[IN|OUT]
> > mean "input" and which value means "output"? Else add some text
> > clarifying this.
>
> +/* GPIO Direction types */
> +#define VIRTIO_GPIO_DIRECTION_NONE              0x00
> +#define VIRTIO_GPIO_DIRECTION_OUT               0x01
> +#define VIRTIO_GPIO_DIRECTION_IN                0x02
>
> Not sure if you missed this.
>
> Were you looking for this or some text saying
> "VIRTIO_GPIO_DIRECTION_IN means direction-input", which looked self
> explanatory to me at least, though I can surely add it anyway.

I just mean that the #define for the directions is elsewhere so maybe
the actual numeric value should be mentioned here as well.

Nevermind, it's such a small nit.

Yours,
Linus Walleij

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Arnd Bergmann]

On Thu, Jul 29, 2021 at 12:59 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 28-07-21, 14:05, Arnd Bergmann wrote:
> > I think the last sentence here is somewhat ambiguous, I'm not sure
> > what it actually means to unmask a level triggered line if the driver has not
> > queued an event request for that line.
>
> I think I now understand what you meant by this, I wasn't thinking of
> fasteoi earlier :)
>
> What about following diff over this patch:
>
>  The device MUST mask the interrupt for a GPIO line, if the trigger type is set
>  to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
> -status associated with the line. The device MUST unmask the interrupt for any
> -other value of the trigger type.
> +event associated with the line. The device MUST NOT notify the driver of an
> +interrupt event if the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}
> +for the GPIO line.

I actually thought that setting VIRTIO_GPIO_IRQ_TYPE_NONE would
return the buffer to the driver, with status indicating that no event
has happened.

> +For level trigger type, the device only reports an interrupt if the
> +corresponding interrupt level was sensed when the buffers are received.

I think you mean the right thing, but this part is very misleading: we
clearly want
to be notified if the line was active when the buffer is received and also as
soon as it becomes active afterwards, just not when it was active in the past.

> +\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 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 interrupt is fully configured at this point.
>
>  \item The device, on sensing an interrupt on a GPIO line, finds the matching
> @@ -469,19 +483,29 @@ \subsubsection{eventq Operation: Message Flow}\label{sec:Device Types / GPIO Dev
>  \item The device notifies the driver of the presence of new buffers on the
>      \field{eventq} virtqueue.
>
> -\item If the GPIO line is configured for Level interrupts, the device MUST mask
> -    the interrupts for this GPIO line, until the time the buffers are made
> -    available again by the driver.
> +\item If the GPIO line is configured for level interrupts, the device MUST
> +    ignore the interrupts for this GPIO line, until the time the buffers are

Maybe change

s/the interrupts for/an active interrupt signaled on/

> +    made available again by the driver. Once the buffers are available again,
> +    and the interrupt on the line is still active, the device can again notify
> +    the driver of an interrupt event.
>
> -\item If the GPIO line is configured for Edge interrupts, the device MUST latch
> +\item If the GPIO line is configured for edge interrupts, the device MUST latch
>      the latest interrupt received for this GPIO line, until the time the buffers
>      are made available again by the driver. At that point, the device can again
> -    return the buffers for the line if an interrupt was received while the
> -    device was waiting for the buffers to be made available by the driver.
> +    notify the driver if an interrupt was received while the device was waiting
> +    for the buffers to be made available by the driver.

I think this needs to be

s/the device can/the device MUST/

signaling the interrupt is not optional ;-)

The rest looks good to me now, but it's a little hard to read in diff form.
Are you able to post the driver soon? I think our understanding of how this
should work has converged enough that I'd review that next.

       Arnd

Re: [PATCH V7 2/2] virtio-gpio: Add support for interrupts

[Viresh Kumar]

On 29-07-21, 13:39, Arnd Bergmann wrote:
> On Thu, Jul 29, 2021 at 12:59 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 28-07-21, 14:05, Arnd Bergmann wrote:
> > > I think the last sentence here is somewhat ambiguous, I'm not sure
> > > what it actually means to unmask a level triggered line if the driver has not
> > > queued an event request for that line.
> >
> > I think I now understand what you meant by this, I wasn't thinking of
> > fasteoi earlier :)
> >
> > What about following diff over this patch:
> >
> >  The device MUST mask the interrupt for a GPIO line, if the trigger type is set
> >  to \field{VIRTIO_GPIO_IRQ_TYPE_NONE} and also discard any latched interrupt
> > -status associated with the line. The device MUST unmask the interrupt for any
> > -other value of the trigger type.
> > +event associated with the line. The device MUST NOT notify the driver of an
> > +interrupt event if the trigger type is set to \field{VIRTIO_GPIO_IRQ_TYPE_NONE}
> > +for the GPIO line.
> 
> I actually thought that setting VIRTIO_GPIO_IRQ_TYPE_NONE would
> return the buffer to the driver, with status indicating that no event
> has happened.

Yeah, that is actually the case. What I meant here was that an
interrupt must not be reported by the device in this case, but yes the
buffer needs to be returned. Will clarify on that a bit here.

> > +For level trigger type, the device only reports an interrupt if the
> > +corresponding interrupt level was sensed when the buffers are received.
> 
> I think you mean the right thing, but this part is very misleading: we
> clearly want
> to be notified if the line was active when the buffer is received and also as
> soon as it becomes active afterwards, just not when it was active in the past.

Right.

> The rest looks good to me now, but it's a little hard to read in diff form.
> Are you able to post the driver soon? I think our understanding of how this
> should work has converged enough that I'd review that next.

I should be able to post that somewhere next week, making changes
according to spec changes.

-- 
viresh