[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <00e301da63de$bd53db90$37fb92b0$@trustnetic.com>
Date: Tue, 20 Feb 2024 17:25:26 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Andrew Lunn'" <andrew@...n.ch>
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 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
>
Powered by blists - more mailing lists