Message-ID: <31feee1c-f1a2-43cd-a2cd-ee3fc30c0609@amd.com>
Date:   Wed, 6 Dec 2023 14:06:24 -0600
From:   Mario Limonciello <mario.limonciello@....com>
To:     Rahul Rameshbabu <rrameshbabu@...dia.com>
Cc:     dmitry.torokhov@...il.com, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: i8042: Quiet down probe failure messages

On 12/6/2023 13:55, Rahul Rameshbabu wrote:
> On Wed, 06 Dec, 2023 13:22:25 -0600 Mario Limonciello <mario.limonciello@....com> wrote:
>> On 12/6/2023 12:46, Rahul Rameshbabu wrote:
>>> On Wed, 06 Dec, 2023 11:58:18 -0600 Mario Limonciello <mario.limonciello@....com> wrote:
>>>> The Framework 16" laptop doesn't have a PS/2 keyboard. At bootup the
>>>> following messages are emitted:
>>>>
>>>> i8042: PNP: No PS/2 controller found.
>>>> i8042: PNP: Probing ports directly.
>>>> i8042: Can't read CTR while initializing i8042
>>>> i8042: probe of i8042 failed with error -5
>>>>
>>>> The last two messages are ERR and WARN respectively.  These messages
>>>> might be useful for one boot while diagnosing a problem for someone
>>>> but as there is no PS/2 controller in PNP or on the machine they're
>>>> needlessly noisy to emit every boot.
>>>>
>>>> Downgrade the CTR message to debug and change the error code for the
>>>> failure so that the base device code doesn't emit a warning.
>>>>
>>>> If someone has problems with i8042 and they need this information,
>>>> they can turn on dynamic debugging to get these messages.
>>>>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>>> ---
>>> For the Framework 16, I think the following should be done.
>>> Use SERIO_QUIRK_NOPNP for the device to avoid the PS/2 controller
>>> probing. You can find examples in drivers/input/serio/i8042-acpipnpio.h
>>> under the i8042_dmi_quirk_table. This will prevent emitting the first
>>> two messages in the shared snippet.
>>
>> I had tried this initially, and yes those first two messages are removed.  But
>> TBH I'm not too worried about those as they're INFO. Adding a quirk just
>> switches them over to a new INFO message.
>>
> 
> Right, I can imagine someone owning this device panic-ing about the logs
> in red/yellow in journalctl or dmesg output. That said, since we know
> that the device should not bother with PNP, I think we should add the
> quirk, none the less.
> 
>> But the ERR and WARN messages still come up.  The 3 messages that show up are:
>>
>> i8042: PNP detection disabled
>> i8042: Can't read CTR while initializing i8042
>> i8042: probe of i8042 failed with error -5
>>
>> I'm more concerned that ERR and WARN messages show up making you think there is
>> a problem to look into.
>>
>>>
>>>>    drivers/input/serio/i8042.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
>>>> index 9fbb8d31575a..95dd585fdc1a 100644
>>>> --- a/drivers/input/serio/i8042.c
>>>> +++ b/drivers/input/serio/i8042.c
>>>> @@ -1008,8 +1008,8 @@ static int i8042_controller_init(void)
>>>>    			udelay(50);
>>>>      		if (i8042_command(&ctr[n++ % 2], I8042_CMD_CTL_RCTR)) {
>>>> -			pr_err("Can't read CTR while initializing i8042\n");
>>>> -			return i8042_probe_defer ? -EPROBE_DEFER : -EIO;
>>>> +			pr_debug("Can't read CTR while initializing\n");
>>> I also think this error message should be pr_err in the situation that
>>> the SERIO_QUIRK_PROBE_DEFER quirk is not used. I think what you are
>>> likely looking for is avoiding emitting this message when the
>>> SERIO_QUIRK_PROBE_DEFER quirk is used for noise reduction purposes.
>>
>> SERIO_QUIRK_PROBE_DEFER isn't set on this machine.
>>
> 
> Yeah, I would like to add this quirk as well for the device potentially
> along with SERIO_QUIRK_NOPNP. I'll get into why I think this might be a
> good idea later in this email.

OK if we end up with a quirk for this system in the end I'll add both.

> 
>>>
>>>> +			return i8042_probe_defer ? -EPROBE_DEFER : -ENXIO;
>>> I do not think this change makes sense to me personally. It is indeed an
>>> I/O issue with the i8042 controller on the Framework motherboard, so the
>>> error should be -EIO when i8042_probe_defer is not set.
>>
>> With i8042.debug enabled I can see that the error is a wait read timeout.  So I
>> had thought -ENXIO ("No such device or address") made sense here.
> 
> Right, I think that wait read timeout you are seeing is likely a symptom
> of something very similar to the experience on laptops like the ASUS
> ZenBook UX425UA, which inspired the introduction of the probe deferring
> in the i8042 driver.
> 
>    https://lore.kernel.org/lkml/20211112180022.10850-1-tiwai@suse.de/T/
> 
> In this case though, the ASUS ZenBook device did have a PS/2 device
> attached, while in the Framework device this shouldn't be the case. Will
> delve more into this nuance in my next section.

OK let me experiment with this and get back on what happens.
> 
>>
>> If you feel strongly that the errors and warnings should stay as is I I wonder
>> if this should be looked at from i8042_pnp_init().
>>
>> In the no PNP device declared case it still probes, why isn't PNP trusted?
>>
> 
> My understanding is this is due to some PS/2 devices not supporting PNP
> so this manual probe path must still be taken even when we add the
> SERIO_QUIRK_NOPNP quirk. I think if we add the quirk for deferred
> probing for the device, we can then also have a patch that does not omit
> the error to the logs when this quirk is enabled.
> 
> My question now becomes the following. If the Framework device does not
> want to expose its Intel 8042 controller to the host since its some
> unused hardware on the mainboard, why does it bother to advertise the
> 8042 over the ACPI table? Wouldn't it be better to have some UEFI update
> that fixes the ACPI table listing to avoid advertising the 8042?
> 
>    https://wiki.osdev.org/%228042%22_PS/2_Controller

You hit on the head my confusion.  The laptop doesn't advertise any of 
the PNP IDs listed in pnp_kbd_devids.

I double checked in /sys/bus/acpi/devices.

It seems that they're ignored anyway and will probe whether they are 
there or not.

So how could the firmware actually advertise this so the kernel would 
trust it?

> 
>>>
>>>>    		}
>>>>      	} while (n < 2 || ctr[0] != ctr[1]);
> 
> --
> Thanks,
> 
> Rahul Rameshbabu

Powered by blists - more mailing lists