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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <df6b50af-a67c-ae31-ebf8-e37e70fe5ef5@csgroup.eu>
Date:   Fri, 19 Mar 2021 07:58:12 +0100
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     "heying (H)" <heying24@...wei.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Ingo Molnar <mingo@...nel.org>, frederic@...nel.org,
        paulmck@...nel.org, clg@...d.org, qais.yousef@....com,
        johnny.chenyi@...wei.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] smp: kernel/panic.c - silence warnings

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