[PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Nicolas Iooss]

Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
shared_info is initialized") moved xen_init_time_ops() from __init to
__ref without updating xen-ops.h accordingly. Fix this.

Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
shared_info is initialized")
Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 arch/x86/xen/xen-ops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 0d5004477db6..b2a5d48a2c2a 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
 void xen_teardown_timer(int cpu);
 u64 xen_clocksource_read(void);
 void xen_setup_cpu_clockevents(void);
-void __init xen_init_time_ops(void);
+void __ref xen_init_time_ops(void);
 void __init xen_hvm_init_time_ops(void);
 
 irqreturn_t xen_debug_interrupt(int irq, void *dev_id);
-- 
2.14.1

Re: [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Juergen Gross]

On 03/09/17 10:38, Nicolas Iooss wrote:
> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
> shared_info is initialized") moved xen_init_time_ops() from __init to
> __ref without updating xen-ops.h accordingly. Fix this.
> 
> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
> shared_info is initialized")
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> ---
>  arch/x86/xen/xen-ops.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
> index 0d5004477db6..b2a5d48a2c2a 100644
> --- a/arch/x86/xen/xen-ops.h
> +++ b/arch/x86/xen/xen-ops.h
> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>  void xen_teardown_timer(int cpu);
>  u64 xen_clocksource_read(void);
>  void xen_setup_cpu_clockevents(void);
> -void __init xen_init_time_ops(void);
> +void __ref xen_init_time_ops(void);
>  void __init xen_hvm_init_time_ops(void);

When correcting this could you please modify the prototypes to comply to
the intended form as noted in include/linux/init.h (the __ref or __init
annotations should be just before the ending semicolon)?


Juergen

Re: [Xen-devel] [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Jan Beulich]

>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
> On 03/09/17 10:38, Nicolas Iooss wrote:
>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>> shared_info is initialized") moved xen_init_time_ops() from __init to
>> __ref without updating xen-ops.h accordingly. Fix this.
>> 
>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>> shared_info is initialized")
>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>> ---
>>  arch/x86/xen/xen-ops.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>> index 0d5004477db6..b2a5d48a2c2a 100644
>> --- a/arch/x86/xen/xen-ops.h
>> +++ b/arch/x86/xen/xen-ops.h
>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>  void xen_teardown_timer(int cpu);
>>  u64 xen_clocksource_read(void);
>>  void xen_setup_cpu_clockevents(void);
>> -void __init xen_init_time_ops(void);
>> +void __ref xen_init_time_ops(void);
>>  void __init xen_hvm_init_time_ops(void);
> 
> When correcting this could you please modify the prototypes to comply to
> the intended form as noted in include/linux/init.h (the __ref or __init
> annotations should be just before the ending semicolon)?

Why would these annotations be kept on the declarations anyway?
Attributes affecting code/data placement generally belong on the
definitions only.

I also question the suggested placement, despite init.h saying so.

Jan

Re: [Xen-devel] [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Andrew Cooper]

On 04/09/17 11:15, Jan Beulich wrote:
>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>
>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>> shared_info is initialized")
>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>> ---
>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>> --- a/arch/x86/xen/xen-ops.h
>>> +++ b/arch/x86/xen/xen-ops.h
>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>  void xen_teardown_timer(int cpu);
>>>  u64 xen_clocksource_read(void);
>>>  void xen_setup_cpu_clockevents(void);
>>> -void __init xen_init_time_ops(void);
>>> +void __ref xen_init_time_ops(void);
>>>  void __init xen_hvm_init_time_ops(void);
>> When correcting this could you please modify the prototypes to comply to
>> the intended form as noted in include/linux/init.h (the __ref or __init
>> annotations should be just before the ending semicolon)?
> Why would these annotations be kept on the declarations anyway?
> Attributes affecting code/data placement generally belong on the
> definitions only.

Because:

a) That’s what the coding style says, and

b) So various static analysis can be done (e.g. sparse) on an individual
translation unit basis.


Your objection to having annotations on declarations is why I've never
got around to adding sparse to the hypervisor build.

~Andrew

Re: [Xen-devel] [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Jan Beulich]

>>> On 04.09.17 at 12:35, <andrew.cooper3@citrix.com> wrote:
> On 04/09/17 11:15, Jan Beulich wrote:
>>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>>
>>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>> shared_info is initialized")
>>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>>> ---
>>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>>> --- a/arch/x86/xen/xen-ops.h
>>>> +++ b/arch/x86/xen/xen-ops.h
>>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>>  void xen_teardown_timer(int cpu);
>>>>  u64 xen_clocksource_read(void);
>>>>  void xen_setup_cpu_clockevents(void);
>>>> -void __init xen_init_time_ops(void);
>>>> +void __ref xen_init_time_ops(void);
>>>>  void __init xen_hvm_init_time_ops(void);
>>> When correcting this could you please modify the prototypes to comply to
>>> the intended form as noted in include/linux/init.h (the __ref or __init
>>> annotations should be just before the ending semicolon)?
>> Why would these annotations be kept on the declarations anyway?
>> Attributes affecting code/data placement generally belong on the
>> definitions only.
> 
> Because:
> 
> a) That’s what the coding style says, and
> 
> b) So various static analysis can be done (e.g. sparse) on an individual
> translation unit basis.
> 
> 
> Your objection to having annotations on declarations is why I've never
> got around to adding sparse to the hypervisor build.

Would you mind educating me what use these annotations can be
for static analysis? If they're useful in headers, I would stop
objecting to them being added there, but I'd then demand for them
to never be present on non-static definitions (unless there are
attributes where the compiler requires them to be repeated, but I
think all attributes are cumulative).

Jan

Re: [Xen-devel] [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Andrew Cooper]

On 04/09/17 13:41, Jan Beulich wrote:
>>>> On 04.09.17 at 12:35, <andrew.cooper3@citrix.com> wrote:
>> On 04/09/17 11:15, Jan Beulich wrote:
>>>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>>>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>>>
>>>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>> shared_info is initialized")
>>>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>>>> ---
>>>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>>>> --- a/arch/x86/xen/xen-ops.h
>>>>> +++ b/arch/x86/xen/xen-ops.h
>>>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>>>  void xen_teardown_timer(int cpu);
>>>>>  u64 xen_clocksource_read(void);
>>>>>  void xen_setup_cpu_clockevents(void);
>>>>> -void __init xen_init_time_ops(void);
>>>>> +void __ref xen_init_time_ops(void);
>>>>>  void __init xen_hvm_init_time_ops(void);
>>>> When correcting this could you please modify the prototypes to comply to
>>>> the intended form as noted in include/linux/init.h (the __ref or __init
>>>> annotations should be just before the ending semicolon)?
>>> Why would these annotations be kept on the declarations anyway?
>>> Attributes affecting code/data placement generally belong on the
>>> definitions only.
>> Because:
>>
>> a) That’s what the coding style says, and
>>
>> b) So various static analysis can be done (e.g. sparse) on an individual
>> translation unit basis.
>>
>>
>> Your objection to having annotations on declarations is why I've never
>> got around to adding sparse to the hypervisor build.
> Would you mind educating me what use these annotations can be
> for static analysis? If they're useful in headers, I would stop
> objecting to them being added there, but I'd then demand for them
> to never be present on non-static definitions (unless there are
> attributes where the compiler requires them to be repeated, but I
> think all attributes are cumulative).

For one, finding calls to __init functions from non __init functions.

~Andrew

Re: [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Jan Beulich]

>>> On 04.09.17 at 14:43, <andrew.cooper3@citrix.com> wrote:
> On 04/09/17 13:41, Jan Beulich wrote:
>>>>> On 04.09.17 at 12:35, <andrew.cooper3@citrix.com> wrote:
>>> On 04/09/17 11:15, Jan Beulich wrote:
>>>>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>>>>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>>>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>>>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>>>>
>>>>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>>> shared_info is initialized")
>>>>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>>>>> ---
>>>>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>>>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>>>>> --- a/arch/x86/xen/xen-ops.h
>>>>>> +++ b/arch/x86/xen/xen-ops.h
>>>>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>>>>  void xen_teardown_timer(int cpu);
>>>>>>  u64 xen_clocksource_read(void);
>>>>>>  void xen_setup_cpu_clockevents(void);
>>>>>> -void __init xen_init_time_ops(void);
>>>>>> +void __ref xen_init_time_ops(void);
>>>>>>  void __init xen_hvm_init_time_ops(void);
>>>>> When correcting this could you please modify the prototypes to comply to
>>>>> the intended form as noted in include/linux/init.h (the __ref or __init
>>>>> annotations should be just before the ending semicolon)?
>>>> Why would these annotations be kept on the declarations anyway?
>>>> Attributes affecting code/data placement generally belong on the
>>>> definitions only.
>>> Because:
>>>
>>> a) That’s what the coding style says, and
>>>
>>> b) So various static analysis can be done (e.g. sparse) on an individual
>>> translation unit basis.
>>>
>>>
>>> Your objection to having annotations on declarations is why I've never
>>> got around to adding sparse to the hypervisor build.
>> Would you mind educating me what use these annotations can be
>> for static analysis? If they're useful in headers, I would stop
>> objecting to them being added there, but I'd then demand for them
>> to never be present on non-static definitions (unless there are
>> attributes where the compiler requires them to be repeated, but I
>> think all attributes are cumulative).
> 
> For one, finding calls to __init functions from non __init functions.

Hmm, that's been solved differently on Linux, so I wouldn't have
assumed this is something to use sparse for.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Jan Beulich]

>>> On 04.09.17 at 14:43, <andrew.cooper3@citrix.com> wrote:
> On 04/09/17 13:41, Jan Beulich wrote:
>>>>> On 04.09.17 at 12:35, <andrew.cooper3@citrix.com> wrote:
>>> On 04/09/17 11:15, Jan Beulich wrote:
>>>>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>>>>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>>>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>>>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>>>>
>>>>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>>> shared_info is initialized")
>>>>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>>>>> ---
>>>>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>>>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>>>>> --- a/arch/x86/xen/xen-ops.h
>>>>>> +++ b/arch/x86/xen/xen-ops.h
>>>>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>>>>  void xen_teardown_timer(int cpu);
>>>>>>  u64 xen_clocksource_read(void);
>>>>>>  void xen_setup_cpu_clockevents(void);
>>>>>> -void __init xen_init_time_ops(void);
>>>>>> +void __ref xen_init_time_ops(void);
>>>>>>  void __init xen_hvm_init_time_ops(void);
>>>>> When correcting this could you please modify the prototypes to comply to
>>>>> the intended form as noted in include/linux/init.h (the __ref or __init
>>>>> annotations should be just before the ending semicolon)?
>>>> Why would these annotations be kept on the declarations anyway?
>>>> Attributes affecting code/data placement generally belong on the
>>>> definitions only.
>>> Because:
>>>
>>> a) That’s what the coding style says, and
>>>
>>> b) So various static analysis can be done (e.g. sparse) on an individual
>>> translation unit basis.
>>>
>>>
>>> Your objection to having annotations on declarations is why I've never
>>> got around to adding sparse to the hypervisor build.
>> Would you mind educating me what use these annotations can be
>> for static analysis? If they're useful in headers, I would stop
>> objecting to them being added there, but I'd then demand for them
>> to never be present on non-static definitions (unless there are
>> attributes where the compiler requires them to be repeated, but I
>> think all attributes are cumulative).
> 
> For one, finding calls to __init functions from non __init functions.

Hmm, that's been solved differently on Linux, so I wouldn't have
assumed this is something to use sparse for.

Jan

Re: [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Andrew Cooper]

On 04/09/17 13:41, Jan Beulich wrote:
>>>> On 04.09.17 at 12:35, <andrew.cooper3@citrix.com> wrote:
>> On 04/09/17 11:15, Jan Beulich wrote:
>>>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>>>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>>>
>>>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>>> shared_info is initialized")
>>>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>>>> ---
>>>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>>>> --- a/arch/x86/xen/xen-ops.h
>>>>> +++ b/arch/x86/xen/xen-ops.h
>>>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>>>  void xen_teardown_timer(int cpu);
>>>>>  u64 xen_clocksource_read(void);
>>>>>  void xen_setup_cpu_clockevents(void);
>>>>> -void __init xen_init_time_ops(void);
>>>>> +void __ref xen_init_time_ops(void);
>>>>>  void __init xen_hvm_init_time_ops(void);
>>>> When correcting this could you please modify the prototypes to comply to
>>>> the intended form as noted in include/linux/init.h (the __ref or __init
>>>> annotations should be just before the ending semicolon)?
>>> Why would these annotations be kept on the declarations anyway?
>>> Attributes affecting code/data placement generally belong on the
>>> definitions only.
>> Because:
>>
>> a) That’s what the coding style says, and
>>
>> b) So various static analysis can be done (e.g. sparse) on an individual
>> translation unit basis.
>>
>>
>> Your objection to having annotations on declarations is why I've never
>> got around to adding sparse to the hypervisor build.
> Would you mind educating me what use these annotations can be
> for static analysis? If they're useful in headers, I would stop
> objecting to them being added there, but I'd then demand for them
> to never be present on non-static definitions (unless there are
> attributes where the compiler requires them to be repeated, but I
> think all attributes are cumulative).

For one, finding calls to __init functions from non __init functions.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Jan Beulich]

>>> On 04.09.17 at 12:35, <andrew.cooper3@citrix.com> wrote:
> On 04/09/17 11:15, Jan Beulich wrote:
>>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>>
>>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>>> shared_info is initialized")
>>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>>> ---
>>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>>> --- a/arch/x86/xen/xen-ops.h
>>>> +++ b/arch/x86/xen/xen-ops.h
>>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>>  void xen_teardown_timer(int cpu);
>>>>  u64 xen_clocksource_read(void);
>>>>  void xen_setup_cpu_clockevents(void);
>>>> -void __init xen_init_time_ops(void);
>>>> +void __ref xen_init_time_ops(void);
>>>>  void __init xen_hvm_init_time_ops(void);
>>> When correcting this could you please modify the prototypes to comply to
>>> the intended form as noted in include/linux/init.h (the __ref or __init
>>> annotations should be just before the ending semicolon)?
>> Why would these annotations be kept on the declarations anyway?
>> Attributes affecting code/data placement generally belong on the
>> definitions only.
> 
> Because:
> 
> a) That’s what the coding style says, and
> 
> b) So various static analysis can be done (e.g. sparse) on an individual
> translation unit basis.
> 
> 
> Your objection to having annotations on declarations is why I've never
> got around to adding sparse to the hypervisor build.

Would you mind educating me what use these annotations can be
for static analysis? If they're useful in headers, I would stop
objecting to them being added there, but I'd then demand for them
to never be present on non-static definitions (unless there are
attributes where the compiler requires them to be repeated, but I
think all attributes are cumulative).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Andrew Cooper]

On 04/09/17 11:15, Jan Beulich wrote:
>>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
>> On 03/09/17 10:38, Nicolas Iooss wrote:
>>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>> shared_info is initialized") moved xen_init_time_ops() from __init to
>>> __ref without updating xen-ops.h accordingly. Fix this.
>>>
>>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>>> shared_info is initialized")
>>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>>> ---
>>>  arch/x86/xen/xen-ops.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>>> index 0d5004477db6..b2a5d48a2c2a 100644
>>> --- a/arch/x86/xen/xen-ops.h
>>> +++ b/arch/x86/xen/xen-ops.h
>>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>>  void xen_teardown_timer(int cpu);
>>>  u64 xen_clocksource_read(void);
>>>  void xen_setup_cpu_clockevents(void);
>>> -void __init xen_init_time_ops(void);
>>> +void __ref xen_init_time_ops(void);
>>>  void __init xen_hvm_init_time_ops(void);
>> When correcting this could you please modify the prototypes to comply to
>> the intended form as noted in include/linux/init.h (the __ref or __init
>> annotations should be just before the ending semicolon)?
> Why would these annotations be kept on the declarations anyway?
> Attributes affecting code/data placement generally belong on the
> definitions only.

Because:

a) That’s what the coding style says, and

b) So various static analysis can be done (e.g. sparse) on an individual
translation unit basis.


Your objection to having annotations on declarations is why I've never
got around to adding sparse to the hypervisor build.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Jan Beulich]

>>> On 04.09.17 at 10:17, <jgross@suse.com> wrote:
> On 03/09/17 10:38, Nicolas Iooss wrote:
>> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>> shared_info is initialized") moved xen_init_time_ops() from __init to
>> __ref without updating xen-ops.h accordingly. Fix this.
>> 
>> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
>> shared_info is initialized")
>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
>> ---
>>  arch/x86/xen/xen-ops.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
>> index 0d5004477db6..b2a5d48a2c2a 100644
>> --- a/arch/x86/xen/xen-ops.h
>> +++ b/arch/x86/xen/xen-ops.h
>> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>>  void xen_teardown_timer(int cpu);
>>  u64 xen_clocksource_read(void);
>>  void xen_setup_cpu_clockevents(void);
>> -void __init xen_init_time_ops(void);
>> +void __ref xen_init_time_ops(void);
>>  void __init xen_hvm_init_time_ops(void);
> 
> When correcting this could you please modify the prototypes to comply to
> the intended form as noted in include/linux/init.h (the __ref or __init
> annotations should be just before the ending semicolon)?

Why would these annotations be kept on the declarations anyway?
Attributes affecting code/data placement generally belong on the
definitions only.

I also question the suggested placement, despite init.h saying so.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Juergen Gross]

On 03/09/17 10:38, Nicolas Iooss wrote:
> Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
> shared_info is initialized") moved xen_init_time_ops() from __init to
> __ref without updating xen-ops.h accordingly. Fix this.
> 
> Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
> shared_info is initialized")
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> ---
>  arch/x86/xen/xen-ops.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
> index 0d5004477db6..b2a5d48a2c2a 100644
> --- a/arch/x86/xen/xen-ops.h
> +++ b/arch/x86/xen/xen-ops.h
> @@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
>  void xen_teardown_timer(int cpu);
>  u64 xen_clocksource_read(void);
>  void xen_setup_cpu_clockevents(void);
> -void __init xen_init_time_ops(void);
> +void __ref xen_init_time_ops(void);
>  void __init xen_hvm_init_time_ops(void);

When correcting this could you please modify the prototypes to comply to
the intended form as noted in include/linux/init.h (the __ref or __init
annotations should be just before the ending semicolon)?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/1] x86/xen: fix section of xen_init_time_ops() in header

[Nicolas Iooss]

Commit d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
shared_info is initialized") moved xen_init_time_ops() from __init to
__ref without updating xen-ops.h accordingly. Fix this.

Fixes: d162809f85b4 ("xen/x86: Do not call xen_init_time_ops() until
shared_info is initialized")
Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 arch/x86/xen/xen-ops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 0d5004477db6..b2a5d48a2c2a 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -69,7 +69,7 @@ void xen_setup_runstate_info(int cpu);
 void xen_teardown_timer(int cpu);
 u64 xen_clocksource_read(void);
 void xen_setup_cpu_clockevents(void);
-void __init xen_init_time_ops(void);
+void __ref xen_init_time_ops(void);
 void __init xen_hvm_init_time_ops(void);
 
 irqreturn_t xen_debug_interrupt(int irq, void *dev_id);
-- 
2.14.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel