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

[Codrin Constantin Ciubotariu]

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> Sent: Monday, August 17, 2015 5:38 PM
> To: Sun York-R58495
> Cc: Ciubotariu Codrin Constantin-B43658; 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 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.

Is there some other place we could add eth_validate_ethaddr_str()? In a file also used by SPL targets?

Best regards,
Codrin

> 
> -Joe