Re: IMX6 dwc PCI regression through switch - unable to request (legacy) interrupt (pci=nomsi)

[Lucas Stach <l.stach@pengutronix.de>]

Am Mittwoch, den 27.02.2019, 08:29 -0800 schrieb Tim Harvey:
> > On Wed, Feb 27, 2019 at 2:01 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Hi Tim,
> > 
> > Am Dienstag, den 26.02.2019, 13:13 -0800 schrieb Tim Harvey:
> > > Greetings,
> > > 
> > > I've got a miniPCIe card with a TW6869 8x frame grabber that stopped
> > > working on an IMX6 based board with a PLX switch with Linux 4.17 as
> > > the driver errors out with 'tw686x 0000:07:00.0: unable to request
> > > interrupt' (request_irq() fails). I've found this only happens on
> > > boards that have a switch. Note I'm booting with pci=nomsi as well as
> > > the IMX6 has a limitation where legacy IRQ's wont fire if MSI irq's
> > > are enabled. Strangely I don't see this issue with other cards such
> > > as
> > > the ath9k with msi disabled.
> > > 
> > > I bisected the issue to 7c5925afbc58c6d6b384e1dc051bb992969bf787
> > > 'PCI:
> > > dwc: Move MSI IRQs allocation to IRQ domains hierarchical API' which
> > > due to continued changes in drivers/pci/dwc can no longer be
> > > reverted.
> > > 
> > > Any ideas what happened here? IMX6 PCIe through a bridge always seems
> > > not so well tested and very fragile over the past couple of years.
> > 
> > Thanks for the report. Both the DWC PCIe core in general and the i.MX6
> > integration have some rough edges, which makes things fall from time to
> > time. Having a PCIe bridge in the mix does extend the test space quite
> > a bit.
> > 
> > Does the following patch help? I didn't test it yet, but it's based on
> > my assumption of what is going wrong with the referenced commit.
> > 
> > Regards,
> > Lucas
> > 
> > ----------------------------->8--------------------------------------
> > 
> > From 40b4fc03878a935fa81af226a8e759d4c72a3603 Mon Sep 17 00:00:00 2001
> > From: Lucas Stach <l.stach@pengutronix.de>
> > Date: Wed, 27 Feb 2019 10:50:20 +0100
> > Subject: [PATCH] PCI: dwc: skip MSI init if MSIs have been explicitly disabled
> > 
> > Since 7c5925afbc58 (PCI: dwc: Move MSI IRQs allocation to IRQ domains
> > hierarchical API) the MSi init claims one of the controller IRQs as a
> > chained IRQ line for the MSI controller. On some designs, like the i.MX6,
> > this line is shared with a PCIe legacy IRQ. When the line is claimed for
> > the MSI domain, any device trying to use this legacy IRQs will fail to
> > request this IRQ line.
> > 
> > As MSI and legacy IRQs are already mutually exclusive on the DWC core,
> > as the core won't forward any legacy IRQs once any MSI has been enabled,
> > users wishing to use legacy IRQs already need to explictly disable MSI
> > support (usually via the pci=nomsi kernel commandline option). To avoid
> > any issues with MSI conflicting with legacy IRQs, just skip all of the
> > DWC MSI initalization, inclusing the IRQ line claim, when MSI is disabled.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 29a05759a294..f4a8494f616b 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -433,7 +433,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >         if (ret)
> >                 pci->num_viewport = 2;
> > 
> > -       if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +       if (IS_ENABLED(CONFIG_PCI_MSI) && pci_msi_enabled()) {
> >                 /*
> >                  * If a specific SoC driver needs to change the
> >                  * default number of vectors, it needs to implement
> > --
> > 2.20.1
> 
> Lucas,
> 
> Yes, this patch resolves the issue. Where did this come from? It looks
> like it came from back in September but never made it upstream?

Nope, I wrote it just after your report, as seen in the "Date" line.

> I'm a bit baffled why I see this issue pre-patch with 'and' without
> pci=nomsi based on this patch? I do see other checks in
> imx6_pcie_host_init() and imx6_add_pcie_port() for
> IS_ENABLED(CONFIG_PCI_MSI) that do not have an accompanying check of
> pci_msi_enabled() to see if its enabled at runtime... are these
> problematic as well?

I'll look over them, but I think with this added all the relevant
places are covered.

Regards,
Lucas