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

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jan 07, 2022 at 11:13:48PM +0100, Pali Rohár wrote:
> 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@kernel.org/
> 
> > > 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@kernel.org>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  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(...);". 

OK, so even after you implement INTx for the emulated Root Ports, the
default of_irq_parse_and_map_pci() is insufficient, and you will
require an mvebu .map_irq() function.  That's reasonable.

"PCI_INTERRUPT_PIN == 0" is the way software learns that a device
doesn't use INTx, of course, and I suppose PCI_INTERRUPT_PIN already
reads as zero, since mvebu_pci_bridge_emul_init() doesn't set
bridge->conf.intpin, and I assume the default value would be zero?

Bjorn