Tyrel Datwyler [off-list ref] writes:

On 3/11/20 10:43 PM, Michael Ellerman wrote:

quoted

Tyrel Datwyler [off-list ref] writes:

quoted

On 3/10/20 10:25 AM, Nathan Lynch wrote:

quoted

Tyrel Datwyler [off-list ref] writes:

quoted

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.

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.

Thanks. I folded that into the change log.

cheers