| Message ID | 1407814240-4275-1-git-send-email-decui@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-)
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.
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(-)