Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs
So if the user inserts the module without a keyboard then in the
original code they would just put a keyboard in and try again. Now they
have to do an extra rmmod. What about if we just removed the test for
if the keyboard is present?
On Tue, Apr 19, 2016 at 12:23:36AM +0900, Mark Laws wrote:
> As explained in 1407814240-4275-1-git-send-email-de...@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
> ---
> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs
On Tue, Apr 19, 2016 at 1:54 AM, Dan Carpenter 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\ /\ |\ |< |_ /\ \^| // ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs
On Tue, Apr 19, 2016 at 02:24:47AM +0900, Mark Laws wrote: > On Tue, Apr 19, 2016 at 1:54 AM, Dan Carpenter > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs
On Tue, Apr 19, 2016 at 5:36 AM, Dan Carpenter 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 -- |v\ /\ |\ |< |_ /\ \^| // ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs
On Tue, Apr 19, 2016 at 07:00:42AM +0900, Mark Laws wrote: > On Tue, Apr 19, 2016 at 5:36 AM, Dan Carpenter > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs
On Tue, Apr 19, 2016 at 5:22 PM, Dan Carpenter 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\ /\ |\ |< |_ /\ \^| // ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Input: i8042 - Fix console keyboard support on Gen2 Hyper-V VMs
Why is platform_create_bundle() failing? It didn't fail in the first
version of the patch.
Btw, I'm not asking rhetorical questions, if I ask a question it means I
legitimately don't know the answer.
But I don't like this patch. Could you describe how you have tested it
with real hardware? What I want to know is that you loaded the module
without the hardware installed and then installed the hardware and got
it to work. You have made that more complicated and you've said that
you're willing to complicate life for those users slightly because it's
a trade off for fixing your bug... But that's sort of annoying and no
one has even tested how it works.
What I was really wondering last time was why can we not just do this?
Testing to see if the hardware is present is normally done in the
probe() function and not the init() function. I have not tested this
and I don't know what happens when we do this. Apparently, it causes
platform_create_bundle() to fail but I'm not sure why... Maybe the
create bundle call probe() and that fails?
How hard would it be to separate these things out into two modules
really? You say that you'd have to duplicate everything but maybe we
could instead just make the common functions into a library type thing..
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);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
