[PATCH] 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 <decui@microsoft.com>
Reported-by: Claudio Latini <redacted>
Cc: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/input/serio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index bc2d474..3277bff 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig

@@ -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.

-- 
1.9.1

On Mon, Aug 11, 2014 at 08:30:40PM -0700, Dexuan Cui wrote:

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.

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

-----Original Message-----
From: Greg KH [mailto:gregkh@linuxfoundation.org]

quoted

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.

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

On Tue, Aug 12, 2014 at 05:51:28AM +0000, Dexuan Cui wrote:

quoted

-----Original Message-----
From: Greg KH [mailto:gregkh@linuxfoundation.org]

quoted

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

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.

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?

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?

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

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
owner@vger.kernel.org] On Behalf Of Greg KH

quoted

quoted

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

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

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

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

On Tue, Aug 12, 2014 at 07:15:25AM +0000, Dexuan Cui wrote:

quoted

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
owner@vger.kernel.org] On Behalf Of Greg KH

quoted

quoted

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

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

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

quoted

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.

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.

-- 
Dmitry

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Tuesday, August 12, 2014 10:55 AM
To: Dexuan Cui
Cc: Greg KH; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
driverdev-devel@linuxdriverproject.org; olaf@aepfle.de;
apw@canonical.com; jasowang@redhat.com; KY Srinivasan; Haiyang Zhang
Subject: Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on
SERIO_I8042=y

On Tue, Aug 12, 2014 at 07:15:25AM +0000, Dexuan Cui wrote:

quoted

quoted

-----Original Message-----
From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
owner@vger.kernel.org] On Behalf Of Greg KH

quoted

quoted

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

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

quoted

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

quoted

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.

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

-----Original Message-----
From: Dmitry Torokhov
Sent: Wednesday, August 13, 2014 1:55 AM

quoted

quoted

quoted

To decouple the dependency between the hyperv-keyboard and i8042
modules, I suppose we probably have to re-implement hyperv-

keyboard by

quoted

quoted

quoted

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

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.

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

-----Original Message-----
From: KY Srinivasan

quoted

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

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

quoted

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

On Wed, Aug 13, 2014 at 05:24:35AM +0000, Dexuan Cui wrote:

quoted

-----Original Message-----
From: Dmitry Torokhov
Sent: Wednesday, August 13, 2014 1:55 AM

quoted

quoted

quoted

To decouple the dependency between the hyperv-keyboard and i8042
modules, I suppose we probably have to re-implement hyperv-

keyboard by

quoted

quoted

quoted

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

quoted

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

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?

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.

-- 
Dmitry

-----Original Message-----
From: Dmitry Torokhov

quoted

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?

Ok, I got it.

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

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(-)

-- 
2.8.0

As explained in 1407814240-4275-1-git-send-email-decui@microsoft.com:

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)

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 <redacted>
---
 drivers/input/serio/i8042.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 4541957..4d49496 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c

@@ -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);

-- 
2.8.0

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?

On Tue, Apr 19, 2016 at 12:23:36AM +0900, Mark Laws wrote:

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

On Tue, Apr 19, 2016 at 1:54 AM, Dan Carpenter [off-list ref] wrote:

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?

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

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

On Tue, Apr 19, 2016 at 02:24:47AM +0900, Mark Laws wrote:

On Tue, Apr 19, 2016 at 1:54 AM, Dan Carpenter [off-list ref] wrote:

quoted

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

On Tue, Apr 19, 2016 at 5:36 AM, Dan Carpenter [off-list ref] wrote:

On Tue, Apr 19, 2016 at 02:24:47AM +0900, Mark Laws wrote:

quoted

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

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

On Tue, Apr 19, 2016 at 07:00:42AM +0900, Mark Laws wrote:

On Tue, Apr 19, 2016 at 5:36 AM, Dan Carpenter [off-list ref] wrote:

quoted

On Tue, Apr 19, 2016 at 02:24:47AM +0900, Mark Laws wrote:

quoted

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

On Tue, Apr 19, 2016 at 5:22 PM, Dan Carpenter [off-list ref] wrote:

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.

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

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

As explained in 1407814240-4275-1-git-send-email-decui@microsoft.com:

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)

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 <redacted>
---
 drivers/input/serio/i8042.c | 50 ++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 4541957..00f73d3 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c

@@ -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);

-- 
2.8.0

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(-)

-- 
2.8.0

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

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 4541957..4f0bc7c 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c

@@ -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);

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

-- 
2.8.0

As explained in 1407814240-4275-1-git-send-email-decui@microsoft.com:

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)

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

diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index c3d05b4..a8ca89c 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig

@@ -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.

diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 2374ef9..b3a806f 100644
--- a/drivers/input/serio/Makefile
+++ b/drivers/input/serio/Makefile

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

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 4541957..707bb19 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c

@@ -107,30 +107,9 @@ static char i8042_aux_firmware_id[128];
  */
 static DEFINE_SPINLOCK(i8042_lock);
 
-/*
- * Writers to AUX and KBD ports as well as users issuing i8042_command
- * directly should acquire i8042_mutex (by means of calling
- * i8042_lock_chip() and i8042_unlock_ship() helpers) to ensure that
- * they do not disturb each other (unfortunately in many i8042
- * implementations write to one of the ports will immediately abort
- * command that is being processed by another port).
- */
-static DEFINE_MUTEX(i8042_mutex);
-
-struct i8042_port {
-	struct serio *serio;
-	int irq;
-	bool exists;
-	bool driver_bound;
-	signed char mux;
-};
-
 #define I8042_KBD_PORT_NO	0
 #define I8042_AUX_PORT_NO	1
 #define I8042_MUX_PORT_NO	2
-#define I8042_NUM_PORTS		(I8042_NUM_MUX_PORTS + 2)
-
-static struct i8042_port i8042_ports[I8042_NUM_PORTS];
 
 static unsigned char i8042_initial_ctr;
 static unsigned char i8042_ctr;

@@ -145,18 +124,6 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id);
 static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
 				     struct serio *serio);
 
-void i8042_lock_chip(void)
-{
-	mutex_lock(&i8042_mutex);
-}
-EXPORT_SYMBOL(i8042_lock_chip);
-
-void i8042_unlock_chip(void)
-{
-	mutex_unlock(&i8042_mutex);
-}
-EXPORT_SYMBOL(i8042_unlock_chip);
-
 int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
 					struct serio *serio))
 {

@@ -1373,21 +1340,6 @@ static void i8042_unregister_ports(void)
 	}
 }
 
-/*
- * Checks whether port belongs to i8042 controller.
- */
-bool i8042_check_port_owner(const struct serio *port)
-{
-	int i;
-
-	for (i = 0; i < I8042_NUM_PORTS; i++)
-		if (i8042_ports[i].serio == port)
-			return true;
-
-	return false;
-}
-EXPORT_SYMBOL(i8042_check_port_owner);
-
 static void i8042_free_irqs(void)
 {
 	if (i8042_aux_irq_registered)

diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index 1db0a40..7de98ac 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h

@@ -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.
  */
 

diff --git a/drivers/input/serio/libi8042.c b/drivers/input/serio/libi8042.c
new file mode 100644
index 0000000..4505bac
--- /dev/null
+++ b/drivers/input/serio/libi8042.c

@@ -0,0 +1,60 @@
+/*
+ *  i8042 driver shared dependencies
+ *
+ *  Copyright (c) 1999-2004 Vojtech Pavlik
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/libi8042.h>
+
+MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
+MODULE_DESCRIPTION("i8042 driver shared dependencies");
+MODULE_LICENSE("GPL");
+
+/*
+ * Writers to AUX and KBD ports as well as users issuing i8042_command
+ * directly should acquire i8042_mutex (by means of calling
+ * i8042_lock_chip() and i8042_unlock_ship() helpers) to ensure that
+ * they do not disturb each other (unfortunately in many i8042
+ * implementations write to one of the ports will immediately abort
+ * command that is being processed by another port).
+ */
+static DEFINE_MUTEX(i8042_mutex);
+
+struct i8042_port i8042_ports[I8042_NUM_PORTS];
+EXPORT_SYMBOL(i8042_ports);
+
+void i8042_lock_chip(void)
+{
+	mutex_lock(&i8042_mutex);
+}
+EXPORT_SYMBOL(i8042_lock_chip);
+
+void i8042_unlock_chip(void)
+{
+	mutex_unlock(&i8042_mutex);
+}
+EXPORT_SYMBOL(i8042_unlock_chip);
+
+/*
+ * Checks whether port belongs to i8042 controller.
+ */
+bool i8042_check_port_owner(const struct serio *port)
+{
+	int i;
+
+	for (i = 0; i < I8042_NUM_PORTS; i++)
+		if (i8042_ports[i].serio == port)
+			return true;
+
+	return false;
+}
+EXPORT_SYMBOL(i8042_check_port_owner);

diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index 316f2c8..62c0059 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c

@@ -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"

diff --git a/include/linux/i8042.h b/include/linux/i8042.h
index 0f9bafa..77aed40 100644
--- a/include/linux/i8042.h
+++ b/include/linux/i8042.h

@@ -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))
 {

diff --git a/include/linux/libi8042.h b/include/linux/libi8042.h
new file mode 100644
index 0000000..5a730f0
--- /dev/null
+++ b/include/linux/libi8042.h

@@ -0,0 +1,55 @@
+#ifndef _LINUX_LIBI8042_H
+#define _LINUX_LIBI8042_H
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/types.h>
+
+/*
+ * Number of AUX ports on controllers supporting active multiplexing
+ * specification
+ */
+
+#define I8042_NUM_MUX_PORTS	4
+#define I8042_NUM_PORTS		(I8042_NUM_MUX_PORTS + 2)
+
+struct serio;
+
+struct i8042_port {
+	struct serio *serio;
+	int irq;
+	bool exists;
+	bool driver_bound;
+	signed char mux;
+};
+
+#if defined(CONFIG_SERIO_I8042) || defined(CONFIG_SERIO_I8042_MODULE)
+
+extern struct i8042_port i8042_ports[I8042_NUM_PORTS];
+
+void i8042_lock_chip(void);
+void i8042_unlock_chip(void);
+bool i8042_check_port_owner(const struct serio *);
+
+#else
+
+static inline void i8042_lock_chip(void)
+{
+}
+
+static inline void i8042_unlock_chip(void)
+{
+}
+
+static inline bool i8042_check_port_owner(const struct serio *serio)
+{
+	return false;
+}
+
+#endif
+
+#endif

-- 
2.8.0