[v1] Bluetooth: btusb: Fix not being able to reconnect after suspend

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Calls to hci_suspend_dev assumes the system-suspend which doesn't work
well when just the device is being suspended because wakeup flag is only
set for remote devices that can wakeup the system.

Reported-by: Rafael J. Wysocki <rafael@kernel.org>
Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Reported-by: Kenneth Crudup <kenny@panix.com>
Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/btusb.c | 14 --------------
 1 file changed, 14 deletions(-)

Dear Luiz,


Thank you for the patch.


Am 14.10.24 um 22:23 schrieb Luiz Augusto von Dentz:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> Calls to hci_suspend_dev assumes the system-suspend which doesn't work
> well when just the device is being suspended because wakeup flag is only
> set for remote devices that can wakeup the system.

Please mention that you revert most parts of the problematic commit.

> Reported-by: Rafael J. Wysocki <rafael@kernel.org>
> Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
> Reported-by: Kenneth Crudup <kenny@panix.com>
> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests")

That commit is not in the master branch, 
610712298b11b2914be00b35abe9326b5dbb62c8 is.

> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>   drivers/bluetooth/btusb.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d14b941bfde8..c0b6ef8ee5da 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4075,7 +4075,6 @@ static void btusb_disconnect(struct usb_interface *intf)
>   static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>   {
>   	struct btusb_data *data = usb_get_intfdata(intf);
> -	int err;
>   
>   	BT_DBG("intf %p", intf);
>   
> @@ -4088,16 +4087,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>   	if (data->suspend_count++)
>   		return 0;
>   
> -	/* Notify Host stack to suspend; this has to be done before stopping
> -	 * the traffic since the hci_suspend_dev itself may generate some
> -	 * traffic.
> -	 */
> -	err = hci_suspend_dev(data->hdev);
> -	if (err) {
> -		data->suspend_count--;
> -		return err;
> -	}
> -
>   	spin_lock_irq(&data->txlock);
>   	if (!(PMSG_IS_AUTO(message) && data->tx_in_flight)) {
>   		set_bit(BTUSB_SUSPENDING, &data->flags);
> @@ -4105,7 +4094,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>   	} else {
>   		spin_unlock_irq(&data->txlock);
>   		data->suspend_count--;
> -		hci_resume_dev(data->hdev);
>   		return -EBUSY;
>   	}
>   
> @@ -4226,8 +4214,6 @@ static int btusb_resume(struct usb_interface *intf)
>   	spin_unlock_irq(&data->txlock);
>   	schedule_work(&data->work);
>   
> -	hci_resume_dev(data->hdev);
> -
>   	return 0;
>   
>   failed:


Kind regards,

Paul

[CCing Stephen JFYI]

On 16.10.24 07:12, Paul Menzel wrote:
>
> Thank you for the patch.

+1

>> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend
>> requests")
> 
> That commit is not in the master branch,
> 610712298b11b2914be00b35abe9326b5dbb62c8 is.

Luiz, please allow me to ask: is there a reason why the bluetooth tree
does not use a dedicated "-fixes" branch like many other subsystems do?
That would avoid mishaps like the one above and all those "duplicate
patches in the bluetooth tree" messages Stephen has to sent every few
weeks
(https://lore.kernel.org/all/?q=f%3Astephen+duplicate+%22bluetooth+tree%22
); reminder, you can have both your -fixes and your -for-next branch in
linux-next for test coverage.

Ciao, Thorsten

On Mon, Oct 14, 2024 at 10:24 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Calls to hci_suspend_dev assumes the system-suspend which doesn't work
> well when just the device is being suspended because wakeup flag is only
> set for remote devices that can wakeup the system.
>
> Reported-by: Rafael J. Wysocki <rafael@kernel.org>

First of all, the patch works here, so

Tested-by: Rafael J. Wysocki <rafael@kernel.org>

> Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
> Reported-by: Kenneth Crudup <kenny@panix.com>
> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests")

However, this is not the commit ID referred to in my report, as
already mentioned by Paul.

> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  drivers/bluetooth/btusb.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index d14b941bfde8..c0b6ef8ee5da 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4075,7 +4075,6 @@ static void btusb_disconnect(struct usb_interface *intf)
>  static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>         struct btusb_data *data = usb_get_intfdata(intf);
> -       int err;
>
>         BT_DBG("intf %p", intf);
>
> @@ -4088,16 +4087,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>         if (data->suspend_count++)
>                 return 0;
>
> -       /* Notify Host stack to suspend; this has to be done before stopping
> -        * the traffic since the hci_suspend_dev itself may generate some
> -        * traffic.
> -        */
> -       err = hci_suspend_dev(data->hdev);
> -       if (err) {
> -               data->suspend_count--;
> -               return err;
> -       }
> -
>         spin_lock_irq(&data->txlock);
>         if (!(PMSG_IS_AUTO(message) && data->tx_in_flight)) {
>                 set_bit(BTUSB_SUSPENDING, &data->flags);
> @@ -4105,7 +4094,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>         } else {
>                 spin_unlock_irq(&data->txlock);
>                 data->suspend_count--;
> -               hci_resume_dev(data->hdev);
>                 return -EBUSY;
>         }
>
> @@ -4226,8 +4214,6 @@ static int btusb_resume(struct usb_interface *intf)
>         spin_unlock_irq(&data->txlock);
>         schedule_work(&data->work);
>
> -       hci_resume_dev(data->hdev);
> -
>         return 0;
>
>  failed:
> --
> 2.47.0
>

Hi Rafael, Paul,

On Wed, Oct 16, 2024 at 8:06 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Oct 16, 2024 at 7:12 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> >
> > Dear Luiz,
> >
> >
> > Thank you for the patch.
> >
> >
> > Am 14.10.24 um 22:23 schrieb Luiz Augusto von Dentz:
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > >
> > > Calls to hci_suspend_dev assumes the system-suspend which doesn't work
> > > well when just the device is being suspended because wakeup flag is only
> > > set for remote devices that can wakeup the system.
> >
> > Please mention that you revert most parts of the problematic commit.
>
> Yes, it would be good to say in the changelog that this is a partial revert.

Ack.

> > > Reported-by: Rafael J. Wysocki <rafael@kernel.org>
> > > Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
> > > Reported-by: Kenneth Crudup <kenny@panix.com>
> > > Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests")
> >
> > That commit is not in the master branch,
> > 610712298b11b2914be00b35abe9326b5dbb62c8 is.

Right, looks like I need to rebase to get the updated hashes.

> Right.

On 16.10.24 21:01, Luiz Augusto von Dentz wrote:
> On Wed, Oct 16, 2024 at 2:29 AM Thorsten Leemhuis
> <regressions@leemhuis.info> wrote:
>> On 16.10.24 07:12, Paul Menzel wrote:
>>>
>>> Thank you for the patch.
>> +1
>>
>>>> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend
>>>> requests")
>>>
>>> That commit is not in the master branch,
>>> 610712298b11b2914be00b35abe9326b5dbb62c8 is.
>>
>> Luiz, please allow me to ask: is there a reason why the bluetooth tree
>> does not use a dedicated "-fixes" branch like many other subsystems do?
>> That would avoid mishaps like the one above and all those "duplicate
>> patches in the bluetooth tree" messages Stephen has to sent every few
>> weeks
>> (https://lore.kernel.org/all/?q=f%3Astephen+duplicate+%22bluetooth+tree%22
>> ); reminder, you can have both your -fixes and your -for-next branch in
>> linux-next for test coverage.
> 
> Not sure I follow, we do have bluetooth tree
> (https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git)
> for fixes during the RC phase,

Ahh, I see, you have two different trees, and not different branches in
one tree. Sorry, should have noticed that.

> or are you saying the fixes for RC
> shall not be integrated thru bluetooth-next but directly into
> bluetooth tree and then once merged they are pulled into
> bluetooth-next by rebasing to avoid changing the hash?

I'm no expert here, but subsystems use different strategies afaics. Most
afaics have two branches in one tree or (like you) two trees. Both are
in next. And they only merge the -fixes branch into their -next tree
when there is a need, not regularly (that iirc would upset Linus).

> While possible
> this would be hard with our CI which only tests patches against
> bluetooth-next tree

Can't that be changed? Also: that sounds (but I might be wrong there!)
like -fixes you send to Linus don't get tested independently. Wouldn't
that be better?

> so by not integrating the RC fixes we may be able
> to detect similar changes.
> 
> Regarding the duplicate detection, I wonder if that really a problem
> or some script failing to detect it is just a hash change, because git
> seems fine with those and in most cases it will just say it has
> already been applied and move on.

Stephen might be the better one to answer that, but from his mails and
my understanding of it it's more like "if duplicates happen occasionally
(for example because some change queued for -next turns out needs to be
send through -fixes quickly), it's not a problem. But it shouldn't be
something that happens due to the regular workflow.

I've also seen a few people including Greg complain about Fixes: tags
for commits that don't exist -- which is the case until the duplicate
commit (like the 81b3e33bb054 that triggered this discussion) lands
during the next merge window. But during that time window it can easily
confuse people I guess.

Anyway, maybe I shouldn't have send anything, this is not my area of
expertise. It's just that I noticed the mails from Stephen, the
complains from Greg, and that one discussion at maintainers summit or
LPC where "more trees should have their -fixes branch in next" very
briefly came up.

Ciao, Thorsten