diff mbox series

PCI: mediatek: Change MSI interrupt processing sequence

Message ID 20220123033306.29799-1-qizhong.cheng@mediatek.com (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: mediatek: Change MSI interrupt processing sequence | expand

Commit Message

qizhong cheng Jan. 23, 2022, 3:33 a.m. UTC
As an edge-triggered interrupts, its interrupt status should be cleared
before dispatch to the handler of device.

Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chen-Yu Tsai Jan. 24, 2022, 3:12 a.m. UTC | #1
Hi,

On Sun, Jan 23, 2022 at 11:34 AM qizhong cheng
<qizhong.cheng@mediatek.com> wrote:
>
> As an edge-triggered interrupts, its interrupt status should be cleared
> before dispatch to the handler of device.

I'm curious, is this just a code correction or are there real world
cases where something fails?

Also, please add a Fixes tag and maybe Cc stable so this gets backported
automatically.

ChenYu

> Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 2f3f974977a3..705ea33758b1 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -624,12 +624,12 @@ static void mtk_pcie_intr_handler(struct irq_desc *desc)
>                 if (status & MSI_STATUS){
>                         unsigned long imsi_status;
>
> +                       /* Clear MSI interrupt status */
> +                       writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
>                         while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
>                                 for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM)
>                                         generic_handle_domain_irq(port->inner_domain, bit);
>                         }
> -                       /* Clear MSI interrupt status */
> -                       writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
>                 }
>         }
>
> --
> 2.25.1
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Chen-Yu Tsai Jan. 24, 2022, 6:55 a.m. UTC | #2
On Mon, Jan 24, 2022 at 2:27 PM qizhong.cheng
<qizhong.cheng@mediatek.com> wrote:
>
> Hi chenYu,
>
> On Mon, 2022-01-24 at 11:12 +0800, Chen-Yu Tsai wrote:
> > Hi,
> >
> > On Sun, Jan 23, 2022 at 11:34 AM qizhong cheng
> > <qizhong.cheng@mediatek.com> wrote:
> > >
> > > As an edge-triggered interrupts, its interrupt status should be
> > > cleared
> > > before dispatch to the handler of device.
> >
> > I'm curious, is this just a code correction or are there real world
> > cases where something fails?
>
> Yes, we found a failure when used iperf tool for wifi and network cards
> performance testing. The function of "while" has just been executed,
> and the EP sent an MSI before executing "Clear MSI interrupt status".
> After executing "Clear MSI interrupt status", this edge-triggered
> interrupt status is cleared, but EP is still waiting for interrupt
> handler.

Can you also include this in the commit log?  It would be nice to record
the exact scenario that this fix targets.

ChenYu

> >
> > Also, please add a Fixes tag and maybe Cc stable so this gets
> > backported
> > automatically.
>
> Thanks for your review, I will fix it in the next version.
>
> >
> > ChenYu
> >
> > > Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com>
> > > ---
> > >  drivers/pci/controller/pcie-mediatek.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-mediatek.c
> > > b/drivers/pci/controller/pcie-mediatek.c
> > > index 2f3f974977a3..705ea33758b1 100644
> > > --- a/drivers/pci/controller/pcie-mediatek.c
> > > +++ b/drivers/pci/controller/pcie-mediatek.c
> > > @@ -624,12 +624,12 @@ static void mtk_pcie_intr_handler(struct
> > > irq_desc *desc)
> > >                 if (status & MSI_STATUS){
> > >                         unsigned long imsi_status;
> > >
> > > +                       /* Clear MSI interrupt status */
> > > +                       writel(MSI_STATUS, port->base +
> > > PCIE_INT_STATUS);
> > >                         while ((imsi_status = readl(port->base +
> > > PCIE_IMSI_STATUS))) {
> > >                                 for_each_set_bit(bit, &imsi_status,
> > > MTK_MSI_IRQS_NUM)
> > >                                         generic_handle_domain_irq(p
> > > ort->inner_domain, bit);
> > >                         }
> > > -                       /* Clear MSI interrupt status */
> > > -                       writel(MSI_STATUS, port->base +
> > > PCIE_INT_STATUS);
> > >                 }
> > >         }
> > >
> > > --
> > > 2.25.1
> > >
> > >
> > > _______________________________________________
> > > Linux-mediatek mailing list
> > > Linux-mediatek@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
Bjorn Helgaas Jan. 25, 2022, 4:57 p.m. UTC | #3
All patches change *something*.  Can you update the subject line so it
says something specific about the change?

Maybe something like "Clear MSI status before dispatching handler"?

On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> As an edge-triggered interrupts, its interrupt status should be cleared
> before dispatch to the handler of device.

I'm not an IRQ expert, but the reasoning that "we should clear the MSI
interrupt status before dispatching the handler because MSI is an
edge-triggered interrupt" doesn't seem completely convincing because
your code will now look like this:

  /* Clear the INTx */
  writel(1 << bit, port->base + PCIE_INT_STATUS);
  generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
  ...

  /* Clear MSI interrupt status */
  writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
  generic_handle_domain_irq(port->inner_domain, bit);

You clear interrupt status before dispatching the handler for *both*
level-triggered INTx interrupts and edge-triggered MSI interrupts.

So it doesn't seem that simply being edge-triggered is the critical
factor here.

> Signed-off-by: qizhong cheng <qizhong.cheng@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 2f3f974977a3..705ea33758b1 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -624,12 +624,12 @@ static void mtk_pcie_intr_handler(struct irq_desc *desc)
>  		if (status & MSI_STATUS){
>  			unsigned long imsi_status;
>  
> +			/* Clear MSI interrupt status */
> +			writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
>  			while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
>  				for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM)
>  					generic_handle_domain_irq(port->inner_domain, bit);
>  			}
> -			/* Clear MSI interrupt status */
> -			writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
>  		}
>  	}
>  
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Marc Zyngier Jan. 25, 2022, 5:21 p.m. UTC | #4
On 2022-01-25 16:57, Bjorn Helgaas wrote:
> All patches change *something*.  Can you update the subject line so it
> says something specific about the change?
> 
> Maybe something like "Clear MSI status before dispatching handler"?
> 
> On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
>> As an edge-triggered interrupts, its interrupt status should be 
>> cleared
>> before dispatch to the handler of device.
> 
> I'm not an IRQ expert, but the reasoning that "we should clear the MSI
> interrupt status before dispatching the handler because MSI is an
> edge-triggered interrupt" doesn't seem completely convincing because
> your code will now look like this:
> 
>   /* Clear the INTx */
>   writel(1 << bit, port->base + PCIE_INT_STATUS);
>   generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
>   ...
> 
>   /* Clear MSI interrupt status */
>   writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
>   generic_handle_domain_irq(port->inner_domain, bit);
> 
> You clear interrupt status before dispatching the handler for *both*
> level-triggered INTx interrupts and edge-triggered MSI interrupts.
> 
> So it doesn't seem that simply being edge-triggered is the critical
> factor here.

This is the usual problem with these half-baked implementations.
The signalling to the primary interrupt controller is level, as
they take a multitude of input and (crucially) latch the MSI
edges. Effectively, this is an edge-to-level converter, with
all the problems that this creates.

By clearing the status *after* the handling, you lose edges that
have been received and coalesced after the read of the status
register. By clearing it *before*, you are acknowledging the
interrupts early, and allowing them to be coalesced independently
of the ones that have been received earlier.

This is however mostly an educated guess. Someone with access
to the TRM should verify this.

Thanks,

         M.
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 2f3f974977a3..705ea33758b1 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -624,12 +624,12 @@  static void mtk_pcie_intr_handler(struct irq_desc *desc)
 		if (status & MSI_STATUS){
 			unsigned long imsi_status;
 
+			/* Clear MSI interrupt status */
+			writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
 			while ((imsi_status = readl(port->base + PCIE_IMSI_STATUS))) {
 				for_each_set_bit(bit, &imsi_status, MTK_MSI_IRQS_NUM)
 					generic_handle_domain_irq(port->inner_domain, bit);
 			}
-			/* Clear MSI interrupt status */
-			writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
 		}
 	}