virtio: Add specification for virtio-gpio

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

This patch adds the specification for it.

Based on the initial work posted by:
"Enrico Weigelt, metux IT consult" <[email protected]>.

Fixes https://github.com/oasis-tcs/virtio-spec/issues/110

Signed-off-by Viresh Kumar <[email protected]>

---

+\subsubsection{requestq Operation: Get Names}\label{sec:Device Types / GPIO Device / requestq Operation / Get Names}

virtio-gpio.tex new file +6 -0

+
+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 GPIO line names must be unique within a GPIO
+Device and must not be an empty string.
+

+\item The device MAY process messages out of order and in parallel and MAY send
+ message's response to the driver out of order.

I would be more comfortable with leaving out this field, and making the name lookup fixed-length and per line.

Making this a per-line request also has the advantage that the device can have names associated with some of the lines, but leave some other lines unnamed, which I think is something that happens in practice.

Not sure what a good upper limit for the length of the name is, but making this fixed-length would seem simpler to me.

virtio-gpio.tex line 153

Quoted 4 lines

+\subsubsection{requestq Operation: Activate}\label{sec:Device Types / GPIO Device / requestq Operation / Activate}

virtio-gpio.tex new file +3 -0

+
+The driver sends this message to notify the device that one of its lines has
+been assigned for use.

Maybe expand this description a little more according to our previous discussion.
In particular, I would expect this to describe

- what the controller hardware is expected to do as a result
- which of the other calls are allowed on lines that have been requested or have not been requested. This information could be duplicated in the respective descriptions. In particular, it is not obvious whether you should set the direction before or after 'request', and I assume only one of the two possibilities is allowed.

virtio-gpio.tex line 345

Quoted 3 lines

+\item The driver MUST NOT send another message, of any type, for a GPIO line

virtio-gpio.tex new file +2 -0

+    before receiving a response from the device for the previously sent message
+    for the same GPIO line.

I have a question here, not sure if this needs to be answered in the spec: why can't the driver send activate, set-direction and set-value all at once for a line?
Clearly if one request fails, the later ones would fail as well in this example, but
I don't see this as a problem for either side.

Quoted 2 lines

+\item The device MAY process messages out of order and in parallel and MAY send
+ message's response to the driver out of order.

Here I have the opposite question: what is the benefit of allowing the requests to be processed out of order? Should these not all be instantaneous, and easier to handle by making the request queue serialized?

Quoted 2 lines · Arnd Bergmann

I would be more comfortable with leaving out this field, and making the name lookup fixed-length and per line.

I can leave it from configuration, sure.

Doing it per-line seems to be too much overhead, since we are going to call this for every line anyway (at least in Linux implementation). I haven't tried it, but I somehow feel that probe() will take a significant time to finish with that, lets say for 256 GPIO lines.

Quoted 7 lines

Making this a per-line request also has the advantage that the device can have names associated with some of the lines, but leave some other lines unnamed, which I think is something that happens in practice.

Not sure what a good upper limit for the length of the name is, but making this fixed-length would seem simpler to me.

Or allocate 20-25 bytes for each string and allocate a buffer of size

25 * ngpios

The device can write 0 (null) for some of the GPIOs, where it doesn't want to provide a name, or set it to a null terminated string with length <= 25.

Quoted 4 lines

Quoted 3 lines

+\item The driver MUST NOT send another message, of any type, for a GPIO line

virtio-gpio.tex new file +2 -0

+    before receiving a response from the device for the previously sent message
+    for the same GPIO line.

Quoted 6 lines

I have a question here, not sure if this needs to be answered in the spec: why can't the driver send activate, set-direction and set-value all at once for a line?
Clearly if one request fails, the later ones would fail as well in this example, but
I don't see this as a problem for either side.

The driver can already do set-direction and value at once.

Merging activate along with that would mean, that the host needs to keep a reference count at its end to activate the line on the first call to set-value, but not on others.

We would still need to keep the deactivate request, as the host wouldn't know when the guest is done using a line. And for that reason it looked better to have an explicit call to activate here.

Quoted 6 lines

Quoted 2 lines

+\item The device MAY process messages out of order and in parallel and MAY send
+ message's response to the driver out of order.

Here I have the opposite question: what is the benefit of allowing the requests to be processed out of order? Should these not all be instantaneous, and easier to handle by making the request queue serialized?

Lets say the guest sends two requests, almost at the same time to the host and they are queued on the vq in this order only:

- set direction-out-with-value-1 for line 5.
- set direction-in for line 11.

Now, if the host is able to process them in parallel, then the second request for line 11 may finish before the requests for line 5 (as it has lesser amount of work to do).

I don't see why the host should wait for the first request to finish before queuing up the buffers for the second request, while it has already finished.

To be most efficient, the queue should be thought of as a series of buffers which are present in any order, based on what finishes first. It doesn't increase the complexity on any side to be honest. The code remains straight forward with it.

Quoted 4 lines · Viresh Kumar

Doing it per-line seems to be too much overhead, since we are going to call this for every line anyway (at least in Linux implementation). I haven't tried it, but I somehow feel that probe() will take a significant time to finish with that, lets say for 256 GPIO lines.

My guess is that this would not cause any notable overhead, but we will know for sure if you can measure it.

Quoted 7 lines

Or allocate 20-25 bytes for each string and allocate a buffer of size

25 * ngpios

The device can write 0 (null) for some of the GPIOs, where it doesn't want to provide a name, or set it to a null terminated string with length <= 25.

I was thinking of a name limit in the context of doing the operation per line.

If you keep the list of nul-terminated strings, then I think I would also not limit the length per line.

Quoted 10 lines

Quoted 6 lines

I have a question here, not sure if this needs to be answered in the spec: why can't the driver send activate, set-direction and set-value all at once for a line?
Clearly if one request fails, the later ones would fail as well in this example, but
I don't see this as a problem for either side.

Merging activate along with that would mean, that the host needs to keep a reference count at its end to activate the line on the first call to set-value, but not on others.

My question here was about something else, to rephrase: Can't we allow the driver to queue multiple requests for one line and then have the device process them in order (regardless of whether it processes requests for other lines in parallel). The sequence I had in mind here was

driver:
1. queue enable
2. queue set-direction
3. queue set-value
4. notify device
device:
5. process enable
6. queue enable-response
7. process set-direction
8. queue set-direction response
9. process set-value
10. queue set-value response
11. notify driver
driver:
12. mark all requests as done

Quoted 3 lines

We would still need to keep the deactivate request, as the host wouldn't know when the guest is done using a line. And for that reason it looked better to have an explicit call to activate here.

Folding the activate/deactivate is also an interesting option, as discussed before. I don't think we actually need to do reference counting, just making the direction a tristate. I think this would work fine here:

- default direction is VIRTIO_GPIO_MSG_SET_DIRECTION_NONE"
- as long as the direction is not set, nothing else is allowed on the line
- changing the direction from "none" to "in" or "out" implies "activate"
- changing the direction from "in" or "out" back to "none" implies "deactivate"

This is similar to folding the irq type into the direction command, and I would slightly prefer it that way, but I'd like to hear what others think about it.

Quoted 18 lines

Quoted 3 lines

Here I have the opposite question: what is the benefit of allowing the requests to be processed out of order? Should these not all be instantaneous, and easier to handle by making the request queue serialized?

Lets say the guest sends two requests, almost at the same time to the host and they are queued on the vq in this order only:

- set direction-out-with-value-1 for line 5.
- set direction-in for line 11.

Now, if the host is able to process them in parallel, then the second request for line 11 may finish before the requests for line 5 (as it has lesser amount of work to do).

I don't see why the host should wait for the first request to finish before queuing up the buffers for the second request, while it has already finished.

Quoted 4 lines

To be most efficient, the queue should be thought of as a series of buffers which are present in any order, based on what finishes first. It doesn't increase the complexity on any side to be honest. The code remains straight forward with it.

I expected the host side to be atomic here. Under which circumstance would the 'set direction out with value' call in your example block?

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · Viresh Kumar

Doing it per-line seems to be too much overhead, since we are going to call this for every line anyway (at least in Linux implementation). I haven't tried it, but I somehow feel that probe() will take a significant time to finish with that, lets say for 256 GPIO lines.

Quoted 2 lines

My guess is that this would not cause any notable overhead, but we will know for sure if you can measure it.

I will surely try it to get the number out, but sending a request 256 times instead of just once looks inefficient from the face of it :)

Quoted 2 lines

If you keep the list of nul-terminated strings, then I think I would also not limit the length per line.

The problem at hand is knowing the (maximum) size of the buffer to pre-allocate at the driver. And here are the options:

- Keep gpio_names_size in the config structure (like this commit, which is kind of fine since we won't need this multiple times).

- Add a new message type for that, i.e. getting size of the gpio_names_block, but I don't like it since this will be used only once.

- Fix size for each GPIO pin's name to X characters and the driver allocates a buffer of size X * ngpio. This provides good boundaries between line names, more straightforward, less confusing, but more data (unnecessarily) to be sent.

Which one do you like ?

Quoted 16 lines

Quoted 10 lines

Quoted 6 lines

I have a question here, not sure if this needs to be answered in the spec: why can't the driver send activate, set-direction and set-value all at once for a line?
Clearly if one request fails, the later ones would fail as well in this example, but
I don't see this as a problem for either side.

Merging activate along with that would mean, that the host needs to keep a reference count at its end to activate the line on the first call to set-value, but not on others.

My question here was about something else, to rephrase: Can't we allow the driver to queue multiple requests for one line and then have the device process them in order (regardless of whether it processes requests for other lines in parallel). The sequence I had in mind here was

Quoted 15 lines

driver:
1. queue enable
2. queue set-direction
3. queue set-value
4. notify device
device:
5. process enable
6. queue enable-response
7. process set-direction
8. queue set-direction response
9. process set-value
10. queue set-value response
11. notify driver
driver:
12. mark all requests as done

Hmm, we can do that in principle by saying all requests for a particular line must be served in order. But right now I have rather limited the number of requests per line to 1 for a reason, i.e. there will always be a single user of a GPIO line at any point of time. And so the operations will always be sequential.

If you look at users of GPIO, lets say in kernel for now, they all use generic APIs like request, set-direction, set-value, etc. There is no fixed order of how the different callbacks will be called by the user, and so we will never get the requests in bulk or groups (like you mentioned above). Each operation has its own significance, and it won't be right to return from, lets say gpio_set_value(), without getting the result of the operation.

So when can something like this, what you showed above, can be useful ?

Quoted 3 lines

This is similar to folding the irq type into the direction command, and I would slightly prefer it that way, but I'd like to hear what others think about it.

I would really like to keep the irq stuff separate from rest of the basic APIs. This would also be less confusing because the irq stuff is handled separately using a feature flag here.

Over that the users won't club these either, like in Linux the user will do gpio-request, then direction-input, and then request irq.

So these are all required to be sent at different times.

Quoted 7 lines

Quoted 4 lines

To be most efficient, the queue should be thought of as a series of buffers which are present in any order, based on what finishes first. It doesn't increase the complexity on any side to be honest. The code remains straight forward with it.

I expected the host side to be atomic here. Under which circumstance would the 'set direction out with value' call in your example block?

Yes it will block, but another thread can initiate another request for a different GPIO line.

Quoted 7 lines · Viresh Kumar

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · Viresh Kumar

Doing it per-line seems to be too much overhead, since we are going to call this for every line anyway (at least in Linux implementation). I haven't tried it, but I somehow feel that probe() will take a significant time to finish with that, lets say for 256 GPIO lines.

Quoted 5 lines

Quoted 2 lines

My guess is that this would not cause any notable overhead, but we will know for sure if you can measure it.

I will surely try it to get the number out, but sending a request 256 times instead of just once looks inefficient from the face of it :)

It could still be only one notification though, just a lot of requests with a pair of buffers each, that you send all at once.

Quoted 35 lines

Quoted 5 lines

My question here was about something else, to rephrase: Can't we allow the driver to queue multiple requests for one line and then have the device process them in order (regardless of whether it processes requests for other lines in parallel). The sequence I had in mind here was

Quoted 15 lines

driver:
1. queue enable
2. queue set-direction
3. queue set-value
4. notify device
device:
5. process enable
6. queue enable-response
7. process set-direction
8. queue set-direction response
9. process set-value
10. queue set-value response
11. notify driver
driver:
12. mark all requests as done

Hmm, we can do that in principle by saying all requests for a particular line must be served in order. But right now I have rather limited the number of requests per line to 1 for a reason, i.e. there will always be a single user of a GPIO line at any point of time. And so the operations will always be sequential.

If you look at users of GPIO, lets say in kernel for now, they all use generic APIs like request, set-direction, set-value, etc. There is no fixed order of how the different callbacks will be called by the user, and so we will never get the requests in bulk or groups (like you mentioned above). Each operation has its own significance, and it won't be right to return from, lets say gpio_set_value(), without getting the result of the operation.

Quoted 2 lines

So when can something like this, what you showed above, can be useful ?

Maybe this is something we can get advice for from the virtio maintainers. It was just a feeling on my side that slightly relaxing the requirement makes this more like other virtio drivers.

Functionally, I think there is no difference between mandating that the driver only queues one request per gpio line over requiring that the device processes all requests on a gpio line in sequence, but the latter can be slightly more efficient.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

To be most efficient, the queue should be thought of as a series of buffers which are present in any order, based on what finishes first. It doesn't increase the complexity on any side to be honest. The code remains straight forward with it.

Quoted 5 lines

Quoted 2 lines

I expected the host side to be atomic here. Under which circumstance would the 'set direction out with value' call in your example block?

Yes it will block, but another thread can initiate another request for a different GPIO line.

Ok, I was hoping that the gpiolib interfaces would all end up being non-blocking,

but I agree that if the guest has to wait for a notification it can be useful to have more than one going on at once. Especially if the host implementation is in a separate process there is not even a theoretical upper bound on how long it takes to process a request.

Quoted 6 lines · Arnd Bergmann

Quoted 5 lines · Viresh Kumar

Quoted 2 lines

My guess is that this would not cause any notable overhead, but we will know for sure if you can measure it.

I will surely try it to get the number out, but sending a request 256 times instead of just once looks inefficient from the face of it :)

I created a small setup with qemu-arm64 over x86:
- Without any memcpy() of data or buffers in the GPIO drivers on both ends.
- i.e. essentially taking into consideration only the time to send receive requests, the virtio core may be copying packets in between though, not sure.

Quoted 2 lines

It could still be only one notification though, just a lot of requests with a pair of buffers each, that you send all at once.

If we send only one notification at the end, it takes ~ 1ms for 256 GPIO pins, pretty much like a single request only. The downside here is that the device (host) also needs to prepare the queue for equal number (to ngpio) of elements. This is something the spec hasn't touched until now, i.e. how many buffers must be queued for the virtqueues. The max allowed in qemu is 1024 right now.

Even if I send them sequentially, with separate notification for each request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024 lines.

Though a single request still sounds tempting to me :)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I expected the host side to be atomic here. Under which circumstance would the 'set direction out with value' call in your example block?

Yes it will block, but another thread can initiate another request for a different GPIO line.

Ok, I was hoping that the gpiolib interfaces would all end up being non-blocking,

I am not sure how we can do that :(

Quoted 6 lines · Viresh Kumar

If we send only one notification at the end, it takes ~ 1ms for 256 GPIO pins, pretty much like a single request only. The downside here is that the device (host) also needs to prepare the queue for equal number (to ngpio) of elements. This is something the spec hasn't touched until now, i.e. how many buffers must be queued for the virtqueues. The max allowed in qemu is 1024 right now.

1ms is borderline I guess, it's close to the range of things that gets annoying for boot-time optimization.

Quoted 5 lines

Even if I send them sequentially, with separate notification for each request, it takes ~15-20 ms to finish 256 lines and ~80 for 1024 lines.

Though a single request still sounds tempting to me :)

Right, 80ms is definitely noticeable, this can easily make the difference between an instant guest boot and a perceived delay.

Quoted 1 line

I am not sure how we can do that :(

I suppose not without a complete redesign from scratch.

It is also important to mention here that in my current example, there was no copy
of the gpio line name string made anywhere.

If we choose this message to be per-line, then there will be at least one memcpy at the host of the string for each GPIO line, there may be another copy somewhere
in between at guest or host, not sure. Though in case of a single request, this will
be a single memcpy operation at the guest (and others in between as mentioned earlier),
but not per line and maybe more efficient.

So even in the best possible scenario, this will be several ms, at least for separate
message for each GPIO line.

Anyway, how do you suggest I define this call now. Per-line or entire block (fixed size or
from config structure) ? I will do it the way you suggest to cut short the discussion :)

So making it per-line doesn't give us any flexibility that we may be looking for.

Quoted 2 lines · Viresh Kumar

Quoted 1 line · Arnd Bergmann

On Mon, Jul 26, 2021 at 12:16 PM Viresh Kumar <[email protected]> wrote:

Quoted 16 lines

Quoted 16 lines

Quoted 10 lines

Quoted 6 lines

I have a question here, not sure if this needs to be answered in the spec: why can't the driver send activate, set-direction and set-value all at once for a line?
Clearly if one request fails, the later ones would fail as well in this example, but
I don't see this as a problem for either side.

Merging activate along with that would mean, that the host needs to keep a reference count at its end to activate the line on the first call to set-value, but not on others.

My question here was about something else, to rephrase: Can't we allow the driver to queue multiple requests for one line and then have the device process them in order (regardless of whether it processes requests for other lines in parallel). The sequence I had in mind here was

Quoted 7 lines

Quoted 7 lines

driver:
1. queue enable
2. queue set-direction
3. queue set-value
4. notify device
device:
5. process enable

Quoted 8 lines

Quoted 8 lines

6. queue enable-response
7. process set-direction
8. queue set-direction response
9. process set-value
10. queue set-value response
11. notify driver
driver:
12. mark all requests as done

Quoted 7 lines

Hmm, we can do that in principle by saying all requests for a particular line must be served in order. But right now I have rather limited the number of requests per line to 1 for a reason, i.e. there will always be a single user of a GPIO line at any point of time. And so the operations will always be sequential.

Quoted 7 lines

If you look at users of GPIO, lets say in kernel for now, they all use generic APIs like request, set-direction, set-value, etc. There is no fixed order of how the different callbacks will be called by the user, and so we will never get the requests in bulk or groups (like you mentioned above). Each operation has its own significance, and it won't be right to return from, lets say gpio_set_value(), without getting the result of the operation.

Quoted 3 lines

So when can something like this, what you showed above, can be useful ?

Maybe this is something we can get advice for from the virtio maintainers. It was just a feeling on my side that slightly relaxing the requirement makes this more like other virtio drivers.

Functionally, I think there is no difference between mandating that the driver only queues one request per gpio line over requiring that the device processes all requests on a gpio line in sequence, but the latter can be slightly more efficient.

Quoted 7 lines · Arnd Bergmann

Quoted 7 lines

Quoted 7 lines

driver:
1. queue enable
2. queue set-direction
3. queue set-value
4. notify device
device:
5. process enable

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

6. queue enable-response
7. process set-direction
8. queue set-direction response
9. process set-value
10. queue set-value response
11. notify driver
driver:
12. mark all requests as done

What are the ordering/sequence requirements here? E.g., are enable/set-direction or enable/set-value/set-direction valid sequences?

If we consider the sequence 1-4 above as a command chain, what about adding a chaining field that can point to the next command in the chain? The driver would build the chain in one go, the device would process the chain one after another, and stop if it encounters an error. The driver would be free to submit chains for other lines, but not another one for the same line as long as the current chain is not yet finished.

Quoted 8 lines

Maybe this is something we can get advice for from the virtio maintainers. It was just a feeling on my side that slightly relaxing the requirement makes this more like other virtio drivers.

Functionally, I think there is no difference between mandating that the driver only queues one request per gpio line over requiring that the device processes all requests on a gpio line in sequence, but the latter can be slightly more efficient.

It might be worth considering for future drivers, or non-Linux drivers.

Quoted 3 lines · Arnd Bergmann

Here I have the opposite question: what is the benefit of allowing the requests to be processed out of order? Should these not all be instantaneous, and easier to handle by making the request queue serialized?

Lets say the guest sends two requests, almost at the same time to the host and they are queued on the vq in this order only:

- set direction-out-with-value-1 for line 5.
- set direction-in for line 11.

Now, if the host is able to process them in parallel, then the second request for line 11 may finish before the requests for line 5 (as it has lesser amount of work to do).

Hi Viresh,

Quoted 11 lines · Viresh Kumar

Quoted 3 lines · Arnd Bergmann

Here I have the opposite question: what is the benefit of allowing the requests to be processed out of order? Should these not all be instantaneous, and easier to handle by making the request queue serialized?

Lets say the guest sends two requests, almost at the same time to the host and they are queued on the vq in this order only:

- set direction-out-with-value-1 for line 5.
- set direction-in for line 11.

Quoted 3 lines

Now, if the host is able to process them in parallel, then the second request for line 11 may finish before the requests for line 5 (as it has lesser amount of work to do).

Speaking about parallel processing, have you already envisioned support for .[sg]et_multiple()?

This patch adds support for interrupts to the virtio-gpio specification. This uses the feature bit 0 for the same.

Quoted 2 lines · Viresh Kumar

This patch adds support for interrupts to the virtio-gpio specification. This uses the feature bit 0 for the same.

I think the flow of event messages already looks great with this version, and I like the level of detail in the explanations.

The only part I don't like is that it seems overly redundant to have three ways to enable/disable irq delivery. More on that below.

virtio-gpio.tex \subsubsection{requestq Operation: Message Flow}\label{sec:Device Types / GPIO D · line 556

Quoted 3 lines

+\item If the GPIO line is configured for Level interrupts, the device MUST

virtio-gpio.tex +2 -0

+    ignore any further interrupts for the line, until the time the buffers are
+    made available again by the driver.

This wording sounds odd, since there are not technically any interrupt events on the level triggered line, but instead the level is active. I would describe this
as "the device MUST mask the interrupt line, until the time ...", though that conflicts with the terminology for the mask/unmask request.

I would still hope that we can combine the irq_type, mask and request

steps into either one or two steps. I think the minimalistic approach would be to expand the message to include both the "mask" and "set-type" operations, and this should work, as long as we define sensible semantics.

Another option would be to fold the "set irq type" operation into "set direction",

so an input can be configured as any of the five irq types, with "IRQ_TYPE_NONE" implying that interrupts are disabled for this line. This would still provide an extra way of masking the interrupt, so we just need "set-direction" and "request event". In this case, setting the interrupt type to "none" should probably cancel any pending irq and force the message to be returned.

You could actually take that further and fold the "set value" into "set direction",

which also simplifies the output direction and avoids the case where can you set a gpio to output before setting the value first.

Quoted 1 line · Arnd Bergmann

I would still hope that we can combine the irq_type, mask and request

request ? you wanted to say "unmask" ?

Quoted 3 lines

steps into either one or two steps. I think the minimalistic approach would be to expand the message to include both the "mask" and "set-type" operations, and this should work, as long as we define sensible semantics.

I think we can easily merge unmask and set-type into a single message, since that will fit within the request structure used for most of the requests on the requestq. i.e. a single byte of data.

mask() doesn't require irq-type, so it can live separately.

Quoted 9 lines

so an input can be configured as any of the five irq types, with "IRQ_TYPE_NONE" implying that interrupts are disabled for this line. This would still provide an extra way of masking the interrupt, so we just need "set-direction" and "request event". In this case, setting the interrupt type to "none" should probably cancel any pending irq and force the message to be returned.

You could actually take that further and fold the "set value" into "set direction",

At least gpio-direction-out already takes a value with it.

Quoted 5 lines · Viresh Kumar

Quoted 1 line · Arnd Bergmann

I would still hope that we can combine the irq_type, mask and request

request ? you wanted to say "unmask" ?

No, I meant VIRTIO_GPIO_MSG_IRQ_TYPE,

VIRTIO_GPIO_MSG_IRQ_{UN,}MASK and virtio_gpio_irq_request messages.

All three can change the state between 'can interrupt' and 'can not interrupt', so you only get notified if all three are in the active state (irq type != none, mask disabled, request queued).

Quoted 8 lines

Quoted 3 lines

steps into either one or two steps. I think the minimalistic approach would be to expand the message to include both the "mask" and "set-type" operations, and this should work, as long as we define sensible semantics.

I think we can easily merge unmask and set-type into a single message, since that will fit within the request structure used for most of the requests on the requestq. i.e. a single byte of data.

Quoted 1 line

mask() doesn't require irq-type, so it can live separately.

The question is whether it adds any value though. As far as I can tell, it is completely redundant, duplicating either what is implied by irq type==none, or what is implied by the request not being queued.

Quoted 3 lines · Arnd Bergmann

The question is whether it adds any value though. As far as I can tell, it is completely redundant, duplicating either what is implied by irq type==none, or what is implied by the request not being queued.

Right, we shouldn't have redundant stuff in here.

Though, binding of all interrupt operations to the eventq can be trick IMO.

- How will the driver mask an interrupt for which the buffer is already sent over the eventq? There is no API available to recall a buffer. So we would need an API over the requestq for that.

- Latching of edge interrupt between the buffer is returned by the device and requeued by the driver. How will the device know if the user has simply requeued the eventq buffer (where the latched value is meaningful) or it has done something like free_irq() followed by request_irq() in the kernel (where the latched value must be discarded).

Quoted 10 lines · Viresh Kumar

Quoted 3 lines · Arnd Bergmann

The question is whether it adds any value though. As far as I can tell, it is completely redundant, duplicating either what is implied by irq type==none, or what is implied by the request not being queued.

Right, we shouldn't have redundant stuff in here.

Though, binding of all interrupt operations to the eventq can be trick IMO.

Quoted 3 lines

- How will the driver mask an interrupt for which the buffer is already sent over the eventq? There is no API available to recall a buffer. So we would need an API over the requestq for that.

Agreed, there needs to be some way to cancel the request. I don't think this would have to be a fast or common operation, so setting the line back to IRQ_TYPE_NONE would be one way.

Another option would be to make the mask software-only and just ignore an incoming event for a line that is configured as masked, and then not requeue the request.

Quoted 6 lines

- Latching of edge interrupt between the buffer is returned by the device and requeued by the driver. How will the device know if the user has simply requeued the eventq buffer (where the latched value is meaningful) or it has done something like free_irq() followed by request_irq() in the kernel (where the latched value must be discarded).

I was expecting a 'mask' to not discard the latched event but simply not deliver it until it gets unmasked.

Either way, we can define that
a new set-irq-type request discards all latched edge interrupts.

Quoted 15 lines · Arnd Bergmann

Quoted 13 lines · Viresh Kumar

Quoted 3 lines · Arnd Bergmann

The question is whether it adds any value though. As far as I can tell, it is completely redundant, duplicating either what is implied by irq type==none, or what is implied by the request not being queued.

Right, we shouldn't have redundant stuff in here.

Though, binding of all interrupt operations to the eventq can be trick IMO.

- How will the driver mask an interrupt for which the buffer is already sent over the eventq? There is no API available to recall a buffer. So we would need an API over the requestq for that.

Quoted 3 lines

Agreed, there needs to be some way to cancel the request. I don't think this would have to be a fast or common operation, so setting the line back to IRQ_TYPE_NONE would be one way.

Hmm, how would that work ? I thought you were talking about inferring all irq operations from the presence of the buffer in the eventq itself. So once the buffer is sent to the host, we can't send it again to change the irq-type.

Quoted 3 lines

Another option would be to make the mask software-only and just ignore an incoming event for a line that is configured as masked, and then not requeue the request.

Hmm, I wonder what will happen if the driver gets removed/probed again after sending the buffer :)

Quoted 9 lines

Quoted 6 lines

- Latching of edge interrupt between the buffer is returned by the device and requeued by the driver. How will the device know if the user has simply requeued the eventq buffer (where the latched value is meaningful) or it has done something like free_irq() followed by request_irq() in the kernel (where the latched value must be discarded).

I was expecting a 'mask' to not discard the latched event but simply not deliver it until it gets unmasked.

Right, we are aligned on that. Masked == buffer is owned by driver here.

Quoted 2 lines

Either way, we can define that
a new set-irq-type request discards all latched edge interrupts.

Hmm, so you proposing to keep a separate (single) request for irq handling over the controlq ?

This was under the assumption that we decide to still keep some controlq request that sets the irq type and just remove the 'mask' command.

If we go all the way to having only one message for interrupts, I suppose it does get a little uglier than I was hoping for, but it would still be doable: in this case, we could allow a flow like this on the eventq:

- driver requests edge interrupts

- (no event happened, so request remains pending)
- driver queues a new request asking for IRQ_TYPE_NONE notification in order to mask this line

- device replies to both requests saying no interrupt happened

Between this control flow and the version where set-type is part of set-direction, I would prefer the other option, but you already said that you don't like that one.

Quoted 3 lines · Arnd Bergmann

This was under the assumption that we decide to still keep some controlq request that sets the irq type and just remove the 'mask' command.

Right, so I think I can fold all three of irq messages, mask, unmask and type, to a single message type. Where the value of type can play the role of masking/unmasking. This should work fine, I will see if I get any more doubts on this.

Quoted 5 lines

If we go all the way to having only one message for interrupts, I suppose it does get a little uglier than I was hoping for, but it would still be doable: in this case, we could allow a flow like this on the eventq:

- driver requests edge interrupts

This over eventq and ..

Quoted 3 lines

- (no event happened, so request remains pending)
- driver queues a new request asking for IRQ_TYPE_NONE notification in order to mask this line

This over controlq. Right ?

Quoted 1 line

- device replies to both requests saying no interrupt happened

Yes, that can work.

Quoted 3 lines

Between this control flow and the version where set-type is part of set-direction, I would prefer the other option, but you already said that you don't like that one.

Yeah, I would like to keep this away from set-direction, which I am already going to use for activate/deactivate :)