Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1350103 > unrolled thread

[PATCH] klp: remove superfluous errors in asm/livepatch.h

Back to article view | Back to linux.kernel

Contents

  [PATCH] klp: remove superfluous errors in asm/livepatch.h Miroslav Benes <mbenes@suse.cz> - 2016-03-04 11:00 +0100
    Re: [PATCH] klp: remove superfluous errors in asm/livepatch.h Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-04 22:50 +0100
      Re: [PATCH] klp: remove superfluous errors in asm/livepatch.h Jiri Kosina <jikos@kernel.org> - 2016-03-06 22:20 +0100
        Re: [PATCH] klp: remove superfluous errors in asm/livepatch.h Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-07 05:20 +0100
        Re: klp: remove superfluous errors in asm/livepatch.h Jessica Yu <jeyu@redhat.com> - 2016-03-08 22:30 +0100
          Re: klp: remove superfluous errors in asm/livepatch.h Petr Mladek <pmladek@suse.com> - 2016-03-09 11:10 +0100
          Re: klp: remove superfluous errors in asm/livepatch.h Jiri Kosina <jikos@kernel.org> - 2016-03-09 11:40 +0100
            Re: klp: remove superfluous errors in asm/livepatch.h Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-09 17:20 +0100
      Re: [PATCH] klp: remove superfluous errors in asm/livepatch.h Miroslav Benes <mbenes@suse.cz> - 2016-03-15 12:40 +0100

#1350103 — [PATCH] klp: remove superfluous errors in asm/livepatch.h

There is an #error in asm/livepatch.h for both x86 and s390 in
!CONFIG_LIVEPATCH cases. It does not make much sense as pointed out by
Michael Ellerman. One can happily include asm/livepatch.h with
CONFIG_LIVEPATCH. Remove it as useless.

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 arch/s390/include/asm/livepatch.h | 2 --
 arch/x86/include/asm/livepatch.h  | 2 --
 2 files changed, 4 deletions(-)

diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h
index a52b6cca873d..105074582d86 100644
--- a/arch/s390/include/asm/livepatch.h
+++ b/arch/s390/include/asm/livepatch.h
@@ -36,8 +36,6 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
 {
 	regs->psw.addr = ip;
 }
-#else
-#error Include linux/livepatch.h, not asm/livepatch.h
 #endif
 
 #endif
diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index e795f5274217..8acfe798625b 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -40,8 +40,6 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
 {
 	regs->ip = ip;
 }
-#else
-#error Include linux/livepatch.h, not asm/livepatch.h
 #endif
 
 #endif /* _ASM_X86_LIVEPATCH_H */
-- 
2.7.2

[toc] | [next] | [standalone]

#1350630

On Fri, Mar 04, 2016 at 10:53:39AM +0100, Miroslav Benes wrote:
> There is an #error in asm/livepatch.h for both x86 and s390 in
> !CONFIG_LIVEPATCH cases. It does not make much sense as pointed out by
> Michael Ellerman. One can happily include asm/livepatch.h with
> CONFIG_LIVEPATCH. Remove it as useless.
> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Looks fine to me.  While we're at it, do we even need the '#ifdef
CONFIG_LIVEPATCH' in these files?  And in include/linux/livepatch.h?

> ---
>  arch/s390/include/asm/livepatch.h | 2 --
>  arch/x86/include/asm/livepatch.h  | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/livepatch.h b/arch/s390/include/asm/livepatch.h
> index a52b6cca873d..105074582d86 100644
> --- a/arch/s390/include/asm/livepatch.h
> +++ b/arch/s390/include/asm/livepatch.h
> @@ -36,8 +36,6 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>  {
>  	regs->psw.addr = ip;
>  }
> -#else
> -#error Include linux/livepatch.h, not asm/livepatch.h
>  #endif
>  
>  #endif
> diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
> index e795f5274217..8acfe798625b 100644
> --- a/arch/x86/include/asm/livepatch.h
> +++ b/arch/x86/include/asm/livepatch.h
> @@ -40,8 +40,6 @@ static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip)
>  {
>  	regs->ip = ip;
>  }
> -#else
> -#error Include linux/livepatch.h, not asm/livepatch.h
>  #endif
>  
>  #endif /* _ASM_X86_LIVEPATCH_H */
> -- 
> 2.7.2
> 

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1351203

On Fri, 4 Mar 2016, Josh Poimboeuf wrote:

> > There is an #error in asm/livepatch.h for both x86 and s390 in
> > !CONFIG_LIVEPATCH cases. It does not make much sense as pointed out by
> > Michael Ellerman. One can happily include asm/livepatch.h with
> > CONFIG_LIVEPATCH. Remove it as useless.
> > 
> > Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> Looks fine to me.  

Thanks. I consider this to be your Ack then :) (if you disagree, please 
shout loudly).

> While we're at it, do we even need the '#ifdef CONFIG_LIVEPATCH' in 
> these files?  And in include/linux/livepatch.h?

You are right, this seems indeed completely unnecessary. I'll remove it 
for 4.6 as well, if noone has any objections.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1351305

On Sun, Mar 06, 2016 at 10:13:56PM +0100, Jiri Kosina wrote:
> On Fri, 4 Mar 2016, Josh Poimboeuf wrote:
> 
> > > There is an #error in asm/livepatch.h for both x86 and s390 in
> > > !CONFIG_LIVEPATCH cases. It does not make much sense as pointed out by
> > > Michael Ellerman. One can happily include asm/livepatch.h with
> > > CONFIG_LIVEPATCH. Remove it as useless.
> > > 
> > > Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> > > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> > 
> > Looks fine to me.  
> 
> Thanks. I consider this to be your Ack then :) (if you disagree, please 
> shout loudly).

:-)

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

> 
> > While we're at it, do we even need the '#ifdef CONFIG_LIVEPATCH' in 
> > these files?  And in include/linux/livepatch.h?
> 
> You are right, this seems indeed completely unnecessary. I'll remove it 
> for 4.6 as well, if noone has any objections.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1353420 — Re: klp: remove superfluous errors in asm/livepatch.h

+++ Jiri Kosina [06/03/16 22:13 +0100]:
>On Fri, 4 Mar 2016, Josh Poimboeuf wrote:
>
>> > There is an #error in asm/livepatch.h for both x86 and s390 in
>> > !CONFIG_LIVEPATCH cases. It does not make much sense as pointed out by
>> > Michael Ellerman. One can happily include asm/livepatch.h with
>> > CONFIG_LIVEPATCH. Remove it as useless.
>> >
>> > Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>>
>> Looks fine to me.
>
>Thanks. I consider this to be your Ack then :) (if you disagree, please
>shout loudly).
>
>> While we're at it, do we even need the '#ifdef CONFIG_LIVEPATCH' in
>> these files?  And in include/linux/livepatch.h?
>
>You are right, this seems indeed completely unnecessary. I'll remove it
>for 4.6 as well, if noone has any objections.

Hm, I should've caught this earlier, but the notifier cleanup patch
that removes the livepatch module notifier had kernel/module.c include
livepatch.h for the klp_module_{coming,going} function stubs in the
!CONFIG_LIVEPATCH case. See here: https://lkml.org/lkml/2016/2/8/1182

Looking back, I now don't think it makes sense for module.c to include
all those livepatch definitions in the first place, since all it
needed was the klp_module_{coming,going} declarations. I guess my
question is, since we've removed the #ifdef CONFIG_LIVEPATCH blocks
from livepatch.h, where might be a better place for the
klp_module_{coming,going} stubs? Perhaps they could go in module.h
instead?

Jessica

[toc] | [prev] | [next] | [standalone]

#1353935 — Re: klp: remove superfluous errors in asm/livepatch.h

On Tue 2016-03-08 16:28:20, Jessica Yu wrote:
> >On Fri, 4 Mar 2016, Josh Poimboeuf wrote:
> >>While we're at it, do we even need the '#ifdef CONFIG_LIVEPATCH' in
> >>these files?  And in include/linux/livepatch.h?
> >
> >You are right, this seems indeed completely unnecessary. I'll remove it
> >for 4.6 as well, if noone has any objections.
> 
> Hm, I should've caught this earlier, but the notifier cleanup patch
> that removes the livepatch module notifier had kernel/module.c include
> livepatch.h for the klp_module_{coming,going} function stubs in the
> !CONFIG_LIVEPATCH case. See here: https://lkml.org/lkml/2016/2/8/1182

Great catch! And you are in time because Mirek has vacation this week :-)

> Looking back, I now don't think it makes sense for module.c to include
> all those livepatch definitions in the first place, since all it
> needed was the klp_module_{coming,going} declarations. I guess my
> question is, since we've removed the #ifdef CONFIG_LIVEPATCH blocks
> from livepatch.h, where might be a better place for the
> klp_module_{coming,going} stubs? Perhaps they could go in module.h
> instead?

I personally prefer to keep the klp_module_{coming,going} declarations
in livepatch.h. It means to keep the '#ifdef CONFIG_LIVEPATCH' there
as well. It is a standard solution. It helps to find things where
one would expect them.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1353973 — Re: klp: remove superfluous errors in asm/livepatch.h

On Tue, 8 Mar 2016, Jessica Yu wrote:

> Hm, I should've caught this earlier, but the notifier cleanup patch
> that removes the livepatch module notifier had kernel/module.c include
> livepatch.h for the klp_module_{coming,going} function stubs in the
> !CONFIG_LIVEPATCH case. See here: https://lkml.org/lkml/2016/2/8/1182
> 
> Looking back, I now don't think it makes sense for module.c to include
> all those livepatch definitions in the first place, since all it
> needed was the klp_module_{coming,going} declarations. I guess my
> question is, since we've removed the #ifdef CONFIG_LIVEPATCH blocks
> from livepatch.h, where might be a better place for the
> klp_module_{coming,going} stubs? Perhaps they could go in module.h
> instead?

Well, once there actually are alternate stubs to be included through the 
header file (like in the proposed notifier removal), it indeed makes sense 
to reintroduce the #ifdef. And once it's there, it probably doesn't make 
too much sense to have it guard only portion of the file.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1354240 — Re: klp: remove superfluous errors in asm/livepatch.h

On Wed, Mar 09, 2016 at 11:39:05AM +0100, Jiri Kosina wrote:
> On Tue, 8 Mar 2016, Jessica Yu wrote:
> 
> > Hm, I should've caught this earlier, but the notifier cleanup patch
> > that removes the livepatch module notifier had kernel/module.c include
> > livepatch.h for the klp_module_{coming,going} function stubs in the
> > !CONFIG_LIVEPATCH case. See here: https://lkml.org/lkml/2016/2/8/1182
> > 
> > Looking back, I now don't think it makes sense for module.c to include
> > all those livepatch definitions in the first place, since all it
> > needed was the klp_module_{coming,going} declarations. I guess my
> > question is, since we've removed the #ifdef CONFIG_LIVEPATCH blocks
> > from livepatch.h, where might be a better place for the
> > klp_module_{coming,going} stubs? Perhaps they could go in module.h
> > instead?
> 
> Well, once there actually are alternate stubs to be included through the 
> header file (like in the proposed notifier removal), it indeed makes sense 
> to reintroduce the #ifdef. And once it's there, it probably doesn't make 
> too much sense to have it guard only portion of the file.

It's a minor issue but I think it would be cleaner if we only guard the
parts of the file which need to be guarded.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1357999

On Fri, 4 Mar 2016, Josh Poimboeuf wrote:

> On Fri, Mar 04, 2016 at 10:53:39AM +0100, Miroslav Benes wrote:
> > There is an #error in asm/livepatch.h for both x86 and s390 in
> > !CONFIG_LIVEPATCH cases. It does not make much sense as pointed out by
> > Michael Ellerman. One can happily include asm/livepatch.h with
> > CONFIG_LIVEPATCH. Remove it as useless.
> > 
> > Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> Looks fine to me.  While we're at it, do we even need the '#ifdef
> CONFIG_LIVEPATCH' in these files?  And in include/linux/livepatch.h?

IIRC there was a reason to this. If you build a patch module on (against) 
the kernel with CONFIG_LIVEPATCH=n the build fails immediately with 
those ifdefs, because there is no declaration of needed symbols present 
(klp_register_patch(), data structures and such). Without ifdefs the build 
succeeds and modprobe/insmod fails. The former could be advantageous in 
some scenarios. 

But since the guard in include/linux/livepatch.h (which is the only 
important header file in fact) returns with Jessica's patches for better 
reason there is no point to discuss this further :)

Miroslav

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web