Message-ID: <e15ebb26-15ac-ef7a-c91b-28112f44db55@amd.com>
Date: Tue, 12 Aug 2025 08:53:06 -0700
From: Lizhi Hou <lizhi.hou@....com>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>, Rob Herring <robh@...nel.org>
CC: <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 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

Powered by blists - more mailing lists