Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence

[Jianjun Wang <jianjun.wang@mediatek.com>]

Hi Bjorn,

On Thu, 2022-01-27 at 15:21 -0600, Bjorn Helgaas wrote:
> [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
> 
> On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > 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.
> > 
> > Yes, as Maz said, we save the edge-interrupt status so that it
> > becomes a level-interrupt. This is similar to an edge-to-level
> > converter, so we need to clear it *before*. We found this problem
> > through a lot of experiments and tested this patch.
> 
> I thought there might be other host controllers with similar design,
> so I looked at all the other drivers and tried to figure out whether
> any others had similar problems.
> 
> The ones below look suspicious to me because they all clear some sort
> of status register *after* handling an MSI.  Can you guys take a look
> and make sure they are working correctly?
> 
>   keembay_pcie_msi_irq_handler
>     status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
>     if (status & MSI_CTRL_INT)
>       dw_handle_msi_irq
> 	generic_handle_domain_irq
>       writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> 
>   spear13xx_pcie_irq_handler
>     status = readl(&app_reg->int_sts)
>     if (status & MSI_CTRL_INT)
>       dw_handle_msi_irq
> 	generic_handle_domain_irq
>     writel(status, &app_reg->int_clr)
> 
>   advk_pcie_handle_int
>     isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
>     if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
>       advk_pcie_handle_msi
>         advk_readl(pcie, PCIE_MSI_STATUS_REG)
> 	advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> 	generic_handle_irq
> 	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
> 
>   mtk_pcie_irq_handler
>     status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
>     for_each_set_bit_from(irq_bit, &status, ...)
>       mtk_pcie_msi_handler
>         generic_handle_domain_irq
>       writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)

Thanks for mention that. In the hardware corresponding to pcie-
mediatek-gen3.c, the interrupt status in PCIE_INT_STATUS_REG cannot be
cleared if the MSI status remaining in the register of msi_set, so we
have to clear it after handling the MSI.

I guess the root cause of this patch is the interrupt status can be
cleared even the MSI status still remaining, hence that if there are
some MSIs received while clearing the interrupt status, these MSIs
cannot be serviced.

We will discuss and test internally and update the results later,
thanks for your review.

Thanks.

> 
> Bjorn
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek