tsec: fix the return value for tsec_eth_init() and tsec_standard_init()

The Ethernet initialization functions are supposed to return the number of devices initialized, so fix tsec_eth_init() and tsec_standard_init() so that they 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.

In general, if an function initializes only one device, then it should return a negative number if there's an error. If it initializes more than one device, then it should never return a negative number. This is why these functions now return an unsigned integer.

---
 drivers/net/tsec.c |   22 ++++++++++++++++------
 include/tsec.h     |    4 ++--
 2 files changed, 18 insertions(+), 8 deletions(-)

index 3e4c3bd..6ca9735 100644
@@ -95,17 +95,27 @@ static struct tsec_info_struct tsec_info[] = {
 #endif
 };
 
-int tsec_eth_init(bd_t *bis, struct tsec_info_struct *tsecs, int num)
+/*
+ * Initialize all the TSEC devices
+ *
+ * Returns the number of TSEC devices that were initialized
+ */
+unsigned 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)
+			count += ret;
+	}
 
-	return 0;
+	return count;
 }
 
-int tsec_standard_init(bd_t *bis)
+unsigned int tsec_standard_init(bd_t *bis)
 {
 	return tsec_eth_init(bis, tsec_info, ARRAY_SIZE(tsec_info));
 }

index 1e90365..9da2740 100644
@@ -657,7 +657,7 @@ struct tsec_info_struct {
 	u32 flags;
 };
 
-int tsec_standard_init(bd_t *bis);
-int tsec_eth_init(bd_t *bis, struct tsec_info_struct *tsec_info, int num);
+unsigned int tsec_standard_init(bd_t *bis);
+unsigned int tsec_eth_init(bd_t *bis, struct tsec_info_struct *tsecs, int num);
 
 #endif /* __TSEC_H */

1.7.0.1

i dont think this is a good idea. either the init funcs should all be converted to unsigned int, or they should stay int. doing it piecemeal leads to confusion with zero upside.

your fixes no way require these to be unsigned int funcs.
-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/20100607/6f33d0ff/attachment.pgp

I don't want to change all of the functions. For most devices, there's no way I can test them.

Just because pci_eth_int() is incorrect, that doesn't mean that I can't make tsec_eth_init() correct.

What's the point of making the return value a signed integer if it can never be a negative number? The reason I changed the type to unsigned int is to make it very clear that it will never return an error code.

i dont think this is a big deal. plenty of tree wide changes are made and people try their best to break things without actually testing them.

except all the documentation and existing code says "use int", so "correct" is in the eye of the beholder here. i think consistency is more important at this point than an otherwise meaningless value.

the documentation currently states that a negative value is permissible and thus "int" is correct. as for the code that actually reads the result, that is by & large common code, so logic along those lines isnt terribly important. -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/20100608/73dba230/attachment.pgp

The conclusion I drew from Andy's comments is that functions which initialize more than one device should not return a negative value. That makes sense to me. Perhaps the documentation should be updated to incorporate this idea?

That poses the general question what a function that initializes several devices should do if one of the devices should return an error and what to return if ALL devices return an error.
At least in the last case I would assume to return the error code of one of the devices.
In the cases where not all, but at least one of the devices get initialized without error, the number of successful devices might be returned. However, in most contexts that might not be helpful to the rest of the system anyway.

I believe the consensus is that any device initialization function that returns an error is simply ignored and skipped, which is what my patch does:

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

I could have done "if (ret >= 0)", but that's less efficient.

But which error code? What if the first device returns -1, and the second one returns -2?

That's what we do today.

the code and documentation need to be updated in lock step, after we agree on the direction to take. i dont have a problem with picking a direction and going for it, but i do have a problem with things once again falling out of sync and no one having a f-ing clue what's going on because there's no reliable documentation and drivers dont match each other.

i think the direction people are leaning towards is to completely ignore all errors from net drivers and let u-boot continue to load.
-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/20100608/decbd08e/attachment.pgp

Dear Timur Tabi,

Mike is right. If you change it here, _all_ similar places should be changed as well in the same commit.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You Earth people glorified organized violence  for  forty  centuries.
But you imprison those who employ it privately.
	-- Spock, "Dagger of the Mind", stardate 2715.1

Well, since I don't want to change all the other code, I'll keep it as a signed int.

Timur Tabi
Linux kernel developer at Freescale