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

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

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

[Ingo Molnar]

* Dou Liyang  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 
> ---
>  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

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

[Ingo Molnar]

* Dou Liyang  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 
> ---
>  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

[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 
---
 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

[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 
---
 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