From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dirk Behme Date: Fri, 10 Feb 2012 08:06:22 +0100 Subject: [U-Boot] Refactoring eth_write_hwaddr() and friends (was: Re: [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> Message-ID: <4F34C1EE.9090303@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 08:31, Simon Glass wrote: ... >> 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? While discussing about [1] we found that this is only a short term fix (which should go into 2012.03 [2]) and that we should discuss about a more general clean up of eth_write_hwaddr() and friends: http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=net/eth.c;h=b4b9b4341fdecb25869a07cc8dbe9deefd6452bd;hb=HEAD#l172 See Simon's ideas above. Comments? Opinions? Best regards Dirk [1] http://lists.denx.de/pipermail/u-boot/2012-January/116224.html [2] http://lists.denx.de/pipermail/u-boot/2012-January/116436.html