Re: [PATCH 0/3] PCI: designware: Fixing MSI handling flow

From: Marc Zyngier <marc.zyngier@arm.com>
To: Trent Piepho <tpiepho@impinj.com>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"gustavo.pimentel@synopsys.com" <gustavo.pimentel@synopsys.com>
Cc: "jingoohan1@gmail.com" <jingoohan1@gmail.com>,
	"faiz_abbas@ti.com" <faiz_abbas@ti.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"helgaas@google.com" <helgaas@google.com>,
	"vigneshr@ti.com" <vigneshr@ti.com>,
	"Joao.Pinto@synopsys.com" <Joao.Pinto@synopsys.com>
Subject: Re: [PATCH 0/3] PCI: designware: Fixing MSI handling flow
Date: Mon, 10 Dec 2018 18:31:14 +0000	[thread overview]
Message-ID: <bf6cf5c9-6f82-79b3-c23e-05c48bf9f0f2@arm.com> (raw)
In-Reply-To: <1544465752.18519.188.camel@impinj.com>

On 10/12/2018 18:15, Trent Piepho wrote:
> On Mon, 2018-12-10 at 16:17 +0000, Lorenzo Pieralisi wrote:
>> On Tue, Nov 13, 2018 at 10:57:31PM +0000, Marc Zyngier wrote:
>>> It recently came to light that the Designware PCIe driver is rather
>>> broken in the way it handles MSI[1]:
>>>
>>> - It masks interrupt by disabling them, meaning that MSIs generated
>>>   during the masked window are simply lost. Oops.
>>>
>>> - Acking of the currently pending MSI is done outside of the
>>> interrupt
>>>   flow, getting moved around randomly and ultimately breaking the
>>>   driver. Not great.
>>>
>>>
>> I have decided to queue this series - fixed-up as per this thread,
>> available at:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git
>> test/pci-dwc-msi
>>
>> We allowed enough time for people to test it, we can't leave mainline
>> broken for the, apparently few, people who care.
>>
>> I *think* that this is the Fixes: tag to be added to all patches in
>> this
>> series, @Gustavo please countercheck:
>>
>> 7c5925afbc58 ("PCI: dwc: Move MSI IRQs allocation to IRQ domains
>> hierarchical API")
> 
> That's not the correct Fixes.  It should be:
> 
> 8c934095fa2f ("PCI: dwc: Clear MSI interrupt status after it is
> handled, not before")

This only applies to the last one, which should carry both tags.

> I had concerns about what appears to be an unnecessary extra lock taken
> before handling an interrupt and enabling all MSIs even if nothing has
> tried to enable them.
Regarding the lock: I'm quite puzzled that you consider it
"unnecessary", given that all the DWC callbacks expect such a locking. I
suspect you are considering from a pure performance angle, and I'd
suggest that you post numbers showing the unacceptable overhead of an
otherwise uncontended lock.

As for enabling all MSIs upfront, same thing. Please demonstrate how
harmful it is, given that they are all masked by default, consistently
with what other interrupt controllers are doing.

Once you've posted some of the above, we'll queue some additional fixes
on top. In the meantime, we'll fix it for everyone else.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...