[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[Benoît Thébaudeau]

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

[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[Joe Hershberger]

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

[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[Mike Frysinger]

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>

[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[Fabio Estevam]

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

[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[Rob Herring]

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
> 

[U-Boot] [QUESTION] "ethaddr" env. var. vs. dev->enetaddr

[Benoît Thébaudeau]

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