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

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

[Timur Tabi]

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 */
-- 
1.7.0.1

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

[Mike Frysinger]

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
-------------- 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 

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

[Timur Tabi]

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.

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

[Mike Frysinger]

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
-------------- 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 

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

[Wolfgang Denk]

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

-- 
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

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

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Timur Tabi]

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?

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

[Reinhard Meyer]

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.

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

[Timur Tabi]

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.
> 
> 

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

[Mike Frysinger]

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
-------------- 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