Date:   Sat, 8 Jan 2022 00:11:55 +0100
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Marek =?utf-8?B?QmVow7pu?= <kabel@kernel.org>,
        linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 05/15] PCI: mvebu: Disallow mapping interrupts on
 emulated bridges
Message-ID: <20220107231155.juv4skvyqunuilgk@pali>
References: <20220107221348.5s4ehqhxunmoacbm@pali>
 <20220107230155.GA425110@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220107230155.GA425110@bhelgaas>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Friday 07 January 2022 17:01:55 Bjorn Helgaas wrote:
> 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

Yes, looks like that zeros are in emulated config space for fields not
explicitly initialized. Which is the pci-mvebu.c case.

But now I'm looking at pci-aardvark.c driver and it sets
PCI_INTERRUPT_PIN register to A:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?h=v5.16-rc8#n953
And that comment "/* Support interrupt A for MSI feature */" must be
total nonsense as INTA for sure is not required for MSI... Plus we know
that pci-aardvark.c driver does not implement for pci bridge neither
INTx nor MSI... Ach... seems that this code is here since beginning and
needs to be fixed...