Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record

[Tyrel Datwyler <tyreld@linux.ibm.com>]

On 3/11/20 10:43 PM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> On 3/10/20 10:25 AM, Nathan Lynch wrote:
>>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>>> The expectation is that when calling of_read_drc_info_cell()
>>>> repeatedly to parse multiple drc-info records that the in/out curval
>>>> parameter points at the start of the next record on return. However,
>>>> the current behavior has curval still pointing at the final value of
>>>> the record just parsed. The result of which is that if the
>>>> ibm,drc-info property contains multiple properties the parsed value
>>>> of the drc_type for any record after the first has the power_domain
>>>> value of the previous record appended to the type string.
>>>>
>>>> Ex: observed the following 0xffffffff prepended to PHB
>>>>
>>>> [   69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>>>> [   69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>>>> [   69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>>>
>>>> Fix by incrementing curval past the power_domain value to point at
>>>> drc_type string of next record.
>>>>
>>>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
>>>
>>> I have a different commit hash for that:
>>> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes
>>
>> Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
>> the correct upstream commit.
>>
>> Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")
>>
>> Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
>> tag on your end?
> 
> I can update the Fixes tag.
> 
> What's the practical effect of this bug? It seems like it should break
> all the hotplug stuff, but presumably it doesn't in practice for some
> reason?
> 
> It would also be *really* nice if we had some unit tests for this
> parsing code, it's demonstrably very bug prone.
> 
> cheers
> 

In practice PHBs are the only type of connector in the ibm,drc-info property
that has multiple records. So, it breaks PHB hotplug, but by chance not pci,
cpu, slot, or mem because they happen to only ever be a single record.

I have a follow up patch to at least add some pr_debug at the end of this
parsing function to dump each record as its read.

-Tyrel