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

From: qizhong.cheng <qizhong.cheng@mediatek.com>
To: Marc Zyngier <maz@kernel.org>, Bjorn Helgaas <helgaas@kernel.org>
Cc: "Ryder Lee" <ryder.lee@mediatek.com>,
	"Jianjun Wang" <jianjun.wang@mediatek.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, chuanjia.liu@mediatek.com
Subject: Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
Date: Wed, 26 Jan 2022 11:37:58 +0800	[thread overview]
Message-ID: <d78b45e461b204d375830217d0d27ffdd97cedd3.camel@mediatek.com> (raw)
In-Reply-To: <e8e42eba6e7cf49bc2260f20844a7849@kernel.org>

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel