[U-Boot] [RFC PATCH 0/3] fix netconsole for CONFIG_DM_ETH
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(-)
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)
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
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
+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
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
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
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.
helpSupport 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
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
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
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;
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)
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)
participants (3)
-
Bernhard Nortmann -
Joe Hershberger -
Simon Glass