linux-kernel - Re: [PATCH] PCI: j721e: Set .map_irq and .swizzle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240726115609.GF2628@thinkpad>
Date: Fri, 26 Jul 2024 17:26:09 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
	robh@...nel.org, vigneshr@...com, kishon@...nel.org,
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, stable@...r.kernel.org,
	ahalaney@...hat.com, srk@...com
Subject: Re: [PATCH] PCI: j721e: Set .map_irq and .swizzle_irq to NULL

On Thu, Jul 25, 2024 at 01:50:16PM +0530, Siddharth Vadapalli wrote:
> On Thu, Jul 25, 2024 at 09:50:01AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Jul 24, 2024 at 09:49:21PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jul 24, 2024 at 12:20:48PM +0530, Siddharth Vadapalli wrote:
> > > > Since the configuration of Legacy Interrupts (INTx) is not supported, set
> > > > the .map_irq and .swizzle_irq callbacks to NULL. This fixes the error:
> > > >   of_irq_parse_pci: failed with rc=-22
> > > > due to the absence of Legacy Interrupts in the device-tree.
> > > > 
> > > 
> > > Do you really need to set 'swizzle_irq' to NULL? pci_assign_irq() will bail out
> > > if 'map_irq' is set to NULL.
> > > 
> > 
> > Hold on. The errono of of_irq_parse_pci() is not -ENOENT. So the INTx interrupts
> > are described in DT? Then why are they not supported?
> 
> No, the INTx interrupts are not described in the DT. It is the pcieport
> driver that is attempting to setup INTx via "of_irq_parse_and_map_pci()"
> which is the .map_irq callback. The sequence of execution leading to the
> error is as follows:
> 
> pcie_port_probe_service()
>   pci_device_probe()
>     pci_assign_irq()
>       hbrg->map_irq
>         of_pciof_irq_parse_and_map_pci()
> 	  of_irq_parse_pci()
> 	    of_irq_parse_raw()
> 	      rc = -EINVAL
> 	      ...
> 	      [DEBUG] OF: of_irq_parse_raw: ipar=/bus@...000/interrupt-controller@...0000, size=3
> 	      if (out_irq->args_count != intsize)
> 	        goto fail
> 		  return rc
> 
> The call to of_irq_parse_raw() results in the Interrupt-Parent for the
> PCIe node in the device-tree being found via of_irq_find_parent(). The
> Interrupt-Parent for the PCIe node for MSI happens to be GIC_ITS:
> msi-map = <0x0 &gic_its 0x0 0x10000>;
> and the parent of GIC_ITS is:
> gic500: interrupt-controller@...0000
> which has the following:
> #interrupt-cells = <3>;
> 
> The "size=3" portion of the DEBUG print above corresponds to the
> #interrupt-cells property above. Now, "out_irq->args_count" is set to 1
> as __assumed__ by of_irq_parse_pci() and mentioned as a comment in that
> function:
> 	/*
> 	 * Ok, we don't, time to have fun. Let's start by building up an
> 	 * interrupt spec.  we assume #interrupt-cells is 1, which is standard
> 	 * for PCI. If you do different, then don't use that routine.
> 	 */
> 
> In of_irq_parse_pci(), since the PCIe-Port driver doesn't have a
> device-tree node, the following doesn't apply:
>   dn = pci_device_to_OF_node(pdev);
> and we skip to the __assumption__ above and proceed as explained in the
> execution sequence above.
> 
> If the device-tree nodes for the INTx interrupts were present, the
> "ipar" sequence to find the interrupt parent would be skipped and we
> wouldn't end up with the -22 (-EINVAL) error code.
> 
> I hope this clarifies the relation between the -22 error code and the
> missing device-tree nodes for INTx.
> 

Thanks for explaining the logic. Still I think the logic is flawed. Because the
parent (host bridge) doesn't have 'interrupt-map', which means INTx is not
supported. But parsing one level up to the GIC node and not returning -ENOENT
doesn't make sense to me.

Rob, what is your opinion on this behavior?

- Mani

-- 
மணிவண்ணன் சதாசிவம்