[U-Boot] [PATCH] tsec: fix the return value for tsec_eth_init()

[U-Boot] [PATCH] tsec: fix the return value for tsec_eth_init()

[Timur Tabi]

The Ethernet initialization functions are supposed to return the number of
devices initialized, so fix tsec_eth_init() so that it returns the number of
TSECs initialized instead of just zero.  This is safe because the return value
is currently ignored by all callers, but now they don't have to ignore it.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/net/tsec.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
index 3e4c3bd..abfc80a 100644
--- a/drivers/net/tsec.c
+++ b/drivers/net/tsec.c
@@ -97,12 +97,18 @@ static struct tsec_info_struct tsec_info[] = {
 
 int tsec_eth_init(bd_t *bis, struct tsec_info_struct *tsecs, int num)
 {
-	int i;
-
-	for (i = 0; i < num; i++)
-		tsec_initialize(bis, &tsecs[i]);
+	unsigned int i;
+	unsigned int count = 0;
+	int ret;
+
+	for (i = 0; i < num; i++) {
+		ret = tsec_initialize(bis, &tsecs[i]);
+		if (ret < 0)
+			return ret;
+		count += ret;
+	}
 
-	return 0;
+	return count;
 }
 
 int tsec_standard_init(bd_t *bis)
-- 
1.7.0.1

[U-Boot] [PATCH] tsec: fix the return value for tsec_eth_init()

[Andy Fleming]

On Jun 4, 2010, at 3:50 PM, Timur Tabi wrote:

> The Ethernet initialization functions are supposed to return the number of
> devices initialized, so fix tsec_eth_init() so that it returns the number of
> TSECs initialized instead of just zero.  This is safe because the return value
> is currently ignored by all callers, but now they don't have to ignore it.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> drivers/net/tsec.c |   18 ++++++++++++------
> 1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index 3e4c3bd..abfc80a 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -97,12 +97,18 @@ static struct tsec_info_struct tsec_info[] = {
> 
> int tsec_eth_init(bd_t *bis, struct tsec_info_struct *tsecs, int num)
> {
> -	int i;
> -
> -	for (i = 0; i < num; i++)
> -		tsec_initialize(bis, &tsecs[i]);
> +	unsigned int i;
> +	unsigned int count = 0;
> +	int ret;
> +
> +	for (i = 0; i < num; i++) {
> +		ret = tsec_initialize(bis, &tsecs[i]);
> +		if (ret < 0)
> +			return ret;
> +		count += ret;


The old way continued even if one of the tsecs failed to initialize.  Let's preserve the original behavior in that sense:

for (i = 0; i < num; i++) {
	ret = tsec_initialize(bis, &tsecs[i]);
	if (ret >= 0)
		count++;
}

Andy

[U-Boot] [PATCH] tsec: fix the return value for tsec_eth_init()

[Timur Tabi]

Andy Fleming wrote:

> The old way continued even if one of the tsecs failed to initialize.  Let's preserve the original behavior in that sense:
> 
> for (i = 0; i < num; i++) {
> 	ret = tsec_initialize(bis, &tsecs[i]);
> 	if (ret >= 0)
> 		count++;
> }

This code has multiple levels to it.  board_eth_init() calls
tsec_eth_init(), pci_eth_init(), and maybe some other functions.
tsec_eth_init() calls tsec_initialize().  tsec_initialize() calls
init_phy().  Are we always going to ignore an error return code?  Why don't
we just eliminate the possibility of returning a negative number at all levels?

[U-Boot] [PATCH] tsec: fix the return value for tsec_eth_init()

[Andy Fleming]

On Jun 4, 2010, at 4:01 PM, Timur Tabi wrote:

> Andy Fleming wrote:
> 
>> The old way continued even if one of the tsecs failed to initialize.  Let's preserve the original behavior in that sense:
>> 
>> for (i = 0; i < num; i++) {
>> 	ret = tsec_initialize(bis, &tsecs[i]);
>> 	if (ret >= 0)
>> 		count++;
>> }
> 
> This code has multiple levels to it.  board_eth_init() calls
> tsec_eth_init(), pci_eth_init(), and maybe some other functions.
> tsec_eth_init() calls tsec_initialize().  tsec_initialize() calls
> init_phy().  Are we always going to ignore an error return code?  Why don't
> we just eliminate the possibility of returning a negative number at all levels?
> 

You just noted that tsec_eth_init should return the number of tsecs initialized successfully.  Therefore, the callers can check that number, and respond accordingly.  tsec_initialize() can report the error.  If we want more elaborate error handling, we can devise something.  But with the new scheme, an error in one tsec (like a riser card not being plugged in) will cause all of the tsecs to not be initialized, which seems silly.

Andy

[U-Boot] [PATCH] tsec: fix the return value for tsec_eth_init()

[Timur Tabi]

Andy Fleming wrote:
> You just noted that tsec_eth_init should return the number of tsecs
> initialized successfully.  Therefore, the callers can check that number,
> and respond accordingly.  tsec_initialize() can report the error.  If we
> want more elaborate error handling, we can devise something.  But with
> the new scheme, an error in one tsec (like a riser card not being plugged
> in) will cause all of the tsecs to not be initialized, which seems
> silly.

Ok, so the rule is: a function which initializes one interface can return an
error <0, but a function which initializes multiple interfaces should not.
V2 coming right up.

[U-Boot] [PATCH] tsec: fix the return value for tsec_eth_init()

[Ben Warren]

On 6/4/2010 2:42 PM, Timur Tabi wrote:
> Andy Fleming wrote:
>    
>> You just noted that tsec_eth_init should return the number of tsecs
>> initialized successfully.  Therefore, the callers can check that number,
>> and respond accordingly.  tsec_initialize() can report the error.  If we
>> want more elaborate error handling, we can devise something.  But with
>> the new scheme, an error in one tsec (like a riser card not being plugged
>> in) will cause all of the tsecs to not be initialized, which seems
>> silly.
>>      
> Ok, so the rule is: a function which initializes one interface can return an
> error<0, but a function which initializes multiple interfaces should not.
> V2 coming right up.
>    
Sorry for the confusion.  The way we handle errors isn't through return 
code but rather through printf()...  In future, the return code may be 
used to keep track of the number of properly initialized interfaces.  
Looks like v2 should work.

regards,
Ben