[PATCH v2] x86/PCI: Handle PIRQ routing tables with no router device given

[PATCH v2] x86/PCI: Handle PIRQ routing tables with no router device given

[Maciej W. Rozycki]

PIRQ routing tables provided by the PCI BIOS usually specify the PCI 
vendor:device ID as well as the bus address of the device implementing 
the PIRQ router, e.g.:

PCI: Interrupt Routing Table found at 0xc00fde10
[...]
PCI: Attempting to find IRQ router for [8086:7000]
pci 0000:00:07.0: PIIX/ICH IRQ router [8086:7000]

however in some cases they do not, in which case we fail to match the 
router handler, e.g.:

PCI: Interrupt Routing Table found at 0xc00fdae0
[...]
PCI: Attempting to find IRQ router for [0000:0000]
PCI: Interrupt router not found at 00:00

This is because we always match the vendor:device ID and the bus address 
literally, even if they are all zeros.

Handle this case then and iterate over all PCI devices until we find a 
matching router handler if the vendor ID given by the routing table is 
the invalid value of zero.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
Changes from v1:

- preinitialise `dev' in `pirq_find_router' for `for_each_pci_dev',

- avoid calling `pirq_try_router' with null `dev'.
---
 arch/x86/pci/irq.c |   64 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 20 deletions(-)

linux-x86-pirq-router-nodev.diff
Index: linux-macro-ide-tty/arch/x86/pci/irq.c
===================================================================
--- linux-macro-ide-tty.orig/arch/x86/pci/irq.c
+++ linux-macro-ide-tty/arch/x86/pci/irq.c
@@ -908,10 +908,32 @@ static struct pci_dev *pirq_router_dev;
  *	chipset" ?
  */
 
+static bool __init pirq_try_router(struct irq_router *r,
+				   struct irq_routing_table *rt,
+				   struct pci_dev *dev)
+{
+	struct irq_router_handler *h;
+
+	DBG(KERN_DEBUG "PCI: Trying IRQ router for [%04x:%04x]\n",
+	    dev->vendor, dev->device);
+
+	for (h = pirq_routers; h->vendor; h++) {
+		/* First look for a router match */
+		if (rt->rtr_vendor == h->vendor &&
+		    h->probe(r, dev, rt->rtr_device))
+			return true;
+		/* Fall back to a device match */
+		if (dev->vendor == h->vendor &&
+		    h->probe(r, dev, dev->device))
+			return true;
+	}
+	return false;
+}
+
 static void __init pirq_find_router(struct irq_router *r)
 {
 	struct irq_routing_table *rt = pirq_table;
-	struct irq_router_handler *h;
+	struct pci_dev *dev;
 
 #ifdef CONFIG_PCI_BIOS
 	if (!rt->signature) {
@@ -930,27 +952,29 @@ static void __init pirq_find_router(stru
 	DBG(KERN_DEBUG "PCI: Attempting to find IRQ router for [%04x:%04x]\n",
 	    rt->rtr_vendor, rt->rtr_device);
 
-	pirq_router_dev = pci_get_domain_bus_and_slot(0, rt->rtr_bus,
-						      rt->rtr_devfn);
-	if (!pirq_router_dev) {
-		DBG(KERN_DEBUG "PCI: Interrupt router not found at "
-			"%02x:%02x\n", rt->rtr_bus, rt->rtr_devfn);
-		return;
+	/* Use any vendor:device provided by the routing table or try all.  */
+	if (rt->rtr_vendor) {
+		dev = pci_get_domain_bus_and_slot(0, rt->rtr_bus,
+						  rt->rtr_devfn);
+		if (dev && pirq_try_router(r, rt, dev))
+			pirq_router_dev = dev;
+	} else {
+		dev = NULL;
+		for_each_pci_dev(dev) {
+			if (pirq_try_router(r, rt, dev)) {
+				pirq_router_dev = dev;
+				break;
+			}
+		}
 	}
 
-	for (h = pirq_routers; h->vendor; h++) {
-		/* First look for a router match */
-		if (rt->rtr_vendor == h->vendor &&
-			h->probe(r, pirq_router_dev, rt->rtr_device))
-			break;
-		/* Fall back to a device match */
-		if (pirq_router_dev->vendor == h->vendor &&
-			h->probe(r, pirq_router_dev, pirq_router_dev->device))
-			break;
-	}
-	dev_info(&pirq_router_dev->dev, "%s IRQ router [%04x:%04x]\n",
-		 pirq_router.name,
-		 pirq_router_dev->vendor, pirq_router_dev->device);
+	if (pirq_router_dev)
+		dev_info(&pirq_router_dev->dev, "%s IRQ router [%04x:%04x]\n",
+			 pirq_router.name,
+			 pirq_router_dev->vendor, pirq_router_dev->device);
+	else
+		DBG(KERN_DEBUG "PCI: Interrupt router not found at "
+		    "%02x:%02x\n", rt->rtr_bus, rt->rtr_devfn);
 
 	/* The device remains referenced for the kernel lifetime */
 }

Re: [PATCH v2] x86/PCI: Handle PIRQ routing tables with no router device given

[Nikolai Zhubr]

Hello Maciej,

06.07.2021 14:30, Maciej W. Rozycki:
> Changes from v1:
>
> - preinitialise `dev' in `pirq_find_router' for `for_each_pci_dev',
>
> - avoid calling `pirq_try_router' with null `dev'.
> ---
>   arch/x86/pci/irq.c |   64 ++++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 44 insertions(+), 20 deletions(-)
>
> linux-x86-pirq-router-nodev.diff

Success!
Here is new log:

https://pastebin.com/QXaUsCV4

and --

# 8259A.pl
irq 0: 00, edge
irq 1: 00, edge
irq 2: 00, edge
irq 3: 00, edge
irq 4: 00, edge
irq 5: 00, edge
irq 6: 00, edge
irq 7: 00, edge
irq 8: 02, edge
irq 9: 02, level
irq 10: 02, edge
irq 11: 02, edge
irq 12: 02, edge
irq 13: 02, edge
irq 14: 02, edge
irq 15: 02, edge

Some notes:
- please ignore the backtrace from 8139too, it is expected and will be 
worked on later;
- the message line " -> edge" looks somewhat strange, my guess is it 
might be a small unintentional leftover of some bigger and more detailed 
message/debugging, anyway, I'd humbly suppose "Triggering mode adjusted" 
would look better.

Thank you!


Regards,
Nikolai


> Index: linux-macro-ide-tty/arch/x86/pci/irq.c
> ===================================================================
> --- linux-macro-ide-tty.orig/arch/x86/pci/irq.c
> +++ linux-macro-ide-tty/arch/x86/pci/irq.c
> @@ -908,10 +908,32 @@ static struct pci_dev *pirq_router_dev;
>    *	chipset" ?
>    */
>
> +static bool __init pirq_try_router(struct irq_router *r,
> +				   struct irq_routing_table *rt,
> +				   struct pci_dev *dev)
> +{
> +	struct irq_router_handler *h;
> +
> +	DBG(KERN_DEBUG "PCI: Trying IRQ router for [%04x:%04x]\n",
> +	    dev->vendor, dev->device);
> +
> +	for (h = pirq_routers; h->vendor; h++) {
> +		/* First look for a router match */
> +		if (rt->rtr_vendor == h->vendor&&
> +		    h->probe(r, dev, rt->rtr_device))
> +			return true;
> +		/* Fall back to a device match */
> +		if (dev->vendor == h->vendor&&
> +		    h->probe(r, dev, dev->device))
> +			return true;
> +	}
> +	return false;
> +}
> +
>   static void __init pirq_find_router(struct irq_router *r)
>   {
>   	struct irq_routing_table *rt = pirq_table;
> -	struct irq_router_handler *h;
> +	struct pci_dev *dev;
>
>   #ifdef CONFIG_PCI_BIOS
>   	if (!rt->signature) {
> @@ -930,27 +952,29 @@ static void __init pirq_find_router(stru
>   	DBG(KERN_DEBUG "PCI: Attempting to find IRQ router for [%04x:%04x]\n",
>   	    rt->rtr_vendor, rt->rtr_device);
>
> -	pirq_router_dev = pci_get_domain_bus_and_slot(0, rt->rtr_bus,
> -						      rt->rtr_devfn);
> -	if (!pirq_router_dev) {
> -		DBG(KERN_DEBUG "PCI: Interrupt router not found at "
> -			"%02x:%02x\n", rt->rtr_bus, rt->rtr_devfn);
> -		return;
> +	/* Use any vendor:device provided by the routing table or try all.  */
> +	if (rt->rtr_vendor) {
> +		dev = pci_get_domain_bus_and_slot(0, rt->rtr_bus,
> +						  rt->rtr_devfn);
> +		if (dev&&  pirq_try_router(r, rt, dev))
> +			pirq_router_dev = dev;
> +	} else {
> +		dev = NULL;
> +		for_each_pci_dev(dev) {
> +			if (pirq_try_router(r, rt, dev)) {
> +				pirq_router_dev = dev;
> +				break;
> +			}
> +		}
>   	}
>
> -	for (h = pirq_routers; h->vendor; h++) {
> -		/* First look for a router match */
> -		if (rt->rtr_vendor == h->vendor&&
> -			h->probe(r, pirq_router_dev, rt->rtr_device))
> -			break;
> -		/* Fall back to a device match */
> -		if (pirq_router_dev->vendor == h->vendor&&
> -			h->probe(r, pirq_router_dev, pirq_router_dev->device))
> -			break;
> -	}
> -	dev_info(&pirq_router_dev->dev, "%s IRQ router [%04x:%04x]\n",
> -		 pirq_router.name,
> -		 pirq_router_dev->vendor, pirq_router_dev->device);
> +	if (pirq_router_dev)
> +		dev_info(&pirq_router_dev->dev, "%s IRQ router [%04x:%04x]\n",
> +			 pirq_router.name,
> +			 pirq_router_dev->vendor, pirq_router_dev->device);
> +	else
> +		DBG(KERN_DEBUG "PCI: Interrupt router not found at "
> +		    "%02x:%02x\n", rt->rtr_bus, rt->rtr_devfn);
>
>   	/* The device remains referenced for the kernel lifetime */
>   }
>

Re: [PATCH v2] x86/PCI: Handle PIRQ routing tables with no router device given

[Maciej W. Rozycki]

Hi Nikolai,

> > Changes from v1:
> > 
> > - preinitialise `dev' in `pirq_find_router' for `for_each_pci_dev',
> > 
> > - avoid calling `pirq_try_router' with null `dev'.
> > ---
> >   arch/x86/pci/irq.c |   64
> > ++++++++++++++++++++++++++++++++++++-----------------
> >   1 file changed, 44 insertions(+), 20 deletions(-)
> > 
> > linux-x86-pirq-router-nodev.diff
> 
> Success!
> Here is new log:
> 
> https://pastebin.com/QXaUsCV4

 This is great news, thank you for doing this verification!

 I have since discovered and posted a fix for an issue with our PIRQ
routing code that turned out incapable of routing interrupts behind 
PCI-to-PCI bridges, which were sometimes used even on classic PCI option 
cards (I think I have at least two such devices) either to bundle multiple 
devices or to meet PCI bus load limits.

 Such devices may work regardless if the BIOS has been developed enough to 
handle them and assign an IRQ number, but surely they make the likelihood 
of interrupt sharing rise considerably, so I think the more of them we can 
handle ourselves the better.  Plus these days now that PCIe has come into 
picture you can get quite complex topologies with multiple logical 
PCI-to-PCI bridges via external expansion even with classic PCI systems.

 So I have also posted a PIRQ router implementation for the Intel SIO 
southbridge and I have also identified further two Intel devices, the IB 
ISA bridge and the PCEB/ESC EISA bridge combo, that have their PIRQ 
routers documented in my resources but not handled in Linux.  I'll see if 
I can find some time the following days to get them implemented too just 
for the sake of people experimenting with odd hardware.

 Have you tried contacting Nvidia about your ALI chipset?  Back in the day 
I tried to avoid undocumented stuff and Intel was reasonably open about 
most of their chipsets.

  Maciej

Re: [PATCH v2] x86/PCI: Handle PIRQ routing tables with no router device given

[Nikolai Zhubr]

08.07.2021 23:45, Maciej W. Rozycki:
>   Have you tried contacting Nvidia about your ALI chipset?  Back in the day
> I tried to avoid undocumented stuff and Intel was reasonably open about
> most of their chipsets.

Well, being neither their customer nor a kernel developer, I'm not sure 
my request would be considered serious. Anyway, probably I'll give it a 
try a bit later when I have an opportunity to dismount this board for 
more comfortable testing. I was also going to try to modify its BIOS to 
remove some unwanted behaviour unrelated to IRQs, and it might happen 
that I also discover something about PCI handling along with (It is just 
64k size, after all, and I have a 8086 debugger)


Thank you,

Regards,
Nikolai


>    Maciej
>

Re: [PATCH v2] x86/PCI: Handle PIRQ routing tables with no router device given

[Maciej W. Rozycki]

On Fri, 9 Jul 2021, Nikolai Zhubr wrote:

> >   Have you tried contacting Nvidia about your ALI chipset?  Back in the day
> > I tried to avoid undocumented stuff and Intel was reasonably open about
> > most of their chipsets.
> 
> Well, being neither their customer nor a kernel developer, I'm not sure my
> request would be considered serious. Anyway, probably I'll give it a try a bit
> later when I have an opportunity to dismount this board for more comfortable
> testing.

 It never hurts asking.  At worst you'll be ignored, and at the second 
worst they'll say nay.  They may have lost it too.

> I was also going to try to modify its BIOS to remove some unwanted
> behaviour unrelated to IRQs, and it might happen that I also discover
> something about PCI handling along with (It is just 64k size, after all, and I
> have a 8086 debugger)

 Umm, the board may be old enough not to play any tricks with the BIOS, 
but mind that at reset x86 starts in the flat mode from 0xfffffff0 and the 
contents of ROM there may not be what you see at 0xf000:0xfff0 later on.

 I once worked on a project where I had an opportunity to access the BIOS 
at the reset vector (and poke at CPU registers, run, stop, single-step it, 
place hardware breakpoints, etc.) using GDB over JTAG with an Intel Atom 
board.  It was an interesting experience, but sadly most x86 hardware does 
not have the capability let alone a JTAG connector (called XDP or eXtended 
Debug Port in Intel-speak) to attach a probe to.

 You may try disassembling the PCI BIOS 2.1 service however.

  Maciej