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

[Dmitry Torokhov]

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@xxxxxxxxx> 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.

Thanks!

-- 
Dmitry