Keyboard regression by intel-vbtn

Keyboard regression by intel-vbtn

[Takashi Iwai]

Hi Hans,

it seems that the recent update of intel-vtn broke the keyboard input
on some laptops with libinput:
  https://bugzilla.opensuse.org/show_bug.cgi?id=1175599

Blacklisting intel-vtn fixes the issue, so it's likely the falsely
reported tablet mode switch that leads libinput misbehaving.  The
affected machines are Acer E5-511 and ASUS X756UX laptops, and they
shouldn't have the tablet mode at all, AFAIK.

Could you take a look?  I guess it's the commit cfae58ed681c that
broke.  The chassis type is Notebook on those, and this type should be
excluded as well as Laptop.

The dmidecode outputs and other info are found in the bugzilla above:
  https://bugzilla.opensuse.org/attachment.cgi?id=841999
  https://bugzilla.opensuse.org/attachment.cgi?id=842039

The one for ASUS is embedded in hwinfo outpt:
  https://bugzilla.opensuse.org/attachment.cgi?id=841157


thanks,

Takashi

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/29/20 10:48 AM, Takashi Iwai wrote:
> Hi Hans,
> 
> it seems that the recent update of intel-vtn broke the keyboard input
> on some laptops with libinput:
>    https://bugzilla.opensuse.org/show_bug.cgi?id=1175599
> 
> Blacklisting intel-vtn fixes the issue, so it's likely the falsely
> reported tablet mode switch that leads libinput misbehaving.  The
> affected machines are Acer E5-511 and ASUS X756UX laptops, and they
> shouldn't have the tablet mode at all, AFAIK.
> 
> Could you take a look?  I guess it's the commit cfae58ed681c that
> broke.  The chassis type is Notebook on those, and this type should be
> excluded as well as Laptop.
> 
> The dmidecode outputs and other info are found in the bugzilla above:
>    https://bugzilla.opensuse.org/attachment.cgi?id=841999
>    https://bugzilla.opensuse.org/attachment.cgi?id=842039
> 
> The one for ASUS is embedded in hwinfo outpt:
>    https://bugzilla.opensuse.org/attachment.cgi?id=841157

Ugh. What a mess, sorry about this.

So as the commit message from commit cfae58ed681c
("platform/x86: intel-vbtn: Only blacklist SW_TABLET_MODE on the 9 / "Laptop" chasis-type")
explains the reason to NOT NOT report SW_TABLET_MODE on devices
with a chassis type of 10 ("Notebook") is that at least
some HP ... 360 ... models use that chassis type and do
report a correct SW_TABLET_MODE through the intel-vbtn driver.

The SW_TABLET_MODE on these actually got regressed by
de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
which first introduced the chassis-type check.

And to complicate things further even though some
HP ... 360 ... models use that chassis type and from the DSDT
it seems that they do report a correct SW_TABLET_MODE through the
intel-vbtn driver. In practice it is also broken on some
HP ... 360 ... models, see:

https://forum.manjaro.org/t/keyboard-and-touchpad-only-work-on-kernel-5-6/22668
http://git.infradead.org/linux-platform-drivers-x86.git/commit/d823346876a970522ff9e4d2b323c9b734dcc4de
"platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"

Since the problem of wrongly reporting SW_TABLET_MODE=1 in combination
with libinput, leads to a non-usable system. Where as OTOH many people will
not even notice when SW_TABLET_MODE is not being reported, I believe it
is best to move to a dmi based allow-list approach here, as we recently
did for SW_TABLET_MODE reporting by the asus-wmi driver. Allowing:

dmi chassis-types: 8 /* Portable */,  31 /* Convertible */, 32 /* Detachable */
and the HP Stream x360 11-p000nd which has working intel-vbtn SW_TABLET_MODE
support combined with a chassis-type of 10 /* Notebook */.

I will prepare a patch for this right away.

Note this patch will effectively replace:
"platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"
We will no longer need this workaround with the allow list and I believe
that it would be better to drop that one.

Andy can you drop that one from your review-andy branch please?

Regards,

Hans

Re: Keyboard regression by intel-vbtn

[Takashi Iwai]

On Tue, 29 Sep 2020 11:21:27 +0200,
Hans de Goede wrote:
> 
> Hi,
> 
> On 9/29/20 10:48 AM, Takashi Iwai wrote:
> > Hi Hans,
> >
> > it seems that the recent update of intel-vtn broke the keyboard input
> > on some laptops with libinput:
> >    https://bugzilla.opensuse.org/show_bug.cgi?id=1175599
> >
> > Blacklisting intel-vtn fixes the issue, so it's likely the falsely
> > reported tablet mode switch that leads libinput misbehaving.  The
> > affected machines are Acer E5-511 and ASUS X756UX laptops, and they
> > shouldn't have the tablet mode at all, AFAIK.
> >
> > Could you take a look?  I guess it's the commit cfae58ed681c that
> > broke.  The chassis type is Notebook on those, and this type should be
> > excluded as well as Laptop.
> >
> > The dmidecode outputs and other info are found in the bugzilla above:
> >    https://bugzilla.opensuse.org/attachment.cgi?id=841999
> >    https://bugzilla.opensuse.org/attachment.cgi?id=842039
> >
> > The one for ASUS is embedded in hwinfo outpt:
> >    https://bugzilla.opensuse.org/attachment.cgi?id=841157
> 
> Ugh. What a mess, sorry about this.
> 
> So as the commit message from commit cfae58ed681c
> ("platform/x86: intel-vbtn: Only blacklist SW_TABLET_MODE on the 9 / "Laptop" chasis-type")
> explains the reason to NOT NOT report SW_TABLET_MODE on devices
> with a chassis type of 10 ("Notebook") is that at least
> some HP ... 360 ... models use that chassis type and do
> report a correct SW_TABLET_MODE through the intel-vbtn driver.
> 
> The SW_TABLET_MODE on these actually got regressed by
> de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
> which first introduced the chassis-type check.
> 
> And to complicate things further even though some
> HP ... 360 ... models use that chassis type and from the DSDT
> it seems that they do report a correct SW_TABLET_MODE through the
> intel-vbtn driver. In practice it is also broken on some
> HP ... 360 ... models, see:
> 
> https://forum.manjaro.org/t/keyboard-and-touchpad-only-work-on-kernel-5-6/22668
> http://git.infradead.org/linux-platform-drivers-x86.git/commit/d823346876a970522ff9e4d2b323c9b734dcc4de
> "platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"

Oohoo, what a wonderful world :)

> Since the problem of wrongly reporting SW_TABLET_MODE=1 in combination
> with libinput, leads to a non-usable system. Where as OTOH many people will
> not even notice when SW_TABLET_MODE is not being reported, I believe it
> is best to move to a dmi based allow-list approach here, as we recently
> did for SW_TABLET_MODE reporting by the asus-wmi driver. Allowing:
> 
> dmi chassis-types: 8 /* Portable */,  31 /* Convertible */, 32 /* Detachable */
> and the HP Stream x360 11-p000nd which has working intel-vbtn SW_TABLET_MODE
> support combined with a chassis-type of 10 /* Notebook */.
> 
> I will prepare a patch for this right away.

Great, thanks!  I'll build a test kernel when receiving a patch soon.


Takashi

> Note this patch will effectively replace:
> "platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"
> We will no longer need this workaround with the allow list and I believe
> that it would be better to drop that one.
> 
> Andy can you drop that one from your review-andy branch please?
> 
> Regards,
> 
> Hans
> 
> 

Re: Keyboard regression by intel-vbtn

[Barnabás Pőcze]

Hi,

2020. szeptember 29., kedd 11:29 keltezéssel, Takashi Iwai írta:

> On Tue, 29 Sep 2020 11:21:27 +0200,
> Hans de Goede wrote:
>
> > Hi,
> > On 9/29/20 10:48 AM, Takashi Iwai wrote:
> >
> > > Hi Hans,
> > > it seems that the recent update of intel-vtn broke the keyboard input
> > > on some laptops with libinput:
> > > https://bugzilla.opensuse.org/show_bug.cgi?id=1175599
> > > Blacklisting intel-vtn fixes the issue, so it's likely the falsely
> > > reported tablet mode switch that leads libinput misbehaving. The
> > > affected machines are Acer E5-511 and ASUS X756UX laptops, and they
> > > shouldn't have the tablet mode at all, AFAIK.
> > > Could you take a look? I guess it's the commit cfae58ed681c that
> > > broke. The chassis type is Notebook on those, and this type should be
> > > excluded as well as Laptop.
> > > The dmidecode outputs and other info are found in the bugzilla above:
> > > https://bugzilla.opensuse.org/attachment.cgi?id=841999
> > > https://bugzilla.opensuse.org/attachment.cgi?id=842039
> > > The one for ASUS is embedded in hwinfo outpt:
> > > https://bugzilla.opensuse.org/attachment.cgi?id=841157
> >
> > Ugh. What a mess, sorry about this.
> > So as the commit message from commit cfae58ed681c
> > ("platform/x86: intel-vbtn: Only blacklist SW_TABLET_MODE on the 9 / "Laptop" chasis-type")
> > explains the reason to NOT NOT report SW_TABLET_MODE on devices
> > with a chassis type of 10 ("Notebook") is that at least
> > some HP ... 360 ... models use that chassis type and do
> > report a correct SW_TABLET_MODE through the intel-vbtn driver.
> > The SW_TABLET_MODE on these actually got regressed by
> > de9647efeaa9 ("platform/x86: intel-vbtn: Only activate tablet mode switch on 2-in-1's")
> > which first introduced the chassis-type check.
> > And to complicate things further even though some
> > HP ... 360 ... models use that chassis type and from the DSDT
> > it seems that they do report a correct SW_TABLET_MODE through the
> > intel-vbtn driver. In practice it is also broken on some
> > HP ... 360 ... models, see:
> > https://forum.manjaro.org/t/keyboard-and-touchpad-only-work-on-kernel-5-6/22668
> > http://git.infradead.org/linux-platform-drivers-x86.git/commit/d823346876a970522ff9e4d2b323c9b734dcc4de
> > "platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"
>
> Oohoo, what a wonderful world :)
>


Splendid world, indeed. I'm wondering, however, why the incorrect state
is reported? Is it similar to the linked issue on the Manjaro forum, where a
different bit is seemingly used to report the tablet mode state, or something else?
I'm also wondering why it was chosen that a *set* bit means that the tablet
mode is *off*. All these problems could've been easily avoided... (given that
I'm not missing anything obvious).

> [...]


Regards,
Barnabás Pőcze

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/29/20 11:59 AM, Barnabás Pőcze wrote:

<snip>

>> Oohoo, what a wonderful world :)
>>
> 
> 
> Splendid world, indeed. I'm wondering, however, why the incorrect state
> is reported? Is it similar to the linked issue on the Manjaro forum, where a
> different bit is seemingly used to report the tablet mode state, or something else?
> I'm also wondering why it was chosen that a *set* bit means that the tablet
> mode is *off*. All these problems could've been easily avoided... (given that
> I'm not missing anything obvious).

I'm afraid that the only answer which I have to these questions
is not helpful, but in my experience it is true: "firmware sucks".

Regards,

Hans

Re: Keyboard regression by intel-vbtn

[Limonciello, Mario]

>I'm afraid that the only answer which I have to these questions
>is not helpful, but in my experience it is true: "firmware sucks".

So FWIW there is a Dell 2-in-1 that has been conflated into this same issue.
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394

Something that is confusing to me is that on the Windows side all these
machines use the same Intel driver for this infrastructure no matter the
OEM.
So they can't possibly be putting in quirk specific stuff in the driver side
can they?

It has to make you wonder if some baseline assumptions made in the
driver early on around tablet mode support are completely false.

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/29/20 2:27 PM, Limonciello, Mario wrote:
>> I'm afraid that the only answer which I have to these questions
>> is not helpful, but in my experience it is true: "firmware sucks".
> 
> So FWIW there is a Dell 2-in-1 that has been conflated into this same issue.
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394

That is what a somewhat old kernel (5.0.0) which I guess may
lack your fix to check the chassis-type.

Interesting that this actually is a 2-in-1 though.

Also interesting that according to the reporter this was
triggered by a BIOS update.

If you by any chance can provide an acpidump with both the
1.2.0 and 1.4.0 BIOS versions that would be very interesting.

> Something that is confusing to me is that on the Windows side all these
> machines use the same Intel driver for this infrastructure no matter the
> OEM.
> So they can't possibly be putting in quirk specific stuff in the driver side
> can they?
> 
> It has to make you wonder if some baseline assumptions made in the
> driver early on around tablet mode support are completely false.

I'm not saying your wrong. If you can get Intel to provide
us with some documentation, or Windows driver source code
for this, then that would be great.

AFAICT the Linux driver currently is entirely based on
reverse engineering.

Regards,

Hans

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/29/20 11:21 AM, Hans de Goede wrote:

<snip>

> Since the problem of wrongly reporting SW_TABLET_MODE=1 in combination
> with libinput, leads to a non-usable system. Where as OTOH many people will
> not even notice when SW_TABLET_MODE is not being reported, I believe it
> is best to move to a dmi based allow-list approach here, as we recently
> did for SW_TABLET_MODE reporting by the asus-wmi driver. Allowing:
> 
> dmi chassis-types: 8 /* Portable */,  31 /* Convertible */, 32 /* Detachable */
> and the HP Stream x360 11-p000nd which has working intel-vbtn SW_TABLET_MODE
> support combined with a chassis-type of 10 /* Notebook */.
> 
> I will prepare a patch for this right away.

Quick status update on this, it is taking a bit longer then
I was hoping for since I'm trying to make sure that I get
things right this time.

Checking the dmidecode-dumps database from:
https://github.com/linuxhw/DMI

It seems that a whole ton of Dell laptop models use 8 ("Portable") as
chassis-type. Most of which are NOT 2-in-1s. So the new plan is to only
put the 31 ("Convertible") and 32 ("Detachable") chassis-types on
the whitelist. I do have 1 Dell 2-in-1, a Dell Venue 11 Pro 7130
with the Portable chassis-type and a working tablet-mode-sw
through intel-vbtn. So I will put that specific model on the
allow-list separately.

> Note this patch will effectively replace:
> "platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"
> We will no longer need this workaround with the allow list and I believe
> that it would be better to drop that one.
> 
> Andy can you drop that one from your review-andy branch please?

Regards,

Hans

RE: Keyboard regression by intel-vbtn

[Limonciello, Mario]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Tuesday, September 29, 2020 7:54
> To: Limonciello, Mario; Barnabás Pőcze; Takashi Iwai
> Cc: Andy Shevchenko; platform-driver-x86@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: Keyboard regression by intel-vbtn
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 9/29/20 2:27 PM, Limonciello, Mario wrote:
> >> I'm afraid that the only answer which I have to these questions
> >> is not helpful, but in my experience it is true: "firmware sucks".
> >
> > So FWIW there is a Dell 2-in-1 that has been conflated into this same issue.
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394
> 
> That is what a somewhat old kernel (5.0.0) which I guess may
> lack your fix to check the chassis-type.
> 
> Interesting that this actually is a 2-in-1 though.
> 
> Also interesting that according to the reporter this was
> triggered by a BIOS update.
> 
> If you by any chance can provide an acpidump with both the
> 1.2.0 and 1.4.0 BIOS versions that would be very interesting.

I requested on the Ubuntu bug for someone to provide these.

> 
> > Something that is confusing to me is that on the Windows side all these
> > machines use the same Intel driver for this infrastructure no matter the
> > OEM.
> > So they can't possibly be putting in quirk specific stuff in the driver side
> > can they?
> >
> > It has to make you wonder if some baseline assumptions made in the
> > driver early on around tablet mode support are completely false.
> 
> I'm not saying your wrong. If you can get Intel to provide
> us with some documentation, or Windows driver source code
> for this, then that would be great.
> 
> AFAICT the Linux driver currently is entirely based on
> reverse engineering.

That's correct it was originally reverse engineered.

Andy,

As there is no publicly available documentation, could you see if it would it be
possible to get someone internal to Intel to compare private documentation to the
driver to see if something basic is missing or done wrong?

Re: Keyboard regression by intel-vbtn

[Andy Shevchenko]

On Tue, Sep 29, 2020 at 02:25:12PM +0000, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Hans de Goede <hdegoede@redhat.com>
> > Sent: Tuesday, September 29, 2020 7:54
> > On 9/29/20 2:27 PM, Limonciello, Mario wrote:

...

> > >> I'm afraid that the only answer which I have to these questions
> > >> is not helpful, but in my experience it is true: "firmware sucks".
> > >
> > > So FWIW there is a Dell 2-in-1 that has been conflated into this same issue.
> > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394
> > 
> > That is what a somewhat old kernel (5.0.0) which I guess may
> > lack your fix to check the chassis-type.
> > 
> > Interesting that this actually is a 2-in-1 though.
> > 
> > Also interesting that according to the reporter this was
> > triggered by a BIOS update.
> > 
> > If you by any chance can provide an acpidump with both the
> > 1.2.0 and 1.4.0 BIOS versions that would be very interesting.
> 
> I requested on the Ubuntu bug for someone to provide these.
> 
> > > Something that is confusing to me is that on the Windows side all these
> > > machines use the same Intel driver for this infrastructure no matter the
> > > OEM.
> > > So they can't possibly be putting in quirk specific stuff in the driver side
> > > can they?
> > >
> > > It has to make you wonder if some baseline assumptions made in the
> > > driver early on around tablet mode support are completely false.
> > 
> > I'm not saying your wrong. If you can get Intel to provide
> > us with some documentation, or Windows driver source code
> > for this, then that would be great.
> > 
> > AFAICT the Linux driver currently is entirely based on
> > reverse engineering.
> 
> That's correct it was originally reverse engineered.
> 
> Andy,
> 
> As there is no publicly available documentation, could you see if it would it be
> possible to get someone internal to Intel to compare private documentation to the
> driver to see if something basic is missing or done wrong?

I'm afraid that's all was designed solely for Windows and all information is
not available to us in any form (basically somebody has to ask for WOS driver
sources, and funny that as a customer your has more power to get this than mere
in-house Linux engineer as me).

-- 
With Best Regards,
Andy Shevchenko

RE: Keyboard regression by intel-vbtn

[Limonciello, Mario]

> > > >> I'm afraid that the only answer which I have to these questions
> > > >> is not helpful, but in my experience it is true: "firmware sucks".
> > > >
> > > > So FWIW there is a Dell 2-in-1 that has been conflated into this same
> issue.
> > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394
> > >
> > > That is what a somewhat old kernel (5.0.0) which I guess may
> > > lack your fix to check the chassis-type.
> > >
> > > Interesting that this actually is a 2-in-1 though.
> > >
> > > Also interesting that according to the reporter this was
> > > triggered by a BIOS update.
> > >
> > > If you by any chance can provide an acpidump with both the
> > > 1.2.0 and 1.4.0 BIOS versions that would be very interesting.
> >
> > I requested on the Ubuntu bug for someone to provide these.
> >
> > > > Something that is confusing to me is that on the Windows side all these
> > > > machines use the same Intel driver for this infrastructure no matter the
> > > > OEM.
> > > > So they can't possibly be putting in quirk specific stuff in the driver
> side
> > > > can they?
> > > >
> > > > It has to make you wonder if some baseline assumptions made in the
> > > > driver early on around tablet mode support are completely false.
> > >
> > > I'm not saying your wrong. If you can get Intel to provide
> > > us with some documentation, or Windows driver source code
> > > for this, then that would be great.
> > >
> > > AFAICT the Linux driver currently is entirely based on
> > > reverse engineering.
> >
> > That's correct it was originally reverse engineered.
> >
> > Andy,
> >
> > As there is no publicly available documentation, could you see if it would
> it be
> > possible to get someone internal to Intel to compare private documentation
> to the
> > driver to see if something basic is missing or done wrong?
> 
> I'm afraid that's all was designed solely for Windows and all information is
> not available to us in any form (basically somebody has to ask for WOS driver
> sources, and funny that as a customer your has more power to get this than
> mere
> in-house Linux engineer as me).
> 

I guess this comes back to why we have reverse engineered driver in the first
place.

Even in this suggested case, such documentation is shared to partners under NDA,
and would need permission to be used in open source.

As such an allowlist for this driver might be the best path forward given the 
quirky behavior that we have seen.

RE: Keyboard regression by intel-vbtn

[Limonciello, Mario]

> 
> 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/5415318/+files/1.2.0.acpidump

Affected:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394/+attachment/5415405/+files/1.13.0.acpidump

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/29/20 10:48 AM, Takashi Iwai wrote:
> Hi Hans,
> 
> it seems that the recent update of intel-vtn broke the keyboard input
> on some laptops with libinput:
>    https://bugzilla.opensuse.org/show_bug.cgi?id=1175599
> 
> Blacklisting intel-vtn fixes the issue, so it's likely the falsely
> reported tablet mode switch that leads libinput misbehaving.  The
> affected machines are Acer E5-511 and ASUS X756UX laptops, and they
> shouldn't have the tablet mode at all, AFAIK.
> 
> Could you take a look?  I guess it's the commit cfae58ed681c that
> broke.  The chassis type is Notebook on those, and this type should be
> excluded as well as Laptop.
> 
> The dmidecode outputs and other info are found in the bugzilla above:
>    https://bugzilla.opensuse.org/attachment.cgi?id=841999
>    https://bugzilla.opensuse.org/attachment.cgi?id=842039
> 
> The one for ASUS is embedded in hwinfo outpt:
>    https://bugzilla.opensuse.org/attachment.cgi?id=841157

Ok, you should have just received a patch fixing this, sorry
that creating it took a bit longer then I had hoped.

Andy, can you drop the:

"platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"

Patch from your review-andy branch, plug in the new one and
then send it out to Linus for merging into 5.9 please ?

Regards,

Hans

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/29/20 10:48 AM, Takashi Iwai wrote:
> Hi Hans,
> 
> it seems that the recent update of intel-vtn broke the keyboard input
> on some laptops with libinput:
>    https://bugzilla.opensuse.org/show_bug.cgi?id=1175599
> 
> Blacklisting intel-vtn fixes the issue, so it's likely the falsely
> reported tablet mode switch that leads libinput misbehaving.  The
> affected machines are Acer E5-511 and ASUS X756UX laptops, and they
> shouldn't have the tablet mode at all, AFAIK.
> 
> Could you take a look?  I guess it's the commit cfae58ed681c that
> broke.  The chassis type is Notebook on those, and this type should be
> excluded as well as Laptop.
> 
> The dmidecode outputs and other info are found in the bugzilla above:
>    https://bugzilla.opensuse.org/attachment.cgi?id=841999
>    https://bugzilla.opensuse.org/attachment.cgi?id=842039
> 
> The one for ASUS is embedded in hwinfo outpt:
>    https://bugzilla.opensuse.org/attachment.cgi?id=841157

Ok, you should have just received a patch fixing this, sorry
that creating it took a bit longer then I had hoped.

Andy, can you drop the:

"platform/x86: intel-vbtn: Fix SW_TABLET_MODE always reporting 1 on the HP Pavilion 11 x360"

Patch from your review-andy branch, plug in the new one and
then send it out to Linus for merging into 5.9 please ?

Regards,

Hans

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

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/5415318/+files/1.2.0.acpidump
> 
> Affected:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1822394/+attachment/5415405/+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

RE: Keyboard regression by intel-vbtn

[Limonciello, Mario]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Wednesday, September 30, 2020 8:28
> To: Limonciello, Mario; Barnabás Pőcze; Andy Shevchenko
> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org; 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.

Re: Keyboard regression by intel-vbtn

[Hans de Goede]

Hi,

On 9/30/20 5:12 PM, Limonciello, Mario wrote:
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Wednesday, September 30, 2020 8:28
>> To: Limonciello, Mario; Barnabás Pőcze; Andy Shevchenko
>> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org; 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

RE: Keyboard regression by intel-vbtn

[Limonciello, Mario]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Wednesday, September 30, 2020 10:37
> To: Limonciello, Mario; Barnabás Pőcze; Andy Shevchenko
> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org; 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@redhat.com>
> >> Sent: Wednesday, September 30, 2020 8:28
> >> To: Limonciello, Mario; Barnabás Pőcze; Andy Shevchenko
> >> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org;
> 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,