[U-Boot] [PATCH 1/3] net/eth: set status to active before calling init

[U-Boot] [PATCH 1/3] net/eth: set status to active before calling init

[Sebastian Andrzej Siewior]

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);
-- 
1.7.9.1

[U-Boot] [PATCH 2/3] net/tsec: convert the printf() to serial_printf()

[Sebastian Andrzej Siewior]

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(&regs->ecntrl, ecntrl);
 	out_be32(&regs->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(&regs->rstat, RSTAT_CLEAR_RHALT);
 		}
 		if (fail) {
-			printf("loopback recv packet error!\n");
+			serial_printf("loopback recv packet error!\n");
 			clrbits_be32(&regs->maccfg1, MACCFG1_RX_EN);
 			udelay(1000);
 			setbits_be32(&regs->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));
 		}
 
-- 
1.7.9.1

[U-Boot] [PATCH 3/3] net/tsec: Don't tell the link status if used with netconsole

[Sebastian Andrzej Siewior]

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(&regs->ecntrl, ecntrl);
 	out_be32(&regs->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
-- 
1.7.9.1

[U-Boot] [PATCH 1/3] net/eth: set status to active before calling init

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120403/3550492a/attachment.pgp>

[U-Boot] [PATCH 3/3] net/tsec: Don't tell the link status if used with netconsole

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120403/6e0ba6f0/attachment.pgp>

[U-Boot] [PATCH 1/3] net/eth: set status to active before calling init

[Sebastian Andrzej Siewior]

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

[U-Boot] [PATCH 3/3] net/tsec: Don't tell the link status if used with netconsole

[Sebastian Andrzej Siewior]

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

[U-Boot] [PATCH 3/3] net/tsec: Don't tell the link status if used with netconsole

[Joe Hershberger]

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

[U-Boot] [PATCH 3/3] net/tsec: Don't tell the link status if used with netconsole

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120408/d8e26af1/attachment.pgp>

[U-Boot] [PATCH 3/3] net/tsec: Don't tell the link status if used with netconsole

[Joe Hershberger]

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