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

[Lizhi Hou <lizhi.hou@amd.com>]

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