Re: [PATCH 05/15] PCI: mvebu: Disallow mapping interrupts on emulated bridges

[Pali Rohár]

On Friday 07 January 2022 15:32:16 Bjorn Helgaas wrote:
> On Thu, Nov 25, 2021 at 01:45:55PM +0100, Pali Rohár wrote:
> > Interrupt support on mvebu emulated bridges is not implemented yet.
> 
> Is this mvebu-specific, or is aardvar also affected?

This is pci-mvebu.c driver specific, it does not implement emulation of
neither INTx, nor MSI interrupts for emulated pci bridge (root port). As
we know this HW does not have compliant pci root port, it needs to be
emulated in driver, and emulation for interrupts is missing. (it means
that also AER interrupt is missing).

And pci-aardvark.c driver has same issue and similar patch is required
for pci-aardvark.c too. Marek should take care of it. But for
pci-aardvark we already have implementation which emulates INTx
interrupts and it is waiting for review on the list:
https://lore.kernel.org/linux-pci/20211208061851.31867-1-kabel@xxxxxxxxxx/

> > So properly indicate return value to callers that they cannot request
> > interrupts from emulated bridge.
> 
> Pet peeve: descriptions that say "do this *properly*".  As though the
> previous authors were just ignorant or intentionally did something
> *improperly* :)
> 
> > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >  drivers/pci/controller/pci-mvebu.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > index 19c6ee298442..a3df352d440e 100644
> > --- a/drivers/pci/controller/pci-mvebu.c
> > +++ b/drivers/pci/controller/pci-mvebu.c
> > @@ -705,6 +705,15 @@ static struct pci_ops mvebu_pcie_ops = {
> >  	.write = mvebu_pcie_wr_conf,
> >  };
> >  
> > +static int mvebu_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> > +{
> > +	/* Interrupt support on mvebu emulated bridges is not implemented yet */
> > +	if (dev->bus->number == 0)
> > +		return 0; /* Proper return code 0 == NO_IRQ */
> > +
> > +	return of_irq_parse_and_map_pci(dev, slot, pin);
> 
> Is this something that could be done with a .read_base() op, e.g.,
> make PCI_INTERRUPT_PIN contain zero (PCI_INTERRUPT_UNKNOWN)?

I'm not sure... maybe. I choose this style as after I implement
emulation of INTx interrupts it allows me just to replace "return 0;" by
"return my_mapping_function_for_root_port(...);". Similarly like it is
in pending pci-aardvark.c patch:
https://lore.kernel.org/linux-pci/20211208061851.31867-15-kabel@xxxxxxxxxx/

> > +}
> > +
> >  static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
> >  						 const struct resource *res,
> >  						 resource_size_t start,
> > @@ -1119,6 +1128,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
> >  	bridge->sysdata = pcie;
> >  	bridge->ops = &mvebu_pcie_ops;
> >  	bridge->align_resource = mvebu_pcie_align_resource;
> > +	bridge->map_irq = mvebu_pcie_map_irq;
> 
> I assume this means INTx doesn't work for some devices?  Which ones?
> I guess anything on the root bus?  But INTx for devices *below* these
> emulated Root Ports *does* work?

Exactly. All devices except emulated root ports (which are on bus 0)
have working MSI, MSI-X and INTx interrupts.

> >  	return pci_host_probe(bridge);
> >  }
> > -- 
> > 2.20.1
> >