Received-SPF: pass (google.com: domain of linux-kernel+bounces-147534-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 16 Apr 2024 15:46:53 -0500
From: Bjorn Helgaas <helgaas@kernel.org>
To: Sunil V L <sunilvl@ventanamicro.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org, linux-acpi@vger.kernel.org,
	linux-pci@vger.kernel.org, acpica-devel@lists.linux.dev,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Anup Patel <anup@brainfault.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Samuel Holland <samuel.holland@sifive.com>,
	Robert Moore <robert.moore@intel.com>,
	Haibo1 Xu <haibo1.xu@intel.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	Andrew Jones <ajones@ventanamicro.com>,
	Atish Kumar Patra <atishp@rivosinc.com>,
	Andrei Warkentin <andrei.warkentin@intel.com>,
	Marc Zyngier <maz@kernel.org>,
	=?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
Subject: Re: [RFC PATCH v4 03/20] PCI: Make pci_create_root_bus() declare its
 reliance on MSI domains
Message-ID: <20240416204653.GA164172@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zh41pOmtAJ0EcbiN@sunil-laptop>

On Tue, Apr 16, 2024 at 01:54:04PM +0530, Sunil V L wrote:
> Hi Bjorn,
> 
> On Mon, Apr 15, 2024 at 06:15:23PM -0500, Bjorn Helgaas wrote:
> > On Mon, Apr 15, 2024 at 10:30:56PM +0530, Sunil V L wrote:
> > > Similar to commit 9ec37efb8783 ("PCI/MSI: Make
> > > pci_host_common_probe() declare its reliance on MSI domains"), declare
> > > this dependency for PCI probe in ACPI based flow.
> > > 
> > > This is required especially for RISC-V platforms where MSI controller
> > > can be absent. However, setting this for all architectures seem to cause
> > > issues on non RISC-V architectures [1]. Hence, enabled this only for
> > > RISC-V.
> > > 
> > > [1] - https://lore.kernel.org/oe-lkp/202403041047.791cb18e-oliver.sang@intel.com
> > > 
> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > ---
> > >  drivers/pci/probe.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 1325fbae2f28..e09915bee2ee 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -3048,6 +3048,9 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > >  	bridge->sysdata = sysdata;
> > >  	bridge->busnr = bus;
> > >  	bridge->ops = ops;
> > > +#ifdef CONFIG_RISCV
> > > +	bridge->msi_domain = true;
> > > +#endif
> > 
> > Ugh.  I looked at [1], but that's not a very good justification for
> > this #ifdef.  The fault mentioned in [1] would need to be fixed, but
> > not this way.
> 
> Thank you again for the feedback!
> 
> I agree. This is due to my limitation with knowledge and resources to
> debug the issue happening on non-UEFI x86 system with some particular
> PCIe RC. Also, I was worried that we get into a rat hole of
> assumptions/quirks with various architecture/PCIe RC combinations.

The problem is that adding #ifdefs like this leads to a rat hole
itself.  We need to understand and fix the underlying issue instead.

> For ex: I think the issue is, somehow MSI domain is not set at the time
> of PCI host bridge registration in pci_register_host_bridge() causing
> PCI_BUS_FLAGS_NO_MSI to be set. This causes pci_alloc_irq_vectors() to
> fail. In portdrv.c, pcie_init_service_irqs() doesn't switch to INTx
> handling if MSI can not be used. It switches only if pcie_pme_no_msi()
> returns true. I couldn't find who actually sets up MSI domain bit late
> on this platform so that it somehow worked when we didn't set this flag.
> 
> Unfortunately, I don't have system to root cause and fix this issue with
> confidence. Also, I don't know if any other architectures have similar
> issues which are not caught yet. Hence, I thought it may be better
> just restrict the change to RISC-V.

It sounds like the above is a good start on analyzing the problem.

I don't quite understand your statement that pcie_init_service_irqs()
doesn't fall back to INTx when MSI/MSI-X is not available.

I'm looking at this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pcie/portdrv.c?id=v6.8#n177
pcie_port_enable_irq_vec() attempts
pci_alloc_irq_vectors(PCI_IRQ_MSIX | PCI_IRQ_MSI) and returns 0 if
successful.  If it returns failure, it looks like
pcie_init_service_irqs() *does* fall through to trying INTx
(PCI_IRQ_LEGACY).

Bjorn