From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dirk Behme Date: Wed, 8 Feb 2012 08:13:10 +0100 Subject: [U-Boot] [PATCH v2] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back In-Reply-To: References: <1326963393-7338-1-git-send-email-dirk.behme@de.bosch.com> <4F1D1A19.4010401@de.bosch.com> Message-ID: <4F322086.1030504@de.bosch.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 23.01.2012 17:17, Simon Glass wrote: > Hi Dirk, > > On Jan 23, 2012 12:30 AM, "Dirk Behme" > wrote: > > > > On 23.01.2012 08:31, Simon Glass wrote: > >> > >> Hi, > >> > >> On Thu, Jan 19, 2012 at 12:56 AM, Dirk Behme > > wrote: > >>> > >>> From: Eric Miao > > >>> > >>> 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 ethaddr 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 > > >>> Acked-by: Simon Glass > > >>> Acked-by: Dirk Behme > > >>> CC: Stefan Roese > > >>> CC: Eric Miao > > >>> CC: Wolfgang Denk > > >>> CC: Philip Balister > > >>> CC: Zach Sadecki > > >>> --- > >>> 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