[PATCH resend] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[PATCH resend] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Hans de Goede]

Hi All,

This is a resend with Kai Heng Feng added to the Cc. Kai can you please
test this patch. This patch replaces yours, as many devices seem to be
affected so a whitelist is the better approach here.

Regards,

Hans

[PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Hans de Goede]

It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
as unique as a GUID should be and is used on some other devices too.

This is causing spurious key-press reports on these other devices.

This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
ensure that it is actually running on a PEAQ 2-in-1, fixing the
spurious key-presses on these other devices.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1497861
BugLink: https://bugzilla.suse.com/attachment.cgi?id=743182
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/peaq-wmi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
index bc98ef95514a..67fa3fa32011 100644
--- a/drivers/platform/x86/peaq-wmi.c
+++ b/drivers/platform/x86/peaq-wmi.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/input-polldev.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -64,8 +65,22 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
 	}
 }
 
+/* Some other devices (Shuttle XS35) use the same WMI GUID for other purposes */
+static const struct dmi_system_id peaq_dmi_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PEAQ"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "PEAQ PMM C1010 MD99187"),
+		},
+	},
+};
+
 static int __init peaq_wmi_init(void)
 {
+	/* WMI GUID is not unique, also check for a DMI match */
+	if (!dmi_check_system(peaq_dmi_table))
+		return -ENODEV;
+
 	if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
 		return -ENODEV;
 
@@ -86,6 +101,9 @@ static int __init peaq_wmi_init(void)
 
 static void __exit peaq_wmi_exit(void)
 {
+	if (!dmi_check_system(peaq_dmi_table))
+		return;
+
 	if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
 		return;
 
-- 
2.14.2

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Andy Shevchenko]

On Thu, Oct 5, 2017 at 9:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
> as unique as a GUID should be and is used on some other devices too.
>
> This is causing spurious key-press reports on these other devices.
>
> This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
> ensure that it is actually running on a PEAQ 2-in-1, fixing the
> spurious key-presses on these other devices.

Thanks!

One comment though.

>  static void __exit peaq_wmi_exit(void)
>  {
> +       if (!dmi_check_system(peaq_dmi_table))
> +               return;
> +
>         if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>                 return;

I was thinking, after got kbuid bot complains on Kai's patch on
sections mismatch, do we need these checks at all?
How would be possible to get a module loaded in the first place if
system is not in whitelist?

If I'm right, please, drop the check and move table to __initconst.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH resend] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Kai-Heng Feng]

Hi Hans,

On Fri, Oct 6, 2017 at 2:04 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi All,
>
> This is a resend with Kai Heng Feng added to the Cc. Kai can you please
> test this patch. This patch replaces yours, as many devices seem to be
> affected so a whitelist is the better approach here.

I'll let the affected user to test the patch.

Thanks!

>
> Regards,
>
> Hans

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Darren Hart]

On Thu, Oct 05, 2017 at 09:19:23PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 5, 2017 at 9:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> > It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
> > as unique as a GUID should be and is used on some other devices too.
> >
> > This is causing spurious key-press reports on these other devices.
> >
> > This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
> > ensure that it is actually running on a PEAQ 2-in-1, fixing the
> > spurious key-presses on these other devices.
> 
> Thanks!
> 
> One comment though.
> 
> >  static void __exit peaq_wmi_exit(void)
> >  {
> > +       if (!dmi_check_system(peaq_dmi_table))
> > +               return;
> > +
> >         if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
> >                 return;
> 
> I was thinking, after got kbuid bot complains on Kai's patch on
> sections mismatch, do we need these checks at all?
> How would be possible to get a module loaded in the first place if
> system is not in whitelist?
> 

I was wondering this myself.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH resend] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Kai-Heng Feng]

Hi,

On Fri, Oct 6, 2017 at 12:23 PM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> Hi Hans,
>
> On Fri, Oct 6, 2017 at 2:04 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi All,
>>
>> This is a resend with Kai Heng Feng added to the Cc. Kai can you please
>> test this patch. This patch replaces yours, as many devices seem to be
>> affected so a whitelist is the better approach here.
>
> I'll let the affected user to test the patch.

The user reports the new patch works.

Thanks.

>
> Thanks!
>
>>
>> Regards,
>>
>> Hans

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Andy Shevchenko]

On Thu, Oct 5, 2017 at 9:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
> as unique as a GUID should be and is used on some other devices too.
>
> This is causing spurious key-press reports on these other devices.
>
> This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
> ensure that it is actually running on a PEAQ 2-in-1, fixing the
> spurious key-presses on these other devices.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1497861
> BugLink: https://bugzilla.suse.com/attachment.cgi?id=743182
> Cc: stable@vger.kernel.org

So, I have pushed to my review queue as is, though please consider my
comments in perhaps a separate patch.
Thanks!

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/peaq-wmi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
> index bc98ef95514a..67fa3fa32011 100644
> --- a/drivers/platform/x86/peaq-wmi.c
> +++ b/drivers/platform/x86/peaq-wmi.c
> @@ -8,6 +8,7 @@
>   */
>
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <linux/input-polldev.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -64,8 +65,22 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
>         }
>  }
>
> +/* Some other devices (Shuttle XS35) use the same WMI GUID for other purposes */
> +static const struct dmi_system_id peaq_dmi_table[] = {
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "PEAQ"),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PEAQ PMM C1010 MD99187"),
> +               },
> +       },
> +};
> +
>  static int __init peaq_wmi_init(void)
>  {
> +       /* WMI GUID is not unique, also check for a DMI match */
> +       if (!dmi_check_system(peaq_dmi_table))
> +               return -ENODEV;
> +
>         if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>                 return -ENODEV;
>
> @@ -86,6 +101,9 @@ static int __init peaq_wmi_init(void)
>
>  static void __exit peaq_wmi_exit(void)
>  {
> +       if (!dmi_check_system(peaq_dmi_table))
> +               return;
> +
>         if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>                 return;
>
> --
> 2.14.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Andy Shevchenko]

On Thu, Oct 5, 2017 at 9:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> On 05-10-17 19:59, Andy Shevchenko wrote:
>> On Thu, Oct 5, 2017 at 5:27 PM, Hans de Goede <hdegoede@redhat.com> wrote:

>> ... however, the patch can't be dropped.
>>
>> Either me, or you in v2 need to revert it. Tell me what you prefer here.

> Isn't the whole purpose of having a testing branch that patches can
> actually be dropped. AFAIK this has not even hit -next yet, so IMHO
> it should just be dropped ?

It used to be until the problem had been discovered, i.e. absence of
committer's Signed-off-by.

So, starting from this cycle testing branch is categorized as
'published' where we can't do any non-fast-forward work anymore.

> If it really should be reverted instead I'll leave doing a revert up
> to you.

I heard you.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Hans de Goede]

Hi,

On 05-10-17 19:59, Andy Shevchenko wrote:
> On Thu, Oct 5, 2017 at 5:27 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 05-10-17 16:23, Andy Shevchenko wrote:
>>>
>>> On Thu, Oct 5, 2017 at 5:20 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
>>>> as unique as a GUID should be and is used on some other devices too.
>>>>
>>>> This is causing spurious key-press reports on these other devices.
>>>>
>>>> This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
>>>> ensure that it is actually running on a PEAQ 2-in-1, fixing the
>>>> spurious key-presses on these other devices.
>>>>
>>>
>>> Recently I have pushed similar patch (another device). Can you rebase
>>> against testing?
>>
>>
>> That patch adds a blacklist, for yet another model then the 2 bugreports:
>>
>>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1497861
>>>> BugLink: https://bugzilla.suse.com/attachment.cgi?id=743182
>>
>>
>> which I've received. My patch adds a whitelist instead as it seems the GUID
>> used is some very generic GUID,
> 
> Since you are maintainer and moreover have a hardware to test I assume
> that's correct thing to do...
> 
>> so the blacklist patch should simply be
>> dropped,
> 
> ... however, the patch can't be dropped.
> 
> Either me, or you in v2 need to revert it. Tell me what you prefer here.

Isn't the whole purpose of having a testing branch that patches can
actually be dropped. AFAIK this has not even hit -next yet, so IMHO
it should just be dropped ?

If it really should be reverted instead I'll leave doing a revert up
to you.

> In any case, please (re)send with Cc to Kai so he or bug reportes may
> have a chance to test it as well.

Ok, done.

Regards,

Hans

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Andy Shevchenko]

On Thu, Oct 5, 2017 at 5:27 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 05-10-17 16:23, Andy Shevchenko wrote:
>>
>> On Thu, Oct 5, 2017 at 5:20 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
>>> as unique as a GUID should be and is used on some other devices too.
>>>
>>> This is causing spurious key-press reports on these other devices.
>>>
>>> This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
>>> ensure that it is actually running on a PEAQ 2-in-1, fixing the
>>> spurious key-presses on these other devices.
>>>
>>
>> Recently I have pushed similar patch (another device). Can you rebase
>> against testing?
>
>
> That patch adds a blacklist, for yet another model then the 2 bugreports:
>
>>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1497861
>>> BugLink: https://bugzilla.suse.com/attachment.cgi?id=743182
>
>
> which I've received. My patch adds a whitelist instead as it seems the GUID
> used is some very generic GUID,

Since you are maintainer and moreover have a hardware to test I assume
that's correct thing to do...

> so the blacklist patch should simply be
> dropped,

... however, the patch can't be dropped.

Either me, or you in v2 need to revert it. Tell me what you prefer here.

In any case, please (re)send with Cc to Kai so he or bug reportes may
have a chance to test it as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Hans de Goede]

Hi,

On 05-10-17 16:23, Andy Shevchenko wrote:
> On Thu, Oct 5, 2017 at 5:20 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
>> as unique as a GUID should be and is used on some other devices too.
>>
>> This is causing spurious key-press reports on these other devices.
>>
>> This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
>> ensure that it is actually running on a PEAQ 2-in-1, fixing the
>> spurious key-presses on these other devices.
>>
> 
> Recently I have pushed similar patch (another device). Can you rebase
> against testing?

That patch adds a blacklist, for yet another model then the 2 bugreports:

>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1497861
>> BugLink: https://bugzilla.suse.com/attachment.cgi?id=743182

which I've received. My patch adds a whitelist instead as it seems the GUID
used is some very generic GUID, so the blacklist patch should simply be
dropped, after which my patch should apply cleanly :)

Regards,

Hans






>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/platform/x86/peaq-wmi.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
>> index bc98ef95514a..67fa3fa32011 100644
>> --- a/drivers/platform/x86/peaq-wmi.c
>> +++ b/drivers/platform/x86/peaq-wmi.c
>> @@ -8,6 +8,7 @@
>>    */
>>
>>   #include <linux/acpi.h>
>> +#include <linux/dmi.h>
>>   #include <linux/input-polldev.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>> @@ -64,8 +65,22 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
>>          }
>>   }
>>
>> +/* Some other devices (Shuttle XS35) use the same WMI GUID for other purposes */
>> +static const struct dmi_system_id peaq_dmi_table[] = {
>> +       {
>> +               .matches = {
>> +                       DMI_MATCH(DMI_SYS_VENDOR, "PEAQ"),
>> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PEAQ PMM C1010 MD99187"),
>> +               },
>> +       },
>> +};
>> +
>>   static int __init peaq_wmi_init(void)
>>   {
>> +       /* WMI GUID is not unique, also check for a DMI match */
>> +       if (!dmi_check_system(peaq_dmi_table))
>> +               return -ENODEV;
>> +
>>          if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>>                  return -ENODEV;
>>
>> @@ -86,6 +101,9 @@ static int __init peaq_wmi_init(void)
>>
>>   static void __exit peaq_wmi_exit(void)
>>   {
>> +       if (!dmi_check_system(peaq_dmi_table))
>> +               return;
>> +
>>          if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>>                  return;
>>
>> --
>> 2.14.2
>>
> 
> 
> 

Re: [PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Andy Shevchenko]

On Thu, Oct 5, 2017 at 5:20 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
> as unique as a GUID should be and is used on some other devices too.
>
> This is causing spurious key-press reports on these other devices.
>
> This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
> ensure that it is actually running on a PEAQ 2-in-1, fixing the
> spurious key-presses on these other devices.
>

Recently I have pushed similar patch (another device). Can you rebase
against testing?

> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1497861
> BugLink: https://bugzilla.suse.com/attachment.cgi?id=743182
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/peaq-wmi.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
> index bc98ef95514a..67fa3fa32011 100644
> --- a/drivers/platform/x86/peaq-wmi.c
> +++ b/drivers/platform/x86/peaq-wmi.c
> @@ -8,6 +8,7 @@
>   */
>
>  #include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <linux/input-polldev.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -64,8 +65,22 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
>         }
>  }
>
> +/* Some other devices (Shuttle XS35) use the same WMI GUID for other purposes */
> +static const struct dmi_system_id peaq_dmi_table[] = {
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "PEAQ"),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "PEAQ PMM C1010 MD99187"),
> +               },
> +       },
> +};
> +
>  static int __init peaq_wmi_init(void)
>  {
> +       /* WMI GUID is not unique, also check for a DMI match */
> +       if (!dmi_check_system(peaq_dmi_table))
> +               return -ENODEV;
> +
>         if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>                 return -ENODEV;
>
> @@ -86,6 +101,9 @@ static int __init peaq_wmi_init(void)
>
>  static void __exit peaq_wmi_exit(void)
>  {
> +       if (!dmi_check_system(peaq_dmi_table))
> +               return;
> +
>         if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
>                 return;
>
> --
> 2.14.2
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH] platform/x86: peaq-wmi: Add DMI check before binding to the WMI interface

[Hans de Goede]

It seems that the WMI GUID used by the PEAQ 2-in-1 WMI hotkeys is not
as unique as a GUID should be and is used on some other devices too.

This is causing spurious key-press reports on these other devices.

This commits adds a DMI check to the PEAQ 2-in-1 WMI hotkeys driver to
ensure that it is actually running on a PEAQ 2-in-1, fixing the
spurious key-presses on these other devices.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1497861
BugLink: https://bugzilla.suse.com/attachment.cgi?id=743182
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/peaq-wmi.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/platform/x86/peaq-wmi.c b/drivers/platform/x86/peaq-wmi.c
index bc98ef95514a..67fa3fa32011 100644
--- a/drivers/platform/x86/peaq-wmi.c
+++ b/drivers/platform/x86/peaq-wmi.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/input-polldev.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -64,8 +65,22 @@ static void peaq_wmi_poll(struct input_polled_dev *dev)
 	}
 }
 
+/* Some other devices (Shuttle XS35) use the same WMI GUID for other purposes */
+static const struct dmi_system_id peaq_dmi_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "PEAQ"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "PEAQ PMM C1010 MD99187"),
+		},
+	},
+};
+
 static int __init peaq_wmi_init(void)
 {
+	/* WMI GUID is not unique, also check for a DMI match */
+	if (!dmi_check_system(peaq_dmi_table))
+		return -ENODEV;
+
 	if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
 		return -ENODEV;
 
@@ -86,6 +101,9 @@ static int __init peaq_wmi_init(void)
 
 static void __exit peaq_wmi_exit(void)
 {
+	if (!dmi_check_system(peaq_dmi_table))
+		return;
+
 	if (!wmi_has_guid(PEAQ_DOLBY_BUTTON_GUID))
 		return;
 
-- 
2.14.2