On Mon, Nov 1, 2010 at 7:44 AM, Sergei Shtylyov <sshtylyov@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>>> Cc: Grant Likely<grant.likely@xxxxxxxxxxxx>
>>>> Cc: Jeremy Kerr<jeremy.kerr@xxxxxxxxxxxxx>
>>>> Cc: Benjamin Herrenschmidt<benh@xxxxxxxxxxxxxxxxxxx>
>>>> Cc: Dan Carpenter<error27@xxxxxxxxx>
>>>> Cc: Greg Kroah-Hartman<gregkh@xxxxxxx>
>>>> ---
>>>> 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.
