| Message ID | 88473a72368b43bdacebca279092ab0f@ausx13mpc120.AMER.DELL.COM (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 02/22/2018 12:05 PM, Mario.Limonciello@dell.com wrote: >> -----Original Message----- >> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- >> owner@vger.kernel.org] On Behalf Of Jeremy Cline >> Sent: Thursday, February 22, 2018 10:42 AM >> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; notmart@gmail.com >> Cc: pali.rohar@gmail.com; andriy.shevchenko@linux.intel.com; >> mjg59@srcf.ucam.org; dvhart@infradead.org; platform-driver- >> x86@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: Re: Regression: Dell XPS 13 9360 keyboard no longer works >> >> On 02/22/2018 11:17 AM, Mario.Limonciello@dell.com wrote: >>>> -----Original Message----- >>>> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- >>>> owner@vger.kernel.org] On Behalf Of Jeremy Cline >>>> Sent: Thursday, February 22, 2018 10:17 AM >>>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; notmart@gmail.com >>>> Cc: pali.rohar@gmail.com; andriy.shevchenko@linux.intel.com; >>>> mjg59@srcf.ucam.org; dvhart@infradead.org; platform-driver- >>>> x86@vger.kernel.org; linux-kernel@vger.kernel.org >>>> Subject: Re: Regression: Dell XPS 13 9360 keyboard no longer works >>>> >>>> On 02/22/2018 11:14 AM, Mario.Limonciello@dell.com wrote: >>>>>> -----Original Message----- >>>>>> From: Jeremy Cline [mailto:jeremy@jcline.org] >>>>>> Sent: Thursday, February 22, 2018 9:59 AM >>>>>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; notmart@gmail.com >>>>>> Cc: pali.rohar@gmail.com; andriy.shevchenko@linux.intel.com; >>>>>> mjg59@srcf.ucam.org; dvhart@infradead.org; platform-driver- >>>>>> x86@vger.kernel.org; linux-kernel@vger.kernel.org >>>>>> Subject: Re: Regression: Dell XPS 13 9360 keyboard no longer works >>>>>> >>>>>> On 02/22/2018 10:21 AM, Mario.Limonciello@dell.com wrote:> I guess that >>>>>> means we got this wrong and the patch should be reverted >>>>>>> until we figure this out. >>>>>>> >>>>>>> Jeremy, >>>>>>> >>>>>>> Can you please confirm what BIOS version you are on? >>>>>>> Also Is this a 9360 with 7th or 8th gen Intel CPU? >>>>>> >>>>>> Hi Mario, >>>>>> >>>>>> I've got BIOS version 2.5.0 with the 7th gen Intel CPU. >>>>>> >>>>>> >>>>>> Regards, >>>>>> Jeremy >>>>> >>>>> Jeremy, >>>>> >>>>> Thanks. Do you have any of the Dell docks (TB16/WD15)? If so are you >> connected >>>> to any dock >>>>> when reproducing this problem? >>>> >>>> Mario, >>>> >>>> I do have a TB16. I can reproduce this whether or not I'm connected to >>>> the dock, though. >>>> >>>> >>>> Regards, >>>> Jeremy >>> >>> Jeremy, >>> >>> Can you try booting up from a cold boot with it connected to see if it still >> happens? >>> >> >> Mario, >> >> Yup, it still happens from a cold boot when connected to the dock. > > OK thanks for confirming. Here's what I've concluded: > > * So looking through the ACPI tables on the 9360 it initializes that status > (slate vs laptop mode) bit to "slate" mode. The 9360 isn't a 2-in1- so that > seems wrong to me, but that's what it does. > It only gets updated based on dock status. > > The 9365 (which is a 2 in 1) however seems to initialize the status properly. > > So that's an impasse of what to do. > It's not clear to me what is really happening: > a) We're missing something else in this driver (eg something else that > indicates whether to trust VGBS output) > > b) Mis-interpreting the results from it (we shouldn't report the switch for > tablet mode based on what we do) > > c) 9360 has a BIOS bug (seems unlikely since Windows doesn't freak out and > show virtual keyboard at wrong time) > > I'm leaning on it's probably <a>. > We should check for tablet mode should only be run if chassis type > matches 2-in-1 (chassis type 0x1F). > > I believe that should fix the problem on the 9360, let it continue to work on > the 9365 (and other 2-in-1's). > > Can you guys please test this? If that works I'll split up the routine and submit > it. > Hi Mario, There's one small error in the patch, after I fixed it, it works. Thanks! > diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c > index b703d6f..07bc489 100644 > --- a/drivers/platform/x86/intel-vbtn.c > +++ b/drivers/platform/x86/intel-vbtn.c > @@ -7,6 +7,7 @@ > */ > > #include <linux/acpi.h> > +#include <linux/dmi.h> > #include <linux/input.h> > #include <linux/input/sparse-keymap.h> > #include <linux/kernel.h> > @@ -102,6 +103,7 @@ static int intel_vbtn_probe(struct platform_device *device) > struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL }; > acpi_handle handle = ACPI_HANDLE(&device->dev); > struct intel_vbtn_priv *priv; > + const char *chassis_type; > acpi_status status; > int err; > > @@ -123,22 +125,24 @@ static int intel_vbtn_probe(struct platform_device *device) > } > > /* > - * VGBS being present and returning something means we have > - * a tablet mode switch. > + * Running on 2-in-1 chassis, VGBS being present and > + * returning something means we have a tablet mode switch. > */ > - status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output); > - if (ACPI_SUCCESS(status)) { > - union acpi_object *obj = vgbs_output.pointer; > + chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE); > + if (chassis_type && strcmp(chassis_type, "31")) {This should be "chassis_type && strcmp(chassis_type, "31") == 0" since strcmp returns 0 for equality. > + status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output); > + if (ACPI_SUCCESS(status)) { > + union acpi_object *obj = vgbs_output.pointer; > > - if (obj && obj->type == ACPI_TYPE_INTEGER) { > - int m = !(obj->integer.value & TABLET_MODE_FLAG); > + if (obj && obj->type == ACPI_TYPE_INTEGER) { > + int m = !(obj->integer.value & TABLET_MODE_FLAG); > > - input_report_switch(priv->input_dev, SW_TABLET_MODE, m); > + input_report_switch(priv->input_dev, SW_TABLET_MODE, m); > + } > } > + kfree(vgbs_output.pointer); > } > - kfree(vgbs_output.pointer); > - > status = acpi_install_notify_handler(handle, > ACPI_DEVICE_NOTIFY, > notify_handler, > Regards, Jeremy
On Thu, Feb 22, 2018 at 05:05:39PM +0000, Mario.Limonciello@dell.com wrote: ... > OK thanks for confirming. Here's what I've concluded: > > * So looking through the ACPI tables on the 9360 it initializes that status > (slate vs laptop mode) bit to "slate" mode. The 9360 isn't a 2-in1- so that > seems wrong to me, but that's what it does. > It only gets updated based on dock status. > > The 9365 (which is a 2 in 1) however seems to initialize the status properly. > > So that's an impasse of what to do. > It's not clear to me what is really happening: > a) We're missing something else in this driver (eg something else that > indicates whether to trust VGBS output) > > b) Mis-interpreting the results from it (we shouldn't report the switch for > tablet mode based on what we do) > > c) 9360 has a BIOS bug (seems unlikely since Windows doesn't freak out and > show virtual keyboard at wrong time) > > I'm leaning on it's probably <a>. > We should check for tablet mode should only be run if chassis type > matches 2-in-1 (chassis type 0x1F). This sounds like a good approach to me. > I believe that should fix the problem on the 9360, let it continue to work on > the 9365 (and other 2-in-1's). > > Can you guys please test this? If that works I'll split up the routine and submit > it. Queueing this up as well.
> -----Original Message----- > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > owner@vger.kernel.org] On Behalf Of Jeremy Cline > Sent: Thursday, February 22, 2018 1:45 PM > To: Limonciello, Mario <Mario_Limonciello@Dell.com>; notmart@gmail.com > Cc: pali.rohar@gmail.com; andriy.shevchenko@linux.intel.com; > mjg59@srcf.ucam.org; dvhart@infradead.org; platform-driver- > x86@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: Regression: Dell XPS 13 9360 keyboard no longer works > > On 02/22/2018 12:05 PM, Mario.Limonciello@dell.com wrote: > >> -----Original Message----- > >> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > >> owner@vger.kernel.org] On Behalf Of Jeremy Cline > >> Sent: Thursday, February 22, 2018 10:42 AM > >> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; notmart@gmail.com > >> Cc: pali.rohar@gmail.com; andriy.shevchenko@linux.intel.com; > >> mjg59@srcf.ucam.org; dvhart@infradead.org; platform-driver- > >> x86@vger.kernel.org; linux-kernel@vger.kernel.org > >> Subject: Re: Regression: Dell XPS 13 9360 keyboard no longer works > >> > >> On 02/22/2018 11:17 AM, Mario.Limonciello@dell.com wrote: > >>>> -----Original Message----- > >>>> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver- > x86- > >>>> owner@vger.kernel.org] On Behalf Of Jeremy Cline > >>>> Sent: Thursday, February 22, 2018 10:17 AM > >>>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; notmart@gmail.com > >>>> Cc: pali.rohar@gmail.com; andriy.shevchenko@linux.intel.com; > >>>> mjg59@srcf.ucam.org; dvhart@infradead.org; platform-driver- > >>>> x86@vger.kernel.org; linux-kernel@vger.kernel.org > >>>> Subject: Re: Regression: Dell XPS 13 9360 keyboard no longer works > >>>> > >>>> On 02/22/2018 11:14 AM, Mario.Limonciello@dell.com wrote: > >>>>>> -----Original Message----- > >>>>>> From: Jeremy Cline [mailto:jeremy@jcline.org] > >>>>>> Sent: Thursday, February 22, 2018 9:59 AM > >>>>>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; > notmart@gmail.com > >>>>>> Cc: pali.rohar@gmail.com; andriy.shevchenko@linux.intel.com; > >>>>>> mjg59@srcf.ucam.org; dvhart@infradead.org; platform-driver- > >>>>>> x86@vger.kernel.org; linux-kernel@vger.kernel.org > >>>>>> Subject: Re: Regression: Dell XPS 13 9360 keyboard no longer works > >>>>>> > >>>>>> On 02/22/2018 10:21 AM, Mario.Limonciello@dell.com wrote:> I guess > that > >>>>>> means we got this wrong and the patch should be reverted > >>>>>>> until we figure this out. > >>>>>>> > >>>>>>> Jeremy, > >>>>>>> > >>>>>>> Can you please confirm what BIOS version you are on? > >>>>>>> Also Is this a 9360 with 7th or 8th gen Intel CPU? > >>>>>> > >>>>>> Hi Mario, > >>>>>> > >>>>>> I've got BIOS version 2.5.0 with the 7th gen Intel CPU. > >>>>>> > >>>>>> > >>>>>> Regards, > >>>>>> Jeremy > >>>>> > >>>>> Jeremy, > >>>>> > >>>>> Thanks. Do you have any of the Dell docks (TB16/WD15)? If so are you > >> connected > >>>> to any dock > >>>>> when reproducing this problem? > >>>> > >>>> Mario, > >>>> > >>>> I do have a TB16. I can reproduce this whether or not I'm connected to > >>>> the dock, though. > >>>> > >>>> > >>>> Regards, > >>>> Jeremy > >>> > >>> Jeremy, > >>> > >>> Can you try booting up from a cold boot with it connected to see if it still > >> happens? > >>> > >> > >> Mario, > >> > >> Yup, it still happens from a cold boot when connected to the dock. > > > > OK thanks for confirming. Here's what I've concluded: > > > > * So looking through the ACPI tables on the 9360 it initializes that status > > (slate vs laptop mode) bit to "slate" mode. The 9360 isn't a 2-in1- so that > > seems wrong to me, but that's what it does. > > It only gets updated based on dock status. > > > > The 9365 (which is a 2 in 1) however seems to initialize the status properly. > > > > So that's an impasse of what to do. > > It's not clear to me what is really happening: > > a) We're missing something else in this driver (eg something else that > > indicates whether to trust VGBS output) > > > > b) Mis-interpreting the results from it (we shouldn't report the switch for > > tablet mode based on what we do) > > > > c) 9360 has a BIOS bug (seems unlikely since Windows doesn't freak out and > > show virtual keyboard at wrong time) > > > > I'm leaning on it's probably <a>. > > We should check for tablet mode should only be run if chassis type > > matches 2-in-1 (chassis type 0x1F). > > > > I believe that should fix the problem on the 9360, let it continue to work on > > the 9365 (and other 2-in-1's). > > > > Can you guys please test this? If that works I'll split up the routine and submit > > it. > > > > Hi Mario, > > There's one small error in the patch, after I fixed it, it works. > Thanks! Thanks, glad to hear. I sent it to the list separately with my cleanups and your fix for strcmp. If you can re-test with the cleaned up patch to add a Tested-By, that would be great. > > > diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c > > index b703d6f..07bc489 100644 > > --- a/drivers/platform/x86/intel-vbtn.c > > +++ b/drivers/platform/x86/intel-vbtn.c > > @@ -7,6 +7,7 @@ > > */ > > > > #include <linux/acpi.h> > > +#include <linux/dmi.h> > > #include <linux/input.h> > > #include <linux/input/sparse-keymap.h> > > #include <linux/kernel.h> > > @@ -102,6 +103,7 @@ static int intel_vbtn_probe(struct platform_device > *device) > > struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL }; > > acpi_handle handle = ACPI_HANDLE(&device->dev); > > struct intel_vbtn_priv *priv; > > + const char *chassis_type; > > acpi_status status; > > int err; > > > > @@ -123,22 +125,24 @@ static int intel_vbtn_probe(struct platform_device > *device) > > } > > > > /* > > - * VGBS being present and returning something means we have > > - * a tablet mode switch. > > + * Running on 2-in-1 chassis, VGBS being present and > > + * returning something means we have a tablet mode switch. > > */ > > - status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output); > > - if (ACPI_SUCCESS(status)) { > > - union acpi_object *obj = vgbs_output.pointer; > > + chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE); > > + if (chassis_type && strcmp(chassis_type, "31")) {This should be > "chassis_type && strcmp(chassis_type, "31") == 0" since > strcmp returns 0 for equality. > > > + status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output); > > + if (ACPI_SUCCESS(status)) { > > + union acpi_object *obj = vgbs_output.pointer; > > > > - if (obj && obj->type == ACPI_TYPE_INTEGER) { > > - int m = !(obj->integer.value & TABLET_MODE_FLAG); > > + if (obj && obj->type == ACPI_TYPE_INTEGER) { > > + int m = !(obj->integer.value & TABLET_MODE_FLAG); > > > > - input_report_switch(priv->input_dev, SW_TABLET_MODE, m); > > + input_report_switch(priv->input_dev, SW_TABLET_MODE, m); > > + } > > } > > + kfree(vgbs_output.pointer); > > } > > - kfree(vgbs_output.pointer); > > - > > status = acpi_install_notify_handler(handle, > > ACPI_DEVICE_NOTIFY, > > notify_handler, > > > > Regards, > Jeremy
diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c index b703d6f..07bc489 100644 --- a/drivers/platform/x86/intel-vbtn.c +++ b/drivers/platform/x86/intel-vbtn.c @@ -7,6 +7,7 @@ */ #include <linux/acpi.h> +#include <linux/dmi.h> #include <linux/input.h> #include <linux/input/sparse-keymap.h> #include <linux/kernel.h> @@ -102,6 +103,7 @@ static int intel_vbtn_probe(struct platform_device *device) struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL }; acpi_handle handle = ACPI_HANDLE(&device->dev); struct intel_vbtn_priv *priv; + const char *chassis_type; acpi_status status; int err; @@ -123,22 +125,24 @@ static int intel_vbtn_probe(struct platform_device *device) } /* - * VGBS being present and returning something means we have - * a tablet mode switch. + * Running on 2-in-1 chassis, VGBS being present and + * returning something means we have a tablet mode switch. */ - status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output); - if (ACPI_SUCCESS(status)) { - union acpi_object *obj = vgbs_output.pointer; + chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE); + if (chassis_type && strcmp(chassis_type, "31")) { + status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output); + if (ACPI_SUCCESS(status)) { + union acpi_object *obj = vgbs_output.pointer; - if (obj && obj->type == ACPI_TYPE_INTEGER) { - int m = !(obj->integer.value & TABLET_MODE_FLAG); + if (obj && obj->type == ACPI_TYPE_INTEGER) { + int m = !(obj->integer.value & TABLET_MODE_FLAG); - input_report_switch(priv->input_dev, SW_TABLET_MODE, m); + input_report_switch(priv->input_dev, SW_TABLET_MODE, m); + } } + kfree(vgbs_output.pointer); } - kfree(vgbs_output.pointer); - status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY, notify_handler,