Date: Wed, 1 Mar 2017 10:04:55 +0100
From: Ingo Molnar <mingo@kernel.org>
To: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: tglx@linutronix.de, hpa@zytor.com, bp@alien8.de,
        wanpeng.li@hotmail.com, nicstange@gmail.com, x86@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/apic: Remove the extra judgement of skipped IO APIC
 setup
Message-ID: <20170301090455.GA23669@gmail.com>
References: <1487841401-1543-1-git-send-email-douly.fnst@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1487841401-1543-1-git-send-email-douly.fnst@cn.fujitsu.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2622
Lines: 82


* 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