| Message ID | 341e0bb72d58c1c7d72ad5352f4bb364d939c0a8.1460985538.git.mdl@60hz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: > 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 <mdl@60hz.org> > --- > 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; 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 -- 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, Apr 19, 2016 at 1:54 AM, Dan Carpenter <dan.carpenter@oracle.com> 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
On Tue, Apr 19, 2016 at 02:24:47AM +0900, Mark Laws wrote: > On Tue, Apr 19, 2016 at 1:54 AM, Dan Carpenter <dan.carpenter@oracle.com> 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? > 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 -- 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, Apr 19, 2016 at 5:36 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Tue, Apr 19, 2016 at 02:24:47AM +0900, Mark Laws wrote: >> 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
On Tue, Apr 19, 2016 at 07:00:42AM +0900, Mark Laws wrote: > On Tue, Apr 19, 2016 at 5:36 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Apr 19, 2016 at 02:24:47AM +0900, Mark Laws wrote: > >> 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 -- 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, Apr 19, 2016 at 5:22 PM, Dan Carpenter <dan.carpenter@oracle.com> 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
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(-)
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);