Patchwork [U-Boot,v5,4/5] net: add eth_setenv_enetaddr_by_index()

login
register
mail settings
Submitter Michael Walle
Date May 11, 2012, 10:50 p.m.
Message ID <1336776649-9316-5-git-send-email-michael@walle.cc>
Download mbox | patch
Permalink /patch/158624/
State Superseded
Delegated to: Prafulla Wadaskar
Headers show

Comments

Michael Walle - May 11, 2012, 10:50 p.m.
Signed-off-by: Michael Walle <michael@walle.cc>
Cc: Joe Hershberger <joe.hershberger@gmail.com>
---
 include/net.h |   16 ++++++++++++++++
 net/eth.c     |   15 +++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)
Michael Walle - May 11, 2012, 10:53 p.m.
Am Samstag 12 Mai 2012, 00:50:48 schrieb Michael Walle:
> Signed-off-by: Michael Walle <michael@walle.cc>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>
> ---
>  include/net.h |   16 ++++++++++++++++
>  net/eth.c     |   15 +++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net.h b/include/net.h
> index f6aeba2..bdc3da6 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int
> index); /* get dev @ index */ extern int eth_get_dev_index (void);		
/* get
> the device index */
>  extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
>  extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
> +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
>  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
> +#endif

sorry, wrong function, will be corrected in next version.

--
michael
Joe Hershberger - May 16, 2012, 12:56 a.m.
Hi Michael,

On Fri, May 11, 2012 at 5:50 PM, Michael Walle <michael@walle.cc> wrote:
> Signed-off-by: Michael Walle <michael@walle.cc>
> Cc: Joe Hershberger <joe.hershberger@gmail.com>
> ---
>  include/net.h |   16 ++++++++++++++++
>  net/eth.c     |   15 +++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index f6aeba2..bdc3da6 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */
>  extern int eth_get_dev_index (void);           /* get the device index */
>  extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
>  extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
> +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
>  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
> +#endif

Which other function did you intend?  You already guarded the only
definition whose implementation is guarded.

>  /*
>  * Get the hardware address for an ethernet interface .
> @@ -118,6 +120,20 @@ extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
>  extern int eth_getenv_enetaddr_by_index(const char *base_name, int index,
>                                        uchar *enetaddr);
>
> +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX

Personally I don't like config options for something so tiny.  If you
really must guard against including it, use the CONFIG_RANDOM_MACADDR
instead.  I'd prefer to just let the linker exclude it when it's never
referenced.

> +/*
> + * Set the hardware address for an ethernet interface .
> + * Args:
> + *     base_name - base name for device (normally "eth")
> + *     index - device index number (0 for first)
> + *     enetaddr - returns 6 byte hardware address
> + * Returns:
> + *     0 on success, else 1.
> + */
> +extern int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +                                       const uchar *enetaddr);
> +#endif
> +
>  #ifdef CONFIG_RANDOM_MACADDR
>  /*
>  * The u-boot policy does not allow hardcoded ethernet addresses. Under the
> diff --git a/net/eth.c b/net/eth.c
> index afce863..d66e22a 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -67,6 +67,21 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index,
>        return eth_getenv_enetaddr(enetvar, enetaddr);
>  }
>
> +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +               const uchar *enetaddr)
> +{
> +       char enetvar[32];
> +
> +       if (index)
> +               sprintf(enetvar, "%s%daddr", base_name, index);
> +       else
> +               sprintf(enetvar, "%saddr", base_name);
> +
> +       return eth_setenv_enetaddr(enetvar, enetaddr);
> +}
> +#endif

You should synchronize with Rob Herring who is also trying to add this
eth_setenv_enetaddr_by_index() function.
http://patchwork.ozlabs.org/patch/152590/

Also since Rob's patch would use this function all the time, the
CONFIG_SETENV_ENETADDR_BY_INDEX would be removed, right?

Thanks,
-Joe
Michael Walle - May 17, 2012, 8:43 p.m.
Hi Joe,

Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger:
> Hi Michael,
> 
> On Fri, May 11, 2012 at 5:50 PM, Michael Walle <michael@walle.cc> wrote:
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > Cc: Joe Hershberger <joe.hershberger@gmail.com>
> > ---
> >  include/net.h |   16 ++++++++++++++++
> >  net/eth.c     |   15 +++++++++++++++
> >  2 files changed, 31 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/net.h b/include/net.h
> > index f6aeba2..bdc3da6 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int
> > index); /* get dev @ index */ extern int eth_get_dev_index (void);      
> >     /* get the device index */ extern void eth_parse_enetaddr(const char
> > *addr, uchar *enetaddr); extern int eth_getenv_enetaddr(char *name,
> > uchar *enetaddr);
> > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> >  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
> > +#endif
> 
> Which other function did you intend?  You already guarded the only
> definition whose implementation is guarded.
That was a mistake. Will be fixed in the next version.

> >  /*
> >  * Get the hardware address for an ethernet interface .
> > @@ -118,6 +120,20 @@ extern int eth_setenv_enetaddr(char *name, const
> > uchar *enetaddr); extern int eth_getenv_enetaddr_by_index(const char
> > *base_name, int index, uchar *enetaddr);
> > 
> > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> 
> Personally I don't like config options for something so tiny.  If you
> really must guard against including it, use the CONFIG_RANDOM_MACADDR
> instead.
Me, too. But Wolfgang doesn't accept patches which increases the code size for 
other boards.

I don't think it is a good idea to use CONFIG_RANDOM_MACADDR for such a 
generic function, which will (hopefully) be used by others too.

> I'd prefer to just let the linker exclude it when it's never
> referenced.
Correct, me if i'm wrong, but the linker can only discard unused sections. But 
there was some discussion on -ffunction-sections and -fdata-sections on the 
ML. So maybe theres some light at the end of the tunnel ;)

> 
> > +/*
> > + * Set the hardware address for an ethernet interface .
> > + * Args:
> > + *     base_name - base name for device (normally "eth")
> > + *     index - device index number (0 for first)
> > + *     enetaddr - returns 6 byte hardware address
> > + * Returns:
> > + *     0 on success, else 1.
> > + */
> > +extern int eth_setenv_enetaddr_by_index(const char *base_name, int
> > index, +                                       const uchar *enetaddr);
> > +#endif
> > +
> >  #ifdef CONFIG_RANDOM_MACADDR
> >  /*
> >  * The u-boot policy does not allow hardcoded ethernet addresses. Under
> > the diff --git a/net/eth.c b/net/eth.c
> > index afce863..d66e22a 100644
> > --- a/net/eth.c
> > +++ b/net/eth.c
> > @@ -67,6 +67,21 @@ int eth_getenv_enetaddr_by_index(const char
> > *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr);
> >  }
> > 
> > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> > +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> > +               const uchar *enetaddr)
> > +{
> > +       char enetvar[32];
> > +
> > +       if (index)
> > +               sprintf(enetvar, "%s%daddr", base_name, index);
> > +       else
> > +               sprintf(enetvar, "%saddr", base_name);
> > +
> > +       return eth_setenv_enetaddr(enetvar, enetaddr);
> > +}
> > +#endif
> 
> You should synchronize with Rob Herring who is also trying to add this
> eth_setenv_enetaddr_by_index() function.
> http://patchwork.ozlabs.org/patch/152590/
ah thanks for the hint.

> 
> Also since Rob's patch would use this function all the time, the
> CONFIG_SETENV_ENETADDR_BY_INDEX would be removed, right?
right, but see above.
Michael Walle - May 22, 2012, 9:18 p.m.
Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger:
> You should synchronize with Rob Herring who is also trying to add this
> eth_setenv_enetaddr_by_index() function.
> http://patchwork.ozlabs.org/patch/152590/

Hi Rob,

Joe mentioned you have the same patch as me here:
  http://patchwork.ozlabs.org/patch/158624/

What is the current status of your patch?

Wolfgang had some comments on my patch:
  http://patchwork.ozlabs.org/patch/158354/

Could you incorporate these changes into your patch?

Unfortunately, i don't see a way to add the helper function without meeting 
Wolfgangs requirement not to increase the binary file for other boards. Unless 
gc-sections and function-sections are used on ARM, or adding an CONFIG_XX 
macro to mask the helper on compile time.
Rob Herring - May 22, 2012, 9:41 p.m.
On 05/22/2012 04:18 PM, Michael Walle wrote:
> Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger:
>> You should synchronize with Rob Herring who is also trying to add this
>> eth_setenv_enetaddr_by_index() function.
>> http://patchwork.ozlabs.org/patch/152590/
> 
> Hi Rob,
> 
> Joe mentioned you have the same patch as me here:
>   http://patchwork.ozlabs.org/patch/158624/
> 
> What is the current status of your patch?

I have no idea. It's been in the patch system for a month, gotten no
comments, and delegated to jhersh.

> Wolfgang had some comments on my patch:
>   http://patchwork.ozlabs.org/patch/158354/
> 
> Could you incorporate these changes into your patch?
> 
> Unfortunately, i don't see a way to add the helper function without meeting 
> Wolfgangs requirement not to increase the binary file for other boards. Unless 
> gc-sections and function-sections are used on ARM, or adding an CONFIG_XX 
> macro to mask the helper on compile time.

There was a patch series for gc-sections recently...

I don't see a way to meet this impossible requirement either, and I
don't really have the bandwidth right now.

Rob
Michael Walle - May 22, 2012, 9:53 p.m.
Am Dienstag 22 Mai 2012, 23:41:55 schrieb Rob Herring:
> On 05/22/2012 04:18 PM, Michael Walle wrote:
> > Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger:
> >> You should synchronize with Rob Herring who is also trying to add this
> >> eth_setenv_enetaddr_by_index() function.
> >> http://patchwork.ozlabs.org/patch/152590/
> > 
> > Hi Rob,
> > 
> > Joe mentioned you have the same patch as me here:
> >   http://patchwork.ozlabs.org/patch/158624/
> > 
> > What is the current status of your patch?
> 
> I have no idea. It's been in the patch system for a month, gotten no
> comments, and delegated to jhersh.
> 
> > Wolfgang had some comments on my patch:
> >   http://patchwork.ozlabs.org/patch/158354/
> > 
> > Could you incorporate these changes into your patch?
> > 
> > Unfortunately, i don't see a way to add the helper function without
> > meeting Wolfgangs requirement not to increase the binary file for other
> > boards. Unless gc-sections and function-sections are used on ARM, or
> > adding an CONFIG_XX macro to mask the helper on compile time.
> 
> There was a patch series for gc-sections recently...
yeah saw that too, but someone mentioned there were problems on arm, iirc.

> I don't see a way to meet this impossible requirement either, and I
> don't really have the bandwidth right now.

ok, thanks for the clarification :)

Patch

diff --git a/include/net.h b/include/net.h
index f6aeba2..bdc3da6 100644
--- a/include/net.h
+++ b/include/net.h
@@ -104,7 +104,9 @@  extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */
 extern int eth_get_dev_index (void);		/* get the device index */
 extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
 extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
+#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
 extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
+#endif
 
 /*
  * Get the hardware address for an ethernet interface .
@@ -118,6 +120,20 @@  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
 extern int eth_getenv_enetaddr_by_index(const char *base_name, int index,
 					uchar *enetaddr);
 
+#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
+/*
+ * Set the hardware address for an ethernet interface .
+ * Args:
+ *	base_name - base name for device (normally "eth")
+ *	index - device index number (0 for first)
+ *	enetaddr - returns 6 byte hardware address
+ * Returns:
+ *	0 on success, else 1.
+ */
+extern int eth_setenv_enetaddr_by_index(const char *base_name, int index,
+					const uchar *enetaddr);
+#endif
+
 #ifdef CONFIG_RANDOM_MACADDR
 /*
  * The u-boot policy does not allow hardcoded ethernet addresses. Under the
diff --git a/net/eth.c b/net/eth.c
index afce863..d66e22a 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -67,6 +67,21 @@  int eth_getenv_enetaddr_by_index(const char *base_name, int index,
 	return eth_getenv_enetaddr(enetvar, enetaddr);
 }
 
+#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
+int eth_setenv_enetaddr_by_index(const char *base_name, int index,
+		const uchar *enetaddr)
+{
+	char enetvar[32];
+
+	if (index)
+		sprintf(enetvar, "%s%daddr", base_name, index);
+	else
+		sprintf(enetvar, "%saddr", base_name);
+
+	return eth_setenv_enetaddr(enetvar, enetaddr);
+}
+#endif
+
 static int eth_mac_skip(int index)
 {
 	char enetvar[15];