| Message ID | 1437746136-14379-8-git-send-email-codrin.ciubotariu@freescale.com |
|---|---|
| State | Superseded |
| Delegated to: | York Sun |
| Headers | show |
Hi Codrin, On Fri, Jul 24, 2015 at 8:55 AM, Codrin Ciubotariu <codrin.ciubotariu@freescale.com> wrote: > The code from common/env_flags.c that checks if a > string has the format of a MAC address has been moved > in net/eth.c as a separate function called > eth_validate_ethaddr_str(). > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> > --- > > Changes for v3: > - none, new patch; > > common/env_flags.c | 15 ++------------- > include/net.h | 1 + > net/eth.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 33 insertions(+), 13 deletions(-) Acked-by: Joe Hershberger <joe.hershberger@ni.com>
On 07/24/2015 06:55 AM, Codrin Ciubotariu wrote: > The code from common/env_flags.c that checks if a > string has the format of a MAC address has been moved > in net/eth.c as a separate function called > eth_validate_ethaddr_str(). > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> > --- > > Changes for v3: > - none, new patch; > > common/env_flags.c | 15 ++------------- > include/net.h | 1 + > net/eth.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 33 insertions(+), 13 deletions(-) > > diff --git a/common/env_flags.c b/common/env_flags.c > index 5189f5b..3e39fd1 100644 > --- a/common/env_flags.c > +++ b/common/env_flags.c > @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value, > } > break; > case env_flags_vartype_macaddr: > - cur = value; > - for (i = 0; i < 6; i++) { > - skip_num(1, cur, &end, 2); > - if (cur == end) > - return -1; > - if (cur + 2 == end && is_hex_prefix(cur)) > - return -1; > - if (i != 5 && *end != ':') > - return -1; > - if (i == 5 && *end != '\0') > - return -1; > - cur = end + 1; > - } > + if (eth_validate_ethaddr_str(value)) > + return -1; > break; > #endif > case env_flags_vartype_end: > diff --git a/include/net.h b/include/net.h > index d17173d..c487aa7 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -218,6 +218,7 @@ void eth_try_another(int first_restart); /* Change the device */ > void eth_set_current(void); /* set nterface to ethcur var */ > > int eth_get_dev_index(void); /* get the device index */ > +int eth_validate_ethaddr_str(const char *addr); > void eth_parse_enetaddr(const char *addr, uchar *enetaddr); > int eth_getenv_enetaddr(char *name, uchar *enetaddr); > int eth_setenv_enetaddr(char *name, const uchar *enetaddr); > diff --git a/net/eth.c b/net/eth.c > index 953b731..a6fdf1b 100644 > --- a/net/eth.c > +++ b/net/eth.c > @@ -7,6 +7,7 @@ > */ > > #include <common.h> > +#include <linux/ctype.h> > #include <command.h> > #include <dm.h> > #include <environment.h> > @@ -19,6 +20,35 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +int eth_validate_ethaddr_str(const char *addr) > +{ > + unsigned long val; > + int i; > + const char *cur; > + char *end; > + > + if (!addr) > + return -1; > + > + cur = addr; > + for (i = 0; i < 6; i++) { > + val = simple_strtoul(cur, &end, 16); > + if (cur + 1 != end && cur + 2 != end) > + return -1; > + if (val > 0xff) > + return -1; > + if (cur + 2 >= end && tolower(*(cur + 1)) == 'x') > + return -1; > + if (i != 5 && *end != ':') > + return -1; > + if (i == 5 && *end != '\0') > + return -1; > + cur = end + 1; > + } > + > + return 0; > +} > + > void eth_parse_enetaddr(const char *addr, uchar *enetaddr) > { > char *end; > Codrin, This patch breaks most SPL targets. Please reconsider the location of eth_validate_ethaddr_str(). York
Hi York, I didn't know that SPL uses net/eth.c . Could you please suggest a place for eth_validate_ethaddr_str()? Thanks and best regards, Codrin > -----Original Message----- > From: Sun York-R58495 > Sent: Saturday, August 08, 2015 3:31 AM > To: Ciubotariu Codrin Constantin-B43658; u-boot@lists.denx.de > Cc: joe.hershberger@ni.com; Kushwaha Prabhakar-B32579 > Subject: Re: [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address > > On 07/24/2015 06:55 AM, Codrin Ciubotariu wrote: > > The code from common/env_flags.c that checks if a string has the > > format of a MAC address has been moved in net/eth.c as a separate > > function called eth_validate_ethaddr_str(). > > > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> > > --- > > > > Changes for v3: > > - none, new patch; > > > > common/env_flags.c | 15 ++------------- > > include/net.h | 1 + > > net/eth.c | 30 ++++++++++++++++++++++++++++++ > > 3 files changed, 33 insertions(+), 13 deletions(-) > > > > diff --git a/common/env_flags.c b/common/env_flags.c index > > 5189f5b..3e39fd1 100644 > > --- a/common/env_flags.c > > +++ b/common/env_flags.c > > @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value, > > } > > break; > > case env_flags_vartype_macaddr: > > - cur = value; > > - for (i = 0; i < 6; i++) { > > - skip_num(1, cur, &end, 2); > > - if (cur == end) > > - return -1; > > - if (cur + 2 == end && is_hex_prefix(cur)) > > - return -1; > > - if (i != 5 && *end != ':') > > - return -1; > > - if (i == 5 && *end != '\0') > > - return -1; > > - cur = end + 1; > > - } > > + if (eth_validate_ethaddr_str(value)) > > + return -1; > > break; > > #endif > > case env_flags_vartype_end: > > diff --git a/include/net.h b/include/net.h index d17173d..c487aa7 > > 100644 > > --- a/include/net.h > > +++ b/include/net.h > > @@ -218,6 +218,7 @@ void eth_try_another(int first_restart); /* Change the > device */ > > void eth_set_current(void); /* set nterface to ethcur var */ > > > > int eth_get_dev_index(void); /* get the device index */ > > +int eth_validate_ethaddr_str(const char *addr); > > void eth_parse_enetaddr(const char *addr, uchar *enetaddr); int > > eth_getenv_enetaddr(char *name, uchar *enetaddr); int > > eth_setenv_enetaddr(char *name, const uchar *enetaddr); diff --git > > a/net/eth.c b/net/eth.c index 953b731..a6fdf1b 100644 > > --- a/net/eth.c > > +++ b/net/eth.c > > @@ -7,6 +7,7 @@ > > */ > > > > #include <common.h> > > +#include <linux/ctype.h> > > #include <command.h> > > #include <dm.h> > > #include <environment.h> > > @@ -19,6 +20,35 @@ > > > > DECLARE_GLOBAL_DATA_PTR; > > > > +int eth_validate_ethaddr_str(const char *addr) { > > + unsigned long val; > > + int i; > > + const char *cur; > > + char *end; > > + > > + if (!addr) > > + return -1; > > + > > + cur = addr; > > + for (i = 0; i < 6; i++) { > > + val = simple_strtoul(cur, &end, 16); > > + if (cur + 1 != end && cur + 2 != end) > > + return -1; > > + if (val > 0xff) > > + return -1; > > + if (cur + 2 >= end && tolower(*(cur + 1)) == 'x') > > + return -1; > > + if (i != 5 && *end != ':') > > + return -1; > > + if (i == 5 && *end != '\0') > > + return -1; > > + cur = end + 1; > > + } > > + > > + return 0; > > +} > > + > > void eth_parse_enetaddr(const char *addr, uchar *enetaddr) { > > char *end; > > > > Codrin, > > This patch breaks most SPL targets. Please reconsider the location of > eth_validate_ethaddr_str(). > > York
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. Another way is to undef CONFIG_CMD_NET for SPL part. It is default to 'y' in Kconfig. Joe may have some good suggestion. York On 08/10/2015 01:44 AM, Ciubotariu Codrin Constantin-B43658 wrote: > Hi York, > > I didn't know that SPL uses net/eth.c . Could you please suggest a place for eth_validate_ethaddr_str()? > > Thanks and best regards, > Codrin > >> -----Original Message----- >> From: Sun York-R58495 >> Sent: Saturday, August 08, 2015 3:31 AM >> To: Ciubotariu Codrin Constantin-B43658; u-boot@lists.denx.de >> Cc: joe.hershberger@ni.com; Kushwaha Prabhakar-B32579 >> Subject: Re: [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address >> >> On 07/24/2015 06:55 AM, Codrin Ciubotariu wrote: >>> The code from common/env_flags.c that checks if a string has the >>> format of a MAC address has been moved in net/eth.c as a separate >>> function called eth_validate_ethaddr_str(). >>> >>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> >>> --- >>> >>> Changes for v3: >>> - none, new patch; >>> >>> common/env_flags.c | 15 ++------------- >>> include/net.h | 1 + >>> net/eth.c | 30 ++++++++++++++++++++++++++++++ >>> 3 files changed, 33 insertions(+), 13 deletions(-) >>> >>> diff --git a/common/env_flags.c b/common/env_flags.c index >>> 5189f5b..3e39fd1 100644 >>> --- a/common/env_flags.c >>> +++ b/common/env_flags.c >>> @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value, >>> } >>> break; >>> case env_flags_vartype_macaddr: >>> - cur = value; >>> - for (i = 0; i < 6; i++) { >>> - skip_num(1, cur, &end, 2); >>> - if (cur == end) >>> - return -1; >>> - if (cur + 2 == end && is_hex_prefix(cur)) >>> - return -1; >>> - if (i != 5 && *end != ':') >>> - return -1; >>> - if (i == 5 && *end != '\0') >>> - return -1; >>> - cur = end + 1; >>> - } >>> + if (eth_validate_ethaddr_str(value)) >>> + return -1; >>> break; >>> #endif >>> case env_flags_vartype_end: >>> diff --git a/include/net.h b/include/net.h index d17173d..c487aa7 >>> 100644 >>> --- a/include/net.h >>> +++ b/include/net.h >>> @@ -218,6 +218,7 @@ void eth_try_another(int first_restart); /* Change the >> device */ >>> void eth_set_current(void); /* set nterface to ethcur var */ >>> >>> int eth_get_dev_index(void); /* get the device index */ >>> +int eth_validate_ethaddr_str(const char *addr); >>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr); int >>> eth_getenv_enetaddr(char *name, uchar *enetaddr); int >>> eth_setenv_enetaddr(char *name, const uchar *enetaddr); diff --git >>> a/net/eth.c b/net/eth.c index 953b731..a6fdf1b 100644 >>> --- a/net/eth.c >>> +++ b/net/eth.c >>> @@ -7,6 +7,7 @@ >>> */ >>> >>> #include <common.h> >>> +#include <linux/ctype.h> >>> #include <command.h> >>> #include <dm.h> >>> #include <environment.h> >>> @@ -19,6 +20,35 @@ >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> +int eth_validate_ethaddr_str(const char *addr) { >>> + unsigned long val; >>> + int i; >>> + const char *cur; >>> + char *end; >>> + >>> + if (!addr) >>> + return -1; >>> + >>> + cur = addr; >>> + for (i = 0; i < 6; i++) { >>> + val = simple_strtoul(cur, &end, 16); >>> + if (cur + 1 != end && cur + 2 != end) >>> + return -1; >>> + if (val > 0xff) >>> + return -1; >>> + if (cur + 2 >= end && tolower(*(cur + 1)) == 'x') >>> + return -1; >>> + if (i != 5 && *end != ':') >>> + return -1; >>> + if (i == 5 && *end != '\0') >>> + return -1; >>> + cur = end + 1; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr) { >>> char *end; >>> >> >> Codrin, >> >> This patch breaks most SPL targets. Please reconsider the location of >> eth_validate_ethaddr_str(). >> >> York
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. > On 08/10/2015 01:44 AM, Ciubotariu Codrin Constantin-B43658 wrote: >> Hi York, >> >> I didn't know that SPL uses net/eth.c . Could you please suggest a place for eth_validate_ethaddr_str()? >> >> Thanks and best regards, >> Codrin >> >>> -----Original Message----- >>> From: Sun York-R58495 >>> Sent: Saturday, August 08, 2015 3:31 AM >>> To: Ciubotariu Codrin Constantin-B43658; u-boot@lists.denx.de >>> Cc: joe.hershberger@ni.com; Kushwaha Prabhakar-B32579 >>> Subject: Re: [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address >>> >>> On 07/24/2015 06:55 AM, Codrin Ciubotariu wrote: >>>> The code from common/env_flags.c that checks if a string has the >>>> format of a MAC address has been moved in net/eth.c as a separate >>>> function called eth_validate_ethaddr_str(). >>>> >>>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> >>>> --- >>>> >>>> Changes for v3: >>>> - none, new patch; >>>> >>>> common/env_flags.c | 15 ++------------- >>>> include/net.h | 1 + >>>> net/eth.c | 30 ++++++++++++++++++++++++++++++ >>>> 3 files changed, 33 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/common/env_flags.c b/common/env_flags.c index >>>> 5189f5b..3e39fd1 100644 >>>> --- a/common/env_flags.c >>>> +++ b/common/env_flags.c >>>> @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value, >>>> } >>>> break; >>>> case env_flags_vartype_macaddr: >>>> - cur = value; >>>> - for (i = 0; i < 6; i++) { >>>> - skip_num(1, cur, &end, 2); >>>> - if (cur == end) >>>> - return -1; >>>> - if (cur + 2 == end && is_hex_prefix(cur)) >>>> - return -1; >>>> - if (i != 5 && *end != ':') >>>> - return -1; >>>> - if (i == 5 && *end != '\0') >>>> - return -1; >>>> - cur = end + 1; >>>> - } >>>> + if (eth_validate_ethaddr_str(value)) >>>> + return -1; >>>> break; >>>> #endif >>>> case env_flags_vartype_end: >>>> diff --git a/include/net.h b/include/net.h index d17173d..c487aa7 >>>> 100644 >>>> --- a/include/net.h >>>> +++ b/include/net.h >>>> @@ -218,6 +218,7 @@ void eth_try_another(int first_restart); /* Change the >>> device */ >>>> void eth_set_current(void); /* set nterface to ethcur var */ >>>> >>>> int eth_get_dev_index(void); /* get the device index */ >>>> +int eth_validate_ethaddr_str(const char *addr); >>>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr); int >>>> eth_getenv_enetaddr(char *name, uchar *enetaddr); int >>>> eth_setenv_enetaddr(char *name, const uchar *enetaddr); diff --git >>>> a/net/eth.c b/net/eth.c index 953b731..a6fdf1b 100644 >>>> --- a/net/eth.c >>>> +++ b/net/eth.c >>>> @@ -7,6 +7,7 @@ >>>> */ >>>> >>>> #include <common.h> >>>> +#include <linux/ctype.h> >>>> #include <command.h> >>>> #include <dm.h> >>>> #include <environment.h> >>>> @@ -19,6 +20,35 @@ >>>> >>>> DECLARE_GLOBAL_DATA_PTR; >>>> >>>> +int eth_validate_ethaddr_str(const char *addr) { >>>> + unsigned long val; >>>> + int i; >>>> + const char *cur; >>>> + char *end; >>>> + >>>> + if (!addr) >>>> + return -1; >>>> + >>>> + cur = addr; >>>> + for (i = 0; i < 6; i++) { >>>> + val = simple_strtoul(cur, &end, 16); >>>> + if (cur + 1 != end && cur + 2 != end) >>>> + return -1; >>>> + if (val > 0xff) >>>> + return -1; >>>> + if (cur + 2 >= end && tolower(*(cur + 1)) == 'x') >>>> + return -1; >>>> + if (i != 5 && *end != ':') >>>> + return -1; >>>> + if (i == 5 && *end != '\0') >>>> + return -1; >>>> + cur = end + 1; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr) { >>>> char *end; >>>> >>> >>> Codrin, >>> >>> This patch breaks most SPL targets. Please reconsider the location of >>> eth_validate_ethaddr_str(). >>> >>> York > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
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. York > >> On 08/10/2015 01:44 AM, Ciubotariu Codrin Constantin-B43658 wrote: >>> Hi York, >>> >>> I didn't know that SPL uses net/eth.c . Could you please suggest a place for eth_validate_ethaddr_str()? >>> >>> Thanks and best regards, >>> Codrin >>> >>>> -----Original Message----- >>>> From: Sun York-R58495 >>>> Sent: Saturday, August 08, 2015 3:31 AM >>>> To: Ciubotariu Codrin Constantin-B43658; u-boot@lists.denx.de >>>> Cc: joe.hershberger@ni.com; Kushwaha Prabhakar-B32579 >>>> Subject: Re: [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address >>>> >>>> On 07/24/2015 06:55 AM, Codrin Ciubotariu wrote: >>>>> The code from common/env_flags.c that checks if a string has the >>>>> format of a MAC address has been moved in net/eth.c as a separate >>>>> function called eth_validate_ethaddr_str(). >>>>> >>>>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> >>>>> --- >>>>> >>>>> Changes for v3: >>>>> - none, new patch; >>>>> >>>>> common/env_flags.c | 15 ++------------- >>>>> include/net.h | 1 + >>>>> net/eth.c | 30 ++++++++++++++++++++++++++++++ >>>>> 3 files changed, 33 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/common/env_flags.c b/common/env_flags.c index >>>>> 5189f5b..3e39fd1 100644 >>>>> --- a/common/env_flags.c >>>>> +++ b/common/env_flags.c >>>>> @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value, >>>>> } >>>>> break; >>>>> case env_flags_vartype_macaddr: >>>>> - cur = value; >>>>> - for (i = 0; i < 6; i++) { >>>>> - skip_num(1, cur, &end, 2); >>>>> - if (cur == end) >>>>> - return -1; >>>>> - if (cur + 2 == end && is_hex_prefix(cur)) >>>>> - return -1; >>>>> - if (i != 5 && *end != ':') >>>>> - return -1; >>>>> - if (i == 5 && *end != '\0') >>>>> - return -1; >>>>> - cur = end + 1; >>>>> - } >>>>> + if (eth_validate_ethaddr_str(value)) >>>>> + return -1; >>>>> break; >>>>> #endif >>>>> case env_flags_vartype_end: >>>>> diff --git a/include/net.h b/include/net.h index d17173d..c487aa7 >>>>> 100644 >>>>> --- a/include/net.h >>>>> +++ b/include/net.h >>>>> @@ -218,6 +218,7 @@ void eth_try_another(int first_restart); /* Change the >>>> device */ >>>>> void eth_set_current(void); /* set nterface to ethcur var */ >>>>> >>>>> int eth_get_dev_index(void); /* get the device index */ >>>>> +int eth_validate_ethaddr_str(const char *addr); >>>>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr); int >>>>> eth_getenv_enetaddr(char *name, uchar *enetaddr); int >>>>> eth_setenv_enetaddr(char *name, const uchar *enetaddr); diff --git >>>>> a/net/eth.c b/net/eth.c index 953b731..a6fdf1b 100644 >>>>> --- a/net/eth.c >>>>> +++ b/net/eth.c >>>>> @@ -7,6 +7,7 @@ >>>>> */ >>>>> >>>>> #include <common.h> >>>>> +#include <linux/ctype.h> >>>>> #include <command.h> >>>>> #include <dm.h> >>>>> #include <environment.h> >>>>> @@ -19,6 +20,35 @@ >>>>> >>>>> DECLARE_GLOBAL_DATA_PTR; >>>>> >>>>> +int eth_validate_ethaddr_str(const char *addr) { >>>>> + unsigned long val; >>>>> + int i; >>>>> + const char *cur; >>>>> + char *end; >>>>> + >>>>> + if (!addr) >>>>> + return -1; >>>>> + >>>>> + cur = addr; >>>>> + for (i = 0; i < 6; i++) { >>>>> + val = simple_strtoul(cur, &end, 16); >>>>> + if (cur + 1 != end && cur + 2 != end) >>>>> + return -1; >>>>> + if (val > 0xff) >>>>> + return -1; >>>>> + if (cur + 2 >= end && tolower(*(cur + 1)) == 'x') >>>>> + return -1; >>>>> + if (i != 5 && *end != ':') >>>>> + return -1; >>>>> + if (i == 5 && *end != '\0') >>>>> + return -1; >>>>> + cur = end + 1; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr) { >>>>> char *end; >>>>> >>>> >>>> Codrin, >>>> >>>> This patch breaks most SPL targets. Please reconsider the location of >>>> eth_validate_ethaddr_str(). >>>> >>>> York >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot
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. >> >>> On 08/10/2015 01:44 AM, Ciubotariu Codrin Constantin-B43658 wrote: >>>> Hi York, >>>> >>>> I didn't know that SPL uses net/eth.c . Could you please suggest a place for eth_validate_ethaddr_str()? >>>> >>>> Thanks and best regards, >>>> Codrin >>>> >>>>> -----Original Message----- >>>>> From: Sun York-R58495 >>>>> Sent: Saturday, August 08, 2015 3:31 AM >>>>> To: Ciubotariu Codrin Constantin-B43658; u-boot@lists.denx.de >>>>> Cc: joe.hershberger@ni.com; Kushwaha Prabhakar-B32579 >>>>> Subject: Re: [PATCH v3 11/16] net/eth.c: Add function to validate a MAC address >>>>> >>>>> On 07/24/2015 06:55 AM, Codrin Ciubotariu wrote: >>>>>> The code from common/env_flags.c that checks if a string has the >>>>>> format of a MAC address has been moved in net/eth.c as a separate >>>>>> function called eth_validate_ethaddr_str(). >>>>>> >>>>>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> >>>>>> --- >>>>>> >>>>>> Changes for v3: >>>>>> - none, new patch; >>>>>> >>>>>> common/env_flags.c | 15 ++------------- >>>>>> include/net.h | 1 + >>>>>> net/eth.c | 30 ++++++++++++++++++++++++++++++ >>>>>> 3 files changed, 33 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/common/env_flags.c b/common/env_flags.c index >>>>>> 5189f5b..3e39fd1 100644 >>>>>> --- a/common/env_flags.c >>>>>> +++ b/common/env_flags.c >>>>>> @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value, >>>>>> } >>>>>> break; >>>>>> case env_flags_vartype_macaddr: >>>>>> - cur = value; >>>>>> - for (i = 0; i < 6; i++) { >>>>>> - skip_num(1, cur, &end, 2); >>>>>> - if (cur == end) >>>>>> - return -1; >>>>>> - if (cur + 2 == end && is_hex_prefix(cur)) >>>>>> - return -1; >>>>>> - if (i != 5 && *end != ':') >>>>>> - return -1; >>>>>> - if (i == 5 && *end != '\0') >>>>>> - return -1; >>>>>> - cur = end + 1; >>>>>> - } >>>>>> + if (eth_validate_ethaddr_str(value)) >>>>>> + return -1; >>>>>> break; >>>>>> #endif >>>>>> case env_flags_vartype_end: >>>>>> diff --git a/include/net.h b/include/net.h index d17173d..c487aa7 >>>>>> 100644 >>>>>> --- a/include/net.h >>>>>> +++ b/include/net.h >>>>>> @@ -218,6 +218,7 @@ void eth_try_another(int first_restart); /* Change the >>>>> device */ >>>>>> void eth_set_current(void); /* set nterface to ethcur var */ >>>>>> >>>>>> int eth_get_dev_index(void); /* get the device index */ >>>>>> +int eth_validate_ethaddr_str(const char *addr); >>>>>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr); int >>>>>> eth_getenv_enetaddr(char *name, uchar *enetaddr); int >>>>>> eth_setenv_enetaddr(char *name, const uchar *enetaddr); diff --git >>>>>> a/net/eth.c b/net/eth.c index 953b731..a6fdf1b 100644 >>>>>> --- a/net/eth.c >>>>>> +++ b/net/eth.c >>>>>> @@ -7,6 +7,7 @@ >>>>>> */ >>>>>> >>>>>> #include <common.h> >>>>>> +#include <linux/ctype.h> >>>>>> #include <command.h> >>>>>> #include <dm.h> >>>>>> #include <environment.h> >>>>>> @@ -19,6 +20,35 @@ >>>>>> >>>>>> DECLARE_GLOBAL_DATA_PTR; >>>>>> >>>>>> +int eth_validate_ethaddr_str(const char *addr) { >>>>>> + unsigned long val; >>>>>> + int i; >>>>>> + const char *cur; >>>>>> + char *end; >>>>>> + >>>>>> + if (!addr) >>>>>> + return -1; >>>>>> + >>>>>> + cur = addr; >>>>>> + for (i = 0; i < 6; i++) { >>>>>> + val = simple_strtoul(cur, &end, 16); >>>>>> + if (cur + 1 != end && cur + 2 != end) >>>>>> + return -1; >>>>>> + if (val > 0xff) >>>>>> + return -1; >>>>>> + if (cur + 2 >= end && tolower(*(cur + 1)) == 'x') >>>>>> + return -1; >>>>>> + if (i != 5 && *end != ':') >>>>>> + return -1; >>>>>> + if (i == 5 && *end != '\0') >>>>>> + return -1; >>>>>> + cur = end + 1; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> void eth_parse_enetaddr(const char *addr, uchar *enetaddr) { >>>>>> char *end; >>>>>> >>>>> >>>>> Codrin, >>>>> >>>>> This patch breaks most SPL targets. Please reconsider the location of >>>>> eth_validate_ethaddr_str(). >>>>> >>>>> York >>> _______________________________________________ >>> U-Boot mailing list >>> U-Boot@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot
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. York
+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
Hi York, Sure, I will make v4 this week for this change only. I will try to shrink the function and inline it in the header file. Thanks and best regards, Codrin > -----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.hershberger@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
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.hershberger@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. Thanks and best regards, Codrin
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.hershberger@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
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.hershberger@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.hershberger@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 Best regards, Codrin
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.hershberger@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.hershberger@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
On Fri, Aug 14, 2015 at 12:59 PM, York Sun <yorksun@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.hershberger@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.hershberger@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? 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. -Joe
On 08/17/2015 07:37 AM, Joe Hershberger wrote: > On Fri, Aug 14, 2015 at 12:59 PM, York Sun <yorksun@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.hershberger@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.hershberger@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? > > 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
> -----Original Message----- > From: Joe Hershberger [mailto:joe.hershberger@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.hershberger@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@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.hershberger@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.hershberger@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? > > 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
diff --git a/common/env_flags.c b/common/env_flags.c index 5189f5b..3e39fd1 100644 --- a/common/env_flags.c +++ b/common/env_flags.c @@ -239,19 +239,8 @@ static int _env_flags_validate_type(const char *value, } break; case env_flags_vartype_macaddr: - cur = value; - for (i = 0; i < 6; i++) { - skip_num(1, cur, &end, 2); - if (cur == end) - return -1; - if (cur + 2 == end && is_hex_prefix(cur)) - return -1; - if (i != 5 && *end != ':') - return -1; - if (i == 5 && *end != '\0') - return -1; - cur = end + 1; - } + if (eth_validate_ethaddr_str(value)) + return -1; break; #endif case env_flags_vartype_end: diff --git a/include/net.h b/include/net.h index d17173d..c487aa7 100644 --- a/include/net.h +++ b/include/net.h @@ -218,6 +218,7 @@ void eth_try_another(int first_restart); /* Change the device */ void eth_set_current(void); /* set nterface to ethcur var */ int eth_get_dev_index(void); /* get the device index */ +int eth_validate_ethaddr_str(const char *addr); void eth_parse_enetaddr(const char *addr, uchar *enetaddr); int eth_getenv_enetaddr(char *name, uchar *enetaddr); int eth_setenv_enetaddr(char *name, const uchar *enetaddr); diff --git a/net/eth.c b/net/eth.c index 953b731..a6fdf1b 100644 --- a/net/eth.c +++ b/net/eth.c @@ -7,6 +7,7 @@ */ #include <common.h> +#include <linux/ctype.h> #include <command.h> #include <dm.h> #include <environment.h> @@ -19,6 +20,35 @@ DECLARE_GLOBAL_DATA_PTR; +int eth_validate_ethaddr_str(const char *addr) +{ + unsigned long val; + int i; + const char *cur; + char *end; + + if (!addr) + return -1; + + cur = addr; + for (i = 0; i < 6; i++) { + val = simple_strtoul(cur, &end, 16); + if (cur + 1 != end && cur + 2 != end) + return -1; + if (val > 0xff) + return -1; + if (cur + 2 >= end && tolower(*(cur + 1)) == 'x') + return -1; + if (i != 5 && *end != ':') + return -1; + if (i == 5 && *end != '\0') + return -1; + cur = end + 1; + } + + return 0; +} + void eth_parse_enetaddr(const char *addr, uchar *enetaddr) { char *end;
The code from common/env_flags.c that checks if a string has the format of a MAC address has been moved in net/eth.c as a separate function called eth_validate_ethaddr_str(). Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com> --- Changes for v3: - none, new patch; common/env_flags.c | 15 ++------------- include/net.h | 1 + net/eth.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 13 deletions(-)