* [U-Boot] [PATCH] Fix zynq returning a wrong retval in read_rom_hwaddr
@ 2016-11-25 15:41 Olliver Schinagl
2016-11-25 15:41 ` [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS Olliver Schinagl
0 siblings, 1 reply; 6+ messages in thread
From: Olliver Schinagl @ 2016-11-25 15:41 UTC (permalink / raw)
To: u-boot
As described in the commit, I think because the return value is never checked,
it just worked. I cleaned it up a bit so should at least return the intended
value.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS
2016-11-25 15:41 [U-Boot] [PATCH] Fix zynq returning a wrong retval in read_rom_hwaddr Olliver Schinagl
@ 2016-11-25 15:41 ` Olliver Schinagl
2016-11-28 7:22 ` Michal Simek
2016-11-30 23:38 ` Joe Hershberger
0 siblings, 2 replies; 6+ messages in thread
From: Olliver Schinagl @ 2016-11-25 15:41 UTC (permalink / raw)
To: u-boot
The .read_rom_hwaddr net_ops hook does not check the return value, which
is why it was never caught that we are currently returning 0 if the
read_rom_hwaddr function return -ENOSYS and -ENOSYS otherwise.
In this case we can simplify this by just returning the result of the
function.
Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
drivers/net/zynq_gem.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
index 8b7c1be..04a3fd4 100644
--- a/drivers/net/zynq_gem.c
+++ b/drivers/net/zynq_gem.c
@@ -593,14 +593,12 @@ __weak int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)
static int zynq_gem_read_rom_mac(struct udevice *dev)
{
- int retval;
struct eth_pdata *pdata = dev_get_platdata(dev);
- retval = zynq_board_read_rom_ethaddr(pdata->enetaddr);
- if (retval == -ENOSYS)
- retval = 0;
+ if (!dev)
+ return -ENOSYS;
- return retval;
+ return zynq_board_read_rom_ethaddr(pdata->enetaddr);
}
static int zynq_gem_miiphy_read(struct mii_dev *bus, int addr,
--
2.10.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS
2016-11-25 15:41 ` [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS Olliver Schinagl
@ 2016-11-28 7:22 ` Michal Simek
2016-11-28 10:41 ` Olliver Schinagl
2016-11-30 23:38 ` Joe Hershberger
1 sibling, 1 reply; 6+ messages in thread
From: Michal Simek @ 2016-11-28 7:22 UTC (permalink / raw)
To: u-boot
On 25.11.2016 16:41, Olliver Schinagl wrote:
> The .read_rom_hwaddr net_ops hook does not check the return value, which
> is why it was never caught that we are currently returning 0 if the
> read_rom_hwaddr function return -ENOSYS and -ENOSYS otherwise.
>
> In this case we can simplify this by just returning the result of the
> function.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
> drivers/net/zynq_gem.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
> index 8b7c1be..04a3fd4 100644
> --- a/drivers/net/zynq_gem.c
> +++ b/drivers/net/zynq_gem.c
> @@ -593,14 +593,12 @@ __weak int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)
>
> static int zynq_gem_read_rom_mac(struct udevice *dev)
> {
> - int retval;
> struct eth_pdata *pdata = dev_get_platdata(dev);
>
> - retval = zynq_board_read_rom_ethaddr(pdata->enetaddr);
> - if (retval == -ENOSYS)
> - retval = 0;
> + if (!dev)
> + return -ENOSYS;
>
> - return retval;
> + return zynq_board_read_rom_ethaddr(pdata->enetaddr);
> }
>
> static int zynq_gem_miiphy_read(struct mii_dev *bus, int addr,
>
Not a problem with the patch above but I hope to get rid of this whole
function by using MAC reading from eeprom.
Also board specific functions should return error value when read is not
possible.
Acked-by: Michal Simek <michal.simek@xilinx.com>
Thanks,
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS
2016-11-28 7:22 ` Michal Simek
@ 2016-11-28 10:41 ` Olliver Schinagl
0 siblings, 0 replies; 6+ messages in thread
From: Olliver Schinagl @ 2016-11-28 10:41 UTC (permalink / raw)
To: u-boot
On 28-11-16 08:22, Michal Simek wrote:
> On 25.11.2016 16:41, Olliver Schinagl wrote:
>> The .read_rom_hwaddr net_ops hook does not check the return value, which
>> is why it was never caught that we are currently returning 0 if the
>> read_rom_hwaddr function return -ENOSYS and -ENOSYS otherwise.
>>
>> In this case we can simplify this by just returning the result of the
>> function.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>> drivers/net/zynq_gem.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
>> index 8b7c1be..04a3fd4 100644
>> --- a/drivers/net/zynq_gem.c
>> +++ b/drivers/net/zynq_gem.c
>> @@ -593,14 +593,12 @@ __weak int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)
>>
>> static int zynq_gem_read_rom_mac(struct udevice *dev)
>> {
>> - int retval;
>> struct eth_pdata *pdata = dev_get_platdata(dev);
>>
>> - retval = zynq_board_read_rom_ethaddr(pdata->enetaddr);
>> - if (retval == -ENOSYS)
>> - retval = 0;
>> + if (!dev)
>> + return -ENOSYS;
>>
>> - return retval;
>> + return zynq_board_read_rom_ethaddr(pdata->enetaddr);
>> }
>>
>> static int zynq_gem_miiphy_read(struct mii_dev *bus, int addr,
>>
> Not a problem with the patch above but I hope to get rid of this whole
> function by using MAC reading from eeprom.
Yeah I agree, once the eeprom bit has matured, this could (in your case
for your board) be dropped.
>
> Also board specific functions should return error value when read is not
> possible.
As an unwritten rule you mean? I think the intention is that
*_board_read_rom_hwaddr returns 0 on success < 0 on error.
>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
>
> Thanks,
> Michal
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS
2016-11-25 15:41 ` [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS Olliver Schinagl
2016-11-28 7:22 ` Michal Simek
@ 2016-11-30 23:38 ` Joe Hershberger
2016-12-02 12:39 ` Olliver Schinagl
1 sibling, 1 reply; 6+ messages in thread
From: Joe Hershberger @ 2016-11-30 23:38 UTC (permalink / raw)
To: u-boot
On Fri, Nov 25, 2016 at 9:41 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> The .read_rom_hwaddr net_ops hook does not check the return value, which
> is why it was never caught that we are currently returning 0 if the
> read_rom_hwaddr function return -ENOSYS and -ENOSYS otherwise.
>
> In this case we can simplify this by just returning the result of the
> function.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
> drivers/net/zynq_gem.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
> index 8b7c1be..04a3fd4 100644
> --- a/drivers/net/zynq_gem.c
> +++ b/drivers/net/zynq_gem.c
> @@ -593,14 +593,12 @@ __weak int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)
>
> static int zynq_gem_read_rom_mac(struct udevice *dev)
> {
> - int retval;
> struct eth_pdata *pdata = dev_get_platdata(dev);
>
> - retval = zynq_board_read_rom_ethaddr(pdata->enetaddr);
> - if (retval == -ENOSYS)
> - retval = 0;
> + if (!dev)
> + return -ENOSYS;
>
> - return retval;
> + return zynq_board_read_rom_ethaddr(pdata->enetaddr);
You should check the pdata ptr for NULL before dereferencing.
> }
>
> static int zynq_gem_miiphy_read(struct mii_dev *bus, int addr,
> --
> 2.10.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
^ permalink raw reply [flat|nested] 6+ messages in thread* [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS
2016-11-30 23:38 ` Joe Hershberger
@ 2016-12-02 12:39 ` Olliver Schinagl
0 siblings, 0 replies; 6+ messages in thread
From: Olliver Schinagl @ 2016-12-02 12:39 UTC (permalink / raw)
To: u-boot
Hey Joe,
On 01-12-16 00:38, Joe Hershberger wrote:
> On Fri, Nov 25, 2016 at 9:41 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> The .read_rom_hwaddr net_ops hook does not check the return value, which
>> is why it was never caught that we are currently returning 0 if the
>> read_rom_hwaddr function return -ENOSYS and -ENOSYS otherwise.
>>
>> In this case we can simplify this by just returning the result of the
>> function.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>> drivers/net/zynq_gem.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
>> index 8b7c1be..04a3fd4 100644
>> --- a/drivers/net/zynq_gem.c
>> +++ b/drivers/net/zynq_gem.c
>> @@ -593,14 +593,12 @@ __weak int zynq_board_read_rom_ethaddr(unsigned char *ethaddr)
>>
>> static int zynq_gem_read_rom_mac(struct udevice *dev)
>> {
>> - int retval;
>> struct eth_pdata *pdata = dev_get_platdata(dev);
>>
>> - retval = zynq_board_read_rom_ethaddr(pdata->enetaddr);
>> - if (retval == -ENOSYS)
>> - retval = 0;
>> + if (!dev)
>> + return -ENOSYS;
>>
>> - return retval;
>> + return zynq_board_read_rom_ethaddr(pdata->enetaddr);
> You should check the pdata ptr for NULL before dereferencing.
Actually, is this not violating the whole point subclassed drivers? (I
admit I have not checked into more details of the zynq_gem to see if it
is not allready a subclassed driver).
Even so, I can send an updated patch for this to fix this issue separate
of the rest so atleast this function behaves properly.
>
>> }
>>
>> static int zynq_gem_miiphy_read(struct mii_dev *bus, int addr,
>> --
>> 2.10.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-02 12:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-25 15:41 [U-Boot] [PATCH] Fix zynq returning a wrong retval in read_rom_hwaddr Olliver Schinagl
2016-11-25 15:41 ` [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS Olliver Schinagl
2016-11-28 7:22 ` Michal Simek
2016-11-28 10:41 ` Olliver Schinagl
2016-11-30 23:38 ` Joe Hershberger
2016-12-02 12:39 ` Olliver Schinagl
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.