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

login
register
mail settings
Submitter Chen Gang
Date July 25, 2013, 7:59 a.m.
Message ID <51F0DAF1.9060702@asianux.com>
Download mbox | patch
Permalink /patch/261607/
State Rejected
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Chen Gang - July 25, 2013, 7:59 a.m.
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 end--------------------------------


Thanks
Benjamin Herrenschmidt - July 25, 2013, 8:06 a.m.
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.
Chen Gang - July 25, 2013, 8:22 a.m.
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.
Benjamin Herrenschmidt - July 25, 2013, 8:28 a.m.
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.
Chen Gang - July 25, 2013, 8:36 a.m.
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.

Patch

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)