Re: [Stratos-dev] [PATCH V4 2/2] gpio: virtio: Add IRQ support

On 05-08-21, 14:03, Arnd Bergmann wrote:

On Thu, Aug 5, 2021 at 1:26 PM Viresh Kumar via Stratos-dev

quoted

Based on discussion we had today (offline), I changed the design a bit
and used handle_level_irq() instead, as it provides consistent calls
to mask/unmask(), which simplified the whole thing a bit.

The new flow looks much nicer to me, without the workqueue, and
doing the requeue directly in the unmask() operation.

I don't quite understand the purpose of the type_pending and
mask_pending flags yet, can you explain what they actually
do?

They are required to make sure we don't send unnecessary
VIRTIO_GPIO_MSG_IRQ_TYPE events to the device, every time bus_unlock()
is called.

mask_pending tracks if the masked state has changed since the time
last bus_unlock() was called. So on an interrupt, both mask() and
unmask() will get called by the irq-core now and mask_pending will
change to true (in mask()} and then false (in unmask()). And
eventually in bus_unlock() we won't send an extra
VIRTIO_GPIO_MSG_IRQ_TYPE message.

Also, I have no idea about whether using the handle_level_irq()
function is actually correct here. I suppose if necessary, the driver
could provide its own irq.handler callback in place of that.

After looking at internals of these, I felt handle_level_irq() suits
much better in our case as we need to queue the buffer only at
unmask(). With handle_fasteoi_irq(), we would be required to do the
same from multiple places, unmask(), eoi().

quoted

Also I have broken the rule from specs, maybe we should update spec
with that, where the specs said that the buffer must not be queued
before enabling the interrupt. I just queue the buffer unconditionally
now from unmask().

I am not sure but there may be some race around the "queued" flag and
I wonder if we can land in a scenario where the buffer is left
un-queued somehow, while an interrupt is enabled.

Can that be integrated with the "masked" state now? It looks like
the two flags are always opposites now.

Yeah, but then there can be races and keeping them separate is a
better thing IMO.

I was thinking of something on these lines, disable_irq() followed by
enable_irq():

CPU0                                                 CPU1

disable_irq()
 -> irq_bus_lock()
 -> irq_mask()
 -> irq_bus_sync_unlock()
   -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE
      to disable interrupt
                                                     Backend (at host) disables irq and 
                                                     returns the unused buffer.

enable_irq()
 -> irq_bus_lock()
 -> irq_unmask()
   -> Tries to queues buffer again
      Finds it already queued and returns.
                                                     - virtio_gpio_event_vq() runs at guest
                                                     - Finds VIRTIO_GPIO_IRQ_STATUS_INVALID in status
                                                     - returns without doing anything
 -> irq_bus_sync_unlock()
   -> sends blocking VIRTIO_GPIO_MSG_IRQ_TYPE
      to enable interrupt



So the irq is still enabled and the buffer isn't queued. Yes, need
some locking here for sure, confirmed :)

-- 
viresh