[PATCH] fdtdec: drop needlessly convoluted CONFIG_PHANDLE_CHECK_SEQ

[Rasmus Villemoes]

On 08/06/2022 16.18, Eugen.Hristev at microchip.com wrote:
> On 5/19/22 3:16 PM, Rasmus Villemoes wrote:
>> On 19/05/2022 13.50, Aswath Govindraju wrote:
>>
>>> Understood, thanks for the explanation. I am good with this patch.
>>>
>>> Acked-by: Aswath Govindraju <a-govindraju at ti.com>
>>>
>>
>> Thanks.
>>
>> For completeness, to expand on this:
>>
>>>> it's somewhat fragile to rely on (at least one of) the
>>>> nodes in question to even have a phandle.
>>
>> One way in which to ensure all nodes (with a label) do get a phandle is
>> to build with the -@ flag to dtc, which is implied by setting
>> CONFIG_OF_LIBFDT_OVERLAY=y. Three of the four _defconfigs that set
>> PHANDLE_CHECK_SEQ also set that option, so for them it was guaranteed to
>> work, but mostly by chance - if I randomly discovered
>> CONFIG_PHANDLE_CHECK_SEQ and found I needed to enable it for my board, I
>> wouldn't know to also enable CONFIG_OF_LIBFDT_OVERLAY. And if I didn't
>> need overlay support as such, it would also bloat the .dtb (and U-Boot
>> itself) needlessly; the difference for sama7g5ek_mmc_defconfig is 34784
>> bytes for the current .dtb and 26920 bytes if one disables
>> OF_LIBFDT_OVERLAY.
>>
>> The last _defconfig with PHANDLE_CHECK_SEQ=y didn't actually seem to
>> need it; I've built it and looked at u-boot.dtb, and there are no
>> collisions in basenames in the aliases. Commit ddd778ae doesn't say
>> anything about why it was added.
>>
>> Rasmus
>>
> 
> 
> Hi Rasmus,
> 
> For sama7g5 boards, we have the following situation:
> 
> some node {
> 	i2c1: i2c at 600: {
> 		some props;
> 	};
> };
> 
> some other node {
> 	i2c2: i2c at 600: {
> 		other props;
> 	};
> };
> 
> and then we want to be able to reference
> aliases {
> 	i2c0 = &i2c1;
> 	i2c1 = &i2c2;
> };
> 
> 
> Without CONFIG_PHANDLE_CHECK_SEQ , both i2c0 and i2c1 aliases pointed to 
> the same 'i2c at 600' node, because the code was searching through the DT 
> for 'i2c at 600' and it was hitting the first in both cases.
> 
> With your patch, it can happen that we hit the same problem again ? Or 
> we should be safe ?

I removed the config option partly because one shouldn't need to know
about some magic config option to get correct behaviour. The check which
was previously hidden behind that config knob is now done
unconditionally, and doesn't implicitly rely on the .dtb being built
with -@, and at a lower cost (because two redundant phandle lookups are
gone, and those phandle lookups was what was making the check non-robust
anyway).

Rasmus

And no, the aliases were each pointing at the correct node (the i2c0 =
&i2c1 is merely a shorthand for i2c0 = "/the/full/path/to/the/node").
What this check is about is when one comes with a pointer to some node
and wants to know if there is some i2cX alias pointing at that node, and
if so, what the value of X is - and without properly comparing the full
paths but merely the name of the node passed in to the basename of the
value of the alias, one would incorrectly assign the seq number 0 to
both nodes.