KDB blindly reads keyboard port

KDB blindly reads keyboard port

[Bjorn Helgaas]

get_kbd_char() in arch/ia64/kdb/kdba_io.c does "inb(KBD_STATUS_REG)".

But we don't know whether there's even an i8042 keyboard controller
present.  On HP ia64 boxes, there is no i8042, and trying to read
from it can cause an MCA.

This depends on the specific platform and how it is configured.  I
observed this MCA while booting the SLES10 install kernel on an
HP rx7620 in "default" acpiconfig mode.  The supported acpiconfig
mode on this box is "single-pci-domain", which also puts some
legacy ports into "soft-fail" mode, where the read will just return
0xff instead of causing an MCA.  But I think it's wrong to blindly
poke around in I/O port space.

i8042_pnp_init() uses PNPACPI to figure out whether the i8042
device is present.  That's probably too heavy-weight for what
you want to do in KDB, though.

Bjorn

Re: KDB blindly reads keyboard port

[Keith Owens]

Bjorn Helgaas (on Tue, 26 Sep 2006 13:54:30 -0600) wrote:
>get_kbd_char() in arch/ia64/kdb/kdba_io.c does "inb(KBD_STATUS_REG)".
>
>But we don't know whether there's even an i8042 keyboard controller
>present.  On HP ia64 boxes, there is no i8042, and trying to read
>from it can cause an MCA.
>
>This depends on the specific platform and how it is configured.  I
>observed this MCA while booting the SLES10 install kernel on an
>HP rx7620 in "default" acpiconfig mode.  The supported acpiconfig
>mode on this box is "single-pci-domain", which also puts some
>legacy ports into "soft-fail" mode, where the read will just return
>0xff instead of causing an MCA.  But I think it's wrong to blindly
>poke around in I/O port space.

No support for legacy I/O ports could be a bigger problem than just
KDB.  To fix just KDB, apply this patch over kdb-v4.4-2.6.18-common-1 and add
'kdb_skip_keyboard' to the boot command line on the offending hardware.

---
 arch/ia64/kdb/kdba_io.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Index: linux/arch/ia64/kdb/kdba_io.c
===================================================================
--- linux.orig/arch/ia64/kdb/kdba_io.c
+++ linux/arch/ia64/kdb/kdba_io.c
@@ -38,6 +38,7 @@
 #else
 #undef	KDB_BLINK_LED
 #endif
+static int kdb_skip_keyboard;
 
 #ifdef CONFIG_KDB_USB
 struct kdb_usb_exchange kdb_usb_infos;
@@ -334,7 +335,8 @@ static int get_kbd_char(void)
 		if (kbd_exists == 0)
 			return -1;
 
-		if (inb(KBD_STATUS_REG) == 0xff && inb(KBD_DATA_REG) == 0xff) {
+		if (kdb_skip_keyboard ||
+		    (inb(KBD_STATUS_REG) == 0xff && inb(KBD_DATA_REG) == 0xff)) {
 			kbd_exists = 0;
 			return -1;
 		}
@@ -561,3 +563,14 @@ get_char_func poll_funcs[] = {
 
 void kdba_local_arch_setup(void) {}
 void kdba_local_arch_cleanup(void) {}
+
+/* Some hardware gets an MCA instead of returning 0xff when we read
+ * KBD_STATUS_REG.  If these systems boot a kernel with CONFIG_VT=y then they
+ * need to add 'kdb_skip_keyboard' to the boot line.
+ */
+static int __init kdb_skip_keyboard_setup(char * str)
+{
+	kdb_skip_keyboard = 1;
+	return 1;
+}
+__setup("kdb_skip_keyboard", kdb_skip_keyboard_setup);

Re: KDB blindly reads keyboard port

[Matthew Wilcox]

On Wed, Sep 27, 2006 at 12:45:50PM +1000, Keith Owens wrote:
> No support for legacy I/O ports could be a bigger problem than just
> KDB.  To fix just KDB, apply this patch over kdb-v4.4-2.6.18-common-1 and add
> 'kdb_skip_keyboard' to the boot command line on the offending hardware.

Why can't you use acpi_kbd_controller_present instead of introducing a
new option?

Re: KDB blindly reads keyboard port

[Bjorn Helgaas]

On Tuesday 26 September 2006 20:45, Keith Owens wrote:
> No support for legacy I/O ports could be a bigger problem than just
> KDB.

On Itanium (and I suppose on x86), ACPI theoretically tells us enough
that we don't need to assume any legacy resources.  Of course, Linux
doesn't listen to everything ACPI is trying to tell it.  But that's
a Linux deficiency we should remedy.

> To fix just KDB, apply this patch over kdb-v4.4-2.6.18-common-1 and
> add 'kdb_skip_keyboard' to the boot command line on the offending
> hardware. 

This doesn't feel like the right solution.  Since firmware tells us
whether the device is present, I think we should rely on that.  If
you want to use the device before ACPI is initialized, *then* you
should pass a "kdb_use_keyboard" sort of flag.

Bjorn

Re: KDB blindly reads keyboard port

[Keith Owens]

Bjorn Helgaas (on Wed, 27 Sep 2006 16:11:00 -0600) wrote:
>On Tuesday 26 September 2006 20:45, Keith Owens wrote:
>> No support for legacy I/O ports could be a bigger problem than just
>> KDB.
>
>On Itanium (and I suppose on x86), ACPI theoretically tells us enough
>that we don't need to assume any legacy resources.  Of course, Linux
>doesn't listen to everything ACPI is trying to tell it.  But that's
>a Linux deficiency we should remedy.
>
>> To fix just KDB, apply this patch over kdb-v4.4-2.6.18-common-1 and
>> add 'kdb_skip_keyboard' to the boot command line on the offending
>> hardware. 
>
>This doesn't feel like the right solution.  Since firmware tells us
>whether the device is present, I think we should rely on that.  If
>you want to use the device before ACPI is initialized, *then* you
>should pass a "kdb_use_keyboard" sort of flag.

I have never been a big fan of ACPI, having seen too many broken ACPI
tables.  But if that is what you want ...

Bjorn, could you apply my previous patch anyway, boot your problem
system with kdb_skip_keyboard, drop into KDB and
'md4c1 acpi_kbd_controller_present'.  That will quickly confirm if acpi
is detecting the absence of the keyboard on your system.

Re: KDB blindly reads keyboard port

[Bjorn Helgaas]

On Thursday 28 September 2006 20:18, Keith Owens wrote:
> I have never been a big fan of ACPI, having seen too many broken ACPI
> tables.  But if that is what you want ...

There's always broken firmware, but I think on the whole, it's better
than just assuming that tomorrow's system will be the same as yesterday's.
I think a big reason for broken tables is the fact that ignore many of
them, so the breakage is never discovered.

> Bjorn, could you apply my previous patch anyway, boot your problem
> system with kdb_skip_keyboard, drop into KDB and
> 'md4c1 acpi_kbd_controller_present'.  That will quickly confirm if acpi
> is detecting the absence of the keyboard on your system.

My system says:

  acpi_parse_fadt: acpi_kbd_controller_present 0

Re: KDB blindly reads keyboard port

[Luck, Tony]

On Fri, Sep 29, 2006 at 10:57:41AM -0600, Bjorn Helgaas wrote:
>   acpi_parse_fadt: acpi_kbd_controller_present 0

The logic in the kernel seems backwards here though.  We start
by assuming there is a keyboard, then when parsing the FADT
we reset this assumption if the BAF_8042_KEYBOARD_CONTROLLER
bit isn't set.  Which in turn forced SGI to include some
workaround code for their older PROM (which doesn't provide
the FADT table).

There's also a risk that if some code might get added that
runs before we parse FADT that could be confused into thinking
that the keyboard is present.

Wouldn't it be simpler/better to assume there is no keyboard until
we find positive evidence that there is one?

-Tony

Re: KDB blindly reads keyboard port

[Bjorn Helgaas]

On Friday 29 September 2006 12:01, Luck, Tony wrote:
> On Fri, Sep 29, 2006 at 10:57:41AM -0600, Bjorn Helgaas wrote:
> >   acpi_parse_fadt: acpi_kbd_controller_present 0
> 
> The logic in the kernel seems backwards here though.  We start
> by assuming there is a keyboard, then when parsing the FADT
> we reset this assumption if the BAF_8042_KEYBOARD_CONTROLLER
> bit isn't set.  Which in turn forced SGI to include some
> workaround code for their older PROM (which doesn't provide
> the FADT table).
> 
> There's also a risk that if some code might get added that
> runs before we parse FADT that could be confused into thinking
> that the keyboard is present.
> 
> Wouldn't it be simpler/better to assume there is no keyboard until
> we find positive evidence that there is one?

I added the original check, but I can't remember the reason I
initialized acpi_kbd_controller_present to 1.  Possibly just
timidity.

At the time, it was used in pc_keyb.c to avoid a blind probe.
That usage no longer exists.  i8042 now registers a regular
PNP driver with the appropriate PNP IDs.

Nobody actually uses acpi_kbd_controller_present or
acpi_legacy_devices anymore.  Maybe the best thing is to
just remove both of them.  Then the Keith can add back
the acpi_kbd_controller_present part to the kdb patch if
he decides that's the best route.



ia64: remove unused acpi_kbd_controller_present, acpi_legacy_devices

Nobody uses either one anymore.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-1/arch/ia64/kernel/acpi.c
===================================================================
--- work-1.orig/arch/ia64/kernel/acpi.c	2006-09-27 16:33:05.000000000 -0600
+++ work-1/arch/ia64/kernel/acpi.c	2006-09-29 12:57:20.000000000 -0600
@@ -64,9 +64,6 @@
 void (*pm_power_off) (void);
 EXPORT_SYMBOL(pm_power_off);
 
-unsigned char acpi_kbd_controller_present = 1;
-unsigned char acpi_legacy_devices;
-
 unsigned int acpi_cpei_override;
 unsigned int acpi_cpei_phys_cpuid;
 
@@ -628,12 +625,6 @@
 
 	fadt = (struct fadt_descriptor *)fadt_header;
 
-	if (!(fadt->iapc_boot_arch & BAF_8042_KEYBOARD_CONTROLLER))
-		acpi_kbd_controller_present = 0;
-
-	if (fadt->iapc_boot_arch & BAF_LEGACY_DEVICES)
-		acpi_legacy_devices = 1;
-
 	acpi_register_gsi(fadt->sci_int, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW);
 	return 0;
 }
Index: work-1/arch/ia64/sn/kernel/setup.c
===================================================================
--- work-1.orig/arch/ia64/sn/kernel/setup.c	2006-09-27 16:33:06.000000000 -0600
+++ work-1/arch/ia64/sn/kernel/setup.c	2006-09-29 12:44:28.000000000 -0600
@@ -65,7 +65,6 @@
 extern unsigned long last_time_offset;
 extern void (*ia64_mark_idle) (int);
 extern void snidle(int);
-extern unsigned char acpi_kbd_controller_present;
 extern unsigned long long (*ia64_printk_clock)(void);
 
 unsigned long sn_rtc_cycles_per_second;
@@ -452,17 +451,6 @@
 
 	ia64_printk_clock = ia64_sn2_printk_clock;
 
-	/*
-	 * Old PROMs do not provide an ACPI FADT. Disable legacy keyboard
-	 * support here so we don't have to listen to failed keyboard probe
-	 * messages.
-	 */
-	if (is_shub1() && version <= 0x0209 && acpi_kbd_controller_present) {
-		printk(KERN_INFO "Disabling legacy keyboard support as prom "
-		       "is too old and doesn't provide FADT\n");
-		acpi_kbd_controller_present = 0;
-	}
-
 	printk("SGI SAL version %x.%02x\n", version >> 8, version & 0x00FF);
 
 	/*

Re: KDB blindly reads keyboard port

[Keith Owens]

Bjorn Helgaas (on Wed, 27 Sep 2006 16:11:00 -0600) wrote:
>On Tuesday 26 September 2006 20:45, Keith Owens wrote:
>> No support for legacy I/O ports could be a bigger problem than just
>> KDB.
>
>On Itanium (and I suppose on x86), ACPI theoretically tells us enough
>that we don't need to assume any legacy resources.  Of course, Linux
>doesn't listen to everything ACPI is trying to tell it.  But that's
>a Linux deficiency we should remedy.
>
>> To fix just KDB, apply this patch over kdb-v4.4-2.6.18-common-1 and
>> add 'kdb_skip_keyboard' to the boot command line on the offending
>> hardware. 
>
>This doesn't feel like the right solution.  Since firmware tells us
>whether the device is present, I think we should rely on that.  If
>you want to use the device before ACPI is initialized, *then* you
>should pass a "kdb_use_keyboard" sort of flag.

I implemented this in my kdb tree, but it has a very nasty side effect,
it stops you from debugging that part of the boot process between kdb
startup and when the i8042 is probed.  KDB starts up very early so we
can debug the boot process.  Not being able to use the PC keyboard
until later in boot is not acceptable.  People using USB keyboards
already suffer from this problem and it is very frustrating.

Adding a "kdb_use_keyboard" flag means all existing systems have to
change if they want a debugger during boot, just to workaround a few
systems that get an error when reading from non-existent legacy I/O
ports.  So I am going back to my original idea, add 'kdb_skip_keyboard'
which is only required on the problem machines.

Re: KDB blindly reads keyboard port

[Bjorn Helgaas]

On Wednesday 15 November 2006 21:02, Keith Owens wrote:
> I implemented this in my kdb tree, but it has a very nasty side effect,
> it stops you from debugging that part of the boot process between kdb
> startup and when the i8042 is probed.  KDB starts up very early so we
> can debug the boot process.  Not being able to use the PC keyboard
> until later in boot is not acceptable.  People using USB keyboards
> already suffer from this problem and it is very frustrating.
> 
> Adding a "kdb_use_keyboard" flag means all existing systems have to
> change if they want a debugger during boot, just to workaround a few
> systems that get an error when reading from non-existent legacy I/O
> ports.  So I am going back to my original idea, add 'kdb_skip_keyboard'
> which is only required on the problem machines.

Hold on a minute.  These "problem machines" are completely compliant
with all the relevant ia64 specs in this area.  There is no spec that
says a keyboard controller must be present or that reading a non-
existent I/O port should be safe.

What about the FADT iapc_boot_arch bit?  Did you determine that isn't
sufficient for some reason?  That's available very early.

Bjorn

Re: KDB blindly reads keyboard port

[Keith Owens]

Bjorn Helgaas (on Tue, 26 Sep 2006 13:54:30 -0600) wrote:
>get_kbd_char() in arch/ia64/kdb/kdba_io.c does "inb(KBD_STATUS_REG)".
>
>But we don't know whether there's even an i8042 keyboard controller
>present.  On HP ia64 boxes, there is no i8042, and trying to read
>from it can cause an MCA.
>
>This depends on the specific platform and how it is configured.  I
>observed this MCA while booting the SLES10 install kernel on an
>HP rx7620 in "default" acpiconfig mode.  The supported acpiconfig
>mode on this box is "single-pci-domain", which also puts some
>legacy ports into "soft-fail" mode, where the read will just return
>0xff instead of causing an MCA.  But I think it's wrong to blindly
>poke around in I/O port space.

Bjron, could you try kdb-v4.4-2.6.19-rc5-{common,ia64}-2 on your
problem system?  I changed kdb so it only uses the keyboard if at least
one console matches the pattern /^tty[0-9]*$/.  IOW, if the user
specifies an i8042 style console on the command line (or uses the
default with CONFIG_VT=y) then kdb will attempt to use that keyboard.
Otherwise kdb ignores a VT style console, even when the kernel is
compiled with CONFIG_VT=y.

Re: KDB blindly reads keyboard port

[Matthew Wilcox]

On Fri, Nov 10, 2006 at 03:23:20PM +1100, Keith Owens wrote:
> Bjron, could you try kdb-v4.4-2.6.19-rc5-{common,ia64}-2 on your
> problem system?  I changed kdb so it only uses the keyboard if at least
> one console matches the pattern /^tty[0-9]*$/.  IOW, if the user
> specifies an i8042 style console on the command line (or uses the
> default with CONFIG_VT=y) then kdb will attempt to use that keyboard.
> Otherwise kdb ignores a VT style console, even when the kernel is
> compiled with CONFIG_VT=y.

If I'm using an HP Integrity system with a USB keyboard, won't I still
have a console that matches ^tty[0-9]*$ ?

Re: KDB blindly reads keyboard port

[Keith Owens]

Matthew Wilcox (on Thu, 9 Nov 2006 21:28:03 -0700) wrote:
>On Fri, Nov 10, 2006 at 03:23:20PM +1100, Keith Owens wrote:
>> Bjron, could you try kdb-v4.4-2.6.19-rc5-{common,ia64}-2 on your
>> problem system?  I changed kdb so it only uses the keyboard if at least
>> one console matches the pattern /^tty[0-9]*$/.  IOW, if the user
>> specifies an i8042 style console on the command line (or uses the
>> default with CONFIG_VT=y) then kdb will attempt to use that keyboard.
>> Otherwise kdb ignores a VT style console, even when the kernel is
>> compiled with CONFIG_VT=y.
>
>If I'm using an HP Integrity system with a USB keyboard, won't I still
>have a console that matches ^tty[0-9]*$ ?

Good point.  How about the console list must include /^tty[0-9]*$/
_and_ there must be an interrupt registered with a name of "i8042"
before KDB will attempt to access i8042 ports?