powerpc: kernel: remove useless code which related with 'max_cpus'

PATCH Superseded Chen Gang 4 participants 24 messages Jul 22, 2013 – Jul 25, 2013 PowerPC

lore

Share & tools

lore

Chen Gang

author

2013-07-22 05:59 UTC #0 lore raw

PATCH 1/1 powerpc: kernel: remove useless code which related with 'max_cpus'

Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.

The related commit:

c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase

The related warning:

arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]

Signed-off-by Chen Gang <[email protected]>

---
 arch/powerpc/kernel/smp.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

arch/powerpc/kernel/smp.c +0 -8

index 38b0ba6..8c2bae4 100644
@@ -345,14 +345,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 
 	cpumask_set_cpu(boot_cpuid, cpu_sibling_mask(boot_cpuid));
 	cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
-
-	if (smp_ops)
-		if (smp_ops->probe)
-			max_cpus = smp_ops->probe();
-		else
-			max_cpus = NR_CPUS;
-	else
-		max_cpus = 1;
 }
 
 void smp_prepare_boot_cpu(void)

Signature · 1 line

1.7.7.6

Srivatsa S. Bhat

2013-07-22 06:21 UTC #1 lore raw

Quoted 21 lines · Chen Gang

Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:
c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
The related warning:
arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>
---
 arch/powerpc/kernel/smp.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)
arch/powerpc/kernel/smp.c +0 -0
index 38b0ba6..8c2bae4 100644

arch/powerpc/kernel/smp.c void __init smp_prepare_cpus(unsigned int max_cpus) · line 351

Quoted 7 lines

arch/powerpc/kernel/smp.c +0 -4

@@ -345,14 +345,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 
 	cpumask_set_cpu(boot_cpuid, cpu_sibling_mask(boot_cpuid));
 	cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
-
-	if (smp_ops)
-		if (smp_ops->probe)
-			max_cpus = smp_ops->probe();

No, you can't just remove the call to ->probe(). Its essential. For example, one such ->probe() routine, xics_smp_probe() does some pretty important stuff, such as populating the ->cause_ipi function pointer etc, see below:

143 int __init xics_smp_probe(void)
144 {
145 /* Setup cause_ipi callback based on which ICP is used */
146 smp_ops->cause_ipi = icp_ops->cause_ipi;
147
148 /* Register all the IPIs */
149 xics_request_ipi();
150
151 return cpumask_weight(cpu_possible_mask);
152 }

arch/powerpc/kernel/smp.c void __init smp_prepare_cpus(unsigned int max_cpus) · line 349

Quoted 6 lines

arch/powerpc/kernel/smp.c +0 -4

-		else
-			max_cpus = NR_CPUS;
-	else
-		max_cpus = 1;
 }

Yes, max_cpus is unused, but you have to be careful while removing code.
Regards,
Srivatsa S. Bhat

Chen Gang

author

2013-07-22 06:28 UTC #2 lore raw

Quoted 34 lines · Srivatsa S. Bhat

Quoted 22 lines · Chen Gang
Quoted 22 lines
Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:
c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
The related warning:
arch/powerpc/kernel/smp.c:323:43: warning: parameter �max_cpus� set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>
---
 arch/powerpc/kernel/smp.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)
arch/powerpc/kernel/smp.c +0 -0
index 38b0ba6..8c2bae4 100644
@@ -345,14 +345,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 
Quoted 6 lines
arch/powerpc/kernel/smp.c void __init smp_prepare_cpus(unsigned int max_cpus) · line 351
Quoted 6 lines
arch/powerpc/kernel/smp.c +0 -4
 	cpumask_set_cpu(boot_cpuid, cpu_sibling_mask(boot_cpuid));
 	cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
-
-	if (smp_ops)
-		if (smp_ops->probe)
-			max_cpus = smp_ops->probe();
No, you can't just remove the call to ->probe(). Its essential. For example, one such ->probe() routine, xics_smp_probe() does some pretty important stuff, such as populating the ->cause_ipi function pointer etc, see below:

Quoted 11 lines

143 int __init xics_smp_probe(void)
144 {
145 /* Setup cause_ipi callback based on which ICP is used */
146 smp_ops->cause_ipi = icp_ops->cause_ipi;
147
148 /* Register all the IPIs */
149 xics_request_ipi();
150
151 return cpumask_weight(cpu_possible_mask);
152 }

OK, thanks.

Quoted 8 lines

Quoted 6 lines
arch/powerpc/kernel/smp.c void __init smp_prepare_cpus(unsigned int max_cpus) · line 349
Quoted 6 lines
arch/powerpc/kernel/smp.c +0 -4
-		else
-			max_cpus = NR_CPUS;
-	else
-		max_cpus = 1;
 }
 
Yes, max_cpus is unused, but you have to be careful while removing code.

I should send patch v2, soon. :-)

Quoted 5 lines

Regards,
Srivatsa S. Bhat

Thanks.

Signature · 1 line

Chen Gang

Chen Gang

author

2013-07-22 06:41 UTC #3 lore raw

PATCH v2 1/1 powerpc: kernel: remove useless code which related with 'max_cpus'

Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.

The related commit:

c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase

The related warning:

arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]

Signed-off-by Chen Gang <[email protected]>

---
 arch/powerpc/kernel/smp.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

arch/powerpc/kernel/smp.c +2 -7

index 38b0ba6..7edbd5b 100644
@@ -346,13 +346,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	cpumask_set_cpu(boot_cpuid, cpu_sibling_mask(boot_cpuid));
 	cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
 
-	if (smp_ops)
-		if (smp_ops->probe)
-			max_cpus = smp_ops->probe();
-		else
-			max_cpus = NR_CPUS;
-	else
-		max_cpus = 1;
+	if (smp_ops && smp_ops->probe)
+		smp_ops->probe();
 }
 
 void smp_prepare_boot_cpu(void)

Signature · 1 line

1.7.7.6

Srivatsa S. Bhat

Reviewed-by on v2 1/1

2013-07-22 06:54 UTC #4 lore raw

Quoted 5 lines · Chen Gang

Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:

Quoted 7 lines

c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
The related warning:
arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>

This version looks good.

Reviewed-by Srivatsa S. Bhat <[email protected]>

Regards,
Srivatsa S. Bhat

Quoted 25 lines

---
 arch/powerpc/kernel/smp.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

arch/powerpc/kernel/smp.c +2 -7

index 38b0ba6..7edbd5b 100644
@@ -346,13 +346,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	cpumask_set_cpu(boot_cpuid, cpu_sibling_mask(boot_cpuid));
 	cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
 
-	if (smp_ops)
-		if (smp_ops->probe)
-			max_cpus = smp_ops->probe();
-		else
-			max_cpus = NR_CPUS;
-	else
-		max_cpus = 1;
+	if (smp_ops && smp_ops->probe)
+		smp_ops->probe();
 }
 
 void smp_prepare_boot_cpu(void)

Chen Gang

author

2013-07-22 07:04 UTC #5 lore raw

Quoted 10 lines · Srivatsa S. Bhat

Quoted 9 lines · Chen Gang
Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:
c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
The related warning:

Quoted 8 lines

Quoted 3 lines
arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>
This version looks good.
Reviewed-by Srivatsa S. Bhat <[email protected]>

Thank you very much.

Quoted 32 lines

Regards,
Srivatsa S. Bhat

Quoted 25 lines

---
 arch/powerpc/kernel/smp.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

arch/powerpc/kernel/smp.c +2 -7

index 38b0ba6..7edbd5b 100644
@@ -346,13 +346,8 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	cpumask_set_cpu(boot_cpuid, cpu_sibling_mask(boot_cpuid));
 	cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
 
-	if (smp_ops)
-		if (smp_ops->probe)
-			max_cpus = smp_ops->probe();
-		else
-			max_cpus = NR_CPUS;
-	else
-		max_cpus = 1;
+	if (smp_ops && smp_ops->probe)
+		smp_ops->probe();
 }
 
 void smp_prepare_boot_cpu(void)

Signature · 1 line

Chen Gang

Michael Ellerman

2013-07-23 13:44 UTC #6 lore raw

In reply to Srivatsa S. Bhat

Quoted 8 lines · Srivatsa S. Bhat

Quoted 7 lines · Chen Gang
Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:
c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase

Quoted 7 lines

Quoted 5 lines
The related warning:
arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>
This version looks good.

Agreed.

A good follow up patch, or actually series of patches, would be to change the prototype of smp_ops->probe() to return void, and fix all the implementations to no longer return anything.

cheers

Chen Gang

author

2013-07-24 00:29 UTC #7 lore raw

Quoted 15 lines · Michael Ellerman

Quoted 14 lines · Srivatsa S. Bhat
Quoted 12 lines · Chen Gang
Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:
c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
The related warning:
arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>

Quoted 8 lines

Quoted 1 line
This version looks good.
Agreed.
A good follow up patch, or actually series of patches, would be to change the prototype of smp_ops->probe() to return void, and fix all the implementations to no longer return anything.

Hmm... normally, a function need have a return value, it will make it more extensible (especially, it is an API which need be implemented in various sub modules).

Even though the return value may be useless, now, if the performance is not quite important in our case, I still suggest to have it (especially each various original implementation already has it).

Quoted 3 lines

cheers

Thanks.

Signature · 1 line

Chen Gang

Michael Ellerman

2013-07-24 01:16 UTC #8 lore raw

Quoted 20 lines · Chen Gang

Quoted 19 lines · Michael Ellerman
Quoted 8 lines · Srivatsa S. Bhat
Quoted 7 lines · Chen Gang
Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:
c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
Quoted 7 lines
Quoted 5 lines
The related warning:
arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>
This version looks good.
Agreed.

Quoted 8 lines

Quoted 4 lines
A good follow up patch, or actually series of patches, would be to change the prototype of smp_ops->probe() to return void, and fix all the implementations to no longer return anything.
Hmm... normally, a function need have a return value, it will make it more extensible (especially, it is an API which need be implemented in various sub modules).

A function doesn't need a return value, and if it needs one in future then we'll add it then. We don't carry code around "just in case".

Quoted 3 lines

Even though the return value may be useless, now, if the performance is not quite important in our case, I still suggest to have it (especially each various original implementation already has it).

It's dead code, it should be removed.

cheers

Chen Gang

author

2013-07-24 02:10 UTC #9 lore raw

Quoted 26 lines · Michael Ellerman

Quoted 25 lines · Chen Gang
Quoted 25 lines · Michael Ellerman
Quoted 23 lines
Quoted 23 lines · Srivatsa S. Bhat
Quoted 8 lines
Quoted 8 lines · Chen Gang
Quoted 7 lines
Quoted 7 lines
Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
The related commit:
c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
The related warning:
arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
Signed-off-by Chen Gang <[email protected]>
This version looks good.
Agreed.
A good follow up patch, or actually series of patches, would be to change the prototype of smp_ops->probe() to return void, and fix all the implementations to no longer return anything.

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
Hmm... normally, a function need have a return value, it will make it more extensible (especially, it is an API which need be implemented in various sub modules).
A function doesn't need a return value, and if it needs one in future then we'll add it then. We don't carry code around "just in case".

But for API (also include the internal API), at least, better to always provide the return value which can indicate failure by negative number (if succeed can return the meanness value, e.g. the number of cpus).

Quoted 4 lines

Quoted 3 lines
Quoted 3 lines
Even though the return value may be useless, now, if the performance is not quite important in our case, I still suggest to have it (especially each various original implementation already has it).
It's dead code, it should be removed.

For API, if not cause the real world issue, better to keep compatible (especially, the return value still can indicate failure by negative number).

Thanks.

Signature · 1 line

Chen Gang

Michael Ellerman

2013-07-25 03:15 UTC #10 lore raw

Quoted 16 lines · Chen Gang

Quoted 15 lines · Michael Ellerman
Quoted 14 lines · Chen Gang
Quoted 14 lines · Michael Ellerman
Quoted 9 lines
Quoted 9 lines · Srivatsa S. Bhat
Quoted 3 lines
Quoted 3 lines · Chen Gang
Quoted 2 lines
Quoted 2 lines
Since not need 'max_cpus' after the related commit, the related code are useless too, need be removed.
A good follow up patch, or actually series of patches, would be to change the prototype of smp_ops->probe() to return void, and fix all the implementations to no longer return anything.
Hmm... normally, a function need have a return value, it will make it more extensible (especially, it is an API which need be implemented in various sub modules).

Quoted 6 lines

Quoted 2 lines
A function doesn't need a return value, and if it needs one in future then we'll add it then. We don't carry code around "just in case".
But for API (also include the internal API), at least, better to always provide the return value which can indicate failure by negative number (if succeed can return the meanness value, e.g. the number of cpus).

Are we still talking about this?

There is no point returning a value when no one checks it. Which is the case here.

For a published API maybe it's a good idea to have a return value "just in case", but this is kernel internal and we own both the implementation and the callers of the API.

Quoted 9 lines

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Even though the return value may be useless, now, if the performance is not quite important in our case, I still suggest to have it (especially each various original implementation already has it).
It's dead code, it should be removed.
For API, if not cause the real world issue, better to keep compatible (especially, the return value still can indicate failure by negative number).

No. Dead code is a real world issue. If we ever need a return value we'll add one then.

cheers

Chen Gang

author

2013-07-25 04:03 UTC #11 lore raw

Quoted 23 lines · Michael Ellerman

Quoted 16 lines · Chen Gang
Quoted 16 lines · Michael Ellerman
Quoted 15 lines
Quoted 15 lines · Chen Gang
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines · Michael Ellerman
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines · Srivatsa S. Bhat
On 07/22/2013 12:10 PM, Chen Gang wrote:
Since not need 'max_cpus' after the related commit, the related code
are useless too, need be removed.
A good follow up patch, or actually series of patches, would be to
change the prototype of smp_ops->probe() to return void, and fix all the
implementations to no longer return anything.
Hmm... normally, a function need have a return value, it will make it more extensible (especially, it is an API which need be implemented in various sub modules).
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
A function doesn't need a return value, and if it needs one in future then we'll add it then. We don't carry code around "just in case".
But for API (also include the internal API), at least, better to always provide the return value which can indicate failure by negative number (if succeed can return the meanness value, e.g. the number of cpus).

Quoted 9 lines

Are we still talking about this?
There is no point returning a value when no one checks it. Which is the case here.
For a published API maybe it's a good idea to have a return value "just in case", but this is kernel internal and we own both the implementation and the callers of the API.

API is between caller and callee, but independent with who will use it and who will implement it (may be they are the same member), and also independent with whether "between kernel and user" or not.

Today, you are really the member for both caller and callee, but in the future, may not.

For our case, it is really an API, it is defined in upper level, and implement in various sub modules (extern the declaration and implement in various sub modules).

Quoted 10 lines

Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Even though the return value may be useless, now, if the performance is not quite important in our case, I still suggest to have it (especially each various original implementation already has it).
It's dead code, it should be removed.
For API, if not cause the real world issue, better to keep compatible (especially, the return value still can indicate failure by negative number).
No. Dead code is a real world issue. If we ever need a return value we'll add one then.

Hmm... what my original saying "real world issue" is not precise, it need change to: "if it is not an 'urgent' issue (may be a real world issue), for API, need still keep it compatible (keep no touch) now".

"dead code for an API" does not belong to 'urgent' issue, it belongs to 'important' issue. When we are reconstructing the source code, we can also remove them in that window (at least, it does not often happen).

(although for our case, I don't think "return value for API" is "dead code for an API")

Thanks.

Signature · 1 line

Chen Gang

Benjamin Herrenschmidt

2013-07-25 05:17 UTC #12 lore raw

In reply to Michael Ellerman

Quoted 8 lines · Michael Ellerman

Quoted 3 lines
But for API (also include the internal API), at least, better to always provide the return value which can indicate failure by negative number (if succeed can return the meanness value, e.g. the number of cpus).
Are we still talking about this?
There is no point returning a value when no one checks it. Which is the case here.

Right. The return value is historical, it dates from when we didn't have cpu_possible_mask etc...

Nowadays, the probe() routine is just some early init, and might also affect those masks if needed, the return value has become obsolete.

You are welcome to post a patch removing it.

Cheers,
Ben.

Chen Gang

author

2013-07-25 05:25 UTC #13 lore raw

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 8 lines · Michael Ellerman
Quoted 3 lines
But for API (also include the internal API), at least, better to always provide the return value which can indicate failure by negative number (if succeed can return the meanness value, e.g. the number of cpus).
Are we still talking about this?
There is no point returning a value when no one checks it. Which is the case here.

Quoted 8 lines

Right. The return value is historical, it dates from when we didn't have cpu_possible_mask etc...
Nowadays, the probe() routine is just some early init, and might also affect those masks if needed, the return value has become obsolete.
You are welcome to post a patch removing it.

For an extern function, if the performance is not sensible, better to have the return value which can indicate the failure with the negative number.

Thanks.

Signature · 1 line

Chen Gang

Benjamin Herrenschmidt

2013-07-25 05:52 UTC #14 lore raw

Quoted 3 lines · Chen Gang

For an extern function, if the performance is not sensible, better to have the return value which can indicate the failure with the negative number.

The return value is meaningless.

We don't have a good way to handle it. It has no defined semantics. What does "failure" means in that case ? Nothing !

So just remove it.

Ben.

Benjamin Herrenschmidt

2013-07-25 06:04 UTC #15 lore raw

Quoted 11 lines · Benjamin Herrenschmidt

Quoted 3 lines · Chen Gang
For an extern function, if the performance is not sensible, better to have the return value which can indicate the failure with the negative number.
The return value is meaningless.
We don't have a good way to handle it. It has no defined semantics. What does "failure" means in that case ? Nothing !
So just remove it.

Note: If you want to create a concept of smp_ops->probe() failing, then not only you need to check all the implementations, but *also* add something sensible to do when it fails ... such as disabling bringup of CPUs.

In this case however, we have put the burden of doing whatever makes sense in the probe() function itself. If can adjust the possible map if it fails.

Cheers,
Ben.

Chen Gang

author

2013-07-25 06:31 UTC #16 lore raw

Quoted 11 lines · Benjamin Herrenschmidt

Quoted 10 lines · Benjamin Herrenschmidt
Quoted 3 lines · Chen Gang
For an extern function, if the performance is not sensible, better to have the return value which can indicate the failure with the negative number.
The return value is meaningless.
We don't have a good way to handle it. It has no defined semantics. What does "failure" means in that case ? Nothing !

Quoted 7 lines

Quoted 1 line
So just remove it.
Note: If you want to create a concept of smp_ops->probe() failing, then not only you need to check all the implementations, but *also* add something sensible to do when it fails ... such as disabling bringup of CPUs.

Hmm... if critical, use BUG(), else (none critical), just print a warning message ?

Quoted 4 lines

In this case however, we have put the burden of doing whatever makes sense in the probe() function itself. If can adjust the possible map if it fails.

Excuse me, my English is not quite well, I guss your meaning is: "it can be fail in internal implementation, but has no effect with the final result to caller", is it correct ?

If what I understand is correct, it needn't let caller know about it.

Thanks.

Signature · 1 line

Chen Gang

Chen Gang

author

2013-07-25 06:18 UTC #17 lore raw

In reply to Benjamin Herrenschmidt

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 3 lines · Chen Gang
For an extern function, if the performance is not sensible, better to have the return value which can indicate the failure with the negative number.

Quoted 7 lines

The return value is meaningless.
We don't have a good way to handle it. It has no defined semantics. What does "failure" means in that case ? Nothing !
So just remove it.

Hmm... for an extern function (espeically have been implemented in various modules), normally, we can assume it may fail in some cases (although now, we don't know what cases can cause its failure).

If "we don't have a good way to handle the failure", "print the related warning message" is an executable choice (or "BUG_ON()", if it is critical).

So, if the performance is not sensible, I still suggest to let extern function have return value.

Thanks.

Signature · 1 line

Chen Gang

Benjamin Herrenschmidt

2013-07-25 07:34 UTC #18 lore raw

Quoted 10 lines · Chen Gang

Hmm... for an extern function (espeically have been implemented in various modules), normally, we can assume it may fail in some cases (although now, we don't know what cases can cause its failure).
If "we don't have a good way to handle the failure", "print the related warning message" is an executable choice (or "BUG_ON()", if it is critical).
So, if the performance is not sensible, I still suggest to let extern function have return value.

This is not a module function. We are not doing a uni course on how to write C code here. Be real.

Ben.

Chen Gang

author

2013-07-25 08:00 UTC #19 lore raw

Quoted 6 lines · Benjamin Herrenschmidt

Quoted 5 lines · Chen Gang
Quoted 5 lines
Hmm... for an extern function (espeically have been implemented in various modules), normally, we can assume it may fail in some cases (although now, we don't know what cases can cause its failure).

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
If "we don't have a good way to handle the failure", "print the related warning message" is an executable choice (or "BUG_ON()", if it is critical).
So, if the performance is not sensible, I still suggest to let extern function have return value.
This is not a module function. We are not doing a uni course on how to write C code here. Be real.

In our case, 'module' points to various sub directories of arch/powerpc (maybe 'module' is not quite precise, it is easy misunderstand).

The real world is not conflict with "how to write C code".

For my opinion: one fix may like below (assume have removed max_cpus) which is more reasonable for code readers.

-----------------------------diff begin------------------------------

arch/powerpc/kernel/smp.c +1 -1

index 7edbd5b..53155f4 100644
@@ -347,7 +347,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	cpumask_set_cpu(boot_cpuid, cpu_core_mask(boot_cpuid));
 
 	if (smp_ops && smp_ops->probe)
-		smp_ops->probe();
+		BUG_ON(smp_ops->probe() < 0);
 }
 
 void smp_prepare_boot_cpu(void)

-----------------------------diff end--------------------------------

Thanks

Signature · 1 line

Chen Gang

Benjamin Herrenschmidt

2013-07-25 08:07 UTC #20 lore raw

Quoted 3 lines · Chen Gang

For my opinion: one fix may like below (assume have removed max_cpus) which is more reasonable for code readers.

So instead of just failing to bring the secondary CPUs, but potentially still having a working system, you crash during boot.... potentially before a console is even visible. And this is good how ?

Ben.

Chen Gang

author

2013-07-25 08:23 UTC #21 lore raw

Quoted 9 lines · Benjamin Herrenschmidt

Quoted 3 lines · Chen Gang
For my opinion: one fix may like below (assume have removed max_cpus) which is more reasonable for code readers.
So instead of just failing to bring the secondary CPUs, but potentially still having a working system, you crash during boot.... potentially before a console is even visible. And this is good how ?

Hmm... how about the above DBG("...") within this function ?

One implementation of BUG_ON() is use printk() and coredump, if it is a critical failure, I suggest to use it (if console is really invisible, I guess still can generate the coredump).

Hmm... But do you mean it really can be failed, but it is not a critical failure ? if so we need print the related warning message instead of.

Thanks.

Signature · 1 line

Chen Gang

Benjamin Herrenschmidt

2013-07-25 08:30 UTC #22 lore raw

Quoted 11 lines · Chen Gang

Quoted 9 lines · Benjamin Herrenschmidt
Quoted 3 lines · Chen Gang
For my opinion: one fix may like below (assume have removed max_cpus) which is more reasonable for code readers.
So instead of just failing to bring the secondary CPUs, but potentially still having a working system, you crash during boot.... potentially before a console is even visible. And this is good how ?

Quoted 5 lines

Hmm... how about the above DBG("...") within this function ?
One implementation of BUG_ON() is use printk() and coredump, if it is a critical failure, I suggest to use it (if console is really invisible, I guess still can generate the coredump).

Whatever ... looks like you don't feel like listening so I'm not going to waste my breath anymore, nor will I accept your patches.

Ben.

Chen Gang

author

2013-07-25 08:37 UTC #23 lore raw

Quoted 12 lines · Benjamin Herrenschmidt

Quoted 11 lines · Chen Gang
Quoted 11 lines · Benjamin Herrenschmidt
Quoted 9 lines
Quoted 9 lines · Chen Gang
Quoted 3 lines
Quoted 3 lines
For my opinion: one fix may like below (assume have removed max_cpus) which is more reasonable for code readers.
So instead of just failing to bring the secondary CPUs, but potentially still having a working system, you crash during boot.... potentially before a console is even visible. And this is good how ?

Quoted 7 lines

Quoted 5 lines
Quoted 5 lines
Hmm... how about the above DBG("...") within this function ?
One implementation of BUG_ON() is use printk() and coredump, if it is a critical failure, I suggest to use it (if console is really invisible, I guess still can generate the coredump).
Whatever ... looks like you don't feel like listening so I'm not going to waste my breath anymore, nor will I accept your patches.

I can understand,

But 'patch' or 'patches' ? ;-)

Thanks.

Signature · 1 line

Chen Gang