[U-Boot] [RFC PATCH 0/3] fix netconsole for CONFIG_DM_ETH

[U-Boot] [RFC PATCH 0/3] fix netconsole for CONFIG_DM_ETH

[Bernhard Nortmann]

With the introduction of driver model and accompanying changes,
outdated code in netconsole leads to compilation errors when both
CONFIG_NETCONSOLE and CONFIG_DM_ETH are set.

This is a series of patches to fix these issues and get netconsole
working again. I have deliberately marked it "RFC" since I'm not
entirely sure of what I'm doing here... Especially the bootm.c
change (patch 3/3) probably needs review or improvement by someone
more knowledgable on U-Boot networking / driver model.

Nevertheless I have tested the resulting code on my Banana Pi
(sun7i / Allwinner A20) and had a functional netconsole again.
I've also backported eth_is_active() and netconsole.c to
v2015.04 to make sure they properly worked in case CONFIG_DM_ETH
is absent.

Regards, B. Nortmann


Bernhard Nortmann (3):
  expose eth_is_active() function to test network device state
  fix netconsole when CONFIG_DM_ETH is set
  avoid eth_unregister() call when function is unavailable

 common/bootm.c           |  2 ++
 drivers/net/netconsole.c | 14 +++++++++++---
 include/net.h            |  6 ++++++
 net/eth.c                | 18 +++++++++++++++++-
 4 files changed, 36 insertions(+), 4 deletions(-)

-- 
2.4.6

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Bernhard Nortmann]

The previous eth_device struct returned by eth_get_dev() allowed
code to directly query the state member field. However, with
CONFIG_DM_ETH this data gets encapsulated (i.e. private), and
eth_get_dev() returns a udevice struct 'abstraction' instead.

This breaks legacy code relying on the former behaviour - e.g.
netconsole.
(see http://lists.denx.de/pipermail/u-boot/2015-June/216528.html)

The patch introduces a method to retrieve the ethernet device
state in a 'clean' and uniform way, supporting both legacy code
and driver model. The new function eth_is_active() accepts a
device struct pointer and tests it for ETH_STATE_ACTIVE.

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
---

 include/net.h |  6 ++++++
 net/eth.c     | 18 +++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h
index d09bec9..c135ec4 100644
--- a/include/net.h
+++ b/include/net.h
@@ -149,7 +149,13 @@ struct udevice *eth_get_dev(void); /* get the current device */
  */
 struct udevice *eth_get_dev_by_name(const char *devname);
 unsigned char *eth_get_ethaddr(void); /* get the current device MAC */
+
 /* Used only when NetConsole is enabled */
+# ifdef CONFIG_DM_ETH
+int eth_is_active(struct udevice *dev); /* Test device for active state */
+# else
+int eth_is_active(struct eth_device *dev); /* Test device for active state */
+# endif
 int eth_init_state_only(void); /* Set active state */
 void eth_halt_state_only(void); /* Set passive state */
 #endif
diff --git a/net/eth.c b/net/eth.c
index d3ec8d6..d3ff7c6 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -386,6 +386,17 @@ void eth_halt(void)
 	priv->state = ETH_STATE_PASSIVE;
 }
 
+int eth_is_active(struct udevice *dev)
+{
+	struct eth_device_priv *priv;
+
+	if (!dev || !device_active(dev))
+		return 0;
+
+	priv = dev->uclass_priv;
+	return priv->state == ETH_STATE_ACTIVE;
+}
+
 int eth_send(void *packet, int length)
 {
 	struct udevice *current;
@@ -577,7 +588,7 @@ UCLASS_DRIVER(eth) = {
 	.per_device_auto_alloc_size = sizeof(struct eth_device_priv),
 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
 };
-#endif
+#endif /* #ifdef CONFIG_DM_ETH */
 
 #ifndef CONFIG_DM_ETH
 
@@ -915,6 +926,11 @@ void eth_halt(void)
 	eth_current->state = ETH_STATE_PASSIVE;
 }
 
+int eth_is_active(struct eth_device *dev)
+{
+	return dev && dev->state == ETH_STATE_ACTIVE;
+}
+
 int eth_send(void *packet, int length)
 {
 	if (!eth_current)
-- 
2.4.6

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Simon Glass]

Hi Bernard,

On 21 August 2015 at 16:30, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
> The previous eth_device struct returned by eth_get_dev() allowed
> code to directly query the state member field. However, with
> CONFIG_DM_ETH this data gets encapsulated (i.e. private), and
> eth_get_dev() returns a udevice struct 'abstraction' instead.
>
> This breaks legacy code relying on the former behaviour - e.g.
> netconsole.
> (see http://lists.denx.de/pipermail/u-boot/2015-June/216528.html)
>
> The patch introduces a method to retrieve the ethernet device
> state in a 'clean' and uniform way, supporting both legacy code
> and driver model. The new function eth_is_active() accepts a
> device struct pointer and tests it for ETH_STATE_ACTIVE.
>
> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
> ---
>
>  include/net.h |  6 ++++++
>  net/eth.c     | 18 +++++++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

A few points:

- You should tag your patch with the subsystem it targets - in your
case I think you should prefix the subject with 'net: '
- Do you have a patch to enable netconsole on an existing board? If
the code you add is never built then it could break at any moment

Regards,
Simon

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Bernhard Nortmann]

Hi Simon!

Am 23.08.2015 23:21, schrieb Simon Glass:
> A few points:
>
> - You should tag your patch with the subsystem it targets - in your
> case I think you should prefix the subject with 'net: '
Okay. I'll keep that in mind for future reiterations of this patch set.
Btw: Thanks for looking into my stuff.

> - Do you have a patch to enable netconsole on an existing board? If
> the code you add is never built then it could break at any moment
netconsole seems to receive little love actually :D (It's been broken / 
would
cause compilation errors with CONFIG_DM_ETH for quite some time now,
since commit b6006baf9c2553543e3384983d23d95efbf24fa6 in April.)
So I'm not sure I get your question. As mentioned in the cover letter I've
been testing this code with and without CONFIG_DM_ETH for my Banana Pi,
i.e. sunxi GMAC (by simply adding #define CONFIG_NETCONSOLE).

Regards, B. Nortmann

> Regards,
> Simon

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Simon Glass]

+Hans

Hi Bernhard,

On 24 August 2015 at 04:20, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
> Hi Simon!
>
> Am 23.08.2015 23:21, schrieb Simon Glass:
>>
>> A few points:
>>
>> - You should tag your patch with the subsystem it targets - in your
>> case I think you should prefix the subject with 'net: '
>
> Okay. I'll keep that in mind for future reiterations of this patch set.
> Btw: Thanks for looking into my stuff.
>
>> - Do you have a patch to enable netconsole on an existing board? If
>> the code you add is never built then it could break at any moment
>
> netconsole seems to receive little love actually :D (It's been broken /
> would
> cause compilation errors with CONFIG_DM_ETH for quite some time now,
> since commit b6006baf9c2553543e3384983d23d95efbf24fa6 in April.)
> So I'm not sure I get your question. As mentioned in the cover letter I've
> been testing this code with and without CONFIG_DM_ETH for my Banana Pi,
> i.e. sunxi GMAC (by simply adding #define CONFIG_NETCONSOLE).

In that case how about adding that config to that board? Does it cause
problems for other people?

Regards,
Simon

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Joe Hershberger]

Hi Simon,

On Mon, Aug 24, 2015 at 11:59 AM, Simon Glass <sjg@chromium.org> wrote:
> +Hans
>
> Hi Bernhard,
>
> On 24 August 2015 at 04:20, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
>> Hi Simon!
>>
>> Am 23.08.2015 23:21, schrieb Simon Glass:
>>>
>>> A few points:
>>>
>>> - You should tag your patch with the subsystem it targets - in your
>>> case I think you should prefix the subject with 'net: '
>>
>> Okay. I'll keep that in mind for future reiterations of this patch set.
>> Btw: Thanks for looking into my stuff.
>>
>>> - Do you have a patch to enable netconsole on an existing board? If
>>> the code you add is never built then it could break at any moment
>>
>> netconsole seems to receive little love actually :D (It's been broken /
>> would
>> cause compilation errors with CONFIG_DM_ETH for quite some time now,
>> since commit b6006baf9c2553543e3384983d23d95efbf24fa6 in April.)
>> So I'm not sure I get your question. As mentioned in the cover letter I've
>> been testing this code with and without CONFIG_DM_ETH for my Banana Pi,
>> i.e. sunxi GMAC (by simply adding #define CONFIG_NETCONSOLE).
>
> In that case how about adding that config to that board? Does it cause
> problems for other people?

I'll pile on and agree that it would be great to have at least one
board with this enabled, and even better to have one that supports
DM_ETH and one that does not.

-Joe

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Bernhard Nortmann]

Am 24.08.2015 um 19:02 schrieb Joe Hershberger:
> Hi Simon,
>
> On Mon, Aug 24, 2015 at 11:59 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Bernhard,
>>
>> [...]
>> i.e. sunxi GMAC (by simply adding #define CONFIG_NETCONSOLE).
>> In that case how about adding that config to that board? Does it cause
>> problems for other people?
> I'll pile on and agree that it would be great to have at least one
> board with this enabled, and even better to have one that supports
> DM_ETH and one that does not.
>
> -Joe

grep "#define CONFIG_NETCONSOLE" include/configs/*
lists a considerable number of boards where NETCONSOLE seems to be active
by default. I guess none of these has moved to DM_ETH yet, or I'd have
expected reports of "broken" builds.

If you're all for it, I can of course enable NETCONSOLE for the Banana
Pi/Pro. (It may even be done across-the-board for all SUNXI_[EG]MAC
configs?)

Personally, I'm a bit reluctant to "enforce" this setting, because until
now my understanding was that NETCONSOLE is supposed to be mostly
optional, i.e. at user's choice - especially for boards where other
means of input/output are readily available (serial console, vga, usb
keyboard).

I don't expect this to create problems, it just adds code that probably
won't be used most of the time (as long as "nc" doesn't get used for
stdin/stdout).

I guess the proper way to do it would be to introduce Kconfig support;
will this do?

diff --git a/net/Kconfig b/net/Kconfig
index 915371d..87c1729 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -16,4 +16,10 @@ config NET_RANDOM_ETHADDR
           A new MAC address will be generated on every boot and it will
           not be added to the environment.

+config NETCONSOLE
+       bool "Netconsole support"
+       help
+         Support the 'nc' input/output device for networked console.
+         See README.NetConsole for details.
+
  endif   # if NET


In case this gets a "go", I'd prepare a v2 patch set that includes enabling
CONFIG_NETCONSOLE via Bananapi_defconfig / Bananapro_defconfig.

Regards, B. Nortmann

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Joe Hershberger]

Hi Bernhard,

On Tue, Aug 25, 2015 at 4:53 AM, Bernhard Nortmann
<bernhard.nortmann@web.de> wrote:
> Am 24.08.2015 um 19:02 schrieb Joe Hershberger:
>>
>> Hi Simon,
>>
>> On Mon, Aug 24, 2015 at 11:59 AM, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi Bernhard,
>>>
>>> [...]
>>> i.e. sunxi GMAC (by simply adding #define CONFIG_NETCONSOLE).
>>> In that case how about adding that config to that board? Does it cause
>>> problems for other people?
>>
>> I'll pile on and agree that it would be great to have at least one
>> board with this enabled, and even better to have one that supports
>> DM_ETH and one that does not.
>>
>> -Joe
>
>
> grep "#define CONFIG_NETCONSOLE" include/configs/*
> lists a considerable number of boards where NETCONSOLE seems to be active
> by default. I guess none of these has moved to DM_ETH yet, or I'd have
> expected reports of "broken" builds.
>
> If you're all for it, I can of course enable NETCONSOLE for the Banana
> Pi/Pro. (It may even be done across-the-board for all SUNXI_[EG]MAC
> configs?)

Seems reasonable to me.

> Personally, I'm a bit reluctant to "enforce" this setting, because until
> now my understanding was that NETCONSOLE is supposed to be mostly
> optional, i.e. at user's choice - especially for boards where other
> means of input/output are readily available (serial console, vga, usb
> keyboard).

These are not resource constrained boards, right?

> I don't expect this to create problems, it just adds code that probably
> won't be used most of the time (as long as "nc" doesn't get used for
> stdin/stdout).

It's good to have a build target and also a test or so. We should
maybe enable it in sandbox.

> I guess the proper way to do it would be to introduce Kconfig support;
> will this do?
>
> diff --git a/net/Kconfig b/net/Kconfig
> index 915371d..87c1729 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -16,4 +16,10 @@ config NET_RANDOM_ETHADDR
>           A new MAC address will be generated on every boot and it will
>           not be added to the environment.
>
> +config NETCONSOLE
> +       bool "Netconsole support"

I'm pretty sure CamelCase is used in NetConsole.

> +       help
> +         Support the 'nc' input/output device for networked console.
> +         See README.NetConsole for details.
> +
>  endif   # if NET
>
>
> In case this gets a "go", I'd prepare a v2 patch set that includes enabling
> CONFIG_NETCONSOLE via Bananapi_defconfig / Bananapro_defconfig.

I guess that will work (not running moveconfig.pl) with
CONFIG_NETCONSOLE since so far most boards that don't define
CONFIG_DM_ETH don't define CONFIG_NET or CONFIG_NETDEVICES so that
CONFIG_NETCONSOLE option wouldn't be available. We'll have to change
that for next release.

-Joe

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Bernhard Nortmann]

Am 25.08.2015 um 17:55 schrieb Joe Hershberger:
 > Hi Bernhard,
 >
 > [...]
 > It's good to have a build target and also a test or so. We should
 > maybe enable it in sandbox.

I'm not familiar at all with the U-Boot "sandbox" architecture, so I'd 
prefer
to leave that to someone else.

 > I'm pretty sure CamelCase is used in NetConsole.

Thanks, I'll fix that.

 > I guess that will work (not running moveconfig.pl) with
 > CONFIG_NETCONSOLE since so far most boards that don't define
 > CONFIG_DM_ETH don't define CONFIG_NET or CONFIG_NETDEVICES so that
 > CONFIG_NETCONSOLE option wouldn't be available. We'll have to change
 > that for next release.
 >
 > -Joe

Yes, that Kconfig vs. "legacy" options battle is a bit of a problem. If it
keeps things simpler for now, I could add "NETCONSOLE" to the respective
CONFIG_SYS_EXTRA_OPTIONS as an alternative approach, and leave that Kconfig
migration for the future. Which do you prefer?

Regards, B. Nortmann

[U-Boot] [RFC PATCH 1/3] expose eth_is_active() function to test network device state

[Joe Hershberger]

Hi Bernhard,

On Tue, Aug 25, 2015 at 1:01 PM, Bernhard Nortmann
<bernhard.nortmann@web.de> wrote:
> Am 25.08.2015 um 17:55 schrieb Joe Hershberger:
>> Hi Bernhard,
>>
>> [...]
>> It's good to have a build target and also a test or so. We should
>> maybe enable it in sandbox.
>
> I'm not familiar at all with the U-Boot "sandbox" architecture, so I'd
> prefer
> to leave that to someone else.
>
>> I'm pretty sure CamelCase is used in NetConsole.
>
> Thanks, I'll fix that.
>
>> I guess that will work (not running moveconfig.pl) with
>> CONFIG_NETCONSOLE since so far most boards that don't define
>> CONFIG_DM_ETH don't define CONFIG_NET or CONFIG_NETDEVICES so that
>> CONFIG_NETCONSOLE option wouldn't be available. We'll have to change
>> that for next release.
>>
>> -Joe
>
> Yes, that Kconfig vs. "legacy" options battle is a bit of a problem. If it
> keeps things simpler for now, I could add "NETCONSOLE" to the respective
> CONFIG_SYS_EXTRA_OPTIONS as an alternative approach, and leave that Kconfig
> migration for the future. Which do you prefer?

I think it will be better to take the approach you already are (add to
Kconfig). Just make sure you use savedefconfig when you add the option
to the bananapi defconfig.

Thanks,
-Joe

[U-Boot] [RFC PATCH 2/3] fix netconsole when CONFIG_DM_ETH is set

[Bernhard Nortmann]

This patches uses the eth_is_active() function to work around
issues that prevented compilation with the newer driver model.

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
---

 drivers/net/netconsole.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 31042a6..bf972dc 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -170,7 +170,11 @@ int nc_input_packet(uchar *pkt, struct in_addr src_ip, unsigned dest_port,
 
 static void nc_send_packet(const char *buf, int len)
 {
+#ifdef CONFIG_DM_ETH
+	struct udevice *eth;
+#else
 	struct eth_device *eth;
+#endif
 	int inited = 0;
 	uchar *pkt;
 	uchar *ether;
@@ -183,7 +187,7 @@ static void nc_send_packet(const char *buf, int len)
 		return;
 
 	if (!memcmp(nc_ether, net_null_ethaddr, 6)) {
-		if (eth->state == ETH_STATE_ACTIVE)
+		if (eth_is_active(eth))
 			return;	/* inside net loop */
 		output_packet = buf;
 		output_packet_len = len;
@@ -194,7 +198,7 @@ static void nc_send_packet(const char *buf, int len)
 		return;
 	}
 
-	if (eth->state != ETH_STATE_ACTIVE) {
+	if (!eth_is_active(eth)) {
 		if (eth_is_on_demand_init()) {
 			if (eth_init() < 0)
 				return;
@@ -292,7 +296,11 @@ static int nc_stdio_getc(struct stdio_dev *dev)
 
 static int nc_stdio_tstc(struct stdio_dev *dev)
 {
+#ifdef CONFIG_DM_ETH
+	struct udevice *eth;
+#else
 	struct eth_device *eth;
+#endif
 
 	if (input_recursion)
 		return 0;
@@ -301,7 +309,7 @@ static int nc_stdio_tstc(struct stdio_dev *dev)
 		return 1;
 
 	eth = eth_get_dev();
-	if (eth && eth->state == ETH_STATE_ACTIVE)
+	if (eth_is_active(eth))
 		return 0;	/* inside net loop */
 
 	input_recursion = 1;
-- 
2.4.6

[U-Boot] [RFC PATCH 3/3] avoid eth_unregister() call when function is unavailable

[Bernhard Nortmann]

CONFIG_NETCONSOLE causes common/bootm.c to call eth_unregister()
for network device shutdown. However, with CONFIG_DM_ETH this
function is no longer defined.

This is a workaround to avoid the call in that case, and solely
rely on eth_halt(). In case this is insufficient, a proper way
to unregister / remove network devices needs to be implemented.

Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>

---

 common/bootm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/bootm.c b/common/bootm.c
index 667c934..c0d0d09 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -474,7 +474,9 @@ ulong bootm_disable_interrupts(void)
 #ifdef CONFIG_NETCONSOLE
 	/* Stop the ethernet stack if NetConsole could have left it up */
 	eth_halt();
+# ifndef CONFIG_DM_ETH
 	eth_unregister(eth_get_dev());
+# endif
 #endif
 
 #if defined(CONFIG_CMD_USB)
-- 
2.4.6

[U-Boot] [RFC PATCH 3/3] avoid eth_unregister() call when function is unavailable

[Simon Glass]

On 21 August 2015 at 16:30, Bernhard Nortmann <bernhard.nortmann@web.de> wrote:
> CONFIG_NETCONSOLE causes common/bootm.c to call eth_unregister()
> for network device shutdown. However, with CONFIG_DM_ETH this
> function is no longer defined.
>
> This is a workaround to avoid the call in that case, and solely
> rely on eth_halt(). In case this is insufficient, a proper way
> to unregister / remove network devices needs to be implemented.
>
> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de>
>
> ---
>
>  common/bootm.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

(nit: should have 'net:' prefix)