* [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-10-28 1:03 ` David Daney
0 siblings, 0 replies; 10+ messages in thread
From: David Daney @ 2010-10-28 1:03 UTC (permalink / raw)
To: linux-mips, ralf, devicetree-discuss, grant.likely, linux-kernel
Cc: David Daney, Jeremy Kerr, Benjamin Herrenschmidt, Dan Carpenter,
Greg Kroah-Hartman
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@caviumnetworks.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Dan Carpenter <error27@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@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;
if (mdio->irq) {
- mdio->irq[*addr] = irq_of_parse_and_map(child, 0);
- if (!mdio->irq[*addr])
- mdio->irq[*addr] = PHY_POLL;
+ mdio->irq[addr] = irq_of_parse_and_map(child, 0);
+ if (!mdio->irq[addr])
+ mdio->irq[addr] = PHY_POLL;
}
- phy = get_phy_device(mdio, be32_to_cpup(addr));
+ phy = get_phy_device(mdio, addr);
if (!phy || IS_ERR(phy)) {
dev_err(&mdio->dev, "error probing PHY at address %i\n",
- *addr);
+ addr);
continue;
}
phy_scan_fixups(phy);
@@ -91,7 +96,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
}
dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
- child->name, *addr);
+ child->name, addr);
}
return 0;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-10-28 1:03 ` David Daney
0 siblings, 0 replies; 10+ messages in thread
From: David Daney @ 2010-10-28 1:03 UTC (permalink / raw)
To: linux-mips-6z/3iImG2C8G8FEW9MqTrA, ralf-6z/3iImG2C8G8FEW9MqTrA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
grant.likely-s3s/WqlpOiPyB63q8FvJNQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Jeremy Kerr, Greg Kroah-Hartman, David Daney, Dan Carpenter
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-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
---
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;
if (mdio->irq) {
- mdio->irq[*addr] = irq_of_parse_and_map(child, 0);
- if (!mdio->irq[*addr])
- mdio->irq[*addr] = PHY_POLL;
+ mdio->irq[addr] = irq_of_parse_and_map(child, 0);
+ if (!mdio->irq[addr])
+ mdio->irq[addr] = PHY_POLL;
}
- phy = get_phy_device(mdio, be32_to_cpup(addr));
+ phy = get_phy_device(mdio, addr);
if (!phy || IS_ERR(phy)) {
dev_err(&mdio->dev, "error probing PHY at address %i\n",
- *addr);
+ addr);
continue;
}
phy_scan_fixups(phy);
@@ -91,7 +96,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
}
dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
- child->name, *addr);
+ child->name, addr);
}
return 0;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-10-30 6:32 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2010-10-30 6:32 UTC (permalink / raw)
To: David Daney
Cc: linux-mips, ralf, devicetree-discuss, linux-kernel, Jeremy Kerr,
Benjamin Herrenschmidt, Dan Carpenter, Greg Kroah-Hartman
On Wed, Oct 27, 2010 at 06:03:47PM -0700, David Daney 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@caviumnetworks.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Dan Carpenter <error27@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@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).
g.
>
> if (mdio->irq) {
> - mdio->irq[*addr] = irq_of_parse_and_map(child, 0);
> - if (!mdio->irq[*addr])
> - mdio->irq[*addr] = PHY_POLL;
> + mdio->irq[addr] = irq_of_parse_and_map(child, 0);
> + if (!mdio->irq[addr])
> + mdio->irq[addr] = PHY_POLL;
> }
>
> - phy = get_phy_device(mdio, be32_to_cpup(addr));
> + phy = get_phy_device(mdio, addr);
> if (!phy || IS_ERR(phy)) {
> dev_err(&mdio->dev, "error probing PHY at address %i\n",
> - *addr);
> + addr);
> continue;
> }
> phy_scan_fixups(phy);
> @@ -91,7 +96,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
> }
>
> dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> - child->name, *addr);
> + child->name, addr);
> }
>
> return 0;
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-10-30 6:32 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2010-10-30 6:32 UTC (permalink / raw)
To: David Daney
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, Dan Carpenter,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Greg Kroah-Hartman,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ralf-6z/3iImG2C8G8FEW9MqTrA,
Jeremy Kerr
On Wed, Oct 27, 2010 at 06:03:47PM -0700, David Daney 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-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
> ---
> 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).
g.
>
> if (mdio->irq) {
> - mdio->irq[*addr] = irq_of_parse_and_map(child, 0);
> - if (!mdio->irq[*addr])
> - mdio->irq[*addr] = PHY_POLL;
> + mdio->irq[addr] = irq_of_parse_and_map(child, 0);
> + if (!mdio->irq[addr])
> + mdio->irq[addr] = PHY_POLL;
> }
>
> - phy = get_phy_device(mdio, be32_to_cpup(addr));
> + phy = get_phy_device(mdio, addr);
> if (!phy || IS_ERR(phy)) {
> dev_err(&mdio->dev, "error probing PHY at address %i\n",
> - *addr);
> + addr);
> continue;
> }
> phy_scan_fixups(phy);
> @@ -91,7 +96,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
> }
>
> dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
> - child->name, *addr);
> + child->name, addr);
> }
>
> return 0;
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-10-31 2:54 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2010-10-31 2:54 UTC (permalink / raw)
To: David Daney
Cc: linux-mips, ralf, devicetree-discuss, linux-kernel, Jeremy Kerr,
Benjamin Herrenschmidt, Dan Carpenter, Greg Kroah-Hartman
On Sat, Oct 30, 2010 at 2:32 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Oct 27, 2010 at 06:03:47PM -0700, David Daney 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@caviumnetworks.com>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Dan Carpenter <error27@gmail.com>
>> Cc: Greg Kroah-Hartman <gregkh@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) {
dev_err(&mdio->dev, "%s has invalid PHY address\n",
child->full_name);
continue;
}
g.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-10-31 2:54 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2010-10-31 2:54 UTC (permalink / raw)
To: David Daney
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, Dan Carpenter,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Greg Kroah-Hartman,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ralf-6z/3iImG2C8G8FEW9MqTrA,
Jeremy Kerr
On Sat, Oct 30, 2010 at 2:32 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Wed, Oct 27, 2010 at 06:03:47PM -0700, David Daney 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-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> Cc: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>> Cc: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
>> ---
>> 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) {
dev_err(&mdio->dev, "%s has invalid PHY address\n",
child->full_name);
continue;
}
g.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-11-01 11:44 ` Sergei Shtylyov
0 siblings, 0 replies; 10+ messages in thread
From: Sergei Shtylyov @ 2010-11-01 11:44 UTC (permalink / raw)
To: Grant Likely
Cc: David Daney, linux-mips, ralf, devicetree-discuss, linux-kernel,
Jeremy Kerr, Benjamin Herrenschmidt, Dan Carpenter,
Greg Kroah-Hartman
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@caviumnetworks.com>
>>> Cc: Grant Likely<grant.likely@secretlab.ca>
>>> Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
>>> Cc: Benjamin Herrenschmidt<benh@kernel.crashing.org>
>>> Cc: Dan Carpenter<error27@gmail.com>
>>> Cc: Greg Kroah-Hartman<gregkh@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...
WBR, Sergei
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-11-01 11:44 ` Sergei Shtylyov
0 siblings, 0 replies; 10+ messages in thread
From: Sergei Shtylyov @ 2010-11-01 11:44 UTC (permalink / raw)
To: Grant Likely
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, Dan Carpenter,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Greg Kroah-Hartman,
David Daney, ralf-6z/3iImG2C8G8FEW9MqTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jeremy Kerr
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-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>>> Cc: Grant Likely<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>>> Cc: Jeremy Kerr<jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>>> Cc: Benjamin Herrenschmidt<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>>> Cc: Dan Carpenter<error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Cc: Greg Kroah-Hartman<gregkh-l3A5Bk7waGM@public.gmane.org>
>>> ---
>>> 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...
WBR, Sergei
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-11-01 14:20 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2010-11-01 14:20 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: David Daney, linux-mips, ralf, devicetree-discuss, linux-kernel,
Jeremy Kerr, Benjamin Herrenschmidt, Dan Carpenter,
Greg Kroah-Hartman
On Mon, Nov 1, 2010 at 7:44 AM, Sergei Shtylyov <sshtylyov@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@caviumnetworks.com>
>>>> Cc: Grant Likely<grant.likely@secretlab.ca>
>>>> Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
>>>> Cc: Benjamin Herrenschmidt<benh@kernel.crashing.org>
>>>> Cc: Dan Carpenter<error27@gmail.com>
>>>> Cc: Greg Kroah-Hartman<gregkh@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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: of_mdio: Fix some endianness problems.
@ 2010-11-01 14:20 ` Grant Likely
0 siblings, 0 replies; 10+ messages in thread
From: Grant Likely @ 2010-11-01 14:20 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, Dan Carpenter,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Greg Kroah-Hartman,
David Daney, ralf-6z/3iImG2C8G8FEW9MqTrA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jeremy Kerr
On Mon, Nov 1, 2010 at 7:44 AM, Sergei Shtylyov <sshtylyov-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org> 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-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>>>> Cc: Grant Likely<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>>>> Cc: Jeremy Kerr<jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>>>> Cc: Benjamin Herrenschmidt<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
>>>> Cc: Dan Carpenter<error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> Cc: Greg Kroah-Hartman<gregkh-l3A5Bk7waGM@public.gmane.org>
>>>> ---
>>>> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-11-01 14:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-28 1:03 [PATCH] of: of_mdio: Fix some endianness problems David Daney
2010-10-28 1:03 ` David Daney
2010-10-30 6:32 ` Grant Likely
2010-10-30 6:32 ` Grant Likely
2010-10-31 2:54 ` Grant Likely
2010-10-31 2:54 ` Grant Likely
2010-11-01 11:44 ` Sergei Shtylyov
2010-11-01 11:44 ` Sergei Shtylyov
2010-11-01 14:20 ` Grant Likely
2010-11-01 14:20 ` Grant Likely
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.