Message-ID: <33eed490-7819-409e-8c79-b3c1e4c4fd66@lunn.ch>
Date: Sun, 18 Feb 2024 17:44:55 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, maciej.fijalkowski@...el.com,
	netdev@...r.kernel.org, mengyuanlou@...-swift.com
Subject: Re: [PATCH] net: txgbe: fix GPIO interrupt blocking

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.

> > > 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

Powered by blists - more mailing lists