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

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

[Lorenzo Pieralisi]

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).

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.

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.

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

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

[Lizhi Hou]

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.
>
> 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."

>
> 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.


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

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

[Lorenzo Pieralisi]

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

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

[Lizhi Hou]

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?


Thanks,

Lizhi


>
>>> 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

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

[Lorenzo Pieralisi]

On Tue, Aug 12, 2025 at 08:53:06AM -0700, Lizhi Hou 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'?

Reading the parent "reg" using the parent #address-cells as address size does
not seem correct to me anyway.

> Or maybe zero should be used as parent unit address if 'reg' does not
> exist?

zeros are used for eg GICv3 interrupt-map properties, I suppose that's
a wild card to say "any device in the interrupt-controller bus",
whatever that means.

Just my interpretation, I don't know the history behind this.

> Rob, Could you give us some advise on this?

Please, thanks.

Lorenzo

> Thanks,
> 
> Lizhi
> 
> 
> > 
> > > > 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

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

[Rob Herring]

On Tue, Aug 12, 2025 at 11:17 AM Lorenzo Pieralisi
<lpieralisi@kernel.org> wrote:
>
> On Tue, Aug 12, 2025 at 08:53:06AM -0700, Lizhi Hou 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'?
>
> Reading the parent "reg" using the parent #address-cells as address size does
> not seem correct to me anyway.

Right, the parent #address-cells applies to reg/ranges(parent addr) in
the child node.

> > Or maybe zero should be used as parent unit address if 'reg' does not
> > exist?
>
> zeros are used for eg GICv3 interrupt-map properties, I suppose that's
> a wild card to say "any device in the interrupt-controller bus",
> whatever that means.
>
> Just my interpretation, I don't know the history behind this.

They should be zero simply because a device's address never has any
influence with a device's interrupt connection to a GIC (or any SoC
interrupt controller).

Rob

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

[Rob Herring]

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.

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

[Lorenzo Pieralisi]

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));

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

[Lizhi Hou]

On 8/13/25 01:38, Lorenzo Pieralisi wrote:
> 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 ?

I tested it and did not see any issue on my side.


Thanks,

Lizhi

>
> 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));