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

[qizhong.cheng <qizhong.cheng@mediatek.com>]

On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> 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.
> 

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.

Thanks Helgaas and Maz for your comment.

--
Jazz ain't dead, dreams haven't parted with you.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek