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

Simon Glass
2012-02-09T18:25:29Z

Hi Dirk,

On Tue, Feb 7, 2012 at 11:13 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 23.01.2012 17:17, Simon Glass wrote:
>>
>> Hi Dirk,
>>
>>
>> On Jan 23, 2012 12:30 AM, "Dirk Behme" <dirk.behme@de.bosch.com
>> <mailto:dirk.behme@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@de.bosch.com
>> <mailto:dirk.behme@de.bosch.com>> wrote:
>> ?>>>
>> ?>>> From: Eric Miao <eric.miao at linaro.org <mailto:eric.miao@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@linaro.org
>> <mailto:eric.miao@linaro.org>>
>> ?>>> Acked-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>> ?>>> Acked-by: Dirk Behme <dirk.behme@de.bosch.com
>> <mailto:dirk.behme@de.bosch.com>>
>> ?>>> CC: Stefan Roese <sr at denx.de <mailto:sr@denx.de>>
>> ?>>> CC: Eric Miao <eric.miao at linaro.org <mailto:eric.miao@linaro.org>>
>> ?>>> CC: Wolfgang Denk <wd at denx.de <mailto:wd@denx.de>>
>> ?>>> CC: Philip Balister <philip@balister.org
>> <mailto:philip@balister.org>>
>> ?>>> CC: Zach Sadecki <zach at itwatchdogs.com <mailto:zach@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?

Already acked by me. Are you going to start a separate discussion on
how to clean up this code? If so please cc me.

Regards,
Simon

>
> Many thanks
>
> Dirk
>