[PATCH] powerpc: kernel: remove useless code which related with 'max_cpus'

[PATCH] powerpc: kernel: remove useless code which related with 'max_cpus'

[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 <gang.chen@asianux.com>
---
 arch/powerpc/kernel/smp.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 38b0ba6..8c2bae4 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -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)
-- 
1.7.7.6

Re: [PATCH] powerpc: kernel: remove useless code which related with 'max_cpus'

[Srivatsa S. Bhat]

On 07/22/2013 11:28 AM, Chen Gang wrote:
> 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 <gang.chen@asianux.com>
> ---
>  arch/powerpc/kernel/smp.c |    8 --------
>  1 files changed, 0 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 38b0ba6..8c2bae4 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -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 }

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

Re: [PATCH] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/22/2013 02:18 PM, Srivatsa S. Bhat wrote:
> On 07/22/2013 11:28 AM, Chen Gang wrote:
>> > 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 <gang.chen@asianux.com>
>> > ---
>> >  arch/powerpc/kernel/smp.c |    8 --------
>> >  1 files changed, 0 insertions(+), 8 deletions(-)
>> > 
>> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
>> > index 38b0ba6..8c2bae4 100644
>> > --- a/arch/powerpc/kernel/smp.c
>> > +++ b/arch/powerpc/kernel/smp.c
>> > @@ -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 }
> 

OK, thanks.

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

> Regards,
> Srivatsa S. Bhat
> 
> 
> 

Thanks.
-- 
Chen Gang

[PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[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 <gang.chen@asianux.com>
---
 arch/powerpc/kernel/smp.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 38b0ba6..7edbd5b 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -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)
-- 
1.7.7.6

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[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.
> 
> 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 <gang.chen@asianux.com>

This version looks good.

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

> ---
>  arch/powerpc/kernel/smp.c |    9 ++-------
>  1 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 38b0ba6..7edbd5b 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -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)
> 

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/22/2013 02:51 PM, Srivatsa S. Bhat wrote:
> 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.
>>
>> 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 <gang.chen@asianux.com>
> 
> This version looks good.
> 
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 

Thank you very much.

> Regards,
> Srivatsa S. Bhat
> 
>> ---
>>  arch/powerpc/kernel/smp.c |    9 ++-------
>>  1 files changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
>> index 38b0ba6..7edbd5b 100644
>> --- a/arch/powerpc/kernel/smp.c
>> +++ b/arch/powerpc/kernel/smp.c
>> @@ -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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Michael Ellerman]

On Mon, Jul 22, 2013 at 12:21:16PM +0530, Srivatsa S. Bhat wrote:
> 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.
> > 
> > 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 <gang.chen@asianux.com>
> 
> 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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/23/2013 09:44 PM, Michael Ellerman wrote:
> On Mon, Jul 22, 2013 at 12:21:16PM +0530, Srivatsa S. Bhat wrote:
>> 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.
>>>
>>> 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 <gang.chen@asianux.com>
>>
>> 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).


> cheers
> 
> 

Thanks.
-- 
Chen Gang

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Michael Ellerman]

On Wed, Jul 24, 2013 at 08:28:07AM +0800, Chen Gang wrote:
> On 07/23/2013 09:44 PM, Michael Ellerman wrote:
> > On Mon, Jul 22, 2013 at 12:21:16PM +0530, Srivatsa S. Bhat wrote:
> >> 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.
> >>>
> >>> 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 <gang.chen@asianux.com>
> >>
> >> 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).

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

> 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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/24/2013 09:16 AM, Michael Ellerman wrote:
> On Wed, Jul 24, 2013 at 08:28:07AM +0800, Chen Gang wrote:
>> > On 07/23/2013 09:44 PM, Michael Ellerman wrote:
>>> > > On Mon, Jul 22, 2013 at 12:21:16PM +0530, Srivatsa S. Bhat wrote:
>>>> > >> 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.
>>>>> > >>>
>>>>> > >>> 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 <gang.chen@asianux.com>
>>>> > >>
>>>> > >> 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).
> 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).


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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Michael Ellerman]

On Wed, Jul 24, 2013 at 10:09:33AM +0800, Chen Gang wrote:
> On 07/24/2013 09:16 AM, Michael Ellerman wrote:
> > On Wed, Jul 24, 2013 at 08:28:07AM +0800, Chen Gang wrote:
> >> > On 07/23/2013 09:44 PM, Michael Ellerman wrote:
> >>> > > On Mon, Jul 22, 2013 at 12:21:16PM +0530, Srivatsa S. Bhat wrote:
> >>>> > >> 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).
> > 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.

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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/25/2013 11:15 AM, Michael Ellerman wrote:
> On Wed, Jul 24, 2013 at 10:09:33AM +0800, Chen Gang wrote:
>> > On 07/24/2013 09:16 AM, Michael Ellerman wrote:
>>> > > On Wed, Jul 24, 2013 at 08:28:07AM +0800, Chen Gang wrote:
>>>>> > >> > On 07/23/2013 09:44 PM, Michael Ellerman wrote:
>>>>>>> > >>> > > On Mon, Jul 22, 2013 at 12:21:16PM +0530, Srivatsa S. Bhat wrote:
>>>>>>>>> > >>>> > >> 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).
>>> > > 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.
> 

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


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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Benjamin Herrenschmidt]

On Thu, 2013-07-25 at 13:15 +1000, Michael Ellerman wrote:
> > 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.

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/25/2013 01:16 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 13:15 +1000, Michael Ellerman wrote:
>>> 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.
> 

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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Benjamin Herrenschmidt]

On Thu, 2013-07-25 at 13:24 +0800, Chen Gang wrote:
> 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.

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Benjamin Herrenschmidt]

On Thu, 2013-07-25 at 15:51 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 13:24 +0800, Chen Gang wrote:
> > 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.

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/25/2013 01:51 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 13:24 +0800, Chen Gang wrote:
>> 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.
> 

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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/25/2013 02:03 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 15:51 +1000, Benjamin Herrenschmidt wrote:
>> On Thu, 2013-07-25 at 13:24 +0800, Chen Gang wrote:
>>> 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.
> 

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

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

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Benjamin Herrenschmidt]

On Thu, 2013-07-25 at 14:17 +0800, Chen Gang wrote:
> 
> 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.

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/25/2013 03:33 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 14:17 +0800, Chen Gang wrote:
>> > 
>> > 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.

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

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 7edbd5b..53155f4 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -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
-- 
Chen Gang

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Benjamin Herrenschmidt]

On Thu, 2013-07-25 at 15:59 +0800, Chen Gang wrote:
> 
> 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.

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/25/2013 04:06 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 15:59 +0800, Chen Gang wrote:
>>
>> 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.
-- 
Chen Gang

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Benjamin Herrenschmidt]

On Thu, 2013-07-25 at 16:22 +0800, Chen Gang wrote:
> On 07/25/2013 04:06 PM, Benjamin Herrenschmidt wrote:
> > On Thu, 2013-07-25 at 15:59 +0800, Chen Gang wrote:
> >>
> >> 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).

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.

Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'

[Chen Gang]

On 07/25/2013 04:28 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 16:22 +0800, Chen Gang wrote:
>> > On 07/25/2013 04:06 PM, Benjamin Herrenschmidt wrote:
>>> > > On Thu, 2013-07-25 at 15:59 +0800, Chen Gang wrote:
>>>> > >>
>>>> > >> 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).
> 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.
-- 
Chen Gang