On 04-08-21, 10:27, Arnd Bergmann wrote:

On Wed, Aug 4, 2021 at 9:05 AM Viresh Kumar [off-list ref] wrote:

quoted

On 03-08-21, 17:01, Arnd Bergmann wrote:

quoted

quoted

+static void virtio_gpio_irq_unmask(struct irq_data *d)
+{
+       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+       struct virtio_gpio *vgpio = gpiochip_get_data(gc);
+       struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq];
+
+       irq_line->masked = false;
+       irq_line->update_pending = true;
+}

Same here. unmask is probably less important, but it's the
same operation: if you want interrupts to become active
again when they are not, just queue the request

We can't because its a slow bus ? And unmask can be called from
irq-context. That's exactly why we had the irq_bus_lock/unlock
discussion earlier.

I thought only 'mask' is slow, since that has to wait for the completion,
but 'unmask' just involves sending the eventq request without having
to wait for it.

Ahh, so you are suggesting of just sending the request without waiting
for the response. We can do it, but I am not sure if it is going to be
worth it considering the additional complexity in the driver.

Maybe lets come back to this later, if this is going to make anything
better, once the basic support for irqs look good here.

I don't think it is correct that you cannot issue virtio requests from
atomic context, only that you cannot wait for the reply.

Yes, you are right. I was having a mess because of all the races and
different ways things were getting called.

For 'unmask', there is no waiting, since the reply is the actual IRQ
event. For the others, the sequence makes sense.

quoted

- Interrupt: i.e. buffer sent back by the host over virtio.

  virtio_gpio_event_vq() schedules a work item, which processes the
  items from the eventq virtqueue and eventually calls
  generic_handle_irq(). The irq-core can issue calls to
  ->irq_mask/unmask() here without a prior call to
  irq_bus_lock/unlock(), normally they will balance out by the end,
  but I am not sure if it is guaranteed. Moreover, interrupt should be
  re-enabled only after unmask() is called (for ONESHOT) and not at
  EOI, right ?

  I chose not to queue the buffers back from eoi() as it is possible
  that we won't want to queue them at all, as the interrupt needs to
  be disabled by the time generic_handle_irq() returns. And so I did
  everything from the end of vgpio_work_handler()'s loop, i.e. either
  disable the interrupts with VIRTIO_GPIO_IRQ_TYPE_NONE or enable the
  interrupt again by re-queuing the buffer.

Regarding irq handling using work-item, I had to move to that to take
care of locking for re-queuing the buffers for a GPIO line from
irq-handler and bus-unlock. Nothing else seemed to work, though I am
continuing to look into that to see if there is an alternative here.

I don't think it makes sense to optimize for the rare case that the
irq handler disables the irq, when that makes the common case
(irq remains unmasked and enabled) much slower.

I am not worried about that case only, i.e. where the
interrupt-handler disables the interrupt. It should work fine though,
even if it isn't optimized.

A) If the client has registered with irq with IRQF_ONESHOT, irq core
   will call ->irq_mask(), irq-handler(), ->irq_eoi(), and finally
   ->irq_unmask(). Looking at this basic sequence itself, irq_eoi()
   shouldn't enable the interrupt again by re-queuing the buffer,
   rather it should happen from unmask in this case.

B) If the client has registered a threaded-irq, it gets even more
   complex. The core does this in that case:

   call ->irq_mask(), ->irq_eoi(), and return control back to
   virtio_gpio_event_vq().

   The thread gets scheduled then and calls the irq-handler and then
   the irq-core calls irq-bus-lock(), ->irq_unmask(),
   irq-bus-unlock().

Looking at these, eoi shouldn't be queuing the buffer. Also, I wonder
if we should use handle_fasteoi_irq() here or something else.

-- 
viresh