[PATCH v2] smp: kernel/panic.c - silence warnings

[PATCH v2] smp: kernel/panic.c - silence warnings

[He Ying]

We found these warnings in kernel/panic.c by using sparse tool:
warning: symbol 'panic_smp_self_stop' was not declared.
warning: symbol 'nmi_panic_self_stop' was not declared.
warning: symbol 'crash_smp_send_stop' was not declared.

To avoid them, add declarations for these three functions in
include/linux/smp.h.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: He Ying <heying24@huawei.com>
---
V1->V2:
- fix some misspellings

 include/linux/smp.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index 70c6f6284dcf..27008a1c8111 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -50,6 +50,14 @@ extern unsigned int total_cpus;
 int smp_call_function_single(int cpuid, smp_call_func_t func, void *info,
 			     int wait);
 
+/*
+ * Cpus stopping functions in panic. All have default weak definitions.
+ * Architecture-dependent code may override them.
+ */
+void panic_smp_self_stop(void);
+void nmi_panic_self_stop(struct pt_regs *regs);
+void crash_smp_send_stop(void);
+
 /*
  * Call a function on all processors
  */
-- 
2.17.1

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Ingo Molnar]

* He Ying <heying24@huawei.com> wrote:

> We found these warnings in kernel/panic.c by using sparse tool:
> warning: symbol 'panic_smp_self_stop' was not declared.
> warning: symbol 'nmi_panic_self_stop' was not declared.
> warning: symbol 'crash_smp_send_stop' was not declared.
> 
> To avoid them, add declarations for these three functions in
> include/linux/smp.h.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: He Ying <heying24@huawei.com>
> ---
> V1->V2:
> - fix some misspellings
> 
>  include/linux/smp.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index 70c6f6284dcf..27008a1c8111 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -50,6 +50,14 @@ extern unsigned int total_cpus;
>  int smp_call_function_single(int cpuid, smp_call_func_t func, void *info,
>  			     int wait);
>  
> +/*
> + * Cpus stopping functions in panic. All have default weak definitions.
> + * Architecture-dependent code may override them.
> + */
> +void panic_smp_self_stop(void);
> +void nmi_panic_self_stop(struct pt_regs *regs);
> +void crash_smp_send_stop(void);

Please follow the 'extern' convention used for prototype declarations 
in that header file.

Thanks,

	Ingo

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Christophe Leroy]

Le 17/03/2021 à 10:49, Ingo Molnar a écrit :
> 
> * He Ying <heying24@huawei.com> wrote:
> 
>> We found these warnings in kernel/panic.c by using sparse tool:
>> warning: symbol 'panic_smp_self_stop' was not declared.
>> warning: symbol 'nmi_panic_self_stop' was not declared.
>> warning: symbol 'crash_smp_send_stop' was not declared.
>>
>> To avoid them, add declarations for these three functions in
>> include/linux/smp.h.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: He Ying <heying24@huawei.com>
>> ---
>> V1->V2:
>> - fix some misspellings
>>
>>   include/linux/smp.h | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/include/linux/smp.h b/include/linux/smp.h
>> index 70c6f6284dcf..27008a1c8111 100644
>> --- a/include/linux/smp.h
>> +++ b/include/linux/smp.h
>> @@ -50,6 +50,14 @@ extern unsigned int total_cpus;
>>   int smp_call_function_single(int cpuid, smp_call_func_t func, void *info,
>>   			     int wait);
>>   
>> +/*
>> + * Cpus stopping functions in panic. All have default weak definitions.
>> + * Architecture-dependent code may override them.
>> + */
>> +void panic_smp_self_stop(void);
>> +void nmi_panic_self_stop(struct pt_regs *regs);
>> +void crash_smp_send_stop(void);
> 

What do you mean ? 'extern' prototype is pointless for function prototypes and deprecated, no new 
function prototypes should be added with the 'extern' keyword.

checkpatch.pl tells you: "extern prototypes should be avoided in .h files"

Christophe

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Peter Zijlstra]

On Wed, Mar 17, 2021 at 12:00:29PM +0100, Christophe Leroy wrote:
> What do you mean ? 'extern' prototype is pointless for function prototypes
> and deprecated, no new function prototypes should be added with the 'extern'
> keyword.
> 
> checkpatch.pl tells you: "extern prototypes should be avoided in .h files"

I have a very strong preference for extern on function decls, to match
the extern on variable decl.

Checkpatch is just wrong here.

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Christophe Leroy]

Le 17/03/2021 à 13:23, Peter Zijlstra a écrit :
> On Wed, Mar 17, 2021 at 12:00:29PM +0100, Christophe Leroy wrote:
>> What do you mean ? 'extern' prototype is pointless for function prototypes
>> and deprecated, no new function prototypes should be added with the 'extern'
>> keyword.
>>
>> checkpatch.pl tells you: "extern prototypes should be avoided in .h files"
> 
> I have a very strong preference for extern on function decls, to match
> the extern on variable decl.

You mean you also do 'static inline' variable declarations ?

I think you just can't compare variable declarations and function declaration, that's too different.

> 
> Checkpatch is just wrong here.
> 

checkpatch seems rather pragmatic, the commit message says:

  checkpatch: warn when using extern with function prototypes in .h files

Using the extern keyword on function prototypes is superfluous visual
noise so suggest removing it.

Using extern can cause unnecessary line wrapping at 80 columns and
unnecessarily long multi-line function prototypes.

Signed-off-by: Joe Perches <joe@perches.com>
Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Peter Zijlstra]

On Wed, Mar 17, 2021 at 06:17:26PM +0100, Christophe Leroy wrote:
> 
> 
> Le 17/03/2021 à 13:23, Peter Zijlstra a écrit :
> > On Wed, Mar 17, 2021 at 12:00:29PM +0100, Christophe Leroy wrote:
> > > What do you mean ? 'extern' prototype is pointless for function prototypes
> > > and deprecated, no new function prototypes should be added with the 'extern'
> > > keyword.
> > > 
> > > checkpatch.pl tells you: "extern prototypes should be avoided in .h files"
> > 
> > I have a very strong preference for extern on function decls, to match
> > the extern on variable decl.
> 
> You mean you also do 'static inline' variable declarations ?

That's a func definition, not a declaration. And you _can_ do static
variable definitions in a header file just fine, although that's
typically not what you'd want. Although sometimes I've seen people do:

static const int my_var = 10;

inline is an attribute that obviously doesn't work on variables.

> Using the extern keyword on function prototypes is superfluous visual
> noise so suggest removing it.

I don't agree; and I think the C spec is actually wrong there (too).

The thing is that it distinguishes between a forward declaration of a
function in the same TU and an external declaration for a function in
another TU.

That is; if I see:

void ponies(int legs);

I expect that function to be defined later in the same TU. IOW it's a
forward declaration. OTOH if I see:

extern void ponies(int legs);

I know I won't find it in this TU and the linker will end up involved.

Now, the C people figured that distinction was useless and allowed
sloppiness. But I still think there's merrit to that. And as mentioned
earlier, it is consistent with variable declarations.

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> Now, the C people figured that distinction was useless and allowed 
> sloppiness. But I still think there's merrit to that. And as 
> mentioned earlier, it is consistent with variable declarations.

Fully agreed, and my other point was that it's also consistent with 
the other existing externs were used *in the same header file* 
already.

I.e. there's nothing more sloppy than mixing different styles within 
the same header. Checkpatch needs to be fixed or ignored here.

Thanks,

	Ingo

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[heying (H)]

在 2021/3/18 4:09, Ingo Molnar 写道:
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
>> Now, the C people figured that distinction was useless and allowed
>> sloppiness. But I still think there's merrit to that. And as
>> mentioned earlier, it is consistent with variable declarations.
> Fully agreed, and my other point was that it's also consistent with
> the other existing externs were used *in the same header file*
> already.
>
> I.e. there's nothing more sloppy than mixing different styles within
> the same header. Checkpatch needs to be fixed or ignored here.

Thank you all for the reply!

There are already mixing different styles within linux/smp.h. I mean 
'extern' and

non 'extern' func declarations both exist in this header. Since two of 
you three

think that 'extern' is needed, I'll add it and resend my patch.


Thanks again.

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Christophe Leroy]

Le 17/03/2021 à 18:37, Peter Zijlstra a écrit :
> On Wed, Mar 17, 2021 at 06:17:26PM +0100, Christophe Leroy wrote:
>>
>>
>> Le 17/03/2021 à 13:23, Peter Zijlstra a écrit :
>>> On Wed, Mar 17, 2021 at 12:00:29PM +0100, Christophe Leroy wrote:
>>>> What do you mean ? 'extern' prototype is pointless for function prototypes
>>>> and deprecated, no new function prototypes should be added with the 'extern'
>>>> keyword.
>>>>
>>>> checkpatch.pl tells you: "extern prototypes should be avoided in .h files"
>>>
>>> I have a very strong preference for extern on function decls, to match
>>> the extern on variable decl.
>>
>> You mean you also do 'static inline' variable declarations ?
> 
> That's a func definition, not a declaration. And you _can_ do static
> variable definitions in a header file just fine, although that's
> typically not what you'd want. Although sometimes I've seen people do:
> 
> static const int my_var = 10;
> 
> inline is an attribute that obviously doesn't work on variables.
> 
>> Using the extern keyword on function prototypes is superfluous visual
>> noise so suggest removing it.
> 
> I don't agree; and I think the C spec is actually wrong there (too).
> 
> The thing is that it distinguishes between a forward declaration of a
> function in the same TU and an external declaration for a function in
> another TU.
> 
> That is; if I see:
> 
> void ponies(int legs);
> 
> I expect that function to be defined later in the same TU. IOW it's a
> forward declaration. OTOH if I see:
> 
> extern void ponies(int legs);
> 
> I know I won't find it in this TU and the linker will end up involved.

Yes I can understand that for a .c file where you want to distinguish between forward declaration of 
functions defined in the file and functions declared outside. There, it is definitely an added value.

But in .h, all functions must be defined somewhere else, otherwise you have another problem. So all 
functions would have the 'extern' keyword according to your reasoning. Therefore that's just useless 
and I fully agree with Checkpatch's commit that in that case that's "superfluous visual noise" 
impeding readability and making it more difficult to fit the prototype on a single line.


> 
> Now, the C people figured that distinction was useless and allowed
> sloppiness. But I still think there's merrit to that. And as mentioned
> earlier, it is consistent with variable declarations.
> 

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Christophe Leroy]

Le 17/03/2021 à 21:09, Ingo Molnar a écrit :
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> Now, the C people figured that distinction was useless and allowed
>> sloppiness. But I still think there's merrit to that. And as
>> mentioned earlier, it is consistent with variable declarations.
> 
> Fully agreed, and my other point was that it's also consistent with
> the other existing externs were used *in the same header file*
> already.
> 
> I.e. there's nothing more sloppy than mixing different styles within
> the same header. Checkpatch needs to be fixed or ignored here.
> 

As pointed by He there are already many prototypes not flagged 'extern' in that header. Blaming the 
file shows that most remaining 'extern' are from the old days.

So adding new function prototypes with the 'extern' keywork wouldn't make the file less sloppy but 
would be a step backwards.

I think there is a will to make all those unneccessary 'extern' flags disappear on the long term. To 
achieve that there are two ways: Either smoothly do it on every function prototype modified or added 
or at a point in time big-bang convert the entire file. The later hinders blamability of the file.

Maybe one day we'll convert all remaining 'extern' away once they have become the minority. To 
achieve that we really need to not add new ones.

Christophe

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Christophe Leroy]

Le 18/03/2021 à 03:56, heying (H) a écrit :
> 
> 在 2021/3/18 4:09, Ingo Molnar 写道:
>> * Peter Zijlstra <peterz@infradead.org> wrote:
>>
>>> Now, the C people figured that distinction was useless and allowed
>>> sloppiness. But I still think there's merrit to that. And as
>>> mentioned earlier, it is consistent with variable declarations.
>> Fully agreed, and my other point was that it's also consistent with
>> the other existing externs were used *in the same header file*
>> already.
>>
>> I.e. there's nothing more sloppy than mixing different styles within
>> the same header. Checkpatch needs to be fixed or ignored here.
> 
> Thank you all for the reply!
> 
> There are already mixing different styles within linux/smp.h. I mean 'extern' and
> 
> non 'extern' func declarations both exist in this header. Since two of you three
> 
> think that 'extern' is needed, I'll add it and resend my patch.
> 
> 

As you are pointing, there are already non 'extern' func protoypes in the file, the conversion has 
already started, so flagging new prototypes with 'extern' would be a step backwards.

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[heying (H)]

Dear Ingo, Peter and Christophe,


I'm a bit confused. All of you have a good reason but have opposite 
opinions.

If I don't add 'extern', can you accept it? Please let me know.


Thanks,

He Ying

在 2021/3/18 13:53, Christophe Leroy 写道:
>
>
> Le 17/03/2021 à 18:37, Peter Zijlstra a écrit :
>> On Wed, Mar 17, 2021 at 06:17:26PM +0100, Christophe Leroy wrote:
>>>
>>>
>>> Le 17/03/2021 à 13:23, Peter Zijlstra a écrit :
>>>> On Wed, Mar 17, 2021 at 12:00:29PM +0100, Christophe Leroy wrote:
>>>>> What do you mean ? 'extern' prototype is pointless for function 
>>>>> prototypes
>>>>> and deprecated, no new function prototypes should be added with 
>>>>> the 'extern'
>>>>> keyword.
>>>>>
>>>>> checkpatch.pl tells you: "extern prototypes should be avoided in 
>>>>> .h files"
>>>>
>>>> I have a very strong preference for extern on function decls, to match
>>>> the extern on variable decl.
>>>
>>> You mean you also do 'static inline' variable declarations ?
>>
>> That's a func definition, not a declaration. And you _can_ do static
>> variable definitions in a header file just fine, although that's
>> typically not what you'd want. Although sometimes I've seen people do:
>>
>> static const int my_var = 10;
>>
>> inline is an attribute that obviously doesn't work on variables.
>>
>>> Using the extern keyword on function prototypes is superfluous visual
>>> noise so suggest removing it.
>>
>> I don't agree; and I think the C spec is actually wrong there (too).
>>
>> The thing is that it distinguishes between a forward declaration of a
>> function in the same TU and an external declaration for a function in
>> another TU.
>>
>> That is; if I see:
>>
>> void ponies(int legs);
>>
>> I expect that function to be defined later in the same TU. IOW it's a
>> forward declaration. OTOH if I see:
>>
>> extern void ponies(int legs);
>>
>> I know I won't find it in this TU and the linker will end up involved.
>
> Yes I can understand that for a .c file where you want to distinguish 
> between forward declaration of functions defined in the file and 
> functions declared outside. There, it is definitely an added value.
>
> But in .h, all functions must be defined somewhere else, otherwise you 
> have another problem. So all functions would have the 'extern' keyword 
> according to your reasoning. Therefore that's just useless and I fully 
> agree with Checkpatch's commit that in that case that's "superfluous 
> visual noise" impeding readability and making it more difficult to fit 
> the prototype on a single line.
>
>
>>
>> Now, the C people figured that distinction was useless and allowed
>> sloppiness. But I still think there's merrit to that. And as mentioned
>> earlier, it is consistent with variable declarations.
>>
> .

Re: [PATCH v2] smp: kernel/panic.c - silence warnings

[Christophe Leroy]

+Andrew

Le 19/03/2021 à 02:39, heying (H) a écrit :
> Dear Ingo, Peter and Christophe,
> 
> 
> I'm a bit confused. All of you have a good reason but have opposite opinions.

As far as I understand Ingo is willing to follow the existing "style" of the file.

In include/linux/smp.h we have:

- The following functions flagged 'extern':

	__smp_call_single_queue()
	smp_send_stop()
	smp_send_reschedule()
	smp_prepare_cpus()
	__cpu_up()
	smp_cpus_done()
	setup_nr_cpu_ids()
	smp_init()
	up_late_init()
	debug_smp_processor_id()
	arch_disable_smp_support()
	arch_thaw_secondary_cpus_begin()
	arch_thaw_secondary_cpus_end()

- The following functions not flagged 'extern':

	smp_call_function_single()
	on_each_cpu()
	on_each_cpu_mask()
	on_each_cpu_cond()
	on_each_cpu_cond_mask()
	smp_call_function_single_async()
	smp_call_function()
	smp_call_function_many()
	smp_call_function_any()
	kick_all_cpus_sync()
	wake_up_all_idle_cpus()
	call_function_init()
	generic_smp_call_function_single_interrupt()
	smp_prepare_boot_cpu()
	smp_setup_processor_id()
	smp_call_on_cpu()
	smpcfd_prepare_cpu()
	smpcfd_dead_cpu()
	smpcfd_dying_cpu()


Summary: 13 are 'extern' and 18 are _not_ 'extern'


> 
> If I don't add 'extern', can you accept it? Please let me know.

Who is going to merge that ? I'd expect it to be merged by Andrew as it is related to kernel/panic.c 
and most changes to kernel/panic.c are merged by Andrew.

Christophe

> 
> 
> Thanks,
> 
> He Ying
> 
> 在 2021/3/18 13:53, Christophe Leroy 写道:
>>
>>
>> Le 17/03/2021 à 18:37, Peter Zijlstra a écrit :
>>> On Wed, Mar 17, 2021 at 06:17:26PM +0100, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 17/03/2021 à 13:23, Peter Zijlstra a écrit :
>>>>> On Wed, Mar 17, 2021 at 12:00:29PM +0100, Christophe Leroy wrote:
>>>>>> What do you mean ? 'extern' prototype is pointless for function prototypes
>>>>>> and deprecated, no new function prototypes should be added with the 'extern'
>>>>>> keyword.
>>>>>>
>>>>>> checkpatch.pl tells you: "extern prototypes should be avoided in .h files"
>>>>>
>>>>> I have a very strong preference for extern on function decls, to match
>>>>> the extern on variable decl.
>>>>
>>>> You mean you also do 'static inline' variable declarations ?
>>>
>>> That's a func definition, not a declaration. And you _can_ do static
>>> variable definitions in a header file just fine, although that's
>>> typically not what you'd want. Although sometimes I've seen people do:
>>>
>>> static const int my_var = 10;
>>>
>>> inline is an attribute that obviously doesn't work on variables.
>>>
>>>> Using the extern keyword on function prototypes is superfluous visual
>>>> noise so suggest removing it.
>>>
>>> I don't agree; and I think the C spec is actually wrong there (too).
>>>
>>> The thing is that it distinguishes between a forward declaration of a
>>> function in the same TU and an external declaration for a function in
>>> another TU.
>>>
>>> That is; if I see:
>>>
>>> void ponies(int legs);
>>>
>>> I expect that function to be defined later in the same TU. IOW it's a
>>> forward declaration. OTOH if I see:
>>>
>>> extern void ponies(int legs);
>>>
>>> I know I won't find it in this TU and the linker will end up involved.
>>
>> Yes I can understand that for a .c file where you want to distinguish between forward declaration 
>> of functions defined in the file and functions declared outside. There, it is definitely an added 
>> value.
>>
>> But in .h, all functions must be defined somewhere else, otherwise you have another problem. So 
>> all functions would have the 'extern' keyword according to your reasoning. Therefore that's just 
>> useless and I fully agree with Checkpatch's commit that in that case that's "superfluous visual 
>> noise" impeding readability and making it more difficult to fit the prototype on a single line.
>>
>>
>>>
>>> Now, the C people figured that distinction was useless and allowed
>>> sloppiness. But I still think there's merrit to that. And as mentioned
>>> earlier, it is consistent with variable declarations.
>>>
>> .