Re: Issues with OF_DYNAMIC PCI bridge node generation (kmemleak/interrupt-map IC reg property)

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Tue, Aug 12, 2025 at 11:59:06AM -0500, Rob Herring wrote:
> On Tue, Aug 12, 2025 at 10:53 AM Lizhi Hou <lizhi.hou@amd.com> wrote:
> >
> >
> > On 8/12/25 00:42, Lorenzo Pieralisi wrote:
> > > On Mon, Aug 11, 2025 at 08:26:11PM -0700, Lizhi Hou wrote:
> > >> On 8/11/25 01:42, Lorenzo Pieralisi wrote:
> > >>
> > >>> Hi Lizhi, Rob,
> > >>>
> > >>> while debugging something unrelated I noticed two issues
> > >>> (related) caused by the automatic generation of device nodes
> > >>> for PCI bridges.
> > >>>
> > >>> GICv5 interrupt controller DT top level node [1] does not have a "reg"
> > >>> property, because it represents the top level node, children (IRSes and ITSs)
> > >>> are nested.
> > >>>
> > >>> It does provide #address-cells since it has child nodes, so it has to
> > >>> have a "ranges" property as well.
> > >>>
> > >>> You have added code to automatically generate properties for PCI bridges
> > >>> and in particular this code [2] creates an interrupt-map property for
> > >>> the PCI bridges (other than the host bridge if it has got an OF node
> > >>> already).
> > >>>
> > >>> That code fails on GICv5, because the interrupt controller node does not
> > >>> have a "reg" property (and AFAIU it does not have to - as a matter of
> > >>> fact, INTx mapping works on GICv5 with the interrupt-map in the
> > >>> host bridge node containing zeros in the parent unit interrupt
> > >>> specifier #address-cells).
> > >> Does GICv5 have 'interrupt-controller' but not 'interrupt-map'? I think
> > >> of_irq_parse_raw will not check its parent in this case.
> > > But that's not the problem. GICv5 does not have an interrupt-map,
> > > the issue here is that GICv5 _is_ the parent and does not have
> > > a "reg" property. Why does the code in [2] check the reg property
> > > for the parent node while building the interrupt-map property for
> > > the PCI bridge ?
> >
> > Based on the document, if #address-cells is not zero, it needs to get
> > parent unit address. Maybe there is way to get the parent unit address
> > other than read 'reg'?  Or maybe zero should be used as parent unit
> > address if 'reg' does not exist?
> >
> > Rob, Could you give us some advise on this?
> 
> If there's no 'reg', then 'ranges' parent address can be used. If
> 'ranges' is boolean (i.e. 1:1), then shrug. Just use 0. Probably, 0
> should just always be used because I don't think it ever matters.
> 
> From my read of the kernel's interrupt parsing code, only the original
> device's node (i.e. the one with 'interrupts') address is ever used in
> the parsing and matching. So the values in the parent's address cells
> don't matter. If a subsequent 'interrupt-map' is the parent, then the
> code would compare the original address with the parent's
> interrupt-map entries (if not masked). That kind of seems wrong to me,
> but also unlikely to ever occur if it hasn't already. And that code is
> something I don't want to touch because we tend to break platforms
> when we do. The addresses are intertwined with the interrupt
> translating because interrupts used to be part of the buses (e.g ISA).
> That hasn't been the case for any h/w in the last 20 years.

If the parent address values don't matter I think we can just leave
them as zeroes and be done with it (explaining why obviously).

Something like this (with a big fat comment added summarizing this
thread):

Lizhi are you able to test it please at least to check it does not break
anything before I make it a patch for the MLs ?

Any concerns ?

-- >8 --
diff --git i/drivers/pci/of_property.c w/drivers/pci/of_property.c
index 506fcd507113..dd12691fe43c 100644
--- i/drivers/pci/of_property.c
+++ w/drivers/pci/of_property.c
@@ -279,13 +279,6 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
 			mapp++;
 			*mapp = out_irq[i].np->phandle;
 			mapp++;
-			if (addr_sz[i]) {
-				ret = of_property_read_u32_array(out_irq[i].np,
-								 "reg", mapp,
-								 addr_sz[i]);
-				if (ret)
-					goto failed;
-			}
 			mapp += addr_sz[i];
 			memcpy(mapp, out_irq[i].args,
 			       out_irq[i].args_count * sizeof(u32));