> -----Original Message----- > From: Joe Hershberger [mailto:joe.hershber...@gmail.com] > Sent: Monday, August 17, 2015 5:38 PM > To: Sun York-R58495 > Cc: Ciubotariu Codrin Constantin-B43658; u-boot@lists.denx.de; > joe.hershber...@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 <york...@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@lists.denx.de; joe.hershber...@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@lists.denx.de; joe.hershber...@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 <york...@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 <york...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot