Re: Connection issue since 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests")

[Heiner Kallweit <hkallweit1@gmail.com>]

On 11.10.2024 22:06, Luiz Augusto von Dentz wrote:
> Hi Heiner,
> 
> On Fri, Oct 11, 2024 at 3:44 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 11.10.2024 21:26, Luiz Augusto von Dentz wrote:
>>> Hi Heiner,
>>>
>>> On Fri, Oct 11, 2024 at 3:05 PM Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>>
>>>> Hi Heiner,
>>>>
>>>> On Fri, Oct 11, 2024 at 2:52 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>>>
>>>>> On 11.10.2024 18:36, Luiz Augusto von Dentz wrote:
>>>>>> Hi Heiner,
>>>>>>
>>>>>> On Fri, Oct 11, 2024 at 6:49 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>>>>>
>>>>>>> Since linux-next from Oct 4th my bt speaker fails to connect if I switch it on.
>>>>>>> It just hangs trying. Manually connecting it via bluetoothctl works though.
>>>>>>> With this patch reverted it auto-connects again.
>>>>>>> If you need additional details, please let me know.
>>>>>>
>>>>>> I suspect something is trying to suspend the controller then, it
>>>>>> shouldn't be USB auto-suspend since that should behave as it
>>>>>> previously but if there is something externally (aka. userspace)
>>>>>> trying to suspend then it will force it to suspend.
>>>>>>
>>>>> On the host side it's a combined WiFi/BT PCIe adapter (RTL8822CE).
>>>>> Runtime PM is enabled, so this may kick in. I'm not aware of any
>>>>> userspace tool which may try to suspend the WiFi/BT adapter.
>>>>> Disabling Runtime PM may be a workaround, but I don't think that's
>>>>> the actual solution.
>>>>>
>>>>
>>>> Well I assume it still using USB as transport, not PCIe, otherwise it
>>>> wouldn't be using btusb. Regarding runtime PM, I assume it still means
>>>> PMSG_IS_AUTO Documentation/driver-api/usb/power-management.rst:
>>>>
>>>>      'External suspend calls should never be allowed to fail in this way,
>>>>      only autosuspend calls.  The driver can tell them apart by applying
>>>>      the :c:func:`PMSG_IS_AUTO` macro to the message argument to the
>>>>      ``suspend`` method; it will return True for internal PM events'
>>>>
>>>> --
>>>> Luiz Augusto von Dentz
>>>
>>> Perhaps there is a double call to the likes of hci_suspend_dev due to
>>> system suspend and device suspend acting together, so maybe we need
>>> something like the following:
>>>
>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> index 779c4aeaef22..c257759ae2f4 100644
>>> --- a/net/bluetooth/hci_core.c
>>> +++ b/net/bluetooth/hci_core.c
>>> @@ -2812,7 +2812,7 @@ int hci_suspend_dev(struct hci_dev *hdev)
>>>
>>>         /* Suspend should only act on when powered. */
>>>         if (!hdev_is_powered(hdev) ||
>>> -           hci_dev_test_flag(hdev, HCI_UNREGISTER))
>>> +           hci_dev_test_flag(hdev, HCI_UNREGISTER) || hdev->suspended)
>>>                 return 0;
>>>
>>>         /* If powering down don't attempt to suspend */
>>> @@ -2843,7 +2843,7 @@ int hci_resume_dev(struct hci_dev *hdev)
>>>
>>>         /* Resume should only act on when powered. */
>>>         if (!hdev_is_powered(hdev) ||
>>> -           hci_dev_test_flag(hdev, HCI_UNREGISTER))
>>> +           hci_dev_test_flag(hdev, HCI_UNREGISTER) || !hdev->suspended)
>>>                 return 0;
>>>
>>>         /* If powering down don't attempt to resume */
>>>
>>>
>> No change in behavior with this change.
> 
> Ok, I guess we can rule out system suspend then, does commenting out
> the call to hci_suspend_dev makes it works again? If it does it means
> the device is suspending and it appears the likes of
> HCI_FLT_CONN_SETUP either doesn't work or is not being set to the
> controller to allow incoming connections, since it is an Audio device
> I think the problem is that we don't set HCI_CONN_FLAG_REMOTE_WAKEUP,
> only input device do set it, so we may need to treat device suspend
> and system suspend differently.
> 

The following makes it work again.

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7860750ec..33fd65d49 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2808,6 +2808,8 @@ int hci_suspend_dev(struct hci_dev *hdev)
 {
 	int ret;
 
+	return 0;
+
 	bt_dev_dbg(hdev, "");
 
 	/* Suspend should only act on when powered. */