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

[Viresh Kumar <viresh.kumar@linaro.org>]

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