[U-Boot] [PATCH] Fix zynq returning a wrong retval in read_rom_hwaddr
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.
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,
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
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
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@lists.denx.de http://lists.denx.de/mailman/listinfo/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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
participants (3)
-
Joe Hershberger -
Michal Simek -
Olliver Schinagl