* [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr [not found] <261180882.2030378.1344278963347.JavaMail.root@advansee.com> @ 2012-08-06 19:02 ` Benoît Thébaudeau 2012-08-06 22:45 ` Joe Hershberger 0 siblings, 1 reply; 6+ messages in thread From: Benoît Thébaudeau @ 2012-08-06 19:02 UTC (permalink / raw) To: u-boot Hi all, There's a lot of stuff in U-Boot relying on ethaddr being set, e.g. the bdinfo command, or the linklocal command because of seed_mac. If ethaddr is not set, bdinfo will report exactly that, but linklocal will wait indefinitely without displaying anything. The issue is that dev->enetaddr may be set even if ethaddr is not, e.g. through imx_get_mac_from_fuse. eth_write_hwaddr uses a valid ethaddr to override an already set dev->enetaddr, but it does not require ethaddr to be set. Hence, shouldn't the users of ethaddr rather use dev->enetaddr, or is ethaddr really supposed to be required (bug or feature)? If ethaddr is required, should it be up to the boards to set if for cases like imx_get_mac_from_fuse, or should eth_write_hwaddr set it automatically if dev->enetaddr is valid but ethaddr is unset or invalid? Best regards, Beno?t ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr 2012-08-06 19:02 ` [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr Benoît Thébaudeau @ 2012-08-06 22:45 ` Joe Hershberger 2012-08-06 23:40 ` Mike Frysinger 2012-08-07 0:48 ` Benoît Thébaudeau 0 siblings, 2 replies; 6+ messages in thread From: Joe Hershberger @ 2012-08-06 22:45 UTC (permalink / raw) To: u-boot Hi Beno?t, On Mon, Aug 6, 2012 at 2:02 PM, Beno?t Th?baudeau <benoit.thebaudeau@advansee.com> wrote: > Hi all, > > There's a lot of stuff in U-Boot relying on ethaddr being set, e.g. the bdinfo > command, or the linklocal command because of seed_mac. If ethaddr is not set, > bdinfo will report exactly that, but linklocal will wait indefinitely without > displaying anything. This sounds like a problem to be fixed one way or another. > The issue is that dev->enetaddr may be set even if ethaddr is not, e.g. through > imx_get_mac_from_fuse. eth_write_hwaddr uses a valid ethaddr to override an > already set dev->enetaddr, but it does not require ethaddr to be set. You should get a warning from u-boot that it is using the dev->enetaddr since the ethaddr is missing. > Hence, shouldn't the users of ethaddr rather use dev->enetaddr, or is ethaddr > really supposed to be required (bug or feature)? Because of the logic that prevents dev->enetaddr set from hardware to override ethaddr (since ethaddr should always be the source of the MAC in all but exceptional cases), it seems to me that using ethaddr is correct. Perhaps in the case of bdinfo, it could explicitly say that ethaddr is not set, and if dev->enetaddr is in use, it could also print that. In the case of linklocal, it is difficult to decide. I don't think we want linklocal seeding its IP addresses with a random MAC address. I think we want to fix this bug by warning and giving up or warning and seeding with 0 (the algorithm can still work this way, just not as well). > If ethaddr is required, should it be up to the boards to set if for cases like > imx_get_mac_from_fuse, or should eth_write_hwaddr set it automatically if > dev->enetaddr is valid but ethaddr is unset or invalid? If specific boards may have their MAC stored elsewhere (like imx_get_mac_from_fuse()), then it's probably OK for that board's call to update the ethaddr if it isn't set. In fact it seems like fec_probe() in drivers/net/fec_mxc.c should not be changing edev->enetaddr. It should setenv("ethaddr,...), but only if it is not set. eth_write_hwaddr() should not write it. > Best regards, > Beno?t ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr 2012-08-06 22:45 ` Joe Hershberger @ 2012-08-06 23:40 ` Mike Frysinger 2012-08-07 0:49 ` Fabio Estevam 2012-08-07 0:48 ` Benoît Thébaudeau 1 sibling, 1 reply; 6+ messages in thread From: Mike Frysinger @ 2012-08-06 23:40 UTC (permalink / raw) To: u-boot On Monday 06 August 2012 18:45:58 Joe Hershberger wrote: > On Mon, Aug 6, 2012 at 2:02 PM, Beno?t Th?baudeau wrote: > > There's a lot of stuff in U-Boot relying on ethaddr being set, e.g. the > > bdinfo command, or the linklocal command because of seed_mac. If ethaddr > > is not set, bdinfo will report exactly that, but linklocal will wait > > indefinitely without displaying anything. > > This sounds like a problem to be fixed one way or another. there was a patch floating around for setting up ethaddr env var automatically in the net core during init if it wasn't already set > > Hence, shouldn't the users of ethaddr rather use dev->enetaddr, or is > > ethaddr really supposed to be required (bug or feature)? > > Because of the logic that prevents dev->enetaddr set from hardware to > override ethaddr (since ethaddr should always be the source of the MAC > in all but exceptional cases), it seems to me that using ethaddr is > correct. Perhaps in the case of bdinfo, it could explicitly say that > ethaddr is not set, and if dev->enetaddr is in use, it could also > print that. no, nothing outside of the net layer should ever touch dev->enetaddr. this is clearly documented in doc/README.enetaddr. -mike -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: This is a digitally signed message part. URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120806/2d5f8eed/attachment.pgp> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr 2012-08-06 23:40 ` Mike Frysinger @ 2012-08-07 0:49 ` Fabio Estevam 2012-08-07 2:39 ` Rob Herring 0 siblings, 1 reply; 6+ messages in thread From: Fabio Estevam @ 2012-08-07 0:49 UTC (permalink / raw) To: u-boot On Mon, Aug 6, 2012 at 8:40 PM, Mike Frysinger <vapier@gentoo.org> wrote: > there was a patch floating around for setting up ethaddr env var automatically > in the net core during init if it wasn't already set I guess you are referring to this one: http://patchwork.ozlabs.org/patch/136789/ Regards, Fabio Estevam ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr 2012-08-07 0:49 ` Fabio Estevam @ 2012-08-07 2:39 ` Rob Herring 0 siblings, 0 replies; 6+ messages in thread From: Rob Herring @ 2012-08-07 2:39 UTC (permalink / raw) To: u-boot On 08/06/2012 07:49 PM, Fabio Estevam wrote: > On Mon, Aug 6, 2012 at 8:40 PM, Mike Frysinger <vapier@gentoo.org> wrote: > >> there was a patch floating around for setting up ethaddr env var automatically >> in the net core during init if it wasn't already set > > I guess you are referring to this one: > http://patchwork.ozlabs.org/patch/136789/ Or perhaps the one from me which is in mainline now. Rob > > Regards, > > Fabio Estevam > _______________________________________________ > U-Boot mailing list > U-Boot at lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr 2012-08-06 22:45 ` Joe Hershberger 2012-08-06 23:40 ` Mike Frysinger @ 2012-08-07 0:48 ` Benoît Thébaudeau 1 sibling, 0 replies; 6+ messages in thread From: Benoît Thébaudeau @ 2012-08-07 0:48 UTC (permalink / raw) To: u-boot Hi Joe, On Tue, Aug 7, 2012 at 12:45:58 AM, Joe Hershberger wrote: > Hi Beno?t, > > On Mon, Aug 6, 2012 at 2:02 PM, Beno?t Th?baudeau > <benoit.thebaudeau@advansee.com> wrote: > > Hi all, > > > > There's a lot of stuff in U-Boot relying on ethaddr being set, e.g. > > the bdinfo > > command, or the linklocal command because of seed_mac. If ethaddr > > is not set, > > bdinfo will report exactly that, but linklocal will wait > > indefinitely without > > displaying anything. > > This sounds like a problem to be fixed one way or another. OK. > > The issue is that dev->enetaddr may be set even if ethaddr is not, > > e.g. through > > imx_get_mac_from_fuse. eth_write_hwaddr uses a valid ethaddr to > > override an > > already set dev->enetaddr, but it does not require ethaddr to be > > set. > > You should get a warning from u-boot that it is using the > dev->enetaddr since the ethaddr is missing. Yes, or a fallback ethaddr could be automatically set to dev->enetaddr like Mike said. > > Hence, shouldn't the users of ethaddr rather use dev->enetaddr, or > > is ethaddr > > really supposed to be required (bug or feature)? > > Because of the logic that prevents dev->enetaddr set from hardware to > override ethaddr (since ethaddr should always be the source of the > MAC > in all but exceptional cases), it seems to me that using ethaddr is > correct. Perhaps in the case of bdinfo, it could explicitly say that > ethaddr is not set, and if dev->enetaddr is in use, it could also > print that. No, Mike is correct. > In the case of linklocal, it is difficult to decide. I don't think > we > want linklocal seeding its IP addresses with a random MAC address. I > think we want to fix this bug by warning and giving up or warning and > seeding with 0 (the algorithm can still work this way, just not as > well). If ethaddr is automatically set, this solves this issue by the way. The current code seeds with 0, which results in rand_r returning 0, so that timeout_ms also gets 0, and the mechanism is broken. > > If ethaddr is required, should it be up to the boards to set if for > > cases like > > imx_get_mac_from_fuse, or should eth_write_hwaddr set it > > automatically if > > dev->enetaddr is valid but ethaddr is unset or invalid? > > If specific boards may have their MAC stored elsewhere (like > imx_get_mac_from_fuse()), then it's probably OK for that board's call > to update the ethaddr if it isn't set. In fact it seems like > fec_probe() in drivers/net/fec_mxc.c should not be changing > edev->enetaddr. It should setenv("ethaddr,...), but only if it is > not > set. According to point 3 in doc/README.enetaddr, the current behavior of fec_probe() is correct since it refers to "the driver initialized struct eth_device->enetaddr". The priority given by eth_write_hwaddr() to ethaddr against dev->enetaddr would also not make sense if that were not correct. > eth_write_hwaddr() should not write it. Why? This is what Mike was talking about. I don't find the patch Mike refers to. However, I see an issue if eth_write_hwaddr() sets a fallback ethaddr from a valid dev->enetaddr: depending on the config, ethaddr may be set only once, so that will be one less move for the end user. But we can also consider that if the MAC address has been set by any means (fuses, EEPROM, etc.), it is equivalent to having set ethaddr once, so that could be correct. Mike, what do you think about that? Best regards, Beno?t ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-08-07 2:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <261180882.2030378.1344278963347.JavaMail.root@advansee.com>
2012-08-06 19:02 ` [U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr Benoît Thébaudeau
2012-08-06 22:45 ` Joe Hershberger
2012-08-06 23:40 ` Mike Frysinger
2012-08-07 0:49 ` Fabio Estevam
2012-08-07 2:39 ` Rob Herring
2012-08-07 0:48 ` Benoît Thébaudeau
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.