[U-Boot] [PATCH v2] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Simon Glass]

Hi,

On Thu, Jan 19, 2012 at 12:56 AM, Dirk Behme <dirk.behme at de.bosch.com> wrote:
> From: Eric Miao <eric.miao at linaro.org>
>
> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
> fails, fall back to use dev->enetaddr, which could be filled up by
> the ethernet device driver:
>
> With the current code, introduced with below commit, eth_write_hwaddr()
> will fail immediately if there is no eth<n>addr in the environment variables.
>
> However, e.g. for an overo based product that uses the SMSC911x ethernet
> chip (with the MAC address set via EEPROM connected to the SMSC911x chip),
> the MAC address is still OK.
>
> On mx28 boards that are depending on the OCOTP bits to set the MAC address
> (like the Denx m28 board), the OCOTP bits should be used instead of
> failing on the environment variables.
>
> Actually, this was the original behavior, and was later changed by
> commit 7616e7850804c7c69e0a22c179dfcba9e8f3f587.
>
> Signed-off-by: Eric Miao <eric.miao at linaro.org>
> Acked-by: Simon Glass <sjg at chromium.org>
> Acked-by: Dirk Behme <dirk.behme at de.bosch.com>
> CC: Stefan Roese <sr at denx.de>
> CC: Eric Miao <eric.miao at linaro.org>
> CC: Wolfgang Denk <wd at denx.de>
> CC: Philip Balister <philip at balister.org>
> CC: Zach Sadecki <zach at itwatchdogs.com>
> ---
> v2: Correct the referenced commit ID and update the commit message.
>    No functional change at the code itself.
>
> Note: This resend is based on my understanding from
>
>      http://lists.denx.de/pipermail/u-boot/2012-January/116118.html
>
>      Please let Eric and me know if I missed anything there.

I don't think you have missed anything and I have already acked this.
But I want to start a related discussion.

The code structure does bug me a bit - I think it is too confusing.
eth_getenv_enetaddr() returns an error if there is no environment
variable set or if the address it gets from the environment variable
is invalid. We should probably not conflate those two. The first is ok
here, but the second isn't, I think.

What if the driver has no write_hwaddr method? Do we silently ignore
the environment variable value?

Why use memcmp() against env_enetaddr when the function we just called
returns an error that tells us whether it is supposed to be valid (the
error return your patch squashes)?

We set the hwaddr by writing directly into the dev->enet_addr field
and then calling write_hwaddr() if it exists. Maybe that is ok - is
the lack of write_hwaddr() an indication that the driver does MAC
address handling on the fly, or just that it can't set the MAC address
at all?

Overall I feel that eth_write_hwaddr() should return success or
failure, confident in its determination that there is either a valid
MAC address or there is not. The message you are seeing is I suppose
an indication that it thinks there is a problem, when in fact none
exists in this case. At the moment it feels fragile.

I wonder whether a little refactor here would be best?

That said, your patch restores the original behaviour, hiding the
problem which isn't actually a problem in this case, and which we
don't want to report. So it is better than the status quo. Perhaps a
little comment as to why the return value doesn't matter?

Regards,
Simon
>
>  net/eth.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/net/eth.c b/net/eth.c
> index b4b9b43..451568f 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -175,8 +175,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>        unsigned char env_enetaddr[6];
>        int ret = 0;
>
> -       if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
> -               return -1;
> +       eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>
>        if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
>                if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
> --
> 1.7.0.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot