Re: i8042 error at booting an Intel Cherry Trail-based device

[Takashi Iwai <tiwai@suse.de>]

On Tue, 06 Dec 2016 18:07:05 +0100,
Dmitry Torokhov wrote:
> 
> Hi Takashi,
> 
> On Tue, Dec 06, 2016 at 11:36:09AM +0100, Takashi Iwai wrote:
> > On Tue, 06 Dec 2016 07:07:54 +0100,
> > Dmitry Torokhov wrote:
> > > 
> > > On December 5, 2016 4:56:05 PM PST, Marcos Paulo de Souza <marcos.souza.org@gmail.com> wrote:
> > > >Hi Takashi,
> > > >
> > > >On Fri, Dec 02, 2016 at 11:55:07AM +0100, Takashi Iwai wrote:
> > > >> On Thu, 01 Dec 2016 08:19:46 +0100,
> > > >> Takashi Iwai wrote:
> > > >> > 
> > > >> > On Thu, 01 Dec 2016 03:29:23 +0100,
> > > >> > Dmitry Torokhov wrote:
> > > >> > > 
> > > >> > > Hi Takashi,
> > > >> > > 
> > > >> > > On Mon, Nov 28, 2016 at 02:56:36PM +0100, Takashi Iwai wrote:
> > > >> > > > Hi Dmitry,
> > > >> > > > 
> > > >> > > > I've been testing a small machine with Intel Cherry Trail
> > > >chipset, and
> > > >> > > > noticed that the kernel spews errors always like:
> > > >> > > > 
> > > >> > > >  i8042: PNP: No PS/2 controller found. Probing ports directly.
> > > >> > > >  i8042: Can't read CTR while initializing i8042
> > > >> > > >  i8042: probe of i8042 failed with error -5
> > > >> > > > 
> > > >> > > > Especially the second one ("Can't read CTR...") is annoying
> > > >since it's
> > > >> > > > in KERN_ERR level and thus appears even booted with quiet boot
> > > >> > > > option.  Actually this is the only error message appearing at
> > > >boot, so
> > > >> > > > I'd love to get rid of it.
> > > >> > > > 
> > > >> > > > What is the preferred way to reduce this?  For example, is a
> > > >patch
> > > >> > > > like below OK to simply change the log level and the error
> > > >code?
> > > >> > > 
> > > >> > > No, because if controller is actually present this is a hard
> > > >failure and
> > > >> > > we should be reporting it, not suppressing it.
> > > >> > > 
> > > >> > > The issue is that we did not believe PNP data and in this case we
> > > >should
> > > >> > > have. Unfortunately in old days there was a lot of crap in
> > > >PNP/ACPI
> > > >> > > tables, but it could be better now. We can try, in addition to
> > > >PNP
> > > >> > > matching, checking 8042 flag in "Fixed ACPI Description Table
> > > >Boot
> > > >> > > Architecture Flags" in FADT and if it also shows there is no 8042
> > > >then
> > > >> > > bail.
> > > >> > 
> > > >> > That sounds promising.  Indeed FACL.dsl shows like:
> > > >> > 
> > > >> > [000h 0000   4]                    Signature : "FACP"    [Fixed
> > > >ACPI Description Table (FADT)]
> > > >> > [004h 0004   4]                 Table Length : 0000010C
> > > >> > ....
> > > >> >                Legacy Devices Supported (V2) : 0
> > > >> >             8042 Present on ports 60/64 (V2) : 0
> > > >> > 
> > > >> > If a test patch gets ready, let me know, I'll give it a try.
> > > >> 
> > > >> FYI, a hack like below seems working.
> > > >> 
> > > >> 
> > > >> Takashi
> > > >> 
> > > >> ---
> > > >> diff --git a/drivers/input/serio/i8042-x86ia64io.h
> > > >b/drivers/input/serio/i8042-x86ia64io.h
> > > >> index 073246c7d163..ed6ab702e4b7 100644
> > > >> --- a/drivers/input/serio/i8042-x86ia64io.h
> > > >> +++ b/drivers/input/serio/i8042-x86ia64io.h
> > > >> @@ -9,6 +9,7 @@
> > > >>  
> > > >>  #ifdef CONFIG_X86
> > > >>  #include <asm/x86_init.h>
> > > >> +#include <linux/acpi.h>
> > > >>  #endif
> > > >>  
> > > >>  /*
> > > >> @@ -1055,6 +1056,13 @@ static int __init i8042_pnp_init(void)
> > > >>  #if defined(__ia64__)
> > > >>  		return -ENODEV;
> > > >>  #else
> > > >> +#ifdef CONFIG_ACPI
> > > >> +		if (acpi_gbl_FADT.header.revision >= 3 &&
> > > >> +		    !(acpi_gbl_FADT.boot_flags & ACPI_FADT_8042)) {
> > > >> +			pr_info("PNP: No PS/2 controller found and disabled in ACPI\n");
> > > >> +			return -ENODEV;
> > > >> +		}
> > > >> +#endif
> > > >>  		pr_info("PNP: No PS/2 controller found. Probing ports
> > > >directly.\n");
> > > >>  		return 0;
> > > >>  #endif
> > > >
> > > >I'm not an expert in any subsystem but, maybe this "hack" could be
> > > >added
> > > >to default_i8042_detect in arch/x86/kernel/x86_init.c? Currently it is
> > > >enabled by default, but different Intel platform like ce4100 and
> > > >intel-mid disables it explicit.
> > > >
> > > >I mentioned "hack" because following osdev.org[1] using ACPI is the
> > > >correct way to detect if i8042 exists. Pardon me if this not applies in
> > > >this situation, or if I missed something.
> > > 
> > > That is the proper way of detecting i8042 if you trust firmware; historically we do not, and so we want to make sure that PNP data agrees with fadt data.
> > 
> > So it depends on how well you trust the firmware.  If we assume ACPI
> > providing always correctly, it can be put in default_i8042_detect, and
> > it'd be a better place indeed.  OTOH, if we don't trust ACPI,
> > especially on older machines, and let at first probing ACPI PnP no
> > matter whether FADT bit is set, we'd need to put the check after PnP
> > probe like my patch.
> > 
> > My patch assumes that the BIOS is new and good enough if FADT revision
> > is 3 or greater.  The only concern is whether this is really good
> > enough.  I just hope so.
> 
> FWIW FADT revision 3 is defined in ACPI 2.0 as far as I know. So not too
> new.
> > 
> > In anyway, Dmitry, if you're happy with it, I'll cook up the proper
> > patch for the merge.  Let me know.
> 
> I am happy with the idea, but as far as implementation goes I think we
> need to add this flag to x86_platform.legacy structure, initialize
> x86_platform_legacy.i8042_present = 1 in
> x86_early_init_platform_quirks(), and adjust as needed in
> acpi_parse_fadt().
> 
> Then we can use it in i8042 instead of checking FADT by hand.

That sounds good.  I hope we'll get it soon ;)


Thanks!

Takashi