Delaying i8042 probe?

Delaying i8042 probe?

[Takashi Iwai]

Hi,

we've received a bug report about the missing keyboard on ASUS Zenbook
14 with AMD Ryzen:
  https://bugzilla.suse.com/show_bug.cgi?id=1190256

In short, PS2 keyboard couldn't be probed at the cold boot, while it
could be detected fine at the warm boot.  The failure appears like:

[    0.512668] i8042: PNP: PS/2 Controller [PNP0303:PS2K] at 0x60,0x64 irq 1
[    0.512672] i8042: PNP: PS/2 appears to have AUX port disabled, if this is incorrect please boot with i8042.nopnp
[    1.033609] i8042: Can't read CTR while initializing i8042
[    1.033632] i8042: probe of i8042 failed with error -5

As openSUSE kernel builds PS2 drivers as built-in, and the probe at
the early boot failed.  Meanwhile, when we rebuilt the kernel with
those drivers as modules, it starts working magically.  So, this is
likely a timing problem.

A possibly workaround I can think of would be to allow re-probing the
device at a later point.  Do we have a good way for that, or a better
alternative solution?


thanks,

Takashi

Re: Delaying i8042 probe?

[Fabio Estevam]

Hi Takashi,

On Fri, Sep 10, 2021 at 7:50 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> Hi,
>
> we've received a bug report about the missing keyboard on ASUS Zenbook
> 14 with AMD Ryzen:
>   https://bugzilla.suse.com/show_bug.cgi?id=1190256
>
> In short, PS2 keyboard couldn't be probed at the cold boot, while it
> could be detected fine at the warm boot.  The failure appears like:
>
> [    0.512668] i8042: PNP: PS/2 Controller [PNP0303:PS2K] at 0x60,0x64 irq 1
> [    0.512672] i8042: PNP: PS/2 appears to have AUX port disabled, if this is incorrect please boot with i8042.nopnp
> [    1.033609] i8042: Can't read CTR while initializing i8042
> [    1.033632] i8042: probe of i8042 failed with error -5
>
> As openSUSE kernel builds PS2 drivers as built-in, and the probe at
> the early boot failed.  Meanwhile, when we rebuilt the kernel with
> those drivers as modules, it starts working magically.  So, this is
> likely a timing problem.
>
> A possibly workaround I can think of would be to allow re-probing the
> device at a later point.  Do we have a good way for that, or a better
> alternative solution?

Would probe defer help here?

Something like this:

--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1549,7 +1549,7 @@ static int __init i8042_probe(struct platform_device *dev)

        error = i8042_controller_init();
        if (error)
-               return error;
+               return -EPROBE_DEFER;

 #ifdef CONFIG_X86

Re: Delaying i8042 probe?

[Takashi Iwai]

On Fri, 10 Sep 2021 13:07:17 +0200,
Fabio Estevam wrote:
> 
> Hi Takashi,
> 
> On Fri, Sep 10, 2021 at 7:50 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > Hi,
> >
> > we've received a bug report about the missing keyboard on ASUS Zenbook
> > 14 with AMD Ryzen:
> >   https://bugzilla.suse.com/show_bug.cgi?id=1190256
> >
> > In short, PS2 keyboard couldn't be probed at the cold boot, while it
> > could be detected fine at the warm boot.  The failure appears like:
> >
> > [    0.512668] i8042: PNP: PS/2 Controller [PNP0303:PS2K] at 0x60,0x64 irq 1
> > [    0.512672] i8042: PNP: PS/2 appears to have AUX port disabled, if this is incorrect please boot with i8042.nopnp
> > [    1.033609] i8042: Can't read CTR while initializing i8042
> > [    1.033632] i8042: probe of i8042 failed with error -5
> >
> > As openSUSE kernel builds PS2 drivers as built-in, and the probe at
> > the early boot failed.  Meanwhile, when we rebuilt the kernel with
> > those drivers as modules, it starts working magically.  So, this is
> > likely a timing problem.
> >
> > A possibly workaround I can think of would be to allow re-probing the
> > device at a later point.  Do we have a good way for that, or a better
> > alternative solution?
> 
> Would probe defer help here?
> 
> Something like this:
> 
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -1549,7 +1549,7 @@ static int __init i8042_probe(struct platform_device *dev)
> 
>         error = i8042_controller_init();
>         if (error)
> -               return error;
> +               return -EPROBE_DEFER;
> 
>  #ifdef CONFIG_X86
> 

Thanks, I'll try to build a test kernel and let you know the result.

Although we'd need some more restriction (as we don't want to repeat
endlessly for all devices), it's good to know whether the standard
deferred probe works or not in the first place.


Takashi

Re: Delaying i8042 probe?

[Fabio Estevam]

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

On Fri, Sep 10, 2021 at 9:07 AM Takashi Iwai <tiwai@suse.de> wrote:

> Thanks, I'll try to build a test kernel and let you know the result.

Just saw the report in bugzilla and it failed like this:

[    1.015028] i8042 i8042: probe deferral not supported

This is because the driver is currently registered via platform_create_bundle().

Changing it to platform_driver_register() would allow defer probe and hopefully
fix the problem.

Please find attached a new patch (only build-tested).

Regards,

Fabio Estevam

[-- Attachment #2: 0001-i8042.patch --]
[-- Type: text/x-patch, Size: 4755 bytes --]

From 8e00a4229637e2211b88c6bbe2c425ebff29950a Mon Sep 17 00:00:00 2001
From: Fabio Estevam <festevam@gmail.com>
Date: Fri, 10 Sep 2021 22:27:39 -0300
Subject: [PATCH] Input: i8042 - add probe defer support 

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/input/serio/i8042.c | 38 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 0b9f1d0a8f8b..2b69d2b5c568 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -711,7 +711,7 @@ static int i8042_set_mux_mode(bool multiplex, unsigned char *mux_version)
  * LCS/Telegraphics.
  */
 
-static int __init i8042_check_mux(void)
+static int i8042_check_mux(void)
 {
 	unsigned char mux_version;
 
@@ -740,10 +740,10 @@ static int __init i8042_check_mux(void)
 /*
  * The following is used to test AUX IRQ delivery.
  */
-static struct completion i8042_aux_irq_delivered __initdata;
-static bool i8042_irq_being_tested __initdata;
+static struct completion i8042_aux_irq_delivered;
+static bool i8042_irq_being_tested;
 
-static irqreturn_t __init i8042_aux_test_irq(int irq, void *dev_id)
+static irqreturn_t i8042_aux_test_irq(int irq, void *dev_id)
 {
 	unsigned long flags;
 	unsigned char str, data;
@@ -770,7 +770,7 @@ static irqreturn_t __init i8042_aux_test_irq(int irq, void *dev_id)
  * verifies success by readinng CTR. Used when testing for presence of AUX
  * port.
  */
-static int __init i8042_toggle_aux(bool on)
+static int i8042_toggle_aux(bool on)
 {
 	unsigned char param;
 	int i;
@@ -798,7 +798,7 @@ static int __init i8042_toggle_aux(bool on)
  * the presence of an AUX interface.
  */
 
-static int __init i8042_check_aux(void)
+static int i8042_check_aux(void)
 {
 	int retval = -1;
 	bool irq_registered = false;
@@ -1005,7 +1005,7 @@ static int i8042_controller_init(void)
 
 		if (i8042_command(&ctr[n++ % 2], I8042_CMD_CTL_RCTR)) {
 			pr_err("Can't read CTR while initializing i8042\n");
-			return -EIO;
+			return -EPROBE_DEFER;
 		}
 
 	} while (n < 2 || ctr[0] != ctr[1]);
@@ -1320,7 +1320,7 @@ static void i8042_shutdown(struct platform_device *dev)
 	i8042_controller_reset(false);
 }
 
-static int __init i8042_create_kbd_port(void)
+static int i8042_create_kbd_port(void)
 {
 	struct serio *serio;
 	struct i8042_port *port = &i8042_ports[I8042_KBD_PORT_NO];
@@ -1349,7 +1349,7 @@ static int __init i8042_create_kbd_port(void)
 	return 0;
 }
 
-static int __init i8042_create_aux_port(int idx)
+static int i8042_create_aux_port(int idx)
 {
 	struct serio *serio;
 	int port_no = idx < 0 ? I8042_AUX_PORT_NO : I8042_MUX_PORT_NO + idx;
@@ -1386,13 +1386,13 @@ static int __init i8042_create_aux_port(int idx)
 	return 0;
 }
 
-static void __init i8042_free_kbd_port(void)
+static void i8042_free_kbd_port(void)
 {
 	kfree(i8042_ports[I8042_KBD_PORT_NO].serio);
 	i8042_ports[I8042_KBD_PORT_NO].serio = NULL;
 }
 
-static void __init i8042_free_aux_ports(void)
+static void i8042_free_aux_ports(void)
 {
 	int i;
 
@@ -1402,7 +1402,7 @@ static void __init i8042_free_aux_ports(void)
 	}
 }
 
-static void __init i8042_register_ports(void)
+static void i8042_register_ports(void)
 {
 	int i;
 
@@ -1443,7 +1443,7 @@ static void i8042_free_irqs(void)
 	i8042_aux_irq_registered = i8042_kbd_irq_registered = false;
 }
 
-static int __init i8042_setup_aux(void)
+static int i8042_setup_aux(void)
 {
 	int (*aux_enable)(void);
 	int error;
@@ -1485,7 +1485,7 @@ static int __init i8042_setup_aux(void)
 	return error;
 }
 
-static int __init i8042_setup_kbd(void)
+static int i8042_setup_kbd(void)
 {
 	int error;
 
@@ -1535,7 +1535,7 @@ static int i8042_kbd_bind_notifier(struct notifier_block *nb,
 	return 0;
 }
 
-static int __init i8042_probe(struct platform_device *dev)
+static int i8042_probe(struct platform_device *dev)
 {
 	int error;
 
@@ -1600,6 +1600,7 @@ static struct platform_driver i8042_driver = {
 		.pm	= &i8042_pm_ops,
 #endif
 	},
+	.probe		= i8042_probe,
 	.remove		= i8042_remove,
 	.shutdown	= i8042_shutdown,
 };
@@ -1610,7 +1611,6 @@ static struct notifier_block i8042_kbd_bind_notifier_block = {
 
 static int __init i8042_init(void)
 {
-	struct platform_device *pdev;
 	int err;
 
 	dbg_init();
@@ -1626,11 +1626,9 @@ static int __init i8042_init(void)
 	/* Set this before creating the dev to allow i8042_command to work right away */
 	i8042_present = true;
 
-	pdev = platform_create_bundle(&i8042_driver, i8042_probe, NULL, 0, NULL, 0);
-	if (IS_ERR(pdev)) {
-		err = PTR_ERR(pdev);
+	err = platform_driver_register(&i8042_driver);
+	if (err)
 		goto err_platform_exit;
-	}
 
 	bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
 	panic_blink = i8042_panic_blink;
-- 
2.25.1

Re: Delaying i8042 probe?

[Takashi Iwai]

On Sat, 11 Sep 2021 03:36:21 +0200,
Fabio Estevam wrote:
> 
> On Fri, Sep 10, 2021 at 9:07 AM Takashi Iwai <tiwai@suse.de> wrote:
> 
> > Thanks, I'll try to build a test kernel and let you know the result.
> 
> Just saw the report in bugzilla and it failed like this:
> 
> [    1.015028] i8042 i8042: probe deferral not supported
> 
> This is because the driver is currently registered via platform_create_bundle().
> 
> Changing it to platform_driver_register() would allow defer probe and hopefully
> fix the problem.
> 
> Please find attached a new patch (only build-tested).

OK, I'll update and let the reporter testing it.


thanks,

Takashi

Re: Delaying i8042 probe?

[Fabio Estevam]

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

On Sat, Sep 11, 2021 at 4:32 AM Takashi Iwai <tiwai@suse.de> wrote:

> OK, I'll update and let the reporter testing it.

Sorry, platform_device_alloc() and platform_device_add() were missing
in the earlier patch.

New patch atached.

Dmitry, does this look correct?

Thanks

[-- Attachment #2: 0001-i8042v3.patch --]
[-- Type: text/x-patch, Size: 5923 bytes --]

From 327cdadd65cd764c8f1c68627bb1470fc95799af Mon Sep 17 00:00:00 2001
From: Fabio Estevam <festevam@gmail.com>
Date: Sat, 11 Sep 2021 15:39:05 -0300
Subject: [PATCH] Input: i8042 - add probe defer support

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/input/serio/i8042.c | 52 +++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 0b9f1d0a8f8b..36aaae931b14 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -173,6 +173,7 @@ static struct notifier_block i8042_kbd_bind_notifier_block;
 static irqreturn_t i8042_interrupt(int irq, void *dev_id);
 static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
 				     struct serio *serio);
+static struct platform_device *i8042_platform_device;
 
 void i8042_lock_chip(void)
 {
@@ -711,7 +712,7 @@ static int i8042_set_mux_mode(bool multiplex, unsigned char *mux_version)
  * LCS/Telegraphics.
  */
 
-static int __init i8042_check_mux(void)
+static int i8042_check_mux(void)
 {
 	unsigned char mux_version;
 
@@ -740,10 +741,10 @@ static int __init i8042_check_mux(void)
 /*
  * The following is used to test AUX IRQ delivery.
  */
-static struct completion i8042_aux_irq_delivered __initdata;
-static bool i8042_irq_being_tested __initdata;
+static struct completion i8042_aux_irq_delivered;
+static bool i8042_irq_being_tested;
 
-static irqreturn_t __init i8042_aux_test_irq(int irq, void *dev_id)
+static irqreturn_t i8042_aux_test_irq(int irq, void *dev_id)
 {
 	unsigned long flags;
 	unsigned char str, data;
@@ -770,7 +771,7 @@ static irqreturn_t __init i8042_aux_test_irq(int irq, void *dev_id)
  * verifies success by readinng CTR. Used when testing for presence of AUX
  * port.
  */
-static int __init i8042_toggle_aux(bool on)
+static int i8042_toggle_aux(bool on)
 {
 	unsigned char param;
 	int i;
@@ -798,7 +799,7 @@ static int __init i8042_toggle_aux(bool on)
  * the presence of an AUX interface.
  */
 
-static int __init i8042_check_aux(void)
+static int i8042_check_aux(void)
 {
 	int retval = -1;
 	bool irq_registered = false;
@@ -1005,7 +1006,7 @@ static int i8042_controller_init(void)
 
 		if (i8042_command(&ctr[n++ % 2], I8042_CMD_CTL_RCTR)) {
 			pr_err("Can't read CTR while initializing i8042\n");
-			return -EIO;
+			return -EPROBE_DEFER;
 		}
 
 	} while (n < 2 || ctr[0] != ctr[1]);
@@ -1320,7 +1321,7 @@ static void i8042_shutdown(struct platform_device *dev)
 	i8042_controller_reset(false);
 }
 
-static int __init i8042_create_kbd_port(void)
+static int i8042_create_kbd_port(void)
 {
 	struct serio *serio;
 	struct i8042_port *port = &i8042_ports[I8042_KBD_PORT_NO];
@@ -1349,7 +1350,7 @@ static int __init i8042_create_kbd_port(void)
 	return 0;
 }
 
-static int __init i8042_create_aux_port(int idx)
+static int i8042_create_aux_port(int idx)
 {
 	struct serio *serio;
 	int port_no = idx < 0 ? I8042_AUX_PORT_NO : I8042_MUX_PORT_NO + idx;
@@ -1366,6 +1367,7 @@ static int __init i8042_create_aux_port(int idx)
 	serio->ps2_cmd_mutex	= &i8042_mutex;
 	serio->port_data	= port;
 	serio->dev.parent	= &i8042_platform_device->dev;
+	serio->dev.parent	= &i8042_platform_device->dev;
 	if (idx < 0) {
 		strlcpy(serio->name, "i8042 AUX port", sizeof(serio->name));
 		strlcpy(serio->phys, I8042_AUX_PHYS_DESC, sizeof(serio->phys));
@@ -1386,13 +1388,13 @@ static int __init i8042_create_aux_port(int idx)
 	return 0;
 }
 
-static void __init i8042_free_kbd_port(void)
+static void i8042_free_kbd_port(void)
 {
 	kfree(i8042_ports[I8042_KBD_PORT_NO].serio);
 	i8042_ports[I8042_KBD_PORT_NO].serio = NULL;
 }
 
-static void __init i8042_free_aux_ports(void)
+static void i8042_free_aux_ports(void)
 {
 	int i;
 
@@ -1402,7 +1404,7 @@ static void __init i8042_free_aux_ports(void)
 	}
 }
 
-static void __init i8042_register_ports(void)
+static void i8042_register_ports(void)
 {
 	int i;
 
@@ -1443,7 +1445,7 @@ static void i8042_free_irqs(void)
 	i8042_aux_irq_registered = i8042_kbd_irq_registered = false;
 }
 
-static int __init i8042_setup_aux(void)
+static int i8042_setup_aux(void)
 {
 	int (*aux_enable)(void);
 	int error;
@@ -1485,7 +1487,7 @@ static int __init i8042_setup_aux(void)
 	return error;
 }
 
-static int __init i8042_setup_kbd(void)
+static int i8042_setup_kbd(void)
 {
 	int error;
 
@@ -1535,7 +1537,7 @@ static int i8042_kbd_bind_notifier(struct notifier_block *nb,
 	return 0;
 }
 
-static int __init i8042_probe(struct platform_device *dev)
+static int i8042_probe(struct platform_device *dev)
 {
 	int error;
 
@@ -1600,6 +1602,7 @@ static struct platform_driver i8042_driver = {
 		.pm	= &i8042_pm_ops,
 #endif
 	},
+	.probe		= i8042_probe,
 	.remove		= i8042_remove,
 	.shutdown	= i8042_shutdown,
 };
@@ -1610,7 +1613,6 @@ static struct notifier_block i8042_kbd_bind_notifier_block = {
 
 static int __init i8042_init(void)
 {
-	struct platform_device *pdev;
 	int err;
 
 	dbg_init();
@@ -1626,17 +1628,29 @@ static int __init i8042_init(void)
 	/* Set this before creating the dev to allow i8042_command to work right away */
 	i8042_present = true;
 
-	pdev = platform_create_bundle(&i8042_driver, i8042_probe, NULL, 0, NULL, 0);
-	if (IS_ERR(pdev)) {
-		err = PTR_ERR(pdev);
+	err = platform_driver_register(&i8042_driver);
+	if (err)
 		goto err_platform_exit;
+
+	i8042_platform_device = platform_device_alloc("i8042", -1);
+	if (!i8042_platform_device) {
+		err = -ENOMEM;
+		goto err_unregister_driver;
 	}
 
+	err = platform_device_add(i8042_platform_device);
+	if (err)
+		goto err_free_device;
+
 	bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
 	panic_blink = i8042_panic_blink;
 
 	return 0;
 
+err_free_device:
+	platform_device_put(i8042_platform_device);
+err_unregister_driver:
+	platform_driver_unregister(&i8042_driver);
  err_platform_exit:
 	i8042_platform_exit();
 	return err;
-- 
2.25.1

Re: Delaying i8042 probe?

[Fabio Estevam]

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

On Sat, Sep 11, 2021 at 3:43 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Sat, Sep 11, 2021 at 4:32 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> > OK, I'll update and let the reporter testing it.
>
> Sorry, platform_device_alloc() and platform_device_add() were missing
> in the earlier patch.
>
> New patch atached.
>
> Dmitry, does this look correct?

Please consider this one instead.

[-- Attachment #2: 0001-i8042v4.patch --]
[-- Type: text/x-patch, Size: 5199 bytes --]

From 7ecbc4761aa2b2a82d217e7a3cf285768fc85c75 Mon Sep 17 00:00:00 2001
From: Fabio Estevam <festevam@gmail.com>
Date: Sat, 11 Sep 2021 15:48:39 -0300
Subject: [PATCH] Input: i8042 - add probe defer support

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---
 drivers/input/serio/i8042.c | 50 +++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 0b9f1d0a8f8b..a72a8c538164 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -711,7 +711,7 @@ static int i8042_set_mux_mode(bool multiplex, unsigned char *mux_version)
  * LCS/Telegraphics.
  */
 
-static int __init i8042_check_mux(void)
+static int i8042_check_mux(void)
 {
 	unsigned char mux_version;
 
@@ -740,10 +740,10 @@ static int __init i8042_check_mux(void)
 /*
  * The following is used to test AUX IRQ delivery.
  */
-static struct completion i8042_aux_irq_delivered __initdata;
-static bool i8042_irq_being_tested __initdata;
+static struct completion i8042_aux_irq_delivered;
+static bool i8042_irq_being_tested;
 
-static irqreturn_t __init i8042_aux_test_irq(int irq, void *dev_id)
+static irqreturn_t i8042_aux_test_irq(int irq, void *dev_id)
 {
 	unsigned long flags;
 	unsigned char str, data;
@@ -770,7 +770,7 @@ static irqreturn_t __init i8042_aux_test_irq(int irq, void *dev_id)
  * verifies success by readinng CTR. Used when testing for presence of AUX
  * port.
  */
-static int __init i8042_toggle_aux(bool on)
+static int i8042_toggle_aux(bool on)
 {
 	unsigned char param;
 	int i;
@@ -798,7 +798,7 @@ static int __init i8042_toggle_aux(bool on)
  * the presence of an AUX interface.
  */
 
-static int __init i8042_check_aux(void)
+static int i8042_check_aux(void)
 {
 	int retval = -1;
 	bool irq_registered = false;
@@ -1005,7 +1005,7 @@ static int i8042_controller_init(void)
 
 		if (i8042_command(&ctr[n++ % 2], I8042_CMD_CTL_RCTR)) {
 			pr_err("Can't read CTR while initializing i8042\n");
-			return -EIO;
+			return -EPROBE_DEFER;
 		}
 
 	} while (n < 2 || ctr[0] != ctr[1]);
@@ -1320,7 +1320,7 @@ static void i8042_shutdown(struct platform_device *dev)
 	i8042_controller_reset(false);
 }
 
-static int __init i8042_create_kbd_port(void)
+static int i8042_create_kbd_port(void)
 {
 	struct serio *serio;
 	struct i8042_port *port = &i8042_ports[I8042_KBD_PORT_NO];
@@ -1349,7 +1349,7 @@ static int __init i8042_create_kbd_port(void)
 	return 0;
 }
 
-static int __init i8042_create_aux_port(int idx)
+static int i8042_create_aux_port(int idx)
 {
 	struct serio *serio;
 	int port_no = idx < 0 ? I8042_AUX_PORT_NO : I8042_MUX_PORT_NO + idx;
@@ -1386,13 +1386,13 @@ static int __init i8042_create_aux_port(int idx)
 	return 0;
 }
 
-static void __init i8042_free_kbd_port(void)
+static void i8042_free_kbd_port(void)
 {
 	kfree(i8042_ports[I8042_KBD_PORT_NO].serio);
 	i8042_ports[I8042_KBD_PORT_NO].serio = NULL;
 }
 
-static void __init i8042_free_aux_ports(void)
+static void i8042_free_aux_ports(void)
 {
 	int i;
 
@@ -1402,7 +1402,7 @@ static void __init i8042_free_aux_ports(void)
 	}
 }
 
-static void __init i8042_register_ports(void)
+static void i8042_register_ports(void)
 {
 	int i;
 
@@ -1443,7 +1443,7 @@ static void i8042_free_irqs(void)
 	i8042_aux_irq_registered = i8042_kbd_irq_registered = false;
 }
 
-static int __init i8042_setup_aux(void)
+static int i8042_setup_aux(void)
 {
 	int (*aux_enable)(void);
 	int error;
@@ -1485,7 +1485,7 @@ static int __init i8042_setup_aux(void)
 	return error;
 }
 
-static int __init i8042_setup_kbd(void)
+static int i8042_setup_kbd(void)
 {
 	int error;
 
@@ -1535,7 +1535,7 @@ static int i8042_kbd_bind_notifier(struct notifier_block *nb,
 	return 0;
 }
 
-static int __init i8042_probe(struct platform_device *dev)
+static int i8042_probe(struct platform_device *dev)
 {
 	int error;
 
@@ -1600,6 +1600,7 @@ static struct platform_driver i8042_driver = {
 		.pm	= &i8042_pm_ops,
 #endif
 	},
+	.probe		= i8042_probe,
 	.remove		= i8042_remove,
 	.shutdown	= i8042_shutdown,
 };
@@ -1610,7 +1611,6 @@ static struct notifier_block i8042_kbd_bind_notifier_block = {
 
 static int __init i8042_init(void)
 {
-	struct platform_device *pdev;
 	int err;
 
 	dbg_init();
@@ -1626,17 +1626,29 @@ static int __init i8042_init(void)
 	/* Set this before creating the dev to allow i8042_command to work right away */
 	i8042_present = true;
 
-	pdev = platform_create_bundle(&i8042_driver, i8042_probe, NULL, 0, NULL, 0);
-	if (IS_ERR(pdev)) {
-		err = PTR_ERR(pdev);
+	err = platform_driver_register(&i8042_driver);
+	if (err)
 		goto err_platform_exit;
+
+	i8042_platform_device = platform_device_alloc("i8042", -1);
+	if (!i8042_platform_device) {
+		err = -ENOMEM;
+		goto err_unregister_driver;
 	}
 
+	err = platform_device_add(i8042_platform_device);
+	if (err)
+		goto err_free_device;
+
 	bus_register_notifier(&serio_bus, &i8042_kbd_bind_notifier_block);
 	panic_blink = i8042_panic_blink;
 
 	return 0;
 
+err_free_device:
+	platform_device_put(i8042_platform_device);
+err_unregister_driver:
+	platform_driver_unregister(&i8042_driver);
  err_platform_exit:
 	i8042_platform_exit();
 	return err;
-- 
2.25.1

Re: Delaying i8042 probe?

[Dmitry Torokhov]

Hi Fabio,

On Sat, Sep 11, 2021 at 03:50:25PM -0300, Fabio Estevam wrote:
> On Sat, Sep 11, 2021 at 3:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > On Sat, Sep 11, 2021 at 4:32 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > > OK, I'll update and let the reporter testing it.
> >
> > Sorry, platform_device_alloc() and platform_device_add() were missing
> > in the earlier patch.
> >
> > New patch atached.
> >
> > Dmitry, does this look correct?
> 
> Please consider this one instead.

This is unfortunately is a band-aid. I wonder what other driver pokes
the embedded controller on these devices for it to start responding to
8042 queries...

Does the laptop have the latest BIOS? I wonder what ACPI tables look
like.

Thanks.

-- 
Dmitry

Re: Delaying i8042 probe?

[Takashi Iwai]

On Sat, 11 Sep 2021 20:50:25 +0200,
Fabio Estevam wrote:
> 
> On Sat, Sep 11, 2021 at 3:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > On Sat, Sep 11, 2021 at 4:32 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > > OK, I'll update and let the reporter testing it.
> >
> > Sorry, platform_device_alloc() and platform_device_add() were missing
> > in the earlier patch.
> >
> > New patch atached.
> >
> > Dmitry, does this look correct?
> 
> Please consider this one instead.

The patch was confirmed to work.
  https://bugzilla.suse.com/show_bug.cgi?id=1190256#c19

| That one worked despite the CTR error:
| 
| [    0.000000] Linux version 5.14.2-3.g9458b22-default (geeko@buildhost) (gcc (SUSE Linux) 11.2.1 20210816 [revision 056e324ce46a7924b5cf10f61010cf9dd2ca10e9], GNU ld (GNU Binutils; openSUSE Tumbleweed) 2.36.1.20210326-4) #1 SMP Sun Sep 12 20:15:58 UTC 2021 (9458b22)
| [    0.484315] i8042: PNP: PS/2 Controller [PNP0303:PS2K] at 0x60,0x64 irq 1
| [    0.484318] i8042: PNP: PS/2 appears to have AUX port disabled, if this is incorrect please boot with i8042.nopnp
| [    1.005361] i8042: Can't read CTR while initializing i8042
| [    1.477053] serio: i8042 KBD port at 0x60,0x64 irq 1
| [    1.503700] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input2
| 
| Seems it takes more than a secod to initialize, ugh

I'm not sure what's done during this 0.5 second period after CTR read
failure.  At least only shuffling the driver init order for built-in
drivers didn't suffice.

A full log was requested, but I don't think it'll give much more
information.


Takashi

Re: Delaying i8042 probe?

[Takashi Iwai]

On Sun, 12 Sep 2021 00:54:55 +0200,
Dmitry Torokhov wrote:
> 
> Hi Fabio,
> 
> On Sat, Sep 11, 2021 at 03:50:25PM -0300, Fabio Estevam wrote:
> > On Sat, Sep 11, 2021 at 3:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > On Sat, Sep 11, 2021 at 4:32 AM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > > OK, I'll update and let the reporter testing it.
> > >
> > > Sorry, platform_device_alloc() and platform_device_add() were missing
> > > in the earlier patch.
> > >
> > > New patch atached.
> > >
> > > Dmitry, does this look correct?
> > 
> > Please consider this one instead.
> 
> This is unfortunately is a band-aid. I wonder what other driver pokes
> the embedded controller on these devices for it to start responding to
> 8042 queries...
> 
> Does the laptop have the latest BIOS? I wonder what ACPI tables look
> like.

ACPI dump is included in hwinfo output,
  https://bugzilla.suse.com/show_bug.cgi?id=1190256#c1

If other format is required, let me know.  I thought this could be a
typical pinctrl thing, alas it doesn't look so.  The pinctrl-amd is
also built-in, and it's initialized before the input stuff...

And about BIOS: I don't think we can expect every user updates BIOS.
This report is not alone; as I checked through net, there have been a
few other reports for other distros like Arch.  On Arch, they "fixed"
the problem by reverting the config from built-in to modules (the bug
surfaced after their kconfig changes).

That said, even if it's a band-aid, we need some fix.  Can the
deferred probe be applied in some restricted manner, e.g. only for the
known buggy devices (and/or module option) and cap the max repeats?


thanks,

Takashi

Re: Delaying i8042 probe?

[Fabio Estevam]

Hi Takashi,

On Thu, Sep 16, 2021 at 6:16 AM Takashi Iwai <tiwai@suse.de> wrote:

> The patch was confirmed to work.

That's good news.

>   https://bugzilla.suse.com/show_bug.cgi?id=1190256#c19
>
> | That one worked despite the CTR error:
> |
> | [    0.000000] Linux version 5.14.2-3.g9458b22-default (geeko@buildhost) (gcc (SUSE Linux) 11.2.1 20210816 [revision 056e324ce46a7924b5cf10f61010cf9dd2ca10e9], GNU ld (GNU Binutils; openSUSE Tumbleweed) 2.36.1.20210326-4) #1 SMP Sun Sep 12 20:15:58 UTC 2021 (9458b22)
> | [    0.484315] i8042: PNP: PS/2 Controller [PNP0303:PS2K] at 0x60,0x64 irq 1
> | [    0.484318] i8042: PNP: PS/2 appears to have AUX port disabled, if this is incorrect please boot with i8042.nopnp
> | [    1.005361] i8042: Can't read CTR while initializing i8042
> | [    1.477053] serio: i8042 KBD port at 0x60,0x64 irq 1
> | [    1.503700] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input2
> |
> | Seems it takes more than a secod to initialize, ugh
>
> I'm not sure what's done during this 0.5 second period after CTR read
> failure.  At least only shuffling the driver init order for built-in
> drivers didn't suffice.

This extra 0.5 seconds is the time it takes to re-probe the driver at
a later stage after deferral.

I am not familiar with this driver and its BIOS dependencies, so let's
see what Dmitry suggests as a proper fix.

Thanks

Re: Delaying i8042 probe?

[Takashi Iwai]

On Thu, 16 Sep 2021 11:22:03 +0200,
Takashi Iwai wrote:
> 
> On Sun, 12 Sep 2021 00:54:55 +0200,
> Dmitry Torokhov wrote:
> > 
> > Hi Fabio,
> > 
> > On Sat, Sep 11, 2021 at 03:50:25PM -0300, Fabio Estevam wrote:
> > > On Sat, Sep 11, 2021 at 3:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> > > >
> > > > On Sat, Sep 11, 2021 at 4:32 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > >
> > > > > OK, I'll update and let the reporter testing it.
> > > >
> > > > Sorry, platform_device_alloc() and platform_device_add() were missing
> > > > in the earlier patch.
> > > >
> > > > New patch atached.
> > > >
> > > > Dmitry, does this look correct?
> > > 
> > > Please consider this one instead.
> > 
> > This is unfortunately is a band-aid. I wonder what other driver pokes
> > the embedded controller on these devices for it to start responding to
> > 8042 queries...
> > 
> > Does the laptop have the latest BIOS? I wonder what ACPI tables look
> > like.
> 
> ACPI dump is included in hwinfo output,
>   https://bugzilla.suse.com/show_bug.cgi?id=1190256#c1
> 
> If other format is required, let me know.  I thought this could be a
> typical pinctrl thing, alas it doesn't look so.  The pinctrl-amd is
> also built-in, and it's initialized before the input stuff...
> 
> And about BIOS: I don't think we can expect every user updates BIOS.
> This report is not alone; as I checked through net, there have been a
> few other reports for other distros like Arch.  On Arch, they "fixed"
> the problem by reverting the config from built-in to modules (the bug
> surfaced after their kconfig changes).
> 
> That said, even if it's a band-aid, we need some fix.  Can the
> deferred probe be applied in some restricted manner, e.g. only for the
> known buggy devices (and/or module option) and cap the max repeats?

Dmitry, what is your take for fixing this bug?  

I find Fabio's deferred probe patch reasonable.  Maybe we may restrict
the application of the deferred probe only for known working devices
and an option, something like a patch below, just to be safer.
(There are devices exposing PnP for i8042 although there isn't really,
IIRC.)


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] Input: i8042 - Perform deferred probe only with DMI list and
 option

Add an i8042.probe_defer option to enable the deferred probing
feature, otherwise it'll be disabled and done in a single shot like
before.  For the known devices that need the deferred probe, we
provide a DMI-based allow-list.  As of this patch, ASUS ZenBook
UX425UA is added there.

BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1190256
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 Documentation/admin-guide/kernel-parameters.txt |  2 ++
 drivers/input/serio/i8042-x86ia64io.h           | 14 ++++++++++++++
 drivers/input/serio/i8042.c                     |  6 +++++-
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 91ba391f9b32..6e6622d8f4a0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1690,6 +1690,8 @@
 			architectures force reset to be always executed
 	i8042.unlock	[HW] Unlock (ignore) the keylock
 	i8042.kbdreset	[HW] Reset device connected to KBD port
+	i8042.probe_defer
+			[HW] Allow deferred probing upon i8042 probe errors
 
 	i810=		[HW,DRM]
 
diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
index a5a003553646..41335a1d7001 100644
--- a/drivers/input/serio/i8042-x86ia64io.h
+++ b/drivers/input/serio/i8042-x86ia64io.h
@@ -981,6 +981,17 @@ static const struct dmi_system_id __initconst i8042_dmi_kbdreset_table[] = {
 	{ }
 };
 
+static const struct dmi_system_id i8042_dmi_probe_defer_table[] __initconst = {
+	{
+		/* ASUS ZenBook UX425UA */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX425UA"),
+		},
+	},
+	{ }
+};
+
 #endif /* CONFIG_X86 */
 
 #ifdef CONFIG_PNP
@@ -1301,6 +1312,9 @@ static int __init i8042_platform_init(void)
 	if (dmi_check_system(i8042_dmi_kbdreset_table))
 		i8042_kbdreset = true;
 
+	if (dmi_check_system(i8042_dmi_probe_defer_table))
+		i8042_probe_defer = true;
+
 	/*
 	 * A20 was already enabled during early kernel init. But some buggy
 	 * BIOSes (in MSI Laptops) require A20 to be enabled using 8042 to
diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index a72a8c538164..ea0c52ca95c4 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -45,6 +45,10 @@ static bool i8042_unlock;
 module_param_named(unlock, i8042_unlock, bool, 0);
 MODULE_PARM_DESC(unlock, "Ignore keyboard lock.");
 
+static bool i8042_probe_defer;
+module_param_named(probe_defer, i8042_probe_defer, bool, 0);
+MODULE_PARM_DESC(unlock, "Allow deferred probing.");
+
 enum i8042_controller_reset_mode {
 	I8042_RESET_NEVER,
 	I8042_RESET_ALWAYS,
@@ -1005,7 +1009,7 @@ static int i8042_controller_init(void)
 
 		if (i8042_command(&ctr[n++ % 2], I8042_CMD_CTL_RCTR)) {
 			pr_err("Can't read CTR while initializing i8042\n");
-			return -EPROBE_DEFER;
+			return i8042_probe_defer ? -EPROBE_DEFER : -EIO;
 		}
 
 	} while (n < 2 || ctr[0] != ctr[1]);
-- 
2.31.1

Re: Delaying i8042 probe?

[Takashi Iwai]

On Tue, 05 Oct 2021 17:43:23 +0200,
Takashi Iwai wrote:
> 
> On Thu, 16 Sep 2021 11:22:03 +0200,
> Takashi Iwai wrote:
> > 
> > On Sun, 12 Sep 2021 00:54:55 +0200,
> > Dmitry Torokhov wrote:
> > > 
> > > Hi Fabio,
> > > 
> > > On Sat, Sep 11, 2021 at 03:50:25PM -0300, Fabio Estevam wrote:
> > > > On Sat, Sep 11, 2021 at 3:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> > > > >
> > > > > On Sat, Sep 11, 2021 at 4:32 AM Takashi Iwai <tiwai@suse.de> wrote:
> > > > >
> > > > > > OK, I'll update and let the reporter testing it.
> > > > >
> > > > > Sorry, platform_device_alloc() and platform_device_add() were missing
> > > > > in the earlier patch.
> > > > >
> > > > > New patch atached.
> > > > >
> > > > > Dmitry, does this look correct?
> > > > 
> > > > Please consider this one instead.
> > > 
> > > This is unfortunately is a band-aid. I wonder what other driver pokes
> > > the embedded controller on these devices for it to start responding to
> > > 8042 queries...
> > > 
> > > Does the laptop have the latest BIOS? I wonder what ACPI tables look
> > > like.
> > 
> > ACPI dump is included in hwinfo output,
> >   https://bugzilla.suse.com/show_bug.cgi?id=1190256#c1
> > 
> > If other format is required, let me know.  I thought this could be a
> > typical pinctrl thing, alas it doesn't look so.  The pinctrl-amd is
> > also built-in, and it's initialized before the input stuff...
> > 
> > And about BIOS: I don't think we can expect every user updates BIOS.
> > This report is not alone; as I checked through net, there have been a
> > few other reports for other distros like Arch.  On Arch, they "fixed"
> > the problem by reverting the config from built-in to modules (the bug
> > surfaced after their kconfig changes).
> > 
> > That said, even if it's a band-aid, we need some fix.  Can the
> > deferred probe be applied in some restricted manner, e.g. only for the
> > known buggy devices (and/or module option) and cap the max repeats?
> 
> Dmitry, what is your take for fixing this bug?  

Any news for this?  The bug seem still there unfixed.


thanks,

Takashi

> I find Fabio's deferred probe patch reasonable.  Maybe we may restrict
> the application of the deferred probe only for known working devices
> and an option, something like a patch below, just to be safer.
> (There are devices exposing PnP for i8042 although there isn't really,
> IIRC.)
> 
> 
> thanks,
> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] Input: i8042 - Perform deferred probe only with DMI list and
>  option
> 
> Add an i8042.probe_defer option to enable the deferred probing
> feature, otherwise it'll be disabled and done in a single shot like
> before.  For the known devices that need the deferred probe, we
> provide a DMI-based allow-list.  As of this patch, ASUS ZenBook
> UX425UA is added there.
> 
> BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1190256
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  2 ++
>  drivers/input/serio/i8042-x86ia64io.h           | 14 ++++++++++++++
>  drivers/input/serio/i8042.c                     |  6 +++++-
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 91ba391f9b32..6e6622d8f4a0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1690,6 +1690,8 @@
>  			architectures force reset to be always executed
>  	i8042.unlock	[HW] Unlock (ignore) the keylock
>  	i8042.kbdreset	[HW] Reset device connected to KBD port
> +	i8042.probe_defer
> +			[HW] Allow deferred probing upon i8042 probe errors
>  
>  	i810=		[HW,DRM]
>  
> diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
> index a5a003553646..41335a1d7001 100644
> --- a/drivers/input/serio/i8042-x86ia64io.h
> +++ b/drivers/input/serio/i8042-x86ia64io.h
> @@ -981,6 +981,17 @@ static const struct dmi_system_id __initconst i8042_dmi_kbdreset_table[] = {
>  	{ }
>  };
>  
> +static const struct dmi_system_id i8042_dmi_probe_defer_table[] __initconst = {
> +	{
> +		/* ASUS ZenBook UX425UA */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX425UA"),
> +		},
> +	},
> +	{ }
> +};
> +
>  #endif /* CONFIG_X86 */
>  
>  #ifdef CONFIG_PNP
> @@ -1301,6 +1312,9 @@ static int __init i8042_platform_init(void)
>  	if (dmi_check_system(i8042_dmi_kbdreset_table))
>  		i8042_kbdreset = true;
>  
> +	if (dmi_check_system(i8042_dmi_probe_defer_table))
> +		i8042_probe_defer = true;
> +
>  	/*
>  	 * A20 was already enabled during early kernel init. But some buggy
>  	 * BIOSes (in MSI Laptops) require A20 to be enabled using 8042 to
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index a72a8c538164..ea0c52ca95c4 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -45,6 +45,10 @@ static bool i8042_unlock;
>  module_param_named(unlock, i8042_unlock, bool, 0);
>  MODULE_PARM_DESC(unlock, "Ignore keyboard lock.");
>  
> +static bool i8042_probe_defer;
> +module_param_named(probe_defer, i8042_probe_defer, bool, 0);
> +MODULE_PARM_DESC(unlock, "Allow deferred probing.");
> +
>  enum i8042_controller_reset_mode {
>  	I8042_RESET_NEVER,
>  	I8042_RESET_ALWAYS,
> @@ -1005,7 +1009,7 @@ static int i8042_controller_init(void)
>  
>  		if (i8042_command(&ctr[n++ % 2], I8042_CMD_CTL_RCTR)) {
>  			pr_err("Can't read CTR while initializing i8042\n");
> -			return -EPROBE_DEFER;
> +			return i8042_probe_defer ? -EPROBE_DEFER : -EIO;
>  		}
>  
>  	} while (n < 2 || ctr[0] != ctr[1]);
> -- 
> 2.31.1
>