[PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

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

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 <claudio.latini@live.com>
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

Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Greg KH]

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

RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Dexuan Cui]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > 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

Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Greg KH]

On Tue, Aug 12, 2014 at 05:51:28AM +0000, Dexuan Cui wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > 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.

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

RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Dexuan Cui]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Greg KH
> > > 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. :-(

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

Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Dmitry Torokhov]

On Tue, Aug 12, 2014 at 07:15:25AM +0000, Dexuan Cui wrote:
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> > owner@vger.kernel.org] On Behalf Of Greg KH
> > > > 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?

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

RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[KY Srinivasan]

> -----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:
> > > -----Original Message-----
> > > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> > > owner@vger.kernel.org] On Behalf Of Greg KH
> > > > > 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?
> 
> > > 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. :-(
> >
> > > > 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

RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Dexuan Cui]

> -----Original Message-----
> From: KY Srinivasan
> > 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.
> 
> K. Y

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

Thanks,
-- Dexuan

RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Dexuan Cui]

> -----Original Message-----
> From: Dmitry Torokhov
> Sent: Wednesday, August 13, 2014 1:55 AM
> > > > 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?

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

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

Re: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Dmitry Torokhov]

On Wed, Aug 13, 2014 at 05:24:35AM +0000, Dexuan Cui wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov
> > Sent: Wednesday, August 13, 2014 1:55 AM
> > > > > 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?
> 
> Hi Dmitry,
> Thanks for pointing this out!
> I didn't realize input_report_key() can't accept AT keyboard scan codes.
> 
> > > 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?

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

RE: [PATCH] Input: serio: make HYPERV_KEYBOARD depend on SERIO_I8042=y

[Dexuan Cui]

> -----Original Message-----
> From: Dmitry Torokhov
> > 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