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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
