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

[Olliver Schinagl]

Hey,


On 15-11-16 11:27, Hans de Goede wrote:
> 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.
In my other e-mail contradiciting myself I just found this somewhat out, 
and that indeed is another usecase worth thinking about yeah. So I'll 
need to re-think that part too.

And then thinks come to mind 'if there are 5 address in the eeprom, but 
only 2 drivers, do the drivers get the first two?' ... I guess there 
needs to be a general agreement on strange cases as such. How are 
non-driver devices handled. The dts obviously is one method, but I'm 
sure board manufactures will hate us for forcing board specific dts. 
They want to just produce boards en-masse and may be kind enough to 
supply eeprom or MAC-prom's with fixed mac addresses stored there.

I think this is an architectural based decision which deserves its own 
thread?

Olliver

>
> Regards,
>
> Hans