On Thu, Aug 08, 2024 at 03:56:10PM -0500, Andrew Halaney wrote:
[...]
There's a lot of history and the interrupt parsing is fragile due to all the "interesting" DT interrupt hierarchies. So while I think it would work, that's just a guess. I'm open to trying it and seeing.
Would something like this be what you're imagining? If so I can post a patch if this patch is a dead end:
diff --git a/drivers/pci/of.c b/drivers/pci/of.c index dacea3fc5128..4e4ecaa95599 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -512,6 +512,10 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args * if (ppnode == NULL) { rc = -EINVAL; goto err; + } else if (!of_get_property(ppnode, "interrupt-map", NULL)) { + /* No interrupt-map on a host bridge means we're done here */ + rc = -ENOENT; + goto err; } } else { /* We found a P2P bridge, check if it has a node */
This is a reasonable change if the parent is the host bridge. But if parent is a PCI bridge node (note the else condition), then of_irq_parse_raw() will get called and we will hit the same issue.
IMO, either we need to fix of_irq_parse_raw() or come up with another implementation that does the right thing i.e., travese upto the host bridge and check for the 'interrupt-map'. Currently it goes till the top level interrupt controller.
I must admit that you being nervous has me being nervous since I'm not all that familiar with PCI... but if y'all think this is ok then I'm for it. I'm sure I'm not picturing all the cases here so would appreciate some scrutiny.
You still end up with warnings, which kind of sucks, since as I understand it the lack of INTx interrupts on this platform is *intentional*:
[ 3.342548] pci_bus 0000:00: 2-byte config write to 0000:00:00.0 offset 0x4 may corrupt adjacent RW1C bits [ 3.346716] pcieport 0000:00:00.0: of_irq_parse_pci: no interrupt-map found, INTx interrupts not available [ 3.346721] PCI: OF: of_irq_parse_pci: possibly some PCI slots don't have level triggered interrupts capability
I propose to demote these prints to debug as these are not warnings by any means.
You could have a combo of both this patch (to indicate that a specific driver (even further limited to a match data based on compatible) doesn't support these) as well as the above diff (to improve the message printed in the situation where a driver *does* claim to support these interrupts but fails to describe them properly).
Again, I don't think we need to have the change in the driver. DT already indicated that there is no INTx support, so why should driver duplicate the same info?
- Mani