[U-Boot] [PATCH 1/3] net/eth: set status to active before calling init
If we set the status after successful init call then we get in trouble if stdout (or setderr) is set to netconsole. If we are going to use one of those (lets say printf) during ->init() the following happens: - network is of (state passive) - we switch on netconsole - nc_getc() gets called - in NetLoop() we switch on ethernet via eth_init() - we end up in tsec_init() (inc case we use the tsec driver). Here we call a printf() - That printf() ends up in nc_puts() because netconsole is our default output. - The state is not active yet, so we call eth_init() once again. - and we are again in tsec_init(). Another printf() is waiting. However, due to the recursion check nc_puts() returns early before doing anything. - we return from each function. Sine nc_puts() thinks that it was in charge of enabling the ethernet, it disables it before leaving. - We return now to the top-most eth_init() function. Since everything went fine, it sets the status to active. In reality the network is switched off. - nc_getc() gets called over and over to receive new packets. Sadly the nic is disabled and new network packets won't be noticed.
This patch sets the network status early so nc_puts() does not get confused and disables the network interface in case of a printf() on its way.
Signed-off-by: Sebastian Andrzej Siewior bigeasy@linutronix.de --- net/eth.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/eth.c b/net/eth.c index 4280d6d..bca405a 100644 --- a/net/eth.c +++ b/net/eth.c @@ -380,14 +380,17 @@ int eth_init(bd_t *bis)
old_current = eth_current; do { + int old_state; + debug("Trying %s\n", eth_current->name);
- if (eth_current->init(eth_current,bis) >= 0) { - eth_current->state = ETH_STATE_ACTIVE; - + old_state = eth_current->state; + eth_current->state = ETH_STATE_ACTIVE; + if (eth_current->init(eth_current,bis) >= 0) return 0; - } + debug("FAIL\n"); + eth_current->state = old_state;
eth_try_another(0); } while (old_current != eth_current);
In case we use netconsole for stdout and something goes wrong here and we run into one of this printf() then there is no point of sending this over network again since it is likely that will fail again.
Signed-off-by: Sebastian Andrzej Siewior bigeasy@linutronix.de --- drivers/net/tsec.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 160bc05..78badfa 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -199,7 +199,7 @@ static void adjust_link(struct tsec_private *priv, struct phy_device *phydev) u32 ecntrl, maccfg2;
if (!phydev->link) { - printf("%s: No link.\n", phydev->dev->name); + serial_printf("%s: No link.\n", phydev->dev->name); return; }
@@ -228,14 +228,14 @@ static void adjust_link(struct tsec_private *priv, struct phy_device *phydev) ecntrl |= ECNTRL_R100; break; default: - printf("%s: Speed was bad\n", phydev->dev->name); + serial_printf("%s: Speed was bad\n", phydev->dev->name); break; }
out_be32(®s->ecntrl, ecntrl); out_be32(®s->maccfg2, maccfg2);
- printf("Speed: %d, %s duplex%s\n", phydev->speed, + serial_printf("Speed: %d, %s duplex%s\n", phydev->speed, (phydev->duplex) ? "full" : "half", (phydev->port == PORT_FIBRE) ? ", fiber mode" : ""); } @@ -287,7 +287,7 @@ void redundant_init(struct eth_device *dev) /* Wait for buffer to be received */ for (t = 0; rtx.rxbd[rxIdx].status & RXBD_EMPTY; t++) { if (t >= 10 * TOUT_LOOP) { - printf("%s: tsec: rx error\n", dev->name); + serial_printf("%s: tsec: rx error\n", dev->name); break; } } @@ -305,7 +305,7 @@ void redundant_init(struct eth_device *dev) out_be32(®s->rstat, RSTAT_CLEAR_RHALT); } if (fail) { - printf("loopback recv packet error!\n"); + serial_printf("loopback recv packet error!\n"); clrbits_be32(®s->maccfg1, MACCFG1_RX_EN); udelay(1000); setbits_be32(®s->maccfg1, MACCFG1_RX_EN); @@ -428,7 +428,7 @@ static int tsec_recv(struct eth_device *dev) if (!(rtx.rxbd[rxIdx].status & RXBD_STATS)) { NetReceive(NetRxPackets[rxIdx], length - 4); } else { - printf("Got error %x\n", + serial_printf("Got error %x\n", (rtx.rxbd[rxIdx].status & RXBD_STATS)); }
Since we start/stop the network after each character we see that line printed a lot.
Signed-off-by: Sebastian Andrzej Siewior bigeasy@linutronix.de --- drivers/net/tsec.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 78badfa..9af3c7e 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -235,9 +235,11 @@ static void adjust_link(struct tsec_private *priv, struct phy_device *phydev) out_be32(®s->ecntrl, ecntrl); out_be32(®s->maccfg2, maccfg2);
- serial_printf("Speed: %d, %s duplex%s\n", phydev->speed, - (phydev->duplex) ? "full" : "half", - (phydev->port == PORT_FIBRE) ? ", fiber mode" : ""); + if (strcmp(getenv("stdout"), "nc")) + serial_printf("Speed: %d, %s duplex%s\n", phydev->speed, + (phydev->duplex) ? "full" : "half", + (phydev->port == PORT_FIBRE) ? ", fiber mode" + : ""); }
#ifdef CONFIG_SYS_FSL_ERRATUM_NMG_ETSEC129
On Friday 23 March 2012 16:11:19 Sebastian Andrzej Siewior wrote:
--- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c
- if (strcmp(getenv("stdout"), "nc"))
i really don't like special casing devices like this
- serial_printf("Speed: %d, %s duplex%s\n", phydev->speed,
(phydev->duplex) ? "full" : "half",(phydev->port == PORT_FIBRE) ? ", fiber mode" : "");
serial_printf("Speed: %d, %s duplex%s\n", phydev->speed,(phydev->duplex) ? "full" : "half",(phydev->port == PORT_FIBRE) ? ", fiber mode": "");
why not just delete this line completely ? -mike
On 04/03/2012 10:42 PM, Mike Frysinger wrote:
On Friday 23 March 2012 16:11:19 Sebastian Andrzej Siewior wrote:
--- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c
- if (strcmp(getenv("stdout"), "nc"))
i really don't like special casing devices like this
- serial_printf("Speed: %d, %s duplex%s\n", phydev->speed,
(phydev->duplex) ? "full" : "half",(phydev->port == PORT_FIBRE) ? ", fiber mode" : "");
serial_printf("Speed: %d, %s duplex%s\n", phydev->speed,(phydev->duplex) ? "full" : "half",(phydev->port == PORT_FIBRE) ? ", fiber mode": "");why not just delete this line completely ?
*I* don't mind but others might complain about missing important information. So in my re-do of the series I remove it instead and see what happens.
-mike
Sebastian
Hi Sebastian,
On Tue, Apr 3, 2012 at 3:54 PM, Sebastian Andrzej Siewior bigeasy@linutronix.de wrote:
On 04/03/2012 10:42 PM, Mike Frysinger wrote:
On Friday 23 March 2012 16:11:19 Sebastian Andrzej Siewior wrote:
--- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c
- if (strcmp(getenv("stdout"), "nc"))
i really don't like special casing devices like this
In this case I think it is better to check if stdout is nc, but not to explicitly write to serial_printf(). The nc device is the reason to avoid printing this since it uses the network. The serial_printf seems like the special case to avoid. Consider the case of using a SPI UART. There is no reason these traces should not go to it via the normal printf routing.
- serial_printf("Speed: %d, %s duplex%s\n", phydev->speed,
- (phydev->duplex) ? "full" : "half",
- (phydev->port == PORT_FIBRE) ? ", fiber mode" :
"");
- serial_printf("Speed: %d, %s duplex%s\n", phydev->speed,
- (phydev->duplex) ? "full" : "half",
- (phydev->port == PORT_FIBRE) ? ", fiber
mode"
- : "");
why not just delete this line completely ?
*I* don't mind but others might complain about missing important information. So in my re-do of the series I remove it instead and see what happens.
It is pretty typical to print the result of the auto-negotiation.
Are you working on an updated series?
Thanks, -Joe
On Wednesday 04 April 2012 11:27:44 Joe Hershberger wrote:
On Tue, Apr 3, 2012 at 3:54 PM, Sebastian Andrzej Siewior wrote:
On 04/03/2012 10:42 PM, Mike Frysinger wrote:
On Friday 23 March 2012 16:11:19 Sebastian Andrzej Siewior wrote:
--- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c
if (strcmp(getenv("stdout"), "nc"))i really don't like special casing devices like this
In this case I think it is better to check if stdout is nc, but not to explicitly write to serial_printf(). The nc device is the reason to avoid printing this since it uses the network. The serial_printf seems like the special case to avoid. Consider the case of using a SPI UART. There is no reason these traces should not go to it via the normal printf routing.
my point is that this doesn't scale ... not even close. either drop the printf calls in the core net case (when it'd be a problem with the netconsole), or figure out a solution that does actually scale. maybe a net_printf(). -mike
On Sun, Apr 8, 2012 at 3:26 AM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday 04 April 2012 11:27:44 Joe Hershberger wrote:
On Tue, Apr 3, 2012 at 3:54 PM, Sebastian Andrzej Siewior wrote:
On 04/03/2012 10:42 PM, Mike Frysinger wrote:
On Friday 23 March 2012 16:11:19 Sebastian Andrzej Siewior wrote:
--- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c
- if (strcmp(getenv("stdout"), "nc"))
i really don't like special casing devices like this
In this case I think it is better to check if stdout is nc, but not to explicitly write to serial_printf(). The nc device is the reason to avoid printing this since it uses the network. The serial_printf seems like the special case to avoid. Consider the case of using a SPI UART. There is no reason these traces should not go to it via the normal printf routing.
my point is that this doesn't scale ... not even close. either drop the printf calls in the core net case (when it'd be a problem with the netconsole), or figure out a solution that does actually scale. maybe a net_printf().
I agree. A net_printf sounds like a good solution.
-Joe
On Friday 23 March 2012 16:11:17 Sebastian Andrzej Siewior wrote:
If we set the status after successful init call then we get in trouble if stdout (or setderr) is set to netconsole. If we are going to use one of those (lets say printf) during ->init() the following happens:
- network is of (state passive)
- we switch on netconsole
- nc_getc() gets called
- in NetLoop() we switch on ethernet via eth_init()
- we end up in tsec_init() (inc case we use the tsec driver). Here we call a printf()
considering your followup patches convert printf() to serial_printf(), is this patch still needed ?
--- a/net/eth.c +++ b/net/eth.c @@ -380,14 +380,17 @@ int eth_init(bd_t *bis)
old_current = eth_current; do {
int old_state;- debug("Trying %s\n", eth_current->name);
if (eth_current->init(eth_current,bis) >= 0) {eth_current->state = ETH_STATE_ACTIVE;
old_state = eth_current->state;eth_current->state = ETH_STATE_ACTIVE;if (eth_current->init(eth_current,bis) >= 0) return 0;
}
debug("FAIL\n");
eth_current->state = old_state;eth_try_another(0); } while (old_current != eth_current);
this needs a comment in the code explaining why you're setting things active early and then turning it off -mike
On 04/03/2012 10:41 PM, Mike Frysinger wrote:
On Friday 23 March 2012 16:11:17 Sebastian Andrzej Siewior wrote:
If we set the status after successful init call then we get in trouble if stdout (or setderr) is set to netconsole. If we are going to use one of those (lets say printf) during ->init() the following happens:
- network is of (state passive)
- we switch on netconsole
- nc_getc() gets called
- in NetLoop() we switch on ethernet via eth_init()
- we end up in tsec_init() (inc case we use the tsec driver). Here we call a printf()
considering your followup patches convert printf() to serial_printf(), is this patch still needed ?
Not for the tsec driver but it is possible that other driver do have the same problem. I haven't also checked all the phy devices which might do some prints. Therefore I would prefer to have this in.
--- a/net/eth.c +++ b/net/eth.c @@ -380,14 +380,17 @@ int eth_init(bd_t *bis)
old_current = eth_current; do {
int old_state;- debug("Trying %s\n", eth_current->name);
if (eth_current->init(eth_current,bis)>= 0) {eth_current->state = ETH_STATE_ACTIVE;
old_state = eth_current->state;eth_current->state = ETH_STATE_ACTIVE;if (eth_current->init(eth_current,bis)>= 0) return 0;
}
debug("FAIL\n");
eth_current->state = old_state;eth_try_another(0); } while (old_current != eth_current);
this needs a comment in the code explaining why you're setting things active early and then turning it off
Okay.
-mike
Sebastian
participants (3)
-
Joe Hershberger -
Mike Frysinger -
Sebastian Andrzej Siewior