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

[Gustavo Pimentel <gustavo.pimentel@synopsys.com>]

On 14/11/2018 18:28, Trent Piepho wrote:
> On Tue, 2018-11-13 at 22:57 +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.
>>
>> This series attempts to address this by switching to using the MASK
>> register for masking interrupts (!), and move the ack into the
>> appropriate callback, giving it a fixed place in the MSI handling
>> flow.
>>
>> Note that this is only compile-tested on my arm64 laptop, as I'm
>> travelling and do not have the required HW to test it anyway. I'd
>> welcome both review and testing by the interested parties (dwc
>> maintainer and users affected by existing bugs).
>>
> 
> I've started to test this series after porting all the patches needed
> to make IMX7d work from 4.16.8 to 4.20.0-rc2.
> 
> Took a little while to figure out that the pcieport driver has a new
> config entry to enable, or one gets no interrupts.  I'm not sure if
> this is entirely correct behavior.
> 
> The new domain stuff does not appear to integrate into the existing irq
> framework perfectly.  My interrupt has changed from MSI #1 to MSI
> #524288.  Not the most user friendly number.
> 
> 292:          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
> 293:          1          0   PCI-MSI 524288 Edge      impinj-rfid-modem
> 
> Previously the dwc controller would show up as the owner of GPCv2 IRQ
> 122.  It doesn't any more.  Seems like the kernel info for it is wrong.
> 
> /sys/kernel/irq/65/actions:(null)
> /sys/kernel/irq/65/chip_name:GPCv2
> /sys/kernel/irq/65/hwirq:122
> /sys/kernel/irq/65/per_cpu_count:0,0
> /sys/kernel/irq/65/type:edge
> 
> Should be level and the count should be 1,0.  The debugfs interface is
> more accurate:
> 
> # cat /sys/kernel/debug/irq/irqs/65
> handler:  dw_chained_msi_isr
> device:   (null)
> status:   0x00010c00
>             _IRQ_NOPROBE
>             _IRQ_NOREQUEST
>             _IRQ_NOTHREAD
> dstate:   0x03400204
>             IRQ_TYPE_LEVEL_HIGH
>             IRQD_ACTIVATED
>             IRQD_IRQ_STARTED
>             IRQD_SINGLE_TARGET
> 
> Still doesn't know what device it's for.
> 
> Now I can finally test it!
> 
> Confirmed interrupt race is still there in stock kernel.
> 
> Confirmed after my patch I didn't see the race.  Didn't check why the
> broken enable/disable as mask didn't appear cause a new race, but
> something must be wrong somewhere.
> 
> Tried your 1st patch.  As I mentioned before in a reply to Gustavo,
> just changing the enable to mask results in the MSI never getting
> enabled in the first place.  Nothing else writes to the enable
> register...
> 
> As a workaround, I added an irq_enable method to dw_pcie_msi_irq_chip
> that just chains to the parent, and then a hacky irq_enable in
> dw_pci_msi_bottom_irq_chip that manipulates the enable register.
> 
> Now it works again.  Race still present.  I don't see the
> dw_pci_msi_bottom_(un)mask methods ever get called.  I seem to recall
> that they are called as a substitute if enable/disable are not present,
> but haven't confirmed that, which would explain why they are not called
> after I added enable.

Hum, this probably is correlated with [1] where on the describition the this
enumerator says that "One shot does not require mask/unmask" see [2]

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/msi.c?h=v4.20-rc2#n1453

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/irq.h?h=v4.20-rc2#n504

I'm still waiting for an internal confirmation from the IP team about the good
procedure to take on this matter.

As soon I get something I'll post here.

Regards,
Gustavo

> 
> Next tried your next two patches.  No longer see lost interrupts, as
> the status is cleared before the handler is called.
> 
> From what I see the clear of the status bit is effectively at the same
> point in the irq path as the way I cleared it in my patch.  There's
> just a longer call chain to get to it in the ack method.  Not that it's
> not a better place for it (which isn't there in 4.16), but I don't
> think it changes anything.  Is there some reason dw_pci_bottom_ack
> would not be called?
> 
> Since I don't see the un(mask) methods ever get called, I'm not sure if
> they are correct or not.  I also had some unanswered details of
> behavior on unmask.  I can see possible flaws, depending on how this
> works.
>