[PATCH] of: of_mdio: Fix some endianness problems.

[Grant Likely]

On Mon, Nov 1, 2010 at 7:44 AM, Sergei Shtylyov <sshtylyov at mvista.com> wrote:
> Hello.
>
> On 31-10-2010 5:54, Grant Likely wrote:
>
>>>> In of_mdiobus_register(), the __be32 *addr variable is dereferenced.
>>>> This will not work on little-endian targets.  Also since it is
>>>> unsigned, checking for less than zero is redundant.
>
>>>> Fix these two issues.
>
>>>> Signed-off-by: David Daney<ddaney at caviumnetworks.com>
>>>> Cc: Grant Likely<grant.likely at secretlab.ca>
>>>> Cc: Jeremy Kerr<jeremy.kerr at canonical.com>
>>>> Cc: Benjamin Herrenschmidt<benh at kernel.crashing.org>
>>>> Cc: Dan Carpenter<error27 at gmail.com>
>>>> Cc: Greg Kroah-Hartman<gregkh at suse.de>
>>>> ---
>>>>  drivers/of/of_mdio.c |   23 ++++++++++++++---------
>>>>  1 files changed, 14 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>>>> index 1fce00e..b370306 100644
>>>> --- a/drivers/of/of_mdio.c
>>>> +++ b/drivers/of/of_mdio.c
>>>> @@ -52,27 +52,32 @@ int of_mdiobus_register(struct mii_bus *mdio, struct
>>>> device_node *np)
>>>>
>>>>       /* Loop over the child nodes and register a phy_device for each
>>>> one */
>>>>       for_each_child_of_node(np, child) {
>>>> -             const __be32 *addr;
>>>> +             const __be32 *paddr;
>>>> +             u32 addr;
>>>>               int len;
>>>>
>>>>               /* A PHY must have a reg property in the range [0-31] */
>>>> -             addr = of_get_property(child, "reg",&len);
>>>> -             if (!addr || len<  sizeof(*addr) || *addr>= 32 || *addr<
>>>>  0) {
>>>> +             paddr = of_get_property(child, "reg",&len);
>>>> +             if (!paddr || len<  sizeof(*paddr)) {
>>>> +addr_err:
>>>>                       dev_err(&mdio->dev, "%s has invalid PHY
>>>> address\n",
>>>>                               child->full_name);
>>>>                       continue;
>>>>               }
>>>> +             addr = be32_to_cpup(paddr);
>>>> +             if (addr>= 32)
>>>> +                     goto addr_err;
>
>>> goto's are fine for jumping to the end of a function to unwind
>>> allocations, but please don't use it in this manner.  The original
>>> structure will actually work just fine if you do it thusly:
>
>>>                if (!paddr || len<  sizeof(*paddr) ||
>>>                    *(addr = be32_to_cpup(paddr))>= 32) {
>>>                        dev_err(&mdio->dev, "%s has invalid PHY
>>> address\n",
>>>                                child->full_name);
>>>                        continue;
>>>                }
>
>>> Otherwise this patch looks good. After you've reworked and retested
>>> I'll pick it up for 2.6.37 (or dave will).
>
>> Actually, I mistyped this.  I think it should be:
>>
>>                if (!paddr || len<  sizeof(*paddr) ||
>>                    (addr = be32_to_cpup(paddr))>= 32) {
>
>  This assignment would probably cause checkpatch.pl to complain...

checkpatch isn't always right.  Alternately, I'd also be okay with
David's approach of splitting the tests into two if() blocks if
without the goto...

Actually, don't worry about it.  I just merged David's patch and
removed the goto myself in the process.

g.