Message-ID: <aJrwgKUNh68Dx1Fo@lpieralisi>
Date: Tue, 12 Aug 2025 09:42:56 +0200
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Lizhi Hou <lizhi.hou@....com>
Cc: Rob Herring <robh@...nel.org>, maz@...nel.org,
	devicetree@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: Issues with OF_DYNAMIC PCI bridge node generation
 (kmemleak/interrupt-map IC reg property)

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 ?

> > It is not clear to me why, to create an interrupt-map property, we
> > are reading the "reg" value of the parent IC node to create the
> > interrupt-map unit interrupt specifier address bits (could not we
> > just copy the address in the parent unit interrupt specifier reported
> > in the host bridge interrupt-map property ?).
> > 
> > - #address-cells of the parent describes the number of address cells of
> >    parent's child nodes not the parent itself, again, AFAIK, so parsing "reg"
> >    using #address-cells of the parent node is not entirely correct, is it ?
> > - It is unclear to me, from an OF spec perspective what the address value
> >    in the parent unit interrupt specifier ought to be. I think that, at
> >    least for dts including a GICv3 IC, the address values are always 0,
> >    regardless of the GICv3 reg property.
> > 
> > I need your feedback on this because the automatic generation must
> > work seamlessly for GICv5 as well (as well as all other ICs with no "reg"
> > property) and I could not find anything in the OF specs describing
> > how the address cells in the unit interrupt specifier must be computed.
> 
> Please see: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html
> 
> 2.4.3.1 mentions:
> 
> "Both the child node and the interrupt parent node are required to have
> #address-cells and #interrupt-cells properties defined. If a unit address
> component is not required, #address-cells shall be explicitly defined to be
> zero."

Yes, but again, that's not what I am asking. GICv5 requires
#address-cells (2.3.5 - link above - it has child nodes and it
has to define "ranges") but it does not require a "reg" property,
code in [2] fails.

That boils down to what does "a unit address component is not required"
means.

Why does the code in [2] read "reg" to build the parent unit interrupt
specifier (with #address-cells size of the parent, which, again, I
don't think it is correct) ?

> > I found this [3] link where in section 7 there is an interrupt mapping
> > algorithm; I don't understand it fully and I think it is possibly misleading.
> > 
> > Now, the failure in [2] (caused by the lack of a "reg" property in the
> > IC node) triggers an interrupt-map property generation failure for PCI
> > bridges that are upstream devices that need INTx swizzling.
> > 
> > In turn, that leads to a kmemleak detection:
> > 
> > unreferenced object 0xffff000800368780 (size 128):
> >    comm "swapper/0", pid 1, jiffies 4294892824
> >    hex dump (first 32 bytes):
> >      f0 b8 34 00 08 00 ff ff 04 00 00 00 00 00 00 00  ..4.............
> >      70 c2 30 00 08 00 ff ff 00 00 00 00 00 00 00 00  p.0.............
> >    backtrace (crc 1652b62a):
> >      kmemleak_alloc+0x30/0x3c
> >      __kmalloc_cache_noprof+0x1fc/0x360
> >      __of_prop_dup+0x68/0x110
> >      of_changeset_add_prop_helper+0x28/0xac
> >      of_changeset_add_prop_string+0x74/0xa4
> >      of_pci_add_properties+0xa0/0x4e0
> >      of_pci_make_dev_node+0x198/0x230
> >      pci_bus_add_device+0x44/0x13c
> >      pci_bus_add_devices+0x40/0x80
> >      pci_host_probe+0x138/0x1b0
> >      pci_host_common_probe+0x8c/0xb0
> >      platform_probe+0x5c/0x9c
> >      really_probe+0x134/0x2d8
> >      __driver_probe_device+0x98/0xd0
> >      driver_probe_device+0x3c/0x1f8
> >      __driver_attach+0xd8/0x1a0
> > 
> > I have not grokked it yet but it seems genuine, so whatever we decide
> > in relation to "reg" above, this ought to be addressed too, if it
> > is indeed a memleak.
> 
> Not sure what is the leak. I will look into more.

Thanks,
Lorenzo

> 
> 
> Lizhi
> 
> > 
> > Please let me know if something is unclear I can provide further
> > details.
> > 
> > Thanks,
> > Lorenzo
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml?h=v6.17-rc1
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/of_property.c?h=v6.17-rc1#n283
> > [3] https://www.devicetree.org/open-firmware/practice/imap/imap0_9d.html

Powered by blists - more mailing lists