Re: [PATCH 1/2] input/serio/i8042: Skip selftest on i8042 controller in some ASUS laptops

Re: [PATCH 1/2] input/serio/i8042: Skip selftest on i8042 controller in some ASUS laptops

[Dmitry Torokhov]

[ resending to all - mutt is for some reason confused about recipent
list ]
On Mon, Jul 25, 2016 at 02:20:16PM -0300, Marcos Paulo de Souza wrote:
> On suspend/resume cycle, selftest is executed on to reset i8042 controller. But
> when this is done in these devices, posterior calls to detect/init functions
> to elantech driver fails. Skipping selftest fixes this problem.
> 
> An easier step to reproduce this problem is adding i8042.reset=1 as a kernel
> parameter. On problematic laptops, it'll make the system to start with the
> touchpad already stucked, since psmouse_probe forcibly calls the
> selftest function.
> 
> This patch was inspired by John Hiesey's change[1].
> 
> [1]: https://marc.info/?l=linux-input&m=144312209020616&w=2
> 
> Fixes: "ETPS/2 Elantech Touchpad dies after resume from suspend" (https://bugzilla.kernel.org/show_bug.cgi?id=7739)

*sigh* You just cant win, they'll manage to mess up the firmware one way
or another :( Witness:

"
commit 1ca56e513a9fd356d5a9e0de45dbe0e189e00386
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date:   Tue Jul 20 20:25:34 2010 -0700

    Input: i8042 - reset keyboard controller wehen resuming from S2R

    Some laptops, such as Lenovo 3000 N100, require keyboard controller reset
    in order to have touchpad operable after suspend to RAM. Even if box does
    not need the reset it should be safe to do so, so instead of chasing
    after misbehaving boxes and grow DMI tables, let's reset the controller
    unconditionally.

    Reported-and-tested-by: Jerome Lacoste <jerome.lacoste@gmail.com>
    Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
"

so apparently it is not safe to reset the controller on resume. Oh joy.

The issue I have here is selftest is the same as reset, and we already
have i8042_reset flag, so 2 flags controlling the same functionality is
not great. Could we extend i8042 to an enum, like:

I8042_RESET_ALWAYS
I8042_RESET_NEVER
I8042_RESET_ON_RESUME (default)

and have a custom module parameter for it?

i8042.reset and i8042_reset={1|Y|y} would result in I8042_RESET_ALWAYS
i8042.reset={0|N|n} would result in I8042_RESET_NEVER
Without i8042.reset we just reset it when resuming from S3.

What do you think?

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] input/serio/i8042: Skip selftest on i8042 controller in some ASUS laptops

[Marcos Souza]

[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]

Hi Dmitry,

On Mon, Jul 25, 2016 at 3:48 PM, Marcos Souza
<marcos.souza.org@gmail.com> wrote:
> Hi Dmitry,
>
> On Mon, Jul 25, 2016 at 3:22 PM Dmitry Torokhov <dmitry.torokhov@gmail.com>
> wrote:
>>
>> [ resending to all - mutt is for some reason confused about recipent
>> list ]
>> On Mon, Jul 25, 2016 at 02:20:16PM -0300, Marcos Paulo de Souza wrote:
>> > On suspend/resume cycle, selftest is executed on to reset i8042
>> > controller. But
>> > when this is done in these devices, posterior calls to detect/init
>> > functions
>> > to elantech driver fails. Skipping selftest fixes this problem.
>> >
>> > An easier step to reproduce this problem is adding i8042.reset=1 as a
>> > kernel
>> > parameter. On problematic laptops, it'll make the system to start with
>> > the
>> > touchpad already stucked, since psmouse_probe forcibly calls the
>> > selftest function.
>> >
>> > This patch was inspired by John Hiesey's change[1].
>> >
>> > [1]: https://marc.info/?l=linux-input&m=144312209020616&w=2
>> >
>> > Fixes: "ETPS/2 Elantech Touchpad dies after resume from suspend"
>> > (https://bugzilla.kernel.org/show_bug.cgi?id=7739)
>>
>> *sigh* You just cant win, they'll manage to mess up the firmware one way
>> or another :( Witness:
>
>
> They're experts on messing things up :(
>
>>
>>
>> "
>> commit 1ca56e513a9fd356d5a9e0de45dbe0e189e00386
>> Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Date:   Tue Jul 20 20:25:34 2010 -0700
>>
>>     Input: i8042 - reset keyboard controller wehen resuming from S2R
>>
>>     Some laptops, such as Lenovo 3000 N100, require keyboard controller
>> reset
>>     in order to have touchpad operable after suspend to RAM. Even if box
>> does
>>     not need the reset it should be safe to do so, so instead of chasing
>>     after misbehaving boxes and grow DMI tables, let's reset the
>> controller
>>     unconditionally.
>>
>>     Reported-and-tested-by: Jerome Lacoste <jerome.lacoste@gmail.com>
>>     Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>> "
>>
>> so apparently it is not safe to reset the controller on resume. Oh joy.
>>
>> The issue I have here is selftest is the same as reset, and we already
>> have i8042_reset flag, so 2 flags controlling the same functionality is
>> not great. Could we extend i8042 to an enum, like:
>>
>> I8042_RESET_ALWAYS
>> I8042_RESET_NEVER
>> I8042_RESET_ON_RESUME (default)
>>
>> and have a custom module parameter for it?
>>
>> i8042.reset and i8042_reset={1|Y|y} would result in I8042_RESET_ALWAYS
>> i8042.reset={0|N|n} would result in I8042_RESET_NEVER
>> Without i8042.reset we just reset it when resuming from S3.
>>
>> What do you think?
>
>
> Seems to be a very good approach. I'll send a new version.

You asked me to adapt my previous patch, making it accept:

i8042.reset
i8042.reset={1,y,Y}
i8042.reset={0,n,N}

But, when using a charp as module parameter, it doesn't allocate
memory to i8042_reset when we don't use the equal sign. So, in this
case I can't tell if the user is using i8042.reset, or don't any
parameter (in this case it means to use I8042_RESET_ON_RESUME)

So, in this case, should we just change i8042.reset to accept 1,y,n?
This should solve the problem too (although I think people who already
use such parameter will complain...)

What do you think?

I attached my current diff here, by using a new kernel parameter
reset_mode. Besides the new parameter,It works great if I remove the
comment about the ASUS DMI info.

Thanks,

>
>>
>>
>> Thanks.
>>
>> --
>> Dmitry



-- 
Att,

Marcos Paulo de Souza
Github: https://github.com/marcosps/

[-- Attachment #2: i8042_controller_reset_fix.patch --]
[-- Type: text/x-patch, Size: 4617 bytes --]

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c9..ac0d5cd 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1431,6 +1431,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			     controllers
 	i8042.notimeout	[HW] Ignore timeout condition signalled by controller
 	i8042.reset	[HW] Reset the controller during init and cleanup
+	i8042.reset_mode
+			[HW] Reset the controller, always, never or just on
+			resume
+			Format: { 1 | Y | y | 0 | N | n }
+			1, Y, y: Always reset controller (same as the i8042.reset)
+			0, N, n: Never reset controller
+			Default: Just reset on S3 resume
 	i8042.unlock	[HW] Unlock (ignore) the keylock
 	i8042.kbdreset  [HW] Reset device connected to KBD port
 
diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
index 68f5f4a..40a78d1 100644
--- a/drivers/input/serio/i8042-x86ia64io.h
+++ b/drivers/input/serio/i8042-x86ia64io.h
@@ -510,6 +510,60 @@ static const struct dmi_system_id __initconst i8042_dmi_nomux_table[] = {
 	{ }
 };
 
+/*
+ * On some hardware just running the self test causes problems.
+ */
+static const struct dmi_system_id __initconst i8042_dmi_noselftest_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "K401LB"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "K501LX"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "K501LD"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "X302LA"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "X450LCP"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "X450LD"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "X455LAB"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "X455LDB"),
+		},
+	},
+	{ }
+};
 static const struct dmi_system_id __initconst i8042_dmi_reset_table[] = {
 	{
 		/* MSI Wind U-100 */
@@ -1077,7 +1131,11 @@ static int __init i8042_platform_init(void)
 
 #ifdef CONFIG_X86
 	if (dmi_check_system(i8042_dmi_reset_table))
-		i8042_reset = true;
+		i8042_reset_mode = I8042_RESET_ALWAYS;
+
+	// just disable controller reset
+	//if (dmi_check_system(i8042_dmi_noselftest_table))
+	//	i8042_reset_mode = I8042_RESET_NEVER;
 
 	if (dmi_check_system(i8042_dmi_noloop_table))
 		i8042_noloop = true;
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 4541957..cfc1590 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -52,6 +52,17 @@ static bool i8042_reset;
 module_param_named(reset, i8042_reset, bool, 0);
 MODULE_PARM_DESC(reset, "Reset controller during init and cleanup.");
 
+enum i8042_controller_reset_mode {
+	I8042_RESET_NEVER,
+	I8042_RESET_ALWAYS,
+	I8042_RESET_ON_RESUME
+};
+
+static unsigned int i8042_reset_mode = I8042_RESET_ON_RESUME;
+static char *i8042_reset_param;
+module_param_named(reset_mode, i8042_reset_param, charp, 0);
+MODULE_PARM_DESC(reset_mode, "Reset controller mode, being always, never or just on resume");
+
 static bool i8042_direct;
 module_param_named(direct, i8042_direct, bool, 0);
 MODULE_PARM_DESC(direct, "Put keyboard port into non-translated mode.");
@@ -890,6 +901,9 @@ static int i8042_controller_selftest(void)
 	unsigned char param;
 	int i = 0;
 
+	if (i8042_reset_mode == I8042_RESET_NEVER)
+		return 0;
+
 	/*
 	 * We try this 5 times; on some really fragile systems this does not
 	 * take the first time...
@@ -1495,7 +1509,20 @@ static int __init i8042_probe(struct platform_device *dev)
 
 	i8042_platform_device = dev;
 
-	if (i8042_reset) {
+	if (i8042_reset_param != NULL) {
+		if (!strncmp(i8042_reset_param, "1", 1)
+			|| !strncasecmp(i8042_reset_param, "y", 1)) {
+			i8042_reset_mode = I8042_RESET_ALWAYS;
+		} else if (!strncmp(i8042_reset_param, "0", 1)
+			|| !strncasecmp(i8042_reset_param, "n", 1)) {
+			i8042_reset_mode = I8042_RESET_NEVER;
+		}
+	}
+
+	if (i8042_reset)
+		i8042_reset_mode = I8042_RESET_ALWAYS;
+
+	if (i8042_reset_mode == I8042_RESET_ALWAYS) {
 		error = i8042_controller_selftest();
 		if (error)
 			return error;

Re: [PATCH 1/2] input/serio/i8042: Skip selftest on i8042 controller in some ASUS laptops

[Dmitry Torokhov]

On Tue, Jul 26, 2016 at 6:13 PM, Marcos Souza
<marcos.souza.org@gmail.com> wrote:
> Hi Dmitry,
>
> On Mon, Jul 25, 2016 at 3:48 PM, Marcos Souza
> <marcos.souza.org@gmail.com> wrote:
>> Hi Dmitry,
>>
>> On Mon, Jul 25, 2016 at 3:22 PM Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> wrote:
>>>
>>> [ resending to all - mutt is for some reason confused about recipent
>>> list ]
>>> On Mon, Jul 25, 2016 at 02:20:16PM -0300, Marcos Paulo de Souza wrote:
>>> > On suspend/resume cycle, selftest is executed on to reset i8042
>>> > controller. But
>>> > when this is done in these devices, posterior calls to detect/init
>>> > functions
>>> > to elantech driver fails. Skipping selftest fixes this problem.
>>> >
>>> > An easier step to reproduce this problem is adding i8042.reset=1 as a
>>> > kernel
>>> > parameter. On problematic laptops, it'll make the system to start with
>>> > the
>>> > touchpad already stucked, since psmouse_probe forcibly calls the
>>> > selftest function.
>>> >
>>> > This patch was inspired by John Hiesey's change[1].
>>> >
>>> > [1]: https://marc.info/?l=linux-input&m=144312209020616&w=2
>>> >
>>> > Fixes: "ETPS/2 Elantech Touchpad dies after resume from suspend"
>>> > (https://bugzilla.kernel.org/show_bug.cgi?id=7739)
>>>
>>> *sigh* You just cant win, they'll manage to mess up the firmware one way
>>> or another :( Witness:
>>
>>
>> They're experts on messing things up :(
>>
>>>
>>>
>>> "
>>> commit 1ca56e513a9fd356d5a9e0de45dbe0e189e00386
>>> Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Date:   Tue Jul 20 20:25:34 2010 -0700
>>>
>>>     Input: i8042 - reset keyboard controller wehen resuming from S2R
>>>
>>>     Some laptops, such as Lenovo 3000 N100, require keyboard controller
>>> reset
>>>     in order to have touchpad operable after suspend to RAM. Even if box
>>> does
>>>     not need the reset it should be safe to do so, so instead of chasing
>>>     after misbehaving boxes and grow DMI tables, let's reset the
>>> controller
>>>     unconditionally.
>>>
>>>     Reported-and-tested-by: Jerome Lacoste <jerome.lacoste@gmail.com>
>>>     Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>>> "
>>>
>>> so apparently it is not safe to reset the controller on resume. Oh joy.
>>>
>>> The issue I have here is selftest is the same as reset, and we already
>>> have i8042_reset flag, so 2 flags controlling the same functionality is
>>> not great. Could we extend i8042 to an enum, like:
>>>
>>> I8042_RESET_ALWAYS
>>> I8042_RESET_NEVER
>>> I8042_RESET_ON_RESUME (default)
>>>
>>> and have a custom module parameter for it?
>>>
>>> i8042.reset and i8042_reset={1|Y|y} would result in I8042_RESET_ALWAYS
>>> i8042.reset={0|N|n} would result in I8042_RESET_NEVER
>>> Without i8042.reset we just reset it when resuming from S3.
>>>
>>> What do you think?
>>
>>
>> Seems to be a very good approach. I'll send a new version.
>
> You asked me to adapt my previous patch, making it accept:
>
> i8042.reset
> i8042.reset={1,y,Y}
> i8042.reset={0,n,N}
>
> But, when using a charp as module parameter, it doesn't allocate
> memory to i8042_reset when we don't use the equal sign. So, in this
> case I can't tell if the user is using i8042.reset, or don't any
> parameter (in this case it means to use I8042_RESET_ON_RESUME)
>
> So, in this case, should we just change i8042.reset to accept 1,y,n?
> This should solve the problem too (although I think people who already
> use such parameter will complain...)
>
> What do you think?

I think you will have to provide custom parameter parsing method.
psmouse module has one, so you can use it as an example.

Thanks.

-- 
Dmitry