All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC 0/1] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirk
@ 2018-02-19 14:37 Hans de Goede
  2018-02-19 14:37 ` [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2018-02-19 14:37 UTC (permalink / raw)
  To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
  Cc: Hans de Goede, linux-bluetooth

Hi All,

As discussed here is a patch to move the reset_resume quirk adding for QCA
devices over from usb-id (chipset) to dmi (platform) matching.

Note this is still a RFC atm, because I still need to get feedback from
the reporter with the Yoga 920.

Kai-Heng, can you extend the dmi table with info from the device with
the problem you have and then either send me a new version with both
entries, or prepare a follow-up patch ?

Regards,

Hans

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking
  2018-02-19 14:37 [RFC 0/1] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirk Hans de Goede
@ 2018-02-19 14:37 ` Hans de Goede
  2018-02-19 15:33   ` Marcel Holtmann
  0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2018-02-19 14:37 UTC (permalink / raw)
  To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
  Cc: Hans de Goede, linux-bluetooth, stable, Brian Norris, Kai-Heng Feng

Commit 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume fix
with a "rewritten" version") applied the USB_QUIRK_RESET_RESUME to all QCA
btusb devices. But it turns out that the resume problems are not caused by
the QCA Rome chipset, on most platforms it resumes fine. The resume
problems are actually a platform problem (likely the platform cutting all
power when suspended).

The USB_QUIRK_RESET_RESUME quirk also disable runtime suspend, so by
matching on usb-ids, we're causing all boards with these chips to use extra
power, to fix resume problems which only happen on some boards.

This commit fixes this by applying the quirk based on DMI matching instead
of on usb-ids, so that we match the platform and not the chipset.

Fixes: 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume..")
Cc: stable@vger.kernel.org
Cc: Brian Norris <briannorris@chromium.org>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 2a55380ad730..a6023667f3b4 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -21,6 +21,7 @@
  *
  */
 
+#include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/usb.h>
 #include <linux/usb/quirks.h>
@@ -379,6 +380,22 @@ static const struct usb_device_id blacklist_table[] = {
 	{ }	/* Terminating entry */
 };
 
+/*
+ * The btusb build into some devices needs to be reset on resume, this is a
+ * problem with the platform (likely shutting off all power) not with the
+ * btusb chip itself. So we use a DMI list to match known broken platforms.
+ */
+static const struct dmi_system_id btusb_plat_needs_reset_resume_list[] = {
+	{
+		/* Lenovo yoga 920 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo YOGA 920"),
+		},
+	},
+	{}
+};
+
 #define BTUSB_MAX_ISOC_FRAMES	10
 
 #define BTUSB_INTR_RUNNING	0
@@ -2945,6 +2962,9 @@ static int btusb_probe(struct usb_interface *intf,
 	hdev->send   = btusb_send_frame;
 	hdev->notify = btusb_notify;
 
+	if (dmi_check_system(btusb_plat_needs_reset_resume_list))
+		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
+
 #ifdef CONFIG_PM
 	err = btusb_config_oob_wake(hdev);
 	if (err)
@@ -3031,12 +3051,6 @@ static int btusb_probe(struct usb_interface *intf,
 	if (id->driver_info & BTUSB_QCA_ROME) {
 		data->setup_on_usb = btusb_setup_qca;
 		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
-
-		/* QCA Rome devices lose their updated firmware over suspend,
-		 * but the USB hub doesn't notice any status change.
-		 * explicitly request a device reset on resume.
-		 */
-		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
 	}
 
 #ifdef CONFIG_BT_HCIBTUSB_RTL
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking
  2018-02-19 14:37 ` [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking Hans de Goede
@ 2018-02-19 15:33   ` Marcel Holtmann
  2018-02-19 16:37     ` Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Marcel Holtmann @ 2018-02-19 15:33 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Gustavo F. Padovan, Johan Hedberg, Bluez mailing list, stable,
	Brian Norris, Kai-Heng Feng

Hi Hans,

> Commit 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume fix
> with a "rewritten" version") applied the USB_QUIRK_RESET_RESUME to all QCA
> btusb devices. But it turns out that the resume problems are not caused by
> the QCA Rome chipset, on most platforms it resumes fine. The resume
> problems are actually a platform problem (likely the platform cutting all
> power when suspended).
> 
> The USB_QUIRK_RESET_RESUME quirk also disable runtime suspend, so by
> matching on usb-ids, we're causing all boards with these chips to use extra
> power, to fix resume problems which only happen on some boards.
> 
> This commit fixes this by applying the quirk based on DMI matching instead
> of on usb-ids, so that we match the platform and not the chipset.

just for the record, can we include the /sys/kernel/debug/usb/devices for that device here.

> 
> Fixes: 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume..")
> Cc: stable@vger.kernel.org
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 2a55380ad730..a6023667f3b4 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -21,6 +21,7 @@
>  *
>  */
> 
> +#include <linux/dmi.h>
> #include <linux/module.h>
> #include <linux/usb.h>
> #include <linux/usb/quirks.h>
> @@ -379,6 +380,22 @@ static const struct usb_device_id blacklist_table[] = {
> 	{ }	/* Terminating entry */
> };
> 
> +/*
> + * The btusb build into some devices needs to be reset on resume, this is a

Actually “btusb” is a driver name. So “Bluetooth USB modules”

> + * problem with the platform (likely shutting off all power) not with the
> + * btusb chip itself. So we use a DMI list to match known broken platforms.

Here s/btusb/module/

> + */
> +static const struct dmi_system_id btusb_plat_needs_reset_resume_list[] = {

I prefer to use _table instead of _list. Also drop the _plat_ part since that seems obvious.

> +	{
> +		/* Lenovo yoga 920 */

Use “Yoga" please.

And I would include “(QCA Rome device VID:PID)” so that we have a record of some sorts.

> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo YOGA 920”),

No DMI_EXACT_MATCH?

> +		},
> +	},
> +	{}
> +};
> +
> #define BTUSB_MAX_ISOC_FRAMES	10
> 
> #define BTUSB_INTR_RUNNING	0
> @@ -2945,6 +2962,9 @@ static int btusb_probe(struct usb_interface *intf,
> 	hdev->send   = btusb_send_frame;
> 	hdev->notify = btusb_notify;
> 
> +	if (dmi_check_system(btusb_plat_needs_reset_resume_list))
> +		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
> +
> #ifdef CONFIG_PM
> 	err = btusb_config_oob_wake(hdev);
> 	if (err)
> @@ -3031,12 +3051,6 @@ static int btusb_probe(struct usb_interface *intf,
> 	if (id->driver_info & BTUSB_QCA_ROME) {
> 		data->setup_on_usb = btusb_setup_qca;
> 		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
> -
> -		/* QCA Rome devices lose their updated firmware over suspend,
> -		 * but the USB hub doesn't notice any status change.
> -		 * explicitly request a device reset on resume.
> -		 */
> -		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
> 	}

It is all cosmetic. So otherwise this looks good.

Regards

Marcel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking
  2018-02-19 15:33   ` Marcel Holtmann
@ 2018-02-19 16:37     ` Hans de Goede
  2018-02-19 17:36       ` Marcel Holtmann
  0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2018-02-19 16:37 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Gustavo F. Padovan, Johan Hedberg, Bluez mailing list, stable,
	Brian Norris, Kai-Heng Feng

Hi,

On 19-02-18 16:33, Marcel Holtmann wrote:
> Hi Hans,
> 
>> Commit 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume fix
>> with a "rewritten" version") applied the USB_QUIRK_RESET_RESUME to all QCA
>> btusb devices. But it turns out that the resume problems are not caused by
>> the QCA Rome chipset, on most platforms it resumes fine. The resume
>> problems are actually a platform problem (likely the platform cutting all
>> power when suspended).
>>
>> The USB_QUIRK_RESET_RESUME quirk also disable runtime suspend, so by
>> matching on usb-ids, we're causing all boards with these chips to use extra
>> power, to fix resume problems which only happen on some boards.
>>
>> This commit fixes this by applying the quirk based on DMI matching instead
>> of on usb-ids, so that we match the platform and not the chipset.
> 
> just for the record, can we include the /sys/kernel/debug/usb/devices for that device here.

Will do.

>> Fixes: 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume..")
>> Cc: stable@vger.kernel.org
>> Cc: Brian Norris <briannorris@chromium.org>
>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++------
>> 1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 2a55380ad730..a6023667f3b4 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -21,6 +21,7 @@
>>   *
>>   */
>>
>> +#include <linux/dmi.h>
>> #include <linux/module.h>
>> #include <linux/usb.h>
>> #include <linux/usb/quirks.h>
>> @@ -379,6 +380,22 @@ static const struct usb_device_id blacklist_table[] = {
>> 	{ }	/* Terminating entry */
>> };
>>
>> +/*
>> + * The btusb build into some devices needs to be reset on resume, this is a
> 
> Actually “btusb” is a driver name. So “Bluetooth USB modules”
> 
>> + * problem with the platform (likely shutting off all power) not with the
>> + * btusb chip itself. So we use a DMI list to match known broken platforms.
> 
> Here s/btusb/module/
> 
>> + */
>> +static const struct dmi_system_id btusb_plat_needs_reset_resume_list[] = {
> 
> I prefer to use _table instead of _list. Also drop the _plat_ part since that seems obvious.
> 
>> +	{
>> +		/* Lenovo yoga 920 */
> 
> Use “Yoga" please.

I will fix all of the above.

> And I would include “(QCA Rome device VID:PID)” so that we have a record of some sorts.
> 
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo YOGA 920”),
> 
> No DMI_EXACT_MATCH?

The DMI data actually has:

"Lenovo YOGA 920-13IKB"

I'm not using DMI_EXACT_MATCH on purpose here, I think Lenovo
might change the "-13IKB" part and I don't expect them to fix
this bug on newer revisions.

Regards,

Hans


> 
>> +		},
>> +	},
>> +	{}
>> +};
>> +
>> #define BTUSB_MAX_ISOC_FRAMES	10
>>
>> #define BTUSB_INTR_RUNNING	0
>> @@ -2945,6 +2962,9 @@ static int btusb_probe(struct usb_interface *intf,
>> 	hdev->send   = btusb_send_frame;
>> 	hdev->notify = btusb_notify;
>>
>> +	if (dmi_check_system(btusb_plat_needs_reset_resume_list))
>> +		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
>> +
>> #ifdef CONFIG_PM
>> 	err = btusb_config_oob_wake(hdev);
>> 	if (err)
>> @@ -3031,12 +3051,6 @@ static int btusb_probe(struct usb_interface *intf,
>> 	if (id->driver_info & BTUSB_QCA_ROME) {
>> 		data->setup_on_usb = btusb_setup_qca;
>> 		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
>> -
>> -		/* QCA Rome devices lose their updated firmware over suspend,
>> -		 * but the USB hub doesn't notice any status change.
>> -		 * explicitly request a device reset on resume.
>> -		 */
>> -		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
>> 	}
> 
> It is all cosmetic. So otherwise this looks good.
> 
> Regards
> 
> Marcel
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking
  2018-02-19 16:37     ` Hans de Goede
@ 2018-02-19 17:36       ` Marcel Holtmann
  2018-02-19 19:55         ` Hans de Goede
  0 siblings, 1 reply; 7+ messages in thread
From: Marcel Holtmann @ 2018-02-19 17:36 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Gustavo F. Padovan, Johan Hedberg, Bluez mailing list, stable,
	Brian Norris, Kai-Heng Feng

Hi Hans,

>>> Commit 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume fix
>>> with a "rewritten" version") applied the USB_QUIRK_RESET_RESUME to all QCA
>>> btusb devices. But it turns out that the resume problems are not caused by
>>> the QCA Rome chipset, on most platforms it resumes fine. The resume
>>> problems are actually a platform problem (likely the platform cutting all
>>> power when suspended).
>>> 
>>> The USB_QUIRK_RESET_RESUME quirk also disable runtime suspend, so by
>>> matching on usb-ids, we're causing all boards with these chips to use extra
>>> power, to fix resume problems which only happen on some boards.
>>> 
>>> This commit fixes this by applying the quirk based on DMI matching instead
>>> of on usb-ids, so that we match the platform and not the chipset.
>> just for the record, can we include the /sys/kernel/debug/usb/devices for that device here.
> 
> Will do.
> 
>>> Fixes: 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume..")
>>> Cc: stable@vger.kernel.org
>>> Cc: Brian Norris <briannorris@chromium.org>
>>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++------
>>> 1 file changed, 20 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 2a55380ad730..a6023667f3b4 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -21,6 +21,7 @@
>>>  *
>>>  */
>>> 
>>> +#include <linux/dmi.h>
>>> #include <linux/module.h>
>>> #include <linux/usb.h>
>>> #include <linux/usb/quirks.h>
>>> @@ -379,6 +380,22 @@ static const struct usb_device_id blacklist_table[] = {
>>> 	{ }	/* Terminating entry */
>>> };
>>> 
>>> +/*
>>> + * The btusb build into some devices needs to be reset on resume, this is a
>> Actually “btusb” is a driver name. So “Bluetooth USB modules”
>>> + * problem with the platform (likely shutting off all power) not with the
>>> + * btusb chip itself. So we use a DMI list to match known broken platforms.
>> Here s/btusb/module/
>>> + */
>>> +static const struct dmi_system_id btusb_plat_needs_reset_resume_list[] = {
>> I prefer to use _table instead of _list. Also drop the _plat_ part since that seems obvious.
>>> +	{
>>> +		/* Lenovo yoga 920 */
>> Use “Yoga" please.
> 
> I will fix all of the above.
> 
>> And I would include “(QCA Rome device VID:PID)” so that we have a record of some sorts.
>>> +		.matches = {
>>> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>> +			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo YOGA 920”),
>> No DMI_EXACT_MATCH?
> 
> The DMI data actually has:
> 
> "Lenovo YOGA 920-13IKB"
> 
> I'm not using DMI_EXACT_MATCH on purpose here, I think Lenovo
> might change the "-13IKB" part and I don't expect them to fix
> this bug on newer revisions.

that is fine. What about the “LENOVO” vendor match? Should that be an exact match?

Regards

Marcel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking
  2018-02-19 17:36       ` Marcel Holtmann
@ 2018-02-19 19:55         ` Hans de Goede
  2018-02-19 20:01           ` Marcel Holtmann
  0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2018-02-19 19:55 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Gustavo F. Padovan, Johan Hedberg, Bluez mailing list, stable,
	Brian Norris, Kai-Heng Feng

Hi,

On 19-02-18 18:36, Marcel Holtmann wrote:
> Hi Hans,
> 
>>>> Commit 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume fix
>>>> with a "rewritten" version") applied the USB_QUIRK_RESET_RESUME to all QCA
>>>> btusb devices. But it turns out that the resume problems are not caused by
>>>> the QCA Rome chipset, on most platforms it resumes fine. The resume
>>>> problems are actually a platform problem (likely the platform cutting all
>>>> power when suspended).
>>>>
>>>> The USB_QUIRK_RESET_RESUME quirk also disable runtime suspend, so by
>>>> matching on usb-ids, we're causing all boards with these chips to use extra
>>>> power, to fix resume problems which only happen on some boards.
>>>>
>>>> This commit fixes this by applying the quirk based on DMI matching instead
>>>> of on usb-ids, so that we match the platform and not the chipset.
>>> just for the record, can we include the /sys/kernel/debug/usb/devices for that device here.
>>
>> Will do.
>>
>>>> Fixes: 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume..")
>>>> Cc: stable@vger.kernel.org
>>>> Cc: Brian Norris <briannorris@chromium.org>
>>>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++------
>>>> 1 file changed, 20 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>> index 2a55380ad730..a6023667f3b4 100644
>>>> --- a/drivers/bluetooth/btusb.c
>>>> +++ b/drivers/bluetooth/btusb.c
>>>> @@ -21,6 +21,7 @@
>>>>   *
>>>>   */
>>>>
>>>> +#include <linux/dmi.h>
>>>> #include <linux/module.h>
>>>> #include <linux/usb.h>
>>>> #include <linux/usb/quirks.h>
>>>> @@ -379,6 +380,22 @@ static const struct usb_device_id blacklist_table[] = {
>>>> 	{ }	/* Terminating entry */
>>>> };
>>>>
>>>> +/*
>>>> + * The btusb build into some devices needs to be reset on resume, this is a
>>> Actually “btusb” is a driver name. So “Bluetooth USB modules”
>>>> + * problem with the platform (likely shutting off all power) not with the
>>>> + * btusb chip itself. So we use a DMI list to match known broken platforms.
>>> Here s/btusb/module/
>>>> + */
>>>> +static const struct dmi_system_id btusb_plat_needs_reset_resume_list[] = {
>>> I prefer to use _table instead of _list. Also drop the _plat_ part since that seems obvious.
>>>> +	{
>>>> +		/* Lenovo yoga 920 */
>>> Use “Yoga" please.
>>
>> I will fix all of the above.
>>
>>> And I would include “(QCA Rome device VID:PID)” so that we have a record of some sorts.
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>> +			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo YOGA 920”),
>>> No DMI_EXACT_MATCH?
>>
>> The DMI data actually has:
>>
>> "Lenovo YOGA 920-13IKB"
>>
>> I'm not using DMI_EXACT_MATCH on purpose here, I think Lenovo
>> might change the "-13IKB" part and I don't expect them to fix
>> this bug on newer revisions.
> 
> that is fine. What about the “LENOVO” vendor match? Should that be an exact match?

It could be an exact match, in general I tend to not use DMI_EXACT_MATCH
unless necessary though, as vendors sometimes add whitespace padding after
the contents. Esp. Asus is known to do this. But if you want me to change it
over to DMI_EXACT_MATCH I can do that for the non RFC version which I plan
to send tomorrow (I just got test feedback that this version works for the
reporter).

Regards,

Hans

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking
  2018-02-19 19:55         ` Hans de Goede
@ 2018-02-19 20:01           ` Marcel Holtmann
  0 siblings, 0 replies; 7+ messages in thread
From: Marcel Holtmann @ 2018-02-19 20:01 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Gustavo F. Padovan, Johan Hedberg, Bluez mailing list, stable,
	Brian Norris, Kai-Heng Feng

Hi Hans,

>>>>> Commit 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume fix
>>>>> with a "rewritten" version") applied the USB_QUIRK_RESET_RESUME to all QCA
>>>>> btusb devices. But it turns out that the resume problems are not caused by
>>>>> the QCA Rome chipset, on most platforms it resumes fine. The resume
>>>>> problems are actually a platform problem (likely the platform cutting all
>>>>> power when suspended).
>>>>> 
>>>>> The USB_QUIRK_RESET_RESUME quirk also disable runtime suspend, so by
>>>>> matching on usb-ids, we're causing all boards with these chips to use extra
>>>>> power, to fix resume problems which only happen on some boards.
>>>>> 
>>>>> This commit fixes this by applying the quirk based on DMI matching instead
>>>>> of on usb-ids, so that we match the platform and not the chipset.
>>>> just for the record, can we include the /sys/kernel/debug/usb/devices for that device here.
>>> 
>>> Will do.
>>> 
>>>>> Fixes: 61f5acea8737 ("Bluetooth: btusb: Restore QCA Rome suspend/resume..")
>>>>> Cc: stable@vger.kernel.org
>>>>> Cc: Brian Norris <briannorris@chromium.org>
>>>>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++------
>>>>> 1 file changed, 20 insertions(+), 6 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index 2a55380ad730..a6023667f3b4 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -21,6 +21,7 @@
>>>>>  *
>>>>>  */
>>>>> 
>>>>> +#include <linux/dmi.h>
>>>>> #include <linux/module.h>
>>>>> #include <linux/usb.h>
>>>>> #include <linux/usb/quirks.h>
>>>>> @@ -379,6 +380,22 @@ static const struct usb_device_id blacklist_table[] = {
>>>>> 	{ }	/* Terminating entry */
>>>>> };
>>>>> 
>>>>> +/*
>>>>> + * The btusb build into some devices needs to be reset on resume, this is a
>>>> Actually “btusb” is a driver name. So “Bluetooth USB modules”
>>>>> + * problem with the platform (likely shutting off all power) not with the
>>>>> + * btusb chip itself. So we use a DMI list to match known broken platforms.
>>>> Here s/btusb/module/
>>>>> + */
>>>>> +static const struct dmi_system_id btusb_plat_needs_reset_resume_list[] = {
>>>> I prefer to use _table instead of _list. Also drop the _plat_ part since that seems obvious.
>>>>> +	{
>>>>> +		/* Lenovo yoga 920 */
>>>> Use “Yoga" please.
>>> 
>>> I will fix all of the above.
>>> 
>>>> And I would include “(QCA Rome device VID:PID)” so that we have a record of some sorts.
>>>>> +		.matches = {
>>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>>> +			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo YOGA 920”),
>>>> No DMI_EXACT_MATCH?
>>> 
>>> The DMI data actually has:
>>> 
>>> "Lenovo YOGA 920-13IKB"
>>> 
>>> I'm not using DMI_EXACT_MATCH on purpose here, I think Lenovo
>>> might change the "-13IKB" part and I don't expect them to fix
>>> this bug on newer revisions.
>> that is fine. What about the “LENOVO” vendor match? Should that be an exact match?
> 
> It could be an exact match, in general I tend to not use DMI_EXACT_MATCH
> unless necessary though, as vendors sometimes add whitespace padding after
> the contents. Esp. Asus is known to do this. But if you want me to change it
> over to DMI_EXACT_MATCH I can do that for the non RFC version which I plan
> to send tomorrow (I just got test feedback that this version works for the
> reporter).

I just mentioned it since in the hci_bcm.c we used DMI_EXACT_MATCH for the Lenovo hardware.

Regards

Marcel


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-02-19 20:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-19 14:37 [RFC 0/1] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirk Hans de Goede
2018-02-19 14:37 ` [RFC] Bluetooth: btusb: Use DMI matching for QCA reset_resume quirking Hans de Goede
2018-02-19 15:33   ` Marcel Holtmann
2018-02-19 16:37     ` Hans de Goede
2018-02-19 17:36       ` Marcel Holtmann
2018-02-19 19:55         ` Hans de Goede
2018-02-19 20:01           ` Marcel Holtmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.