Re: [PATCH 1/1] X86: Probe for PIC and set legacy_pic appropriately

Re: [PATCH 1/1] X86: Probe for PIC and set legacy_pic appropriately

[H. Peter Anvin]

On 04/11/2014 04:02 PM, K. Y. Srinivasan wrote:
>  
> diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c
> index 2e977b5..0a57a19 100644
> --- a/arch/x86/kernel/i8259.c
> +++ b/arch/x86/kernel/i8259.c
> @@ -299,11 +299,22 @@ static void unmask_8259A(void)
>  static void init_8259A(int auto_eoi)
>  {
>  	unsigned long flags;
> +	unsigned char probe_val = 0xfb;
>  
>  	i8259A_auto_eoi = auto_eoi;
>  
>  	raw_spin_lock_irqsave(&i8259A_lock, flags);
>  
> +	/* Check to see if we have a PIC */
> +	outb(probe_val, PIC_MASTER_IMR);
> +	probe_val = inb(PIC_MASTER_IMR);
> +	if (probe_val == 0xff) {
> +		printk(KERN_INFO "Using NULL legacy PIC\n");
> +		legacy_pic = &null_legacy_pic;
> +		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
> +		return;
> +	}
> +
>  	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
>  	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */
>  

I think it is important to document what this actually means here.  0xfb
is "mask all except for the cascade."

Arguably, we should do this after masking the slave PIC.

On a whole other subject... the whole __byte() logic is a just plain
horrific piece of crap.  Clearly whomever wrote it had never heard of a
union.

I'm still horrifically confused, though, why we mask IRQ 2 (the cascade
interrupt) in i8259.c, but we explicitly do *not* mask IRQ 2 in
boot/pm.c (nor in the assembly code on which boot/pm.c was based.)  In
fact, it looks like we never unmask it.  I'm guessing that the masking
status of the cascade input simply doesn't matter.

At the very least, though, we shouldn't have 0xfb as a magic number, but
use ~(1U << PIC_CASCADE_IR).

	-hpa

RE: [PATCH 1/1] X86: Probe for PIC and set legacy_pic appropriately

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2410 bytes --]



> -----Original Message-----
> From: H. Peter Anvin [mailto:hpa@zytor.com]
> Sent: Friday, April 11, 2014 3:36 PM
> To: KY Srinivasan; x86@kernel.org; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com; tglx@linutronix.de;
> JBeulich@suse.com; bp@alien8.de
> Cc: Thomas Gleixner
> Subject: Re: [PATCH 1/1] X86: Probe for PIC and set legacy_pic appropriately
> 
> On 04/11/2014 04:02 PM, K. Y. Srinivasan wrote:
> >
> > diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c index
> > 2e977b5..0a57a19 100644
> > --- a/arch/x86/kernel/i8259.c
> > +++ b/arch/x86/kernel/i8259.c
> > @@ -299,11 +299,22 @@ static void unmask_8259A(void)  static void
> > init_8259A(int auto_eoi)  {
> >  	unsigned long flags;
> > +	unsigned char probe_val = 0xfb;
> >
> >  	i8259A_auto_eoi = auto_eoi;
> >
> >  	raw_spin_lock_irqsave(&i8259A_lock, flags);
> >
> > +	/* Check to see if we have a PIC */
> > +	outb(probe_val, PIC_MASTER_IMR);
> > +	probe_val = inb(PIC_MASTER_IMR);
> > +	if (probe_val == 0xff) {
> > +		printk(KERN_INFO "Using NULL legacy PIC\n");
> > +		legacy_pic = &null_legacy_pic;
> > +		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
> > +		return;
> > +	}
> > +
> >  	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
> >  	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */
> >
> 
> I think it is important to document what this actually means here.  0xfb is
> "mask all except for the cascade."
> 
> Arguably, we should do this after masking the slave PIC.
> 
> On a whole other subject... the whole __byte() logic is a just plain horrific
> piece of crap.  Clearly whomever wrote it had never heard of a union.
> 
> I'm still horrifically confused, though, why we mask IRQ 2 (the cascade
> interrupt) in i8259.c, but we explicitly do *not* mask IRQ 2 in boot/pm.c (nor
> in the assembly code on which boot/pm.c was based.)  In fact, it looks like we
> never unmask it.  I'm guessing that the masking status of the cascade input
> simply doesn't matter.
> 
> At the very least, though, we shouldn't have 0xfb as a magic number, but use
> ~(1U << PIC_CASCADE_IR).

I will get rid of this magic number as you have suggested.

K. Y
 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 1/1] X86: Probe for PIC and set legacy_pic appropriately

[K. Y. Srinivasan]

Probe for the existance of legacy PIC, if one does not exist, use the
null_legacy_pic.

This patch implements the proposal put forth by H. Peter Anvin <hpa@linux.intel.com>.


Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 arch/x86/kernel/cpu/mshyperv.c |    2 --
 arch/x86/kernel/i8259.c        |   11 +++++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 47359f7..9ca9587 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -133,8 +133,6 @@ static void __init ms_hyperv_init_platform(void)
 		printk(KERN_INFO "HyperV: LAPIC Timer Frequency: %#x\n",
 				lapic_timer_frequency);
 
-		printk(KERN_INFO "HyperV: Using null_legacy_pic\n");
-		legacy_pic = &null_legacy_pic;
 	}
 #endif
 
diff --git a/arch/x86/kernel/i8259.c b/arch/x86/kernel/i8259.c
index 2e977b5..0a57a19 100644
--- a/arch/x86/kernel/i8259.c
+++ b/arch/x86/kernel/i8259.c
@@ -299,11 +299,22 @@ static void unmask_8259A(void)
 static void init_8259A(int auto_eoi)
 {
 	unsigned long flags;
+	unsigned char probe_val = 0xfb;
 
 	i8259A_auto_eoi = auto_eoi;
 
 	raw_spin_lock_irqsave(&i8259A_lock, flags);
 
+	/* Check to see if we have a PIC */
+	outb(probe_val, PIC_MASTER_IMR);
+	probe_val = inb(PIC_MASTER_IMR);
+	if (probe_val == 0xff) {
+		printk(KERN_INFO "Using NULL legacy PIC\n");
+		legacy_pic = &null_legacy_pic;
+		raw_spin_unlock_irqrestore(&i8259A_lock, flags);
+		return;
+	}
+
 	outb(0xff, PIC_MASTER_IMR);	/* mask all of 8259A-1 */
 	outb(0xff, PIC_SLAVE_IMR);	/* mask all of 8259A-2 */
 
-- 
1.7.4.1