[PATCH] x86/apic: Remove the extra judgement of skipped IO APIC setup

[PATCH] x86/apic: Remove the extra judgement of skipped IO APIC setup

[Dou Liyang]

As the commit 2e63ad4bd5dd ("x86/apic: Do not init irq remapping
if ioapic is disabled") added the judgement of skipped IO APIC
setup at the beginning of enable_IR_x2apic(). It may be redundant
that we check it again when we try to enable the interrupt mapping.

So, remove the one in try_to_enable_IR() and refine them for
better readability.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/apic/apic.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 8567c85..86e7bd8 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1610,24 +1610,15 @@ static inline void try_to_enable_x2apic(int remap_mode) { }
 static inline void __x2apic_enable(void) { }
 #endif /* !CONFIG_X86_X2APIC */
 
-static int __init try_to_enable_IR(void)
-{
-#ifdef CONFIG_X86_IO_APIC
-	if (!x2apic_enabled() && skip_ioapic_setup) {
-		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");
-		return -1;
-	}
-#endif
-	return irq_remapping_enable();
-}
-
 void __init enable_IR_x2apic(void)
 {
 	unsigned long flags;
 	int ret, ir_stat;
 
-	if (skip_ioapic_setup)
+	if (skip_ioapic_setup) {
+		pr_info("Not init interrupt remapping due to skipped IO-APIC setup\n");
 		return;
+	}
 
 	ir_stat = irq_remapping_prepare();
 	if (ir_stat < 0 && !x2apic_supported())
@@ -1645,7 +1636,7 @@ void __init enable_IR_x2apic(void)
 
 	/* If irq_remapping_prepare() succeeded, try to enable it */
 	if (ir_stat >= 0)
-		ir_stat = try_to_enable_IR();
+		ir_stat = irq_remapping_enable();
 	/* ir_stat contains the remap mode or an error code */
 	try_to_enable_x2apic(ir_stat);
 
-- 
2.5.5

Re: [PATCH] x86/apic: Remove the extra judgement of skipped IO APIC setup

[Ingo Molnar]

* Dou Liyang <douly.fnst@cn.fujitsu.com> wrote:

> As the commit 2e63ad4bd5dd ("x86/apic: Do not init irq remapping
> if ioapic is disabled") added the judgement of skipped IO APIC
> setup at the beginning of enable_IR_x2apic(). It may be redundant
> that we check it again when we try to enable the interrupt mapping.
> 
> So, remove the one in try_to_enable_IR() and refine them for
> better readability.
> 
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>  arch/x86/kernel/apic/apic.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 8567c85..86e7bd8 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1610,24 +1610,15 @@ static inline void try_to_enable_x2apic(int remap_mode) { }
>  static inline void __x2apic_enable(void) { }
>  #endif /* !CONFIG_X86_X2APIC */
>  
> -static int __init try_to_enable_IR(void)
> -{
> -#ifdef CONFIG_X86_IO_APIC
> -	if (!x2apic_enabled() && skip_ioapic_setup) {
> -		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");
> -		return -1;
> -	}
> -#endif
> -	return irq_remapping_enable();
> -}
> -
>  void __init enable_IR_x2apic(void)
>  {
>  	unsigned long flags;
>  	int ret, ir_stat;
>  
> -	if (skip_ioapic_setup)
> +	if (skip_ioapic_setup) {
> +		pr_info("Not init interrupt remapping due to skipped IO-APIC setup\n");

So you replaced a perfectly readable kernel message:

 -		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");

... with an unreadable one:

 +		pr_info("Not init interrupt remapping due to skipped IO-APIC setup\n");

Why?

Also, the changelog is pretty much unreadable as well:

> As the commit 2e63ad4bd5dd ("x86/apic: Do not init irq remapping
> if ioapic is disabled") added the judgement of skipped IO APIC
> setup at the beginning of enable_IR_x2apic(). It may be redundant
> that we check it again when we try to enable the interrupt mapping.
> 
> So, remove the one in try_to_enable_IR() and refine them for
> better readability.

I edited it to:

   The following commit:

     2e63ad4bd5dd ("x86/apic: Do not init irq remapping if ioapic is disabled") 

   ... added a check for skipped IO-APIC setup to enable_IR_x2apic(), but this 
   check is also duplicated in try_to_enable_IR() - and it will never succeed in 
   calling irq_remapping_enable().

   Remove the whole irq_remapping_enable() complication: if the IO-APIC is 
   disabled we cannot enable IRQ remapping.

And I restored the original pr_info() message as well.

Thanks,

	Ingo

[tip:x86/urgent] x86/apic: Simplify enable_IR_x2apic(), remove try_to_enable_IR()

[tip-bot for Dou Liyang]

Commit-ID:  11277aabcbbe13916151af897d29a5e9f71ca73f
Gitweb:     http://git.kernel.org/tip/11277aabcbbe13916151af897d29a5e9f71ca73f
Author:     Dou Liyang <douly.fnst@cn.fujitsu.com>
AuthorDate: Thu, 23 Feb 2017 17:16:41 +0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 1 Mar 2017 10:09:09 +0100

x86/apic: Simplify enable_IR_x2apic(), remove try_to_enable_IR()

The following commit:

  2e63ad4bd5dd ("x86/apic: Do not init irq remapping if ioapic is disabled")

... added a check for skipped IO-APIC setup to enable_IR_x2apic(), but this
check is also duplicated in try_to_enable_IR() - and it will never succeed in
calling irq_remapping_enable().

Remove the whole irq_remapping_enable() complication: if the IO-APIC is
disabled we cannot enable IRQ remapping.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: bp@alien8.de
Cc: nicstange@gmail.com
Cc: wanpeng.li@hotmail.com
Link: http://lkml.kernel.org/r/1487841401-1543-1-git-send-email-douly.fnst@cn.fujitsu.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/apic/apic.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 11088b8..aee7ded 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1610,24 +1610,15 @@ static inline void try_to_enable_x2apic(int remap_mode) { }
 static inline void __x2apic_enable(void) { }
 #endif /* !CONFIG_X86_X2APIC */
 
-static int __init try_to_enable_IR(void)
-{
-#ifdef CONFIG_X86_IO_APIC
-	if (!x2apic_enabled() && skip_ioapic_setup) {
-		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");
-		return -1;
-	}
-#endif
-	return irq_remapping_enable();
-}
-
 void __init enable_IR_x2apic(void)
 {
 	unsigned long flags;
 	int ret, ir_stat;
 
-	if (skip_ioapic_setup)
+	if (skip_ioapic_setup) {
+		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");
 		return;
+	}
 
 	ir_stat = irq_remapping_prepare();
 	if (ir_stat < 0 && !x2apic_supported())
@@ -1645,7 +1636,7 @@ void __init enable_IR_x2apic(void)
 
 	/* If irq_remapping_prepare() succeeded, try to enable it */
 	if (ir_stat >= 0)
-		ir_stat = try_to_enable_IR();
+		ir_stat = irq_remapping_enable();
 	/* ir_stat contains the remap mode or an error code */
 	try_to_enable_x2apic(ir_stat);
 

Re: [PATCH] x86/apic: Remove the extra judgement of skipped IO APIC setup

[Dou Liyang]

Dear Ingo,

At 03/01/2017 05:04 PM, Ingo Molnar wrote:
[...]
>> +		pr_info("Not init interrupt remapping due to skipped IO-APIC setup\n");
>
> So you replaced a perfectly readable kernel message:
>
>  -		pr_info("Not enabling interrupt remapping due to skipped IO-APIC setup\n");
>
> ... with an unreadable one:
>
>  +		pr_info("Not init interrupt remapping due to skipped IO-APIC setup\n");
>
> Why?

I am very sorry.

Because of my weak English skills :) . I am trying to improve my
English ability.

>
> Also, the changelog is pretty much unreadable as well:
>
>> As the commit 2e63ad4bd5dd ("x86/apic: Do not init irq remapping
>> if ioapic is disabled") added the judgement of skipped IO APIC
>> setup at the beginning of enable_IR_x2apic(). It may be redundant
>> that we check it again when we try to enable the interrupt mapping.
>>
>> So, remove the one in try_to_enable_IR() and refine them for
>> better readability.
>
> I edited it to:

Thanks very much ! it became very clear.

>
>    The following commit:
>
>      2e63ad4bd5dd ("x86/apic: Do not init irq remapping if ioapic is disabled")
>
>    ... added a check for skipped IO-APIC setup to enable_IR_x2apic(), but this

Could you tell me what is the meaning of "..." . How to use it?

>    check is also duplicated in try_to_enable_IR() - and it will never succeed in
>    calling irq_remapping_enable().
>
>    Remove the whole irq_remapping_enable() complication: if the IO-APIC is
>    disabled we cannot enable IRQ remapping.
>
> And I restored the original pr_info() message as well.

Yes. Thanks!

Sincerely,

	Liyang



>
> Thanks,
>
> 	Ingo
>
>
>