[U-Boot] [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address

[York Sun]

On 08/17/2015 07:37 AM, Joe Hershberger wrote:
> On Fri, Aug 14, 2015 at 12:59 PM, York Sun <yorksun at freescale.com> wrote:
>>
>>
>> On 08/14/2015 01:28 AM, Ciubotariu Codrin Constantin-B43658 wrote:
>>> Hi York,
>>>
>>>> -----Original Message-----
>>>> From: Sun York-R58495
>>>> Sent: Thursday, August 13, 2015 6:55 PM
>>>> To: Ciubotariu Codrin Constantin-B43658
>>>> Cc: Joe Hershberger; u-boot at lists.denx.de; joe.hershberger at ni.com
>>>> Subject: Re: [U-Boot] [PATCH v3 11/16] net/eth.c: Add function to validate a MAC
>>>> address
>>>>
>>>>
>>>>
>>>> On 08/13/2015 08:42 AM, Ciubotariu Codrin Constantin-B43658 wrote:
>>>>> Hi Joe, York,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Sun York-R58495
>>>>>> Sent: Wednesday, August 12, 2015 10:59 PM
>>>>>> To: Ciubotariu Codrin Constantin-B43658
>>>>>> Cc: Joe Hershberger; u-boot at lists.denx.de; joe.hershberger at ni.com
>>>>>> Subject: Re: [U-Boot] [PATCH v3 11/16] net/eth.c: Add function to
>>>>>> validate a MAC address
>>>>>>
>>>>>> +Codrin
>>>>>>
>>>>>> Somehow I dropped Codrin in last reply.
>>>>>>
>>>>>> On 08/10/2015 01:45 PM, York Sun wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 08/10/2015 01:05 PM, Joe Hershberger wrote:
>>>>>>>> Hi York,
>>>>>>>>
>>>>>>>> On Mon, Aug 10, 2015 at 3:03 PM, York Sun <yorksun at freescale.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 08/10/2015 12:57 PM, Joe Hershberger wrote:
>>>>>>>>>> Too much top-posting.
>>>>>>>>>>
>>>>>>>>>> On Mon, Aug 10, 2015 at 2:41 PM, York Sun <yorksun at freescale.com> wrote:
>>>>>>>>>>> SPL doesn't use net/eth.c. You add a call in env_flags.c.
>>>>>>>>>>>
>>>>>>>>>>> I think you can put it in header file and use static inline, or
>>>>>>>>>>> keep it in the same file where it is called.
>>>>>>>>>>
>>>>>>>>>> That is probably fine.
>>>>>>>>>>
>>>>>>>>>>> Another way is to undef CONFIG_CMD_NET for SPL part. It is
>>>>>>>>>>> default to 'y' in Kconfig. Joe may have some good suggestion.
>>>>>>>>>>
>>>>>>>>>> I don't think this is the reason. The problem is that net is
>>>>>>>>>> *not* build for SPL, but env is.
>>>>>>>>>
>>>>>>>>> Yes, env is built. The offending lines in common/env_flags.c are
>>>>>>>>> gated by "#ifdef CONFIG_CMD_NET". That's why I say it could be another
>>>> way.
>>>>>>>>
>>>>>>>> OK, sure... but that breaks intended behavior, I think.
>>>>>>>>
>>>>>>>
>>>>>>> I see. The CONFIG_CMD_NET is not evaluated separated for SPL build.
>>>>>>> So I guess the fix can be either to put the common function in
>>>>>>> header file after making it really simple to reduce dependency, or
>>>>>>> to keep the
>>>>>> original code in env_flag.c.
>>>>>>>
>>>>>>
>>>>>> Codrin,
>>>>>>
>>>>>> Can you prepare a new patch? You don't have to send the whole set.
>>>>>> All but one have been acked by Joe.
>>>>>>
>>>>>> York
>>>>>
>>>>> I can't inline eth_validate_ethaddr_str in eth.h since it depends on
>>>> simple_strtoul and tolower. Also, I can't let it in common/env_flags.c because I
>>>> need to access if from drivers/net/vsc9953.c . I guess it has to be in a .c file
>>>> that is also used by SPL targets. Could you please suggest such a file?
>>>>>
>>>>> SPL targets make use of CONFIG_CMD_NET? It seems strange that ETH env is
>>>> built, but net/net.c or net/eth.c not.
>>>>>
>>>>
>>>> I was discussing with Joe about the possibility to deselect CONFIG_CMD_NET for
>>>> SPL build. The issue here is Kconfig is not re-evaluated for the SPL part. If
>>>> you can experiment it, you can try to gate the code in env_flags.c with
>>>> CONFIG_SPL_BUILD. It sounds reasonable to me.
>>>>
>>>> York
>>>
>>> Something like
>>>
>>> #if defined(CONFIG_CMD_NET) && !defined(CONFIG_SPL_BUILD)
>>>       case env_flags_vartype_ipaddr:
>>>               cur = value;
>>>               for (i = 0; i < 4; i++) {
>>>                       skip_num(0, cur, &end, 3);
>>>                       if (cur == end)
>>>                               return -1;
>>>                       if (i != 3 && *end != '.')
>>>                               return -1;
>>>                       if (i == 3 && *end != '\0')
>>>                               return -1;
>>>                       cur = end + 1;
>>>               }
>>>               break;
>>>       case env_flags_vartype_macaddr:
>>>               if (eth_validate_ethaddr_str(value))
>>>                       return -1;
>>>               break;
>>> #endif
>>>
>>> ?
>>>
>>> I get two warnings on this:
>>> ../common/env_flags.c: In function '_env_flags_validate_type':
>>> ../common/env_flags.c:203:2: warning: enumeration value 'env_flags_vartype_ipaddr' not handled in switch [-Wswitch]
>>>   switch (type) {
>>>   ^
>>> ../common/env_flags.c:203:2: warning: enumeration value 'env_flags_vartype_macaddr' not handled in switch [-Wswitch]
>>>
>>> Unless I guard these values in env_flags.h:
>>> #if defined(CONFIG_CMD_NET) && !defined(CONFIG_SPL_BUILD)
>>>       env_flags_vartype_ipaddr,
>>>       env_flags_vartype_macaddr,
>>> #endif
>>>
>>
>> It makes sense to me to take out these two for SPL build. The whole purpose of
>> SPL is to load the final image. I don't see a chance users would be able to type
>> any command.
>>
>> Joe, do you agree the CMD_NET shouldn't be used for SPL?
> 
> I tend to agree, but I have a vague recollection that there is at
> least one SPL that uses net. Sorry I don't recall what board it was
> that I'm thinking about.

That's exactly what I was worry about. In this case, can we settle with adding
eth_validate_ethaddr_str() without changing the code in env_flag.c? It is a
little duplication but seems easy and clean.

York