Re: [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS

[Olliver Schinagl]

Hey Joe,


On 01-12-16 00:38, Joe Hershberger wrote:

On Fri, Nov 25, 2016 at 9:41 AM, 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 
---
  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


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS

[Joe Hershberger]

On Fri, Nov 25, 2016 at 9:41 AM, 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 
> ---
>  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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS

[Olliver Schinagl]

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 
---
  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 

Thanks,
Michal



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS

[Michal Simek]

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 
> ---
>  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 

Thanks,
Michal

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] net: zynq_gem: Return 0 on success, not -ENOSYS

[Olliver Schinagl]

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 
---
 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

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot