[RFC,2/9] powerpc/pseries: fix bad drc_index_start value parsing of drc-info entry

The ibm,drc-info property is an array property that contains drc-info
entries such that each entry is made up of 2 string encoded elements
followed by 5 int encoded elements. The of_read_drc_info_cell()
helper contains comments that correctly name the expected elements
and their encoding. However, the usage of of_prop_next_string() and
of_prop_next_u32() introduced a subtle skippage of the first u32.
This is a result of of_prop_next_string() returns a pointer to the
next property value which is not a string, but actually a (__be32 *).
As, a result the following call to of_prop_next_u32() passes over the
current int encoded value and actually stores the next one wrongly.

Simply endian swap the current value in place after reading the first
two string values. The remaining int encoded values can then be read
correctly using of_prop_next_u32().

Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/of_helpers.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> The ibm,drc-info property is an array property that contains drc-info
> entries such that each entry is made up of 2 string encoded elements
> followed by 5 int encoded elements. The of_read_drc_info_cell()
> helper contains comments that correctly name the expected elements
> and their encoding. However, the usage of of_prop_next_string() and
> of_prop_next_u32() introduced a subtle skippage of the first u32.
> This is a result of of_prop_next_string() returns a pointer to the
> next property value which is not a string, but actually a (__be32 *).
> As, a result the following call to of_prop_next_u32() passes over the
> current int encoded value and actually stores the next one wrongly.
>
> Simply endian swap the current value in place after reading the first
> two string values. The remaining int encoded values can then be read
> correctly using of_prop_next_u32().

Good but I think it would make more sense for a fix for
of_read_drc_info_cell() to precede any patch in the series which
introduces new callers, such as patch #1.

hi
I wrote here (1) a couple of years ago, I am still working with kernel
4.11.0 and there is broken support for initializing the PCI.

arch/powerpc/book/cuimage-walnut.c requires "/plb" compatible with
"fsl,pq2-localbus", while the device-tree file (walnut.dts) defines
"/plb" compatible with "ibm,plb3"

I am not an expert, but "fsl,pq2-localbus" != "ibm,plb3"

Therefore the PCI initialization of the PPC405GP seems wrong and every
kernel >= 2.6.26 is not able to correctly address the PDC20265

(1) https://bugzilla.kernel.org/show_bug.cgi?id=195933

an interesting not is:
kernel 2.6.26 can be compiled with arch=ppc and arch=powerpc

with arch=ppc the promise PDC20265 chip is correctly managed
with arch=powerpc the PDC20265 is not correctly managed


any idea? help? suggestion?

thanks
Carlo


--------------------------------------------------------------------------------------
    bus_node = finddevice("/plb");
    if (!bus_node)
    {
        notify_error(module, id, "device /plb not found");
        return;
    }
    if (!dt_is_compatible(bus_node, "fsl,pq2-localbus"))
    {
        notify_warn(module, id, "device fsl,pq2-localbus");
        notify_error(module, id, "device /plb is not compatible");
--------------------------------------------------------------------------------------
plb
        {
                /*
                 * Processor Local Bus (PLB)
                 */
                compatible = "ibm,plb3";
--------------------------------------------------------------------------------------


ide0 at 0x1f0-0x1f7,0x3f6 on irq 31
ide1 at 0x170-0x177,0x376 on irq 31

On 10/10/19 12:04 PM, Nathan Lynch wrote:
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> The ibm,drc-info property is an array property that contains drc-info
>> entries such that each entry is made up of 2 string encoded elements
>> followed by 5 int encoded elements. The of_read_drc_info_cell()
>> helper contains comments that correctly name the expected elements
>> and their encoding. However, the usage of of_prop_next_string() and
>> of_prop_next_u32() introduced a subtle skippage of the first u32.
>> This is a result of of_prop_next_string() returns a pointer to the
>> next property value which is not a string, but actually a (__be32 *).
>> As, a result the following call to of_prop_next_u32() passes over the
>> current int encoded value and actually stores the next one wrongly.
>>
>> Simply endian swap the current value in place after reading the first
>> two string values. The remaining int encoded values can then be read
>> correctly using of_prop_next_u32().
> 
> Good but I think it would make more sense for a fix for
> of_read_drc_info_cell() to precede any patch in the series which
> introduces new callers, such as patch #1.
> 

Not sure it matters that much since everything in the series is required to
properly enable a working drc-info implementation and the call already exists so
it doesn't break bisectability. It ended up second in the series since testing
patch 1 exposed the flaw.

I'll go ahead and move it first, and add the appropriate fixes tag as well which
is currently missing.

-Tyrel