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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 10-06-21, 23:30, Linus Walleij wrote:
> On Thu, Jun 10, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > +static void virtio_gpio_irq_unmask(struct irq_data *d)
> > +{
> > +       struct gpio_chip *gc = irq_data_to_gpio_chip(d);
> > +       struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
> > +       struct vgpio_line *line = &vgpio->lines[d->hwirq];
> > +
> > +       line->masked = false;
> > +       line->masked_pending = true;
> > +}
> 
> This looks dangerous in combination with this:
> 
> > +static void virtio_gpio_interrupt(struct virtqueue *vq)
> > +{
> (...)
> > +       local_irq_disable();
> > +       ret = generic_handle_irq(irq);
> > +       local_irq_enable();
> 
> Nominally slow IRQs like those being marshalled over
> virtio should be nested, handle_nested_irq(irq);
> but are they?

Hmm, this is the call trace:

Call trace:
 virtio_gpio_interrupt+0x34/0x168
 vring_interrupt+0x64/0x98
 vp_vring_interrupt+0x5c/0xa8
 vp_interrupt+0x40/0x78
 __handle_irq_event_percpu+0x5c/0x180
 handle_irq_event_percpu+0x38/0x90
 handle_irq_event+0x48/0xe0
 handle_fasteoi_irq+0xb0/0x138
 generic_handle_irq+0x30/0x48
 __handle_domain_irq+0x60/0xb8
 gic_handle_irq+0x58/0x128
 el1_irq+0xb0/0x180
 arch_cpu_idle+0x18/0x28
 default_idle_call+0x24/0x5c
 do_idle+0x1ec/0x288
 cpu_startup_entry+0x28/0x68
 rest_init+0xd8/0xe8
 arch_call_rest_init+0x10/0x1c
 start_kernel+0x508/0x540

I don't see a threaded interrupt in the path and vp_vring_interrupt()
already takes spin_lock_irqsave().

This is what handle_nested_irq() says:

 *	Handle interrupts which are nested into a threaded interrupt
 *	handler. The handler function is called inside the calling
 *	threads context.

So AFAICT, handle_nested_irq() is relevant if the irq-chip's handler
is called in threaded context instead of hard one. In this case it is
called from hard-irq context and so calling generic_handle_irq() looks
to be the right thing.

Right ?

> Or are they just quite slow not super slow?

It doesn't use another slow bus like I2C, but this should be slow
anyway.

> If a threaded IRQF_ONESHOT was requested the
> IRQ core will kick the thread and *MASK* this IRQ,
> which means it will call back to your .irq_mask() function
> and expect it to be masked from this
> point.
> 
> But the IRQ will not actually be masked until you issue
> your callbacks in the .irq_bus_sync_unlock() callback
> right?

Yes.

> So from this point until .irq_bus_sync_unlock()
> get called and actually mask the IRQ, it could be
> fired again?

Since we are defining the spec right now, this is up to us to decide
how we want to do it.

> I suppose the IRQ handler is reentrant?

It shouldn't happen because of the locking in place in the virtqueue
core (vp_vring_interrupt()).

> This would violate the API.
> 
> I would say that from this point and until you sync
> you need a spinlock or other locking primitive to
> stop this IRQ from fireing again, and a spinlock will
> imply local_irq_disable() so this gets really complex.
> 
> I would say only using nesting IRQs or guarantee this
> some other way, one way would be to specify that
> whatever is at the other side of virtio cannot send another
> GPIO IRQ message before the last one is handled,
> so you would need to send a specific (new)
> VIRTIO_GPIO_REQ_IRQ_ACK after all other messages
> have been sent in .irq_bus_sync_unlock()
> so that the next GPIO IRQ can be dispatched after that.

I was thinking of mentioning this clearly in the spec at first, but
now after checking the sequence of things it looks like Linux will do
it anyway. Though adding this clearly in the spec can be better. We
should just send a response message here instead of another message
type VIRTIO_GPIO_REQ_IRQ_ACK.

> (Is this how messaged signalled interrupts work? No idea.
> When in doubt ask the IRQ maintainers.)

-- 
viresh