| Message ID | 20240415170113.662318-4-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V: ACPI: Add external interrupt controller support | expand |
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. > error = pci_register_host_bridge(bridge); > if (error < 0) > -- > 2.40.1 >
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. 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. Let me know your thoughts. If there are better ways, I will be happy to update. Thanks, Sunil
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
On Tue, Apr 16, 2024 at 03:46:53PM -0500, Bjorn Helgaas wrote: > 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. > Agree. Ideally, from my reading of code, this change should have worked across architectures. > > 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). > You are right. Not sure what I was looking at :-(. I think fundamentally there are two issues here. 1) MSI domain should have been setup properly when pci_register_host_bridge() is called. I see that pci_arch_init() which is supposed to get called early calls x86_create_pci_msi_domain(). pci_register_host_bridge() also calls pci_set_bus_msi_domain() to setup the MSI domain which can walk up to host bridge to find. So, not sure why PCI_BUS_FLAGS_NO_MSI is getting set. Is there an issue in walking up the tree? 2) When it switches to legacy interrupt since MSI domain is not found, for some reason there is an interrupt enabled without a handler. I was suspecting PME since it was matching the IRQ#16 but it looks like PME handlers are present. I am unable to find anything suspicious from the log alone. I really don't know how to proceed further. With my limited understanding, I don't get any hint what is happening from the log. Thanks, Sunil
Hi Bjorn, On Wed, Apr 17, 2024 at 09:03:13PM +0530, Sunil V L wrote: <snip> > > I think fundamentally there are two issues here. > > 1) MSI domain should have been setup properly when > pci_register_host_bridge() is called. I see that pci_arch_init() which > is supposed to get called early calls x86_create_pci_msi_domain(). > pci_register_host_bridge() also calls pci_set_bus_msi_domain() to setup > the MSI domain which can walk up to host bridge to find. So, not sure > why PCI_BUS_FLAGS_NO_MSI is getting set. Is there an issue in walking up > the tree? > I think I understand now why this is happening on X86. In X86, even though x86_create_pci_msi_domain() sets up the MSI domain, it never informs IRQ framework. Only later in pcibios_device_add(), the device's MSI domain will be set which is too late for this bridge logic. So, when pci_set_bus_msi_domain() is called from pci_register_host_bridge(), it will not get it. It works in RISC-V (I guess ARM64 as well) because, the irqchip driver for MSI controller registers using pci_msi_register_fwnode_provider(). But, if we set bridge->msi_domain = true in X86, it will disable MSI even though it supports MSI which is really bad. Please correct me if I am wrong with the analysis. If correct, what do you think the way forward if using CONFIG option is not good? I tried to register the domain in below commit but I don't feel it is a cleaner way. https://github.com/vlsunil/linux/commit/d6cf76f10c225c94fffb286b7a3e803a4e37df54 Thanks, Sunil
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 error = pci_register_host_bridge(bridge); if (error < 0)
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(+)