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

[Dirk Behme]

On 23.01.2012 17:17, Simon Glass wrote:
> Hi Dirk,
> 
> On Jan 23, 2012 12:30 AM, "Dirk Behme" <dirk.behme at de.bosch.com 
> <mailto:dirk.behme at de.bosch.com>> wrote:
>  >
>  > On 23.01.2012 08:31, Simon Glass wrote:
>  >>
>  >> Hi,
>  >>
>  >> On Thu, Jan 19, 2012 at 12:56 AM, Dirk Behme 
> <dirk.behme at de.bosch.com <mailto:dirk.behme at de.bosch.com>> wrote:
>  >>>
>  >>> From: Eric Miao <eric.miao at linaro.org <mailto: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 
> <mailto:eric.miao at linaro.org>>
>  >>> Acked-by: Simon Glass <sjg at chromium.org <mailto:sjg at chromium.org>>
>  >>> Acked-by: Dirk Behme <dirk.behme at de.bosch.com 
> <mailto:dirk.behme at de.bosch.com>>
>  >>> CC: Stefan Roese <sr at denx.de <mailto:sr at denx.de>>
>  >>> CC: Eric Miao <eric.miao at linaro.org <mailto:eric.miao at linaro.org>>
>  >>> CC: Wolfgang Denk <wd at denx.de <mailto:wd at denx.de>>
>  >>> CC: Philip Balister <philip at balister.org <mailto:philip at balister.org>>
>  >>> CC: Zach Sadecki <zach at itwatchdogs.com <mailto: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.
>  >
>  >
>  > Ok, thanks.
>  >
>  > I'm not an expert for this code, nor is the patch from me. It's from 
> Eric ;) I just try to help to mainline all the stuff we have collected 
> for i.MX6.
>  >
>  > Therefore I wonder if it would be possible to split this into two steps:
>  >
>  > a) Improve the status quo by applying this patch
>  > b) In parallel discuss how to refactor and improve this code as you 
> describe above
>  >
>  > It's my feeling that with (a) we still have a chance to improve 
> v2012.03. But I doubt that (b) would make it into v2012.03.
> 
> Yes agreed, it is a separate discussion. I added Wolfgang on cc to see 
> what he thinks.

Any news to this?

Many thanks

Dirk