Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices"
@ 2020-07-29 23:17 Abhishek Pandit-Subedi
  2020-07-30  4:14 ` Kai-Heng Feng
  2020-07-30  9:55 ` Marcel Holtmann
  0 siblings, 2 replies; 5+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-07-29 23:17 UTC (permalink / raw)
  To: marcel
  Cc: chromeos-bluetooth-upstreaming, Kai-Heng Feng, linux-bluetooth,
	Alex Lu, linux-pm, Abhishek Pandit-Subedi, Johan Hedberg,
	linux-kernel

This reverts commit 7ecacafc240638148567742cca41aa7144b4fe1e.

Testing this change on a board with RTL8822CE, I found that enabling
autosuspend has no effect on the stability of the system. The board
continued working after autosuspend, suspend and reboot.

The original commit makes it impossible to enable autosuspend on working
systems so it should be reverted. Disabling autosuspend should be done
via module param or udev in userspace instead.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
We have a few Chromebooks using the RTL 8822CE part over USB and they
are running without problems with autosuspend enabled. While bringing up
a new board, I found some power regressions that I was able to narrow
down to this change so I'm requesting a revert.

I tested this on Hp Chromebook 14a (running 4.14 kernel and 5.4 kernel)
with this revert:
* Enabled autosuspend, used it normally with a HID device
* Suspended the Chromebook and verified it worked normally on resume
* Rebooted the Chromebook and verified Bluetooth was working on next
  boot

I didn't see the issue that was originally reported with this fix. For
the original reporter, if you're still seeing this issue, there are
other ways to disable autosuspend for your device:
* set module param: enable_autosuspend=0
* change your kconfig so BT_HCIBTUSB_AUTOSUSPEND=n
* use a udev rule to disable autosuspend for specific vid:pid

Keeping this change in the kernel makes it impossible to enable
autosuspend so it should be reverted.

 drivers/bluetooth/btusb.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1f51494f581812..8d2608ddfd0875 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -4086,10 +4086,6 @@ static int btusb_probe(struct usb_interface *intf,
 			set_bit(BTUSB_USE_ALT1_FOR_WBS, &data->flags);
 		else
 			bt_dev_err(hdev, "Device does not support ALT setting 1");
-
-		err = usb_autopm_get_interface(intf);
-		if (err < 0)
-			goto out_free_dev;
 	}
 
 	if (!reset)
-- 
2.28.0.rc0.142.g3c755180ce-goog


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

* Re: [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices"
  2020-07-29 23:17 [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices" Abhishek Pandit-Subedi
@ 2020-07-30  4:14 ` Kai-Heng Feng
  2020-07-30  7:30   ` Marcel Holtmann
  2020-07-30  9:55 ` Marcel Holtmann
  1 sibling, 1 reply; 5+ messages in thread
From: Kai-Heng Feng @ 2020-07-30  4:14 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: marcel, chromeos-bluetooth-upstreaming, linux-bluetooth, Alex Lu,
	linux-pm, Johan Hedberg, linux-kernel

Hi Abhishek,

> On Jul 30, 2020, at 07:17, Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote:
> 
> This reverts commit 7ecacafc240638148567742cca41aa7144b4fe1e.
> 
> Testing this change on a board with RTL8822CE, I found that enabling
> autosuspend has no effect on the stability of the system. The board
> continued working after autosuspend, suspend and reboot.

The original issue was found on 8723DE. Do you have one to test with?
The rtw88 codebase has changed a lot and maybe it's already fixed in mainline.
Let me do some test and I'll report back.

> 
> The original commit makes it impossible to enable autosuspend on working
> systems so it should be reverted. Disabling autosuspend should be done
> via module param or udev in userspace instead.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> We have a few Chromebooks using the RTL 8822CE part over USB and they
> are running without problems with autosuspend enabled. While bringing up
> a new board, I found some power regressions that I was able to narrow
> down to this change so I'm requesting a revert.
> 
> I tested this on Hp Chromebook 14a (running 4.14 kernel and 5.4 kernel)
> with this revert:
> * Enabled autosuspend, used it normally with a HID device
> * Suspended the Chromebook and verified it worked normally on resume
> * Rebooted the Chromebook and verified Bluetooth was working on next
>  boot
> 
> I didn't see the issue that was originally reported with this fix. For
> the original reporter, if you're still seeing this issue, there are
> other ways to disable autosuspend for your device:
> * set module param: enable_autosuspend=0
> * change your kconfig so BT_HCIBTUSB_AUTOSUSPEND=n
> * use a udev rule to disable autosuspend for specific vid:pid
> 
> Keeping this change in the kernel makes it impossible to enable
> autosuspend so it should be reverted.

It's apparently a driver/firmware/hardware issue, so the fix should keep inside the kernel.
However, the fix can be more precise and target only 8723DE.

Kai-Heng

> 
> drivers/bluetooth/btusb.c | 4 ----
> 1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1f51494f581812..8d2608ddfd0875 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4086,10 +4086,6 @@ static int btusb_probe(struct usb_interface *intf,
> 			set_bit(BTUSB_USE_ALT1_FOR_WBS, &data->flags);
> 		else
> 			bt_dev_err(hdev, "Device does not support ALT setting 1");
> -
> -		err = usb_autopm_get_interface(intf);
> -		if (err < 0)
> -			goto out_free_dev;
> 	}
> 
> 	if (!reset)
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 


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

* Re: [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices"
  2020-07-30  4:14 ` Kai-Heng Feng
@ 2020-07-30  7:30   ` Marcel Holtmann
  2020-07-30  9:50     ` Kai-Heng Feng
  0 siblings, 1 reply; 5+ messages in thread
From: Marcel Holtmann @ 2020-07-30  7:30 UTC (permalink / raw)
  To: Kai-Heng Feng
  Cc: Abhishek Pandit-Subedi, chromeos-bluetooth-upstreaming,
	Bluetooth Kernel Mailing List, Alex Lu, linux-pm, Johan Hedberg,
	linux-kernel

Hi Kai-Heng,

>> On Jul 30, 2020, at 07:17, Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote:
>> 
>> This reverts commit 7ecacafc240638148567742cca41aa7144b4fe1e.
>> 
>> Testing this change on a board with RTL8822CE, I found that enabling
>> autosuspend has no effect on the stability of the system. The board
>> continued working after autosuspend, suspend and reboot.
> 
> The original issue was found on 8723DE. Do you have one to test with?
> The rtw88 codebase has changed a lot and maybe it's already fixed in mainline.
> Let me do some test and I'll report back.
> 
>> 
>> The original commit makes it impossible to enable autosuspend on working
>> systems so it should be reverted. Disabling autosuspend should be done
>> via module param or udev in userspace instead.
>> 
>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> ---
>> We have a few Chromebooks using the RTL 8822CE part over USB and they
>> are running without problems with autosuspend enabled. While bringing up
>> a new board, I found some power regressions that I was able to narrow
>> down to this change so I'm requesting a revert.
>> 
>> I tested this on Hp Chromebook 14a (running 4.14 kernel and 5.4 kernel)
>> with this revert:
>> * Enabled autosuspend, used it normally with a HID device
>> * Suspended the Chromebook and verified it worked normally on resume
>> * Rebooted the Chromebook and verified Bluetooth was working on next
>> boot
>> 
>> I didn't see the issue that was originally reported with this fix. For
>> the original reporter, if you're still seeing this issue, there are
>> other ways to disable autosuspend for your device:
>> * set module param: enable_autosuspend=0
>> * change your kconfig so BT_HCIBTUSB_AUTOSUSPEND=n
>> * use a udev rule to disable autosuspend for specific vid:pid
>> 
>> Keeping this change in the kernel makes it impossible to enable
>> autosuspend so it should be reverted.
> 
> It's apparently a driver/firmware/hardware issue, so the fix should keep inside the kernel.
> However, the fix can be more precise and target only 8723DE.

lets do that and lets do it quickly since the merge window is close. Otherwise I really have to revert that patch.

Regards

Marcel


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

* Re: [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices"
  2020-07-30  7:30   ` Marcel Holtmann
@ 2020-07-30  9:50     ` Kai-Heng Feng
  0 siblings, 0 replies; 5+ messages in thread
From: Kai-Heng Feng @ 2020-07-30  9:50 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Abhishek Pandit-Subedi, chromeos-bluetooth-upstreaming,
	Bluetooth Kernel Mailing List, Alex Lu, linux-pm, Johan Hedberg,
	linux-kernel



> On Jul 30, 2020, at 15:30, Marcel Holtmann <marcel@holtmann.org> wrote:
> 
> Hi Kai-Heng,
> 
>>> On Jul 30, 2020, at 07:17, Abhishek Pandit-Subedi <abhishekpandit@chromium.org> wrote:
>>> 
>>> This reverts commit 7ecacafc240638148567742cca41aa7144b4fe1e.
>>> 
>>> Testing this change on a board with RTL8822CE, I found that enabling
>>> autosuspend has no effect on the stability of the system. The board
>>> continued working after autosuspend, suspend and reboot.
>> 
>> The original issue was found on 8723DE. Do you have one to test with?
>> The rtw88 codebase has changed a lot and maybe it's already fixed in mainline.
>> Let me do some test and I'll report back.
>> 
>>> 
>>> The original commit makes it impossible to enable autosuspend on working
>>> systems so it should be reverted. Disabling autosuspend should be done
>>> via module param or udev in userspace instead.
>>> 
>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>> ---
>>> We have a few Chromebooks using the RTL 8822CE part over USB and they
>>> are running without problems with autosuspend enabled. While bringing up
>>> a new board, I found some power regressions that I was able to narrow
>>> down to this change so I'm requesting a revert.
>>> 
>>> I tested this on Hp Chromebook 14a (running 4.14 kernel and 5.4 kernel)
>>> with this revert:
>>> * Enabled autosuspend, used it normally with a HID device
>>> * Suspended the Chromebook and verified it worked normally on resume
>>> * Rebooted the Chromebook and verified Bluetooth was working on next
>>> boot
>>> 
>>> I didn't see the issue that was originally reported with this fix. For
>>> the original reporter, if you're still seeing this issue, there are
>>> other ways to disable autosuspend for your device:
>>> * set module param: enable_autosuspend=0
>>> * change your kconfig so BT_HCIBTUSB_AUTOSUSPEND=n
>>> * use a udev rule to disable autosuspend for specific vid:pid
>>> 
>>> Keeping this change in the kernel makes it impossible to enable
>>> autosuspend so it should be reverted.
>> 
>> It's apparently a driver/firmware/hardware issue, so the fix should keep inside the kernel.
>> However, the fix can be more precise and target only 8723DE.
> 
> lets do that and lets do it quickly since the merge window is close. Otherwise I really have to revert that patch.

Ok, I no longer observe the original issue with the patch reverted.
Acked-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

> 
> Regards
> 
> Marcel
> 


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

* Re: [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices"
  2020-07-29 23:17 [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices" Abhishek Pandit-Subedi
  2020-07-30  4:14 ` Kai-Heng Feng
@ 2020-07-30  9:55 ` Marcel Holtmann
  1 sibling, 0 replies; 5+ messages in thread
From: Marcel Holtmann @ 2020-07-30  9:55 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: chromeos-bluetooth-upstreaming, Kai-Heng Feng,
	Bluetooth Kernel Mailing List, Alex Lu, linux-pm, Johan Hedberg,
	linux-kernel

Hi Abhishek,

> This reverts commit 7ecacafc240638148567742cca41aa7144b4fe1e.
> 
> Testing this change on a board with RTL8822CE, I found that enabling
> autosuspend has no effect on the stability of the system. The board
> continued working after autosuspend, suspend and reboot.
> 
> The original commit makes it impossible to enable autosuspend on working
> systems so it should be reverted. Disabling autosuspend should be done
> via module param or udev in userspace instead.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> We have a few Chromebooks using the RTL 8822CE part over USB and they
> are running without problems with autosuspend enabled. While bringing up
> a new board, I found some power regressions that I was able to narrow
> down to this change so I'm requesting a revert.
> 
> I tested this on Hp Chromebook 14a (running 4.14 kernel and 5.4 kernel)
> with this revert:
> * Enabled autosuspend, used it normally with a HID device
> * Suspended the Chromebook and verified it worked normally on resume
> * Rebooted the Chromebook and verified Bluetooth was working on next
>  boot
> 
> I didn't see the issue that was originally reported with this fix. For
> the original reporter, if you're still seeing this issue, there are
> other ways to disable autosuspend for your device:
> * set module param: enable_autosuspend=0
> * change your kconfig so BT_HCIBTUSB_AUTOSUSPEND=n
> * use a udev rule to disable autosuspend for specific vid:pid
> 
> Keeping this change in the kernel makes it impossible to enable
> autosuspend so it should be reverted.
> 
> drivers/bluetooth/btusb.c | 4 ----
> 1 file changed, 4 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 23:17 [PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices" Abhishek Pandit-Subedi
2020-07-30  4:14 ` Kai-Heng Feng
2020-07-30  7:30   ` Marcel Holtmann
2020-07-30  9:50     ` Kai-Heng Feng
2020-07-30  9:55 ` Marcel Holtmann

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git