[U-Boot] [PATCH 07/11] net: sunxi: Do not inject ethernet addresses into the env

[Hans de Goede]

Hi,

On 15-11-16 11:17, Olliver Schinagl wrote:
> Hey Hans,
>
> I was hopeing and expecting this :)
>
>
> As you will be able to tell below, I need to learn a bit more as to why we do things and discuss this proper I guess.
>
>
> On 15-11-16 10:26, Hans de Goede wrote:
>> Hi,
>>
>> On 15-11-16 04:25, Joe Hershberger wrote:
>>> On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver at schinagl.nl> wrote:
>>>> Currently we inject 5 ethernet addresses into the environment, just in
>>>> case we may need them. We do this because some boards have no eeprom
>>>> (programmed) with a proper ethernet address. With the recent addition of
>>>> reading actual ethernet addresses from the eeprom via the net_op we
>>>> should not inject environment variables any more.
>>>>
>>>> Signed-off-by: Olliver Schinagl <oliver at schinagl.nl>
>>>
>>> Acked-by: Joe Hershberger <joe.hershberger at ni.com>
>>
>>
>> Erm, this patch seems wrong to me: NACK, let me
>> explain:
>>
>> 1) It does not do what its commit message says, it only
>> removes the second step for setting ethernet addresses
>> from the env, but it keeps the existing code to set them
>> AFAICT, it only does it once now.
>>
>> 2) "Currently we inject 5 ethernet addresses into the environment",
>> this is not true, we only inject ethernet addresses into the
>> environment for devices which have an ethernet alias in dt,
>> so maximum 2 for devices with both wired ethernet and wifi
> If we want the fdt to do mac things, shouldn't that be done at a higher level? This is not really board specific is it?

We want to put mac addresses into the fdt, and this is done at
a higher level, by existing dt code, which looks at ethernet
aliases in dt and then for any which are present, checks
the corresponding ethaddr env setting and if set, injects
that mac address into the dt-node the alias points to.

What is sunxi specific is setting the environment variables
based on the SID. The sunxi specific code also checks the
aliases, exactly to avoid the "inject 5 ethernet addresses"
thing you are describing, as we don't want to inject
ethernet addresses for non existent NICs as that may confuse
the user.

>> 3) The second attempt at setting ethernet addresses in
>> the environment after loading the kernel dt is necessary
>> because the kernel dt may be newer and contain more
>> ethernet aliases, e.g. the u-boot dt may only contain the
>> nodes + alias for the wired, while the (newer) kernel dt
>> may also contain a dt-node + alias for the wireless
> I agree with you here, but I still don't think this should be board specific

Instead of doing this through the environment I guess you
could have the u-boot dt code which is injecting the MACs
into the dt call some board specific callback when there
is no MAC set in the environment, and implement a weak
stub for this. Then all the sunxi environment mangling
code can go away, and sunxi can simply:
1) Try eeprom if present
2) Do the current SID based thing

>> 4) We cannot solely rely on the ethernet driver to set
>> mac-addresses, because the ethernet driver may not be enabled
>> while the kernel does have the ethernet driver enabled; and
>> the kernel relies on u-boot to generate fixed mac-addresses
>> based on the SID independent whether or not u-boot has
>> ethernet enabled, this is especially relevant for wifi
>> chips where the kernel also relies on u-boot generated
>> fixed mac-addresses on e.g. the recent orange-pi boards,
>> which come with a realtek rtl8189etv chip which does not
>> have a mac address programmed.
> I agree, and I'll fix that in my new patch series proper by making rtl8189etv also read rom hook which IS board specific

The problem is that u-boot may not have a driver for one
of the NICs at all, so no place to call the rom hook at all.

Anyways I believe this is solved by my suggestion for making
the u-boot dt code which injects the MAC call a board specific
callback when no ethaddr is set in the env.

>> 5) AFAIK the dt code for passing mac-addresses to the kernel
>> relies on the environment variables, so even if we get the
>> mac-address from a ROM we should still store it in the
>> environment variable.
> The new patch series does that, as the net core layer does that.
>
> What happens is (note code is mangled and might not be 100% accurate, i reduced it the bares):
>
>     eth_read_eeprom_hwaddr(dev);
> first read from the eeprom, which may return all zero's if it is unconfigured/missconfigured or should not be used from the eeprom.
>     if (is_zero_ethaddr(pdata->enetaddr))
>         if (eth_get_ops(dev)->read_rom_hwaddr)
>             eth_get_ops(dev)->read_rom_hwaddr(dev);
> if the eeprom failed to produce a mac, we check the read_rom_hwaddr callback, which hooks into the driver. The driver can be overridden by a board (such as sunxi) where the MAC is generated from the SID.
>
> so at this point we may have a hwaddress actually obtained from the hardware, via the eeprom (or some fixed rom even) or from the hardware itself
> next we allow 'software' overrides. e.g. u-boot env (and i think this is where the fdt method should live as well
>
>     eth_getenv_enetaddr_by_index("eth", dev->seq, env_enetaddr);
>     if (!is_zero_ethaddr(env_enetaddr)) {
>         if (!is_zero_ethaddr(pdata->enetaddr) &&
>             memcmp(pdata->enetaddr, env_enetaddr, ARP_HLEN))
>                  memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>
> // <snip> we compare the HW addr and the ENV addr. if the env is unset, we use whatever the hardware supplied us with.
> if the env is set, it overrides the HW addr.
> I think next would be to check the fdt to override the env?
>
>     } else if (is_valid_ethaddr(pdata->enetaddr)) {
>         eth_setenv_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
> Finally, we set whatever mac has come from the above probing into the environment (if the address is actually a valid MAC).

Ok, good I just wanted to make sure that that would still happen.


>
>     } else if (is_zero_ethaddr(pdata->enetaddr)) {
> #ifdef CONFIG_NET_RANDOM_ETHADDR
>         net_random_ethaddr(pdata->enetaddr);
> otherwise (if configured) let u-boot generate it.
>
>
> So I think here is where the fdt override should live, as it applies to everyone, not just sunxi.

I think you're thinking too much about the case where u-boot
has an actual driver for the NIC (and that driver gets
initialized, what if it gets skipped to speed up the boot?)
and not enough about the case where there is no driver but
we still want to use u-boot's board specific MAC generation code
to provide a fixed MAC address to the kernel.

Regards,

Hans