RE: [PATCH] net: txgbe: fix GPIO interrupt blocking

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Mon, Feb 19, 2024 12:45 AM, Andrew Lunn wrote:
> On Sun, Feb 18, 2024 at 05:04:52PM +0800, Jiawen Wu wrote:
> > On Tue, Feb 6, 2024 11:29 PM, Andrew Lunn wrote:
> > > On Tue, Feb 06, 2024 at 03:08:24PM +0800, Jiawen Wu wrote:
> > > > GPIO interrupt is generated before MAC IRQ is enabled, it causes
> > > > subsequent GPIO interrupts that can no longer be reported if it is
> > > > not cleared in time. So clear GPIO interrupt status at the right
> > > > time.
> > >
> > > This does not sound correct. Since this is an interrupt controller, it
> > > is a level interrupt. If its not cleared, as soon as the parent
> > > interrupt is re-enabled, is should cause another interrupt at the
> > > parent level. Servicing that interrupt, should case a descent to the
> > > child, which will service the interrupt, and atomically clear the
> > > interrupt status.
> > >
> > > Is something wrong here, like you are using edge interrupts, not
> > > level?
> >
> > Yes, it is edge interrupt.
> 
> So fix this first, use level interrupts.

I have a question here.

I've been setting the interrupt type in chip->irq_set_type. The 'type' is
passed as IRQ_TYPE_EDGE_BOTH. Then I config GPIO registers based on
this type, and use edge interrupts. Who decides this type? Can I change
it at will?

> 
> > > > And executing function txgbe_gpio_irq_ack() manually since
> > > > handle_nested_irq() does not call .irq_ack for irq_chip.
> > >
> > > I don't know the interrupt code too well, so could you explain this in
> > > more detail. Your explanation sounds odd to me.
> >
> > This is because I changed the interrupt controller in
> > https://git.kernel.org/netdev/net-next/c/aefd013624a1.
> > In the previous interrupt controller, .irq_ack in struct irq_chip is called
> > to clear the interrupt after the GPIO interrupt is handled. But I found
> > that in the current interrupt controller, this .irq_ack is not called. Maybe
> > I don't know enough about this interrupt code, I have to manually add
> > txgbe_gpio_irq_ack() to clear the interrupt in the handler.
> 
> You should dig deeper into interrupts.
> [goes and digs]
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/irq.h#L461
>  * @irq_ack:		start of a new interrupt
> 
> The comment makes it sound like irq_ack will be the first callback
> used when handling an interrupt.
> 
> static inline void mask_ack_irq(struct irq_desc *desc)
> {
>         if (desc->irq_data.chip->irq_mask_ack) {
>                 desc->irq_data.chip->irq_mask_ack(&desc->irq_data);
>                 irq_state_set_masked(desc);
>         } else {
>                 mask_irq(desc);
>                 if (desc->irq_data.chip->irq_ack)
>                         desc->irq_data.chip->irq_ack(&desc->irq_data);
>         }
> }
> 
> So the comment might be a little misleading. It will first mask the
> interrupt, and then ack it.
> 
> /**
>  *      handle_level_irq - Level type irq handler
>  *      @desc:  the interrupt description structure for this irq
>  *
>  *      Level type interrupts are active as long as the hardware line has
>  *      the active level. This may require to mask the interrupt and unmask
>  *      it after the associated handler has acknowledged the device, so the
>  *      interrupt line is back to inactive.
>  */
> void handle_level_irq(struct irq_desc *desc)
> {
>         raw_spin_lock(&desc->lock);
>         mask_ack_irq(desc);
> 
> So when handling a level interrupt, mask and then ack is the first
> thing done. And it is unconditional.
> 
> edge interrupts are different:
> 
> /**
>  *      handle_edge_irq - edge type IRQ handler
>  *      @desc:  the interrupt description structure for this irq
>  *
>  *      Interrupt occurs on the falling and/or rising edge of a hardware
>  *      signal. The occurrence is latched into the irq controller hardware
>  *      and must be acked in order to be reenabled. After the ack another
>  *      interrupt can happen on the same source even before the first one
>  *      is handled by the associated event handler. If this happens it
>  *      might be necessary to disable (mask) the interrupt depending on the
>  *      controller hardware. This requires to reenable the interrupt inside
>  *      of the loop which handles the interrupts which have arrived while
>  *      the handler was running. If all pending interrupts are handled, the
>  *      loop is left.
>  */
> void handle_edge_irq(struct irq_desc *desc)
> {
>         raw_spin_lock(&desc->lock);
> 
>         desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> 
>         if (!irq_may_run(desc)) {
>                 desc->istate |= IRQS_PENDING;
>                 mask_ack_irq(desc);
>                 goto out_unlock;
>         }
> 
>         /*
>          * If its disabled or no action available then mask it and get
>          * out of here.
>          */
>         if (irqd_irq_disabled(&desc->irq_data) || !desc->action) {
>                 desc->istate |= IRQS_PENDING;
>                 mask_ack_irq(desc);
>                 goto out_unlock;
>         }
> 
>        /* Start handling the irq */
>         desc->irq_data.chip->irq_ack(&desc->irq_data);
> 
> So if the interrupt handler is already running, it will mask and ack
> it, but not handle it, since there is already a handler running.
> Otherwise it will ack the interrupt and then handle it. And it loops
> handling the interrupt while IRQS_PENDING is set, i.e. another
> interrupt has arrived while the handler was running.
> 
> I would suggest you first get it using level interrupts, and then dig
> into how level interrupts are used, which do appear to be simpler than
> edge.
> 
> 	Andrew
>