[U-Boot] [PATCH] [v2] 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.
Signed-off-by: Timur Tabi timur@freescale.com --- drivers/net/tsec.c | 22 ++++++++++++++++------ include/tsec.h | 4 ++-- 2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 3e4c3bd..6ca9735 100644 --- a/drivers/net/tsec.c +++ b/drivers/net/tsec.c @@ -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)); } diff --git a/include/tsec.h b/include/tsec.h index 1e90365..9da2740 100644 --- a/include/tsec.h +++ b/include/tsec.h @@ -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 */
On Monday, June 07, 2010 14:31:27 Timur Tabi wrote:
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.
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
Mike Frysinger wrote:
On Monday, June 07, 2010 14:31:27 Timur Tabi wrote:
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.
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.
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.
your fixes no way require these to be unsigned int funcs.
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.
On Monday, June 07, 2010 18:55:18 Timur Tabi wrote:
Mike Frysinger wrote:
On Monday, June 07, 2010 14:31:27 Timur Tabi wrote:
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.
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.
I don't want to change all of the functions. For most devices, there's no way I can test them.
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.
Just because pci_eth_int() is incorrect, that doesn't mean that I can't make tsec_eth_init() correct.
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.
your fixes no way require these to be unsigned int funcs.
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.
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
Mike Frysinger wrote:
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.
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?
Timur Tabi schrieb:
Mike Frysinger wrote:
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.
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.
Reinhard Meyer (-VC) wrote:
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.
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.
At least in the last case I would assume to return the error code of one of the devices.
But which error code? What if the first device returns -1, and the second one returns -2?
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.
That's what we do today.
However, in most contexts that might not be helpful to the rest of the system anyway.
On Tuesday, June 08, 2010 10:34:28 Timur Tabi wrote:
Mike Frysinger wrote:
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.
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?
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
Dear Timur Tabi,
In message 4C0D78D6.2010707@freescale.com you wrote:
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.
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.
your fixes no way require these to be unsigned int funcs.
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.
Mike is right. If you change it here, _all_ similar places should be changed as well in the same commit.
Best regards,
Wolfgang Denk
On Tue, Jun 8, 2010 at 2:14 AM, Wolfgang Denk wd@denx.de wrote:
Mike is right. If you change it here, _all_ similar places should be changed as well in the same commit.
Well, since I don't want to change all the other code, I'll keep it as a signed int.
participants (4)
-
Mike Frysinger -
Reinhard Meyer (-VC) -
Timur Tabi -
Wolfgang Denk