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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 12.10.2024 00:19, Luiz Augusto von Dentz wrote:
> Hi Heiner,
> 
> On Fri, Oct 11, 2024 at 4:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> 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. */
>>
> 
> Can you try with the following changes:
> 
> https://gist.github.com/Vudentz/52c0cf8b9472a03b8eb447951be13811
> 
In this patch all tabs have been replaced with spaces, therefore it
didn't apply and I had to apply the changes manually.
Unfortunately the changes don't fix the issue for me.