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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 06-08-21, 10:00, Arnd Bergmann wrote:
> On Fri, Aug 6, 2021 at 9:44 AM Viresh Kumar via Stratos-dev
> <stratos-dev@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On 05-08-21, 15:10, Arnd Bergmann wrote:
> > > I hope this can still be simplified by working out better which state
> > > transitions are needed exactly. In particular, I would expect that we
> > > can get away with not sending a VIRTIO_GPIO_MSG_IRQ_TYPE
> > > for 'mask' state changes at all, but use that only for forcing 'enabled'
> > > state changes.
> >
> > Something like this ?
> 
> > static void virtio_gpio_irq_mask(struct irq_data *d)
> > {
> >         /* Nothing to do here */
> > }
> 
> You'd have to do /something/ here I think, if only setting the flag
> that we don't want to deliver the next interrupt.
> 
> > 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);
> >
> >         /* Queue the buffer unconditionally on unmask */
> >         virtio_gpio_irq_prepare(vgpio, d->hwirq);
> > }
> 
> And check the flag here to not requeue it if it's masked.

I am not sure I understand why this would be required. If the
interrupt is getting disabled, then unmask will not get called here
and so we won't requeue anything. Same will happen with threaded
handlers where the interrupt gets unmasked at a later point of time.

> Now, there is already a flag in the irq descriptor, so rather than
> having double accounting, the easy way may be to
> just use irqd_irq_masked()/irq_state_set_masked(), or
> have the irq core take care of this.

-- 
viresh