Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

PATCH In review Dexuan Cui 6 participants 24 messages Aug 12, 2014 – Apr 22, 2016

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Dexuan Cui

author

2014-08-12 02:28 UTC #0 lore raw

PATCH 1/1 Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver like atkbd.c.
atkbd.c depends on libps2.c because it invokes ps2_command(). libps2.c depends on i8042.c because it invokes i8042_check_port_owner(). As a result, hyperv_keyboard actually depends on i8042.c.

For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than =y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device emulated) and finally hyperv_keyboard can't work and the user can't input:
https://bugs.archlinux.org/task/39820
(Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)

Decoupling the dependency between hyperv_keyboard and i8042 needs non-trivial efforts and is hence a long term goal.

For now, let's make the dependency explicit so people can beware of this.

Thank Claudio for the initial reporting, investigation and suggesting the fix.

Signed-off-by Dexuan Cui <[email protected]>

Reported-by Claudio Latini <[email protected]>

Cc K. Y. Srinivasan <[email protected]>

---
 drivers/input/serio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/input/serio/Kconfig +1 -1

index bc2d474..3277bff 100644
@@ -273,7 +273,7 @@ config SERIO_OLPC_APSP
 
 config HYPERV_KEYBOARD
 	tristate "Microsoft Synthetic Keyboard driver"
-	depends on HYPERV
+	depends on HYPERV && SERIO_I8042=y
 	default HYPERV
 	help
 	  Select this option to enable the Hyper-V Keyboard driver.

Signature · 1 line

1.9.1

Greg KH

2014-08-12 03:21 UTC #1 lore raw

Quoted 13 lines · Dexuan Cui

hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver like atkbd.c.
atkbd.c depends on libps2.c because it invokes ps2_command(). libps2.c depends on i8042.c because it invokes i8042_check_port_owner(). As a result, hyperv_keyboard actually depends on i8042.c.
For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than =y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device emulated) and finally hyperv_keyboard can't work and the user can't input:
https://bugs.archlinux.org/task/39820
(Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)

Quoted 4 lines

Decoupling the dependency between hyperv_keyboard and i8042 needs non-trivial efforts and is hence a long term goal.
For now, let's make the dependency explicit so people can beware of this.

You didn't make anyone "aware" of this, you just prevented people from being able to select the module unless they build the driver into the kernel, which isn't very nice.

What exactly needs to be done to fix this "correctly" that is going to take too much work at the moment?

thanks,

greg k-h

Dexuan Cui

author

2014-08-12 05:52 UTC #2 lore raw

Quoted 10 lines

-----Original Message-----
From: Greg KH [mailto:[email protected]]
Quoted 4 lines
Decoupling the dependency between hyperv_keyboard and i8042 needs non-trivial efforts and is hence a long term goal.
For now, let's make the dependency explicit so people can beware of this.
You didn't make anyone "aware" of this, you just prevented people from being able to select the module unless they build the driver into the kernel, which isn't very nice.

Yes, exactly.

Quoted 2 lines

What exactly needs to be done to fix this "correctly" that is going to take too much work at the moment?

Hi Greg,
The current implementation of hyperv-keyboard.c borrows serio_interrupt() to pass the key strokes to the high level component. serio_interrupt() actually depends on atkbd.c and i8042.c, so this doesn't work for a Generation 2 hyper-v guest because no i8042 keyboard controller is emulated: http://technet.microsoft.com/en-us/library/dn282285.aspx

To decouple the dependency between the hyperv-keyboard and i8042 modules, I suppose we probably have to re-implement hyperv-keyboard by using input_allocate_device(), input_register_device(), and using input_report_key() to pass the key strokes to the high level.

I'll have to need some time for further investigation and a new implementation. Before the new code is completely ready, IMHO the patch can help to avoid a bad user experience like Arch Linux working as a Generation 2 hyper-v guest.

BTW, looks most of Linux distros (like RHEL, Ubuntu, SUSE) have CONFIG_SERIO_I8042=y, probably because it's the result of "make defconfig". So the patch actually doesn't affect these distros.

Thanks,
-- Dexuan

Greg KH

2014-08-12 06:01 UTC #3 lore raw

Quoted 22 lines · Dexuan Cui

Quoted 10 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Quoted 4 lines
Decoupling the dependency between hyperv_keyboard and i8042 needs non-trivial efforts and is hence a long term goal.
For now, let's make the dependency explicit so people can beware of this.
You didn't make anyone "aware" of this, you just prevented people from being able to select the module unless they build the driver into the kernel, which isn't very nice.
Yes, exactly.
Quoted 2 lines
What exactly needs to be done to fix this "correctly" that is going to take too much work at the moment?
Hi Greg,
The current implementation of hyperv-keyboard.c borrows serio_interrupt() to pass the key strokes to the high level component. serio_interrupt() actually depends on atkbd.c and i8042.c, so this doesn't work for a Generation 2 hyper-v guest because no i8042 keyboard controller is emulated: http://technet.microsoft.com/en-us/library/dn282285.aspx

Quoted 4 lines

To decouple the dependency between the hyperv-keyboard and i8042 modules, I suppose we probably have to re-implement hyperv-keyboard by using input_allocate_device(), input_register_device(), and using input_report_key() to pass the key strokes to the high level.

Yes, that would be the best thing to do, and shouldn't be that hard to create an input driver, it's pretty simple code, right?

Quoted 4 lines

I'll have to need some time for further investigation and a new implementation. Before the new code is completely ready, IMHO the patch can help to avoid a bad user experience like Arch Linux working as a Generation 2 hyper-v guest.

You are still preventing Arch from working here, as the driver can't be built at all, right?

Quoted 3 lines

BTW, looks most of Linux distros (like RHEL, Ubuntu, SUSE) have CONFIG_SERIO_I8042=y, probably because it's the result of "make defconfig". So the patch actually doesn't affect these distros.

Or maybe it is beause they use older kernels and like to turn on every single possible option :)

thanks,

greg k-h

Dexuan Cui

author

2014-08-12 07:15 UTC #4 lore raw

Quoted 6 lines

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Greg KH
Quoted 3 lines
Quoted 2 lines
What exactly needs to be done to fix this "correctly" that is going to take too much work at the moment?

Quoted 7 lines

Quoted 4 lines
To decouple the dependency between the hyperv-keyboard and i8042 modules, I suppose we probably have to re-implement hyperv-keyboard by using input_allocate_device(), input_register_device(), and using input_report_key() to pass the key strokes to the high level.
Yes, that would be the best thing to do, and shouldn't be that hard to create an input driver, it's pretty simple code, right?

Hi Greg,
Thanks for the confirmation!
I didn't use the APIs before.
I think I need a couple of days to code, test and debug it while I have many
things at hand at present. :-(

Quoted 7 lines

Quoted 4 lines
I'll have to need some time for further investigation and a new implementation. Before the new code is completely ready, IMHO the patch can help to avoid a bad user experience like Arch Linux working as a Generation 2 hyper-v guest.
You are still preventing Arch from working here, as the driver can't be built at all, right?

The driver can build(compile) fine.
The issue is: the latest Arch Linux release doesn't have a working (virtual) keyboard when it runs as Generation 2 hyper-v guest -- when it runs as a "traditional" Generation 1 hyper-v guest, everything works fine. I hope this patch can temporarily help Arch users if they find the issue and if they can rebuild the kernel.

Quoted 6 lines

Quoted 3 lines
BTW, looks most of Linux distros (like RHEL, Ubuntu, SUSE) have CONFIG_SERIO_I8042=y, probably because it's the result of "make defconfig". So the patch actually doesn't affect these distros.
Or maybe it is beause they use older kernels and like to turn on every single possible option :)

If these 3 distros had =m, we could have found the issue earlier.

Thanks,
-- Dexuan

Dmitry Torokhov

maintainer

2014-08-12 17:54 UTC #5 lore raw

Quoted 6 lines · Dexuan Cui

Quoted 6 lines
-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Greg KH
Quoted 3 lines
Quoted 2 lines
What exactly needs to be done to fix this "correctly" that is going to take too much work at the moment?

Quoted 6 lines

Quoted 6 lines
Quoted 4 lines
To decouple the dependency between the hyperv-keyboard and i8042 modules, I suppose we probably have to re-implement hyperv-keyboard by using input_allocate_device(), input_register_device(), and using input_report_key() to pass the key strokes to the high level.
Yes, that would be the best thing to do,

Why? The backend still delivers AT keyboard data, so why does it make sense to write a new driver instead of making sure you can load atkbd/libps2 even without i8042 loaded?

Quoted 13 lines

Quoted 2 lines
and shouldn't be that hard to
create an input driver, it's pretty simple code, right?
Hi Greg,
Thanks for the confirmation!
I didn't use the APIs before.
I think I need a couple of days to code, test and debug it while I have many
things at hand at present. :-(
Quoted 5 lines
Quoted 4 lines
I'll have to need some time for further investigation and a new implementation. Before the new code is completely ready, IMHO the patch can help to avoid a bad user experience like Arch Linux working as a Generation 2 hyper-v guest.

Quoted 8 lines

Quoted 2 lines
You are still preventing Arch from working here, as the driver can't be built at all, right?
The driver can build(compile) fine.
The issue is: the latest Arch Linux release doesn't have a working (virtual) keyboard when it runs as Generation 2 hyper-v guest -- when it runs as a "traditional" Generation 1 hyper-v guest, everything works fine. I hope this patch can temporarily help Arch users if they find the issue and if they can rebuild the kernel.

The Arch users can simply select to build i8042 into the kernel as a workaround.

The proper solution is to allow loading libps2 module even if i8042 did not find its device. I wish I could simply drop this i8042_lock_chip and stuff, but unfortunately i8042 ports are not truly independent. We need to figure a way for libps2 to engage locking in i8042 if the driver is loaded, otherwise just ignore it.

Thanks.

Signature · 1 line

Dmitry

KY Srinivasan

2014-08-12 18:01 UTC #6 lore raw

Quoted 53 lines

-----Original Message-----
From: Dmitry Torokhov [mailto:[email protected]]
Sent: Tuesday, August 12, 2014 10:55 AM
To: Dexuan Cui
Cc: Greg KH; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; KY Srinivasan; Haiyang Zhang
Subject: Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y
Quoted 6 lines · Dexuan Cui
Quoted 6 lines
-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Greg KH
Quoted 3 lines
Quoted 2 lines
What exactly needs to be done to fix this "correctly" that is going to take too much work at the moment?
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
To decouple the dependency between the hyperv-keyboard and i8042 modules, I suppose we probably have to re-implement hyperv-keyboard by using input_allocate_device(), input_register_device(), and using
input_report_key() to pass the key strokes to the high level.
Yes, that would be the best thing to do,
Why? The backend still delivers AT keyboard data, so why does it make sense to write a new driver instead of making sure you can load atkbd/libps2 even without i8042 loaded?
Quoted 13 lines
Quoted 2 lines
and shouldn't be that hard to
create an input driver, it's pretty simple code, right?
Hi Greg,
Thanks for the confirmation!
I didn't use the APIs before.
I think I need a couple of days to code, test and debug it while I have many things at hand at present. :-(
Quoted 5 lines
Quoted 4 lines
I'll have to need some time for further investigation and a new implementation. Before the new code is completely ready, IMHO the patch can help to avoid a bad user experience like Arch Linux working as a Generation 2 hyper-v guest.
Quoted 8 lines
Quoted 2 lines
You are still preventing Arch from working here, as the driver can't be built at all, right?
The driver can build(compile) fine.
The issue is: the latest Arch Linux release doesn't have a working (virtual) keyboard when it runs as Generation 2 hyper-v guest -- when it runs as a "traditional" Generation 1 hyper-v guest, everything works fine. I hope this patch can temporarily help Arch users if they find the issue and if they can rebuild the kernel.

Quoted 8 lines

The Arch users can simply select to build i8042 into the kernel as a workaround.
The proper solution is to allow loading libps2 module even if i8042 did not find its device. I wish I could simply drop this i8042_lock_chip and stuff, but unfortunately i8042 ports are not truly independent. We need to figure a way for libps2 to engage locking in i8042 if the driver is loaded, otherwise just ignore it.

When I first wrote this driver, we took the decision not to replicate all the code in the at keyboard driver and to make this a serio based driver. I like the proposal made by Dmitry here.

Regards,

K. Y

Dexuan Cui

author

2014-08-13 05:28 UTC #7 lore raw

Quoted 10 lines

-----Original Message-----
From: KY Srinivasan
Quoted 4 lines
The Arch users can simply select to build i8042 into the kernel as a workaround.
The proper solution is to allow loading libps2 module even if i8042 did not
find
Quoted 3 lines
its device. I wish I could simply drop this i8042_lock_chip and stuff, but unfortunately i8042 ports are not truly independent. We need to figure a way for libps2 to engage locking in i8042 if the driver is loaded, otherwise

Quoted 8 lines

just
Quoted 1 line
ignore it.
When I first wrote this driver, we took the decision not to replicate all the code in the at keyboard driver and to
make this a serio based driver. I like the proposal made by Dmitry here.
K. Y

Thanks for explaining the background, KY!
I like Dmitry's proposal too.

Thanks,
-- Dexuan

Dexuan Cui

author

2014-08-13 05:24 UTC #8 lore raw

In reply to Dmitry Torokhov

Quoted 6 lines

-----Original Message-----
From: Dmitry Torokhov
Sent: Wednesday, August 13, 2014 1:55 AM
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
To decouple the dependency between the hyperv-keyboard and i8042 modules, I suppose we probably have to re-implement hyperv-
keyboard by

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
using input_allocate_device(), input_register_device(), and using input_report_key() to pass the key strokes to the high level.
Yes, that would be the best thing to do,
Why? The backend still delivers AT keyboard data, so why does it make sense to write a new driver instead of making sure you can load atkbd/libps2 even without i8042 loaded?

Hi Dmitry,
Thanks for pointing this out!
I didn't realize input_report_key() can't accept AT keyboard scan codes.

Quoted 8 lines

Quoted 5 lines
The issue is: the latest Arch Linux release doesn't have a working (virtual) keyboard when it runs as Generation 2 hyper-v guest -- when it runs as a "traditional" Generation 1 hyper-v guest, everything works fine. I hope this patch can temporarily help Arch users if they find the issue and if they can rebuild the kernel.
The Arch users can simply select to build i8042 into the kernel as a workaround.

I agree.

Quoted 7 lines

The proper solution is to allow loading libps2 module even if i8042 did not find its device. I wish I could simply drop this i8042_lock_chip and stuff, but unfortunately i8042 ports are not truly independent. We need to figure a way for libps2 to engage locking in i8042 if the driver is loaded, otherwise just ignore it.
Dmitry

Yeah, this seems the right solution.

How about this:
in libps2.c let's add and export a function pointer i8042_lock_chip_if_port_owner: it is used to replace the current

	if (i8042_check_port_owner(ps2dev->serio))
		i8042_lock_chip();

The function pointer has a default value NULL, and in i8042.c: i8042_init() we set the function pointer if an i8042 device is found?

Thanks,
-- Dexuan

Dmitry Torokhov

maintainer

2014-08-13 15:56 UTC #9 lore raw

Quoted 38 lines · Dexuan Cui

Quoted 6 lines
-----Original Message-----
From: Dmitry Torokhov
Sent: Wednesday, August 13, 2014 1:55 AM
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
To decouple the dependency between the hyperv-keyboard and i8042 modules, I suppose we probably have to re-implement hyperv-
keyboard by
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
using input_allocate_device(), input_register_device(), and using input_report_key() to pass the key strokes to the high level.
Yes, that would be the best thing to do,
Why? The backend still delivers AT keyboard data, so why does it make sense to write a new driver instead of making sure you can load atkbd/libps2 even without i8042 loaded?
Hi Dmitry,
Thanks for pointing this out!
I didn't realize input_report_key() can't accept AT keyboard scan codes.
Quoted 8 lines
Quoted 5 lines
The issue is: the latest Arch Linux release doesn't have a working (virtual) keyboard when it runs as Generation 2 hyper-v guest -- when it runs as a "traditional" Generation 1 hyper-v guest, everything works fine. I hope this patch can temporarily help Arch users if they find the issue and if they can rebuild the kernel.
The Arch users can simply select to build i8042 into the kernel as a workaround.
I agree.
Quoted 7 lines
The proper solution is to allow loading libps2 module even if i8042 did not find its device. I wish I could simply drop this i8042_lock_chip and stuff, but unfortunately i8042 ports are not truly independent. We need to figure a way for libps2 to engage locking in i8042 if the driver is loaded, otherwise just ignore it.
Dmitry
Yeah, this seems the right solution.

Quoted 7 lines

How about this:
in libps2.c let's add and export a function pointer i8042_lock_chip_if_port_owner: it is used to replace the current
	if (i8042_check_port_owner(ps2dev->serio))
		i8042_lock_chip();
The function pointer has a default value NULL, and in i8042.c: i8042_init() we set the function pointer if an i8042 device is found?

Hmm, that would make i8042 depend on libps2, which might be OK, but how do you deal with the locking here? I.e. what happens if you unload i8042 right when we go through this sequence?

Maybe we need to split i8042_lock_chip() and friends into a separate module that always loads, even if i8042 is not present.

Thanks.

Signature · 1 line

Dmitry

Dexuan Cui

author

2014-08-14 06:08 UTC #10 lore raw

Quoted 10 lines

-----Original Message-----
From: Dmitry Torokhov
Quoted 7 lines
How about this:
in libps2.c let's add and export a function pointer i8042_lock_chip_if_port_owner: it is used to replace the current
	if (i8042_check_port_owner(ps2dev->serio))
		i8042_lock_chip();
The function pointer has a default value NULL, and in i8042.c: i8042_init() we set the function pointer if an i8042 device is found?

Quoted 3 lines

Hmm, that would make i8042 depend on libps2, which might be OK, but how do you deal with the locking here? I.e. what happens if you unload i8042 right when we go through this sequence?

Ok, I got it.

Quoted 3 lines

Maybe we need to split i8042_lock_chip() and friends into a separate module that always loads, even if i8042 is not present. Dmitry

Good idea!

However the more difficult thing is i8042_check_port_owner() -- used by libps2.c too. Then the separate module will also have to include and EXPORT

	DEFINE_SPINLOCK(i8042_lock);
	struct i8042_port i8042_ports[I8042_NUM_PORTS];

This seems a non-trivial change... :-(

Thanks,
-- Dexuan

Mark Laws

2016-04-18 15:23 UTC #11 lore raw

PATCH 1/1 Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

In reply to Dexuan Cui

Hi,

Please keep me Cc:ed in any replies as I'm not on these lists.

Description of the fix is in the commit message for the patch.

Discussion:

Given that most distributions were already statically linking i8042.c, having it not unload even if there is no i8042 device seems a better fix than the alternatives:

a) requiring users build a kernel with CONFIG_SERIO_I8042=y;
b) duplicating the needed bits from atkbd.c in hyperv-keyboard, or;
c) this patch, but with a "stay_resident=1" option to enable the workaround. Detecting presence of Hyper-V could be handled by udev, which would pass this option, but every distribution would need to fix their rules (certainly we don't want to check for Hyper-V in i8042.c)

Mark Laws (1):
Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

 drivers/input/serio/i8042.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

Signature · 1 line

2.8.0

Mark Laws

2016-04-18 15:24 UTC #12 lore raw

PATCH 1/1 Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

As explained in [email protected]:

Quoted 6 lines

hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver like atkbd.c. atkbd.c depends on libps2.c because it invokes ps2_command(). libps2.c depends on i8042.c because it invokes i8042_check_port_owner(). As a result, hyperv_keyboard actually depends on i8042.c.

Quoted 6 lines

For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than =y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device emulated) and finally hyperv_keyboard can't work and the user can't input: https://bugs.archlinux.org/task/39820
(Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)

The transitive dependency on i8042.c is non-trivial--there appears to be no obvious way to untangle it other than by duplicating much of atkbd.c within hyperv-keyboard--so we employ a simple workaround: keep i8042.ko loaded even if no i8042 device is detected, but set a flag so that any calls into the module simply return (since we don't want to try to interact with the non-existent i8042). This allows atkbd.c and libps2.c to load, solving the problem.

Signed-off-by Mark Laws <[email protected]>

---
 drivers/input/serio/i8042.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

drivers/input/serio/i8042.c +34 -7

index 4541957..4d49496 100644
@@ -132,6 +132,7 @@ struct i8042_port {
 
 static struct i8042_port i8042_ports[I8042_NUM_PORTS];
 
+static bool i8042_present;
 static unsigned char i8042_initial_ctr;
 static unsigned char i8042_ctr;
 static bool i8042_mux_present;

@@ -163,6 +164,9 @@ int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
 	unsigned long flags;
 	int ret = 0;
 
+	if (!i8042_present)
+		return ret;
+
 	spin_lock_irqsave(&i8042_lock, flags);
 
 	if (i8042_platform_filter) {

@@ -184,6 +188,9 @@ int i8042_remove_filter(bool (*filter)(unsigned char data, unsigned char str,
 	unsigned long flags;
 	int ret = 0;
 
+	if (!i8042_present)
+		return ret;
+
 	spin_lock_irqsave(&i8042_lock, flags);
 
 	if (i8042_platform_filter != filter) {

@@ -311,7 +318,10 @@ static int __i8042_command(unsigned char *param, int command)
 int i8042_command(unsigned char *param, int command)
 {
 	unsigned long flags;
-	int retval;
+	int retval = 0;
+
+	if (!i8042_present)
+		return retval;
 
 	spin_lock_irqsave(&i8042_lock, flags);
 	retval = __i8042_command(param, command);

@@ -1380,6 +1390,9 @@ bool i8042_check_port_owner(const struct serio *port)
 {
 	int i;
 
+	if (!i8042_present)
+		return false;
+
 	for (i = 0; i < I8042_NUM_PORTS; i++)
 		if (i8042_ports[i].serio == port)
 			return true;

@@ -1569,13 +1582,17 @@ static int __init i8042_init(void)
 
 	dbg_init();
 
+	i8042_present = false;
+
 	err = i8042_platform_init();
 	if (err)
 		return err;
 
 	err = i8042_controller_check();
-	if (err)
-		goto err_platform_exit;
+	if (err) {
+		pr_info("Staying resident in case of module dependencies\n");
+		goto out;
+	}
 
 	pdev = platform_create_bundle(&i8042_driver, i8042_probe, NULL, 0, NULL, 0);
 	if (IS_ERR(pdev)) {

@@ -1585,7 +1602,9 @@ static int __init i8042_init(void)
 
 	bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
 	panic_blink = i8042_panic_blink;
+	i8042_present = true;
 
+out:
 	return 0;
 
  err_platform_exit:

@@ -1595,12 +1614,20 @@ static int __init i8042_init(void)
 
 static void __exit i8042_exit(void)
 {
-	platform_device_unregister(i8042_platform_device);
-	platform_driver_unregister(&i8042_driver);
+	if (i8042_present) {
+		platform_device_unregister(i8042_platform_device);
+		platform_driver_unregister(&i8042_driver);
+	}
+
 	i8042_platform_exit();
 
-	bus_unregister_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
-	panic_blink = NULL;
+	if (i8042_present) {
+		bus_unregister_notifier(&serio_bus,
+					&i8042_kbd_bind_notifier_block);
+		panic_blink = NULL;
+	}
+
+	i8042_present = false;
 }
 
 module_init(i8042_init);

Signature · 1 line

2.8.0

Dan Carpenter

2016-04-18 16:55 UTC #13 lore raw

Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

So if the user inserts the module without a keyboard then in the original code they would just put a keyboard in and try again. Now they have to do an extra rmmod. What about if we just removed the test for if the keyboard is present?

Quoted 41 lines · Mark Laws

As explained in [email protected]:
Quoted 6 lines
hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver like atkbd.c. atkbd.c depends on libps2.c because it invokes ps2_command(). libps2.c depends on i8042.c because it invokes i8042_check_port_owner(). As a result, hyperv_keyboard actually depends on i8042.c.
Quoted 6 lines
For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than =y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device emulated) and finally hyperv_keyboard can't work and the user can't input: https://bugs.archlinux.org/task/39820
(Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)
The transitive dependency on i8042.c is non-trivial--there appears to be no obvious way to untangle it other than by duplicating much of atkbd.c within hyperv-keyboard--so we employ a simple workaround: keep i8042.ko loaded even if no i8042 device is detected, but set a flag so that any calls into the module simply return (since we don't want to try to interact with the non-existent i8042). This allows atkbd.c and libps2.c to load, solving the problem.
Signed-off-by Mark Laws <[email protected]>
---
 drivers/input/serio/i8042.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)
drivers/input/serio/i8042.c +1 -0
index 4541957..4d49496 100644
@@ -132,6 +132,7 @@ struct i8042_port {
 
 static struct i8042_port i8042_ports[I8042_NUM_PORTS];
 
+static bool i8042_present;
 static unsigned char i8042_initial_ctr;
 static unsigned char i8042_ctr;
 static bool i8042_mux_present;

drivers/input/serio/i8042.c int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str, · line 168

Quoted 5 lines

drivers/input/serio/i8042.c +2 -0

@@ -163,6 +164,9 @@ int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
 	unsigned long flags;
 	int ret = 0;
 
+	if (!i8042_present)
+		return ret;

Don't obfuscate the literal. Just "return 0;". Also if it's goto out just change that to "return 0;" because it's simpler for the reader.

regards,
dan carpenter

Mark Laws

2016-04-18 17:24 UTC #14 lore raw

Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

Quoted 4 lines · Dan Carpenter

So if the user inserts the module without a keyboard then in the original code they would just put a keyboard in and try again. Now they have to do an extra rmmod. What about if we just removed the test for if the keyboard is present?

Sorry, I don't understand--which part are you suggesting we remove?

Quoted 2 lines

Don't obfuscate the literal. Just "return 0;". Also if it's goto out just change that to "return 0;" because it's simpler for the reader.

Will fix.

Regards,
Mark Laws

Signature · 1 line

|v\ /\ |\ |< |_ /\ \^| //

Dan Carpenter

2016-04-18 20:36 UTC #15 lore raw

Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

Quoted 8 lines · Mark Laws

Quoted 4 lines · Dan Carpenter
So if the user inserts the module without a keyboard then in the original code they would just put a keyboard in and try again. Now they have to do an extra rmmod. What about if we just removed the test for if the keyboard is present?
Sorry, I don't understand--which part are you suggesting we remove?

The call to i8042_controller_check() or move it to the probe function or something. Why must we have the hardware to load the module?

regards,
dan carpenter

Mark Laws

2016-04-18 22:00 UTC #16 lore raw

Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

Quoted 5 lines · Dan Carpenter

Quoted 1 line · Mark Laws
Sorry, I don't understand--which part are you suggesting we remove?
The call to i8042_controller_check() or move it to the probe function or something. Why must we have the hardware to load the module?

We don't. That's the point of the patch. Do you mean that since our intent is to load the module regardless of whether or not the hardware is there, the check should be (re)moved simply to clarify the code?

Sorry for the stupid questions--I'm just trying to make sure I understand you correctly!

Regards,
Mark Laws

Signature · 1 line

|v\ /\ |\ |< |_ /\ \^| //

Dan Carpenter

2016-04-19 08:22 UTC #17 lore raw

Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

Quoted 10 lines · Mark Laws

Quoted 5 lines · Dan Carpenter
Quoted 1 line · Mark Laws
Sorry, I don't understand--which part are you suggesting we remove?
The call to i8042_controller_check() or move it to the probe function or something. Why must we have the hardware to load the module?
We don't. That's the point of the patch. Do you mean that since our intent is to load the module regardless of whether or not the hardware is there, the check should be (re)moved simply to clarify the code?

Yeah. Just remove the call to i8042_controller_check(). Wouldn't everyone be happy with that situation?

Your patch makes life slightly more complicated for people who want to use the original hardware if the load the module but the hardware isn't detected.

regards,
dan carpenter

Mark Laws

2016-04-19 10:46 UTC #18 lore raw

Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

Quoted 2 lines · Dan Carpenter

Yeah. Just remove the call to i8042_controller_check(). Wouldn't everyone be happy with that situation?

No problem, I agree this is better--just wasn't sure what you meant initially.

Quoted 3 lines

Your patch makes life slightly more complicated for people who want to use the original hardware if the load the module but the hardware isn't detected.

That is true, but apparently nobody can think of a better solution (including me :)) and this bug has been open for two years. Having to rmmod in the corner case where the module gets loaded but no i8042 is present seems a small price to pay for having the keyboard work regardless of CONFIG_I8042=y or m. Right now, any distribution with CONFIG_I8042=m has a non-functional keyboard on Hyper-V Gen2 VMs, which is probably frustrating for (e.g.) Arch Linux users who find themselves unable to type and thus can't install their distribution.

Regards,
Mark Laws

Signature · 1 line

|v\ /\ |\ |< |_ /\ \^| //

Mark Laws

2016-04-22 13:01 UTC #19 lore raw

PATCH 1/1 Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

This is an updated version of the original patch from this thread. It fixes the style issues Dan Carpenter brought up.

Mark Laws (1):
Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

 drivers/input/serio/i8042.c | 50 ++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 16 deletions(-)

Signature · 1 line

2.8.0

Mark Laws

2016-04-22 13:01 UTC #20 lore raw

PATCH 1/1 Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

As explained in [email protected]:

Quoted 6 lines

hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver like atkbd.c. atkbd.c depends on libps2.c because it invokes ps2_command(). libps2.c depends on i8042.c because it invokes i8042_check_port_owner(). As a result, hyperv_keyboard actually depends on i8042.c.

Quoted 6 lines

For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than =y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device emulated) and finally hyperv_keyboard can't work and the user can't input: https://bugs.archlinux.org/task/39820
(Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)

Signed-off-by Mark Laws <[email protected]>

---
 drivers/input/serio/i8042.c | 50 ++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 16 deletions(-)

drivers/input/serio/i8042.c +34 ~~-16~~

index 4541957..00f73d3 100644
@@ -132,6 +132,7 @@ struct i8042_port {
 
 static struct i8042_port i8042_ports[I8042_NUM_PORTS];
 
+static bool i8042_present;
 static unsigned char i8042_initial_ctr;
 static unsigned char i8042_ctr;
 static bool i8042_mux_present;

@@ -163,6 +164,9 @@ int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
 	unsigned long flags;
 	int ret = 0;
 
+	if (!i8042_present)
+		return 0;
+
 	spin_lock_irqsave(&i8042_lock, flags);
 
 	if (i8042_platform_filter) {

@@ -184,6 +188,9 @@ int i8042_remove_filter(bool (*filter)(unsigned char data, unsigned char str,
 	unsigned long flags;
 	int ret = 0;
 
+	if (!i8042_present)
+		return 0;
+
 	spin_lock_irqsave(&i8042_lock, flags);
 
 	if (i8042_platform_filter != filter) {

@@ -313,6 +320,9 @@ int i8042_command(unsigned char *param, int command)
 	unsigned long flags;
 	int retval;
 
+	if (!i8042_present)
+		return 0;
+
 	spin_lock_irqsave(&i8042_lock, flags);
 	retval = __i8042_command(param, command);
 	spin_unlock_irqrestore(&i8042_lock, flags);

@@ -1380,6 +1390,9 @@ bool i8042_check_port_owner(const struct serio *port)
 {
 	int i;
 
+	if (!i8042_present)
+		return false;
+
 	for (i = 0; i < I8042_NUM_PORTS; i++)
 		if (i8042_ports[i].serio == port)
 			return true;

@@ -1493,6 +1506,10 @@ static int __init i8042_probe(struct platform_device *dev)
 {
 	int error;
 
+	error = i8042_controller_check();
+	if (error)
+		return error;
+
 	i8042_platform_device = dev;
 
 	if (i8042_reset) {

@@ -1569,38 +1586,39 @@ static int __init i8042_init(void)
 
 	dbg_init();
 
+	i8042_present = false;
+
 	err = i8042_platform_init();
 	if (err)
 		return err;
 
-	err = i8042_controller_check();
-	if (err)
-		goto err_platform_exit;
-
 	pdev = platform_create_bundle(&i8042_driver, i8042_probe, NULL, 0, NULL, 0);
-	if (IS_ERR(pdev)) {
-		err = PTR_ERR(pdev);
-		goto err_platform_exit;
-	}
+	if (IS_ERR(pdev))
+		return 0; /* load anyway since some modules depend on our symbols */
 
 	bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
 	panic_blink = i8042_panic_blink;
+	i8042_present = true;
 
 	return 0;
-
- err_platform_exit:
-	i8042_platform_exit();
-	return err;
 }
 
 static void __exit i8042_exit(void)
 {
-	platform_device_unregister(i8042_platform_device);
-	platform_driver_unregister(&i8042_driver);
+	if (i8042_present) {
+		platform_device_unregister(i8042_platform_device);
+		platform_driver_unregister(&i8042_driver);
+	}
+
 	i8042_platform_exit();
 
-	bus_unregister_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
-	panic_blink = NULL;
+	if (i8042_present) {
+		bus_unregister_notifier(&serio_bus,
+					&i8042_kbd_bind_notifier_block);
+		panic_blink = NULL;
+	}
+
+	i8042_present = false;
 }
 
 module_init(i8042_init);

Signature · 1 line

2.8.0

Dan Carpenter

2016-04-22 13:18 UTC #21 lore raw

Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

Why is platform_create_bundle() failing? It didn't fail in the first version of the patch.

Btw, I'm not asking rhetorical questions, if I ask a question it means I legitimately don't know the answer.

But I don't like this patch. Could you describe how you have tested it with real hardware? What I want to know is that you loaded the module without the hardware installed and then installed the hardware and got it to work. You have made that more complicated and you've said that you're willing to complicate life for those users slightly because it's a trade off for fixing your bug... But that's sort of annoying and no one has even tested how it works.

What I was really wondering last time was why can we not just do this? Testing to see if the hardware is present is normally done in the probe() function and not the init() function. I have not tested this and I don't know what happens when we do this. Apparently, it causes platform_create_bundle() to fail but I'm not sure why... Maybe the create bundle call probe() and that fails?

How hard would it be to separate these things out into two modules really? You say that you'd have to duplicate everything but maybe we could instead just make the common functions into a library type thing..

drivers/input/serio/i8042.c +0 -4

index 4541957..4f0bc7c 100644
@@ -1573,10 +1573,6 @@ static int __init i8042_init(void)
 	if (err)
 		return err;
 
-	err = i8042_controller_check();
-	if (err)
-		goto err_platform_exit;
-
 	pdev = platform_create_bundle(&i8042_driver, i8042_probe, NULL, 0, NULL, 0);
 	if (IS_ERR(pdev)) {
 		err = PTR_ERR(pdev);

Mark Laws

2016-04-22 17:30 UTC #22 lore raw

PATCH 1/1 Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

A few tripels later and the patch has been completely rewritten. The problem symbols have been moved to a new module, libi8042.c, which both libps2.c and i8042.c use. libps2.c no longer depends on i8042.c, and i8042.c no longer needs the gross hack of the previous patch.

Since I didn't write anything new, just shuffled things around, I haven't changed any copyrights. I have no idea what the right procedure is here, so please let me know, since I'm probably screwing up somehow.

Mark Laws (1):
Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

 drivers/input/serio/Kconfig    |  7 ++++-
 drivers/input/serio/Makefile   |  1 +
 drivers/input/serio/i8042.c    | 48 ---------------------------------
 drivers/input/serio/i8042.h    |  7 -----
 drivers/input/serio/libi8042.c | 60 ++++++++++++++++++++++++++++++++++++++++++
 drivers/input/serio/libps2.c   |  2 +-
 include/linux/i8042.h          | 17 +-----------
 include/linux/libi8042.h       | 55 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 124 insertions(+), 73 deletions(-)
 create mode 100644 drivers/input/serio/libi8042.c
 create mode 100644 include/linux/libi8042.h

Signature · 1 line

2.8.0

Mark Laws

2016-04-22 17:30 UTC #23 lore raw

PATCH 1/1 Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs

As explained in [email protected]:

Quoted 6 lines

hyperv_keyboard invokes serio_interrupt(), which needs a valid serio driver like atkbd.c. atkbd.c depends on libps2.c because it invokes ps2_command(). libps2.c depends on i8042.c because it invokes i8042_check_port_owner(). As a result, hyperv_keyboard actually depends on i8042.c.

Quoted 6 lines

For a Generation 2 Hyper-V VM (meaning no i8042 device emulated), if a Linux VM (like Arch Linux) happens to configure CONFIG_SERIO_I8042=m rather than =y, atkbd.ko can't load because i8042.ko can't load(due to no i8042 device emulated) and finally hyperv_keyboard can't work and the user can't input: https://bugs.archlinux.org/task/39820
(Ubuntu/RHEL/SUSE aren't affected since they use CONFIG_SERIO_I8042=y)

This eliminates the transitive dependency on i8042.c by moving the symbols libps2.c depends on to a new module, libi8042.c.

Signed-off-by Mark Laws <[email protected]>

---
 drivers/input/serio/Kconfig    |  7 ++++-
 drivers/input/serio/Makefile   |  1 +
 drivers/input/serio/i8042.c    | 48 ---------------------------------
 drivers/input/serio/i8042.h    |  7 -----
 drivers/input/serio/libi8042.c | 60 ++++++++++++++++++++++++++++++++++++++++++
 drivers/input/serio/libps2.c   |  2 +-
 include/linux/i8042.h          | 17 +-----------
 include/linux/libi8042.h       | 55 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 124 insertions(+), 73 deletions(-)
 create mode 100644 drivers/input/serio/libi8042.c
 create mode 100644 include/linux/libi8042.h

drivers/input/serio/Kconfig +6 -1

index c3d05b4..a8ca89c 100644
@@ -25,10 +25,15 @@ config ARCH_MIGHT_HAVE_PC_SERIO
 
 if SERIO
 
+config SERIO_LIBI8042
+	tristate "i8042 driver library"
+	default y
+
 config SERIO_I8042
 	tristate "i8042 PC Keyboard controller"
 	default y
 	depends on ARCH_MIGHT_HAVE_PC_SERIO
+	select SERIO_LIBI8042
 	help
 	  i8042 is the chip over which the standard AT keyboard and PS/2
 	  mouse are connected to the computer. If you use these devices,

@@ -176,7 +181,7 @@ config SERIO_MACEPS2
 
 config SERIO_LIBPS2
 	tristate "PS/2 driver library"
-	depends on SERIO_I8042 || SERIO_I8042=n
+	depends on SERIO_LIBI8042 || SERIO_LIBI8042=n
 	help
 	  Say Y here if you are using a driver for device connected
 	  to a PS/2 port, such as PS/2 mouse or standard AT keyboard.

drivers/input/serio/Makefile +1 -0

index 2374ef9..b3a806f 100644
@@ -5,6 +5,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_SERIO)		+= serio.o
+obj-$(CONFIG_SERIO_LIBI8042)	+= libi8042.o
 obj-$(CONFIG_SERIO_I8042)	+= i8042.o
 obj-$(CONFIG_SERIO_PARKBD)	+= parkbd.o
 obj-$(CONFIG_SERIO_SERPORT)	+= serport.o

drivers/input/serio/i8042.c +0 ~~-48~~

69 lines not on this page · raw message

drivers/input/serio/i8042.h +0 -7

index 1db0a40..7de98ac 100644
@@ -54,13 +54,6 @@
 #define I8042_BUFFER_SIZE	16
 
 /*
- * Number of AUX ports on controllers supporting active multiplexing
- * specification
- */
-
-#define I8042_NUM_MUX_PORTS	4
-
-/*
  * Debug.
  */

drivers/input/serio/libi8042.c new file +60 -0

60 lines not on this page · raw message

drivers/input/serio/libps2.c +1 -1

index 316f2c8..62c0059 100644
@@ -17,7 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/input.h>
 #include <linux/serio.h>
-#include <linux/i8042.h>
+#include <linux/libi8042.h>
 #include <linux/libps2.h>
 
 #define DRIVER_DESC	"PS/2 driver library"

include/linux/i8042.h +1 ~~-16~~

index 0f9bafa..77aed40 100644
@@ -8,6 +8,7 @@
  */
 
 #include <linux/types.h>
+#include <linux/libi8042.h>
 
 /*
  * Standard commands.

@@ -59,10 +60,7 @@ struct serio;
 
 #if defined(CONFIG_SERIO_I8042) || defined(CONFIG_SERIO_I8042_MODULE)
 
-void i8042_lock_chip(void);
-void i8042_unlock_chip(void);
 int i8042_command(unsigned char *param, int command);
-bool i8042_check_port_owner(const struct serio *);
 int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
 					struct serio *serio));
 int i8042_remove_filter(bool (*filter)(unsigned char data, unsigned char str,

@@ -70,24 +68,11 @@ int i8042_remove_filter(bool (*filter)(unsigned char data, unsigned char str,
 
 #else
 
-static inline void i8042_lock_chip(void)
-{
-}
-
-static inline void i8042_unlock_chip(void)
-{
-}
-
 static inline int i8042_command(unsigned char *param, int command)
 {
 	return -ENODEV;
 }
 
-static inline bool i8042_check_port_owner(const struct serio *serio)
-{
-	return false;
-}
-
 static inline int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
 					struct serio *serio))
 {

include/linux/libi8042.h new file +55 -0

55 lines not on this page · raw message

Signature · 1 line

2.8.0