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

[Codrin Constantin Ciubotariu]

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

Best regards,
Codrin