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

[York Sun <yorksun@freescale.com>]

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@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@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?

York