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

[Viresh Kumar]

On 04-08-21, 10:27, Arnd Bergmann wrote:
> On Wed, Aug 4, 2021 at 9:05 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > On 03-08-21, 17:01, Arnd Bergmann wrote:
> > > >
> > > > +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.
 
> > - 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