RE: Keyboard regression by intel-vbtn

[Limonciello, Mario]

> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: Wednesday, September 30, 2020 10:37
> To: Limonciello, Mario; Barnabás Pőcze; Andy Shevchenko
> Cc: platform-driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Takashi
> Iwai
> Subject: Re: Keyboard regression by intel-vbtn
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 9/30/20 5:12 PM, Limonciello, Mario wrote:
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> Sent: Wednesday, September 30, 2020 8:28
> >> To: Limonciello, Mario; Barnabás Pőcze; Andy Shevchenko
> >> Cc: platform-driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Takashi
> >> Iwai
> >> Subject: Re: Keyboard regression by intel-vbtn
> >>
> >>
> >> [EXTERNAL EMAIL]
> >>
> >> Hi,
> >>
> >> On 9/29/20 10:47 PM, Limonciello, Mario wrote:
> >>>>
> >>>> I requested on the Ubuntu bug for someone to provide these.
> >>>>
> >>>
> >>> Joe Barnett was kind enough to share two ACPI dumps to compare.
> >>> Not affected:
> >>>
> >>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394/+attachment/54153
> >> 18/+files/1.2.0.acpidump
> >>>
> >>> Affected:
> >>>
> >>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394/+attachment/54154
> >> 05/+files/1.13.0.acpidump
> >>
> >> Thank you, I took a look at these (before completing my allow-list fix),
> >> but there is not really much which stands out. The only related thing which
> >> stands out is that the 1.13.0 dsdt.dsl has this new bit:
> >>
> >>
> >>                               Case (0x08)
> >>                               {
> >>                                   Return (^^PCI0.LPCB.H_EC.VGBI.VGBS ())
> >>                               }
> >>
> >> Inside the _DSM of the HIDD / INT33D5 device.
> >>
> >>               Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
> >>               {
> >>                   If ((Arg0 == ToUUID ("eeec56b3-4442-408f-a792-
> >> 4edd4d758054")))
> >>
> >>
> >> What is interesting here is that the PCI0.LPCB.H_EC.VGBI.VGBS object/method
> >> does not actually exist the correct path is:
> >>
> >> ^^PCI0.LPCB.ECDV.VGBI.VGBS
> >>
> >> So this does suggest that something around the VGBS handling changed
> >> (and since it points to a non existing ACPI object, possibly broke)
> >> in the newer BIOS version. But what exactly is going on on this XPS 2-in-1
> >> cannot really be derived from the acpidumps.
> >>
> >> Regards,
> >>
> >> Hans
> >
> > Looking through some publicly found content I think I might have figured out
> what
> > bight be the missing link.
> >
> > https://software.intel.com/sites/default/files/detecting-slate-clamshell-
> mode-and-screen-orientation-in-convertible-pc-1.pdf
> >
> > You can see that the device with CID PNP0C60 is supposed to indicate the
> presence
> > of a convertible hinge.  We don't currently have anything that matches that
> _CID or _HID
> > in intel-vbtn.
> >
> > In the DSDT dump you can see that the status method for the INT33D3 device
> returns
> > 0x0F on 2-in-1.s
> >
> >          Device (CIND)
> >          {
> >              Name (_HID, "INT33D3" /* Intel GPIO Buttons */)  // _HID:
> Hardware ID
> >              Name (_CID, "PNP0C60" /* Display Sensor Device */)  // _CID:
> Compatible ID
> >              Method (_STA, 0, Serialized)  // _STA: Status
> >              {
> >                  If ((OSYS >= 0x07DC))
> >                  {
> >                      Return (0x0F)
> >                  }
> >
> >                  Return (Zero)
> >              }
> >          }
> >
> > On a non 2-in-1 device I don't see this present.  So I think we should have
> intel-vbtn
> > look for that INT33D3/PNP0C60 device to decide whether to offer the switch.
> >
> > Similarly as mentioned in that document I think that we should not be
> showing the
> > docking switch only when INT33D4/PNP0C70 is present and returns 0xF.
> 
> That is a good find, thank you for digging into this and finding this.
> 
> But it seems we have a typical case of the microsoft/intel example DSDT code
> being
> blindly copied everywhere here too:
> 
> The chassis-type check was originally introduced by you in:
> commit de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode
> switch on 2-in-1's")
> 
>      Some laptops such as the XPS 9360 support the intel-vbtn INT33D6
>      interface but don't initialize the bit that intel-vbtn uses to
>      represent switching tablet mode.
> 
>      By running this only on real 2-in-1's it shouldn't cause false
>      positives.
> 
>      Fixes: 30323fb6d5 ("Support tablet mode switch")
> 
> I have a XPS 9360 (which is not 2-in-1) acpidump and that has:
> 
>          Device (CIND)
>          {
>              Name (_HID, "INT33D3" /* Intel GPIO Buttons */)  // _HID:
> Hardware ID
>              Name (_CID, "PNP0C60" /* Display Sensor Device */)  // _CID:
> Compatible ID
>              Method (_STA, 0, Serialized)  // _STA: Status
>              {
>                  If ((OSYS >= 0x07DC))
>                  {
>                      Return (0x0F)
>                  }
> 
>                  Return (Zero)
>              }
>          }
> 
> And on an asus e200ha (also not a 2-in-1), where we were seeing
> similar problems, but then using asus custom WMI interface for
> getting SW_TABLET_MODE I see:
> 
>          Device (CIND)
>          {
>              Name (_HID, "INT33D3" /* Intel GPIO Buttons */)  // _HID:
> Hardware ID
>              Name (_CID, "PNP0C60" /* Display Sensor Device */)  // _CID:
> Compatible ID
>              Method (_STA, 0, Serialized)  // _STA: Status
>              {
>                  Return (0x0F)
>              }
>          }
> 
> I have quite a few DSDTs (mostly byt/cht tablets or 2-in-1s though) and
> 65 of them define a "PNP0C60" device and only 1 unconditionally
> returns 0 from the _STA method for this device. Most others have
> an OSYS check. Some also check for some other, presumably BIOS
> configured variable, but by far most always return 0x0F, or do
> so after an OSYS check which will be true in our case.
> 
> So I'm afraid that almost all devices which have the intel-vbtn (INT33D6)
> ACPI device will also have a PNP0C60 device with the exact same
> _STA method as found on the XPS 9360 and that this thus is not
> helpful.
> 
> Regards,
> 
> Hans

Well dang, that's unfortunate.  I guess for now the allowlist is the
best solution then.

Thanks,