Bluetooth: hci_sync: fix hogp device suspend bug

Commit Message

clancy_shang Dec. 26, 2023, 6:08 a.m. UTC

From: Clancy Shang <[e-mail]>

when Bluetooth enters suspend, and disconnects everything with the
disconnect reason code of 0x15, the hogp device could not into sleep
and continued advertising. when use the disconnect reason code of 0x13,
the hogp device going into sleep succeeded.

Signed-off-by: Clancy Shang <[e-mail]>
---
 net/bluetooth/hci_sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

[e-mail] Dec. 26, 2023, 6:35 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=812856

---Test result---

Test Summary:
CheckPatch                    PASS      0.70 seconds
GitLint                       PASS      0.31 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      27.89 seconds
CheckAllWarning               PASS      30.38 seconds
CheckSparse                   PASS      35.87 seconds
CheckSmatch                   PASS      99.92 seconds
BuildKernel32                 PASS      27.69 seconds
TestRunnerSetup               PASS      431.83 seconds
TestRunner_l2cap-tester       PASS      24.93 seconds
TestRunner_iso-tester         PASS      47.13 seconds
TestRunner_bnep-tester        PASS      6.90 seconds
TestRunner_mgmt-tester        PASS      159.28 seconds
TestRunner_rfcomm-tester      PASS      10.89 seconds
TestRunner_sco-tester         PASS      14.29 seconds
TestRunner_ioctl-tester       PASS      11.97 seconds
TestRunner_mesh-tester        PASS      8.68 seconds
TestRunner_smp-tester         PASS      9.60 seconds
TestRunner_userchan-tester    PASS      7.19 seconds
IncrementalBuild              PASS      25.94 seconds



---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz Dec. 26, 2023, 3:40 p.m. UTC | #2

Hi Clancy,

On Tue, Dec 26, 2023 at 1:09 AM <[e-mail]> wrote:
>
> From: Clancy Shang <[e-mail]>
>
> when Bluetooth enters suspend, and disconnects everything with the
> disconnect reason code of 0x15, the hogp device could not into sleep
> and continued advertising. when use the disconnect reason code of 0x13,
> the hogp device going into sleep succeeded.
>
> Signed-off-by: Clancy Shang <[e-mail]>
> ---
>  net/bluetooth/hci_sync.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index d85a7091a116..16b5420c32d0 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5927,7 +5927,7 @@ int hci_suspend_sync(struct hci_dev *hdev)
>
>         if (hci_conn_count(hdev)) {
>                 /* Soft disconnect everything (power off) */
> -               err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_POWER_OFF);
> +               err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_USER_TERM);

So the device interpretes that it can reconnect on power off but not
on user termination error? And you think changing the error would make
this interoperate any better? Afaik user termination can be generated
by various reasons, including the non-clean termination ones, while
power off error is quite clear on the reason, so instead of using a
more generic error you probably should contact the device manufacturer
and have it fix this problem and until then Id probably disable wakeup
support since it doesn't behave properly when receiving
HCI_ERROR_REMOTE_POWER_OFF.

>                 if (err) {
>                         /* Set state to BT_RUNNING so resume doesn't notify */
>                         hdev->suspend_state = BT_RUNNING;
> --
> 2.25.1
>
>

Luiz Augusto von Dentz Dec. 27, 2023, 4:47 p.m. UTC | #3

Hi Clancy,

On Tue, Dec 26, 2023 at 8:29 PM clancy_shang <[e-mail]> wrote:
>
> Hi Luiz Augusto von Dentz,
>
> Thank you for your reply.
>
> >>And you think changing the error would make
> >> this interoperate any better?
>
> > Yes, I think change this make the interoperate better, this is my test report.
> if i don't change disconnect reason,all hogp device can abnormal wakeup host device
> when it into suspend.


It seems you are targeting gaming controllers, none of these models
handle POWER_OFF reason? I bet that we can probably make a case for
both Microsoft and Valve to handle it properly.

The core spec actually describe a proper way to detect the so called Link Loss:

`3.1 LINK SUPERVISION FOR ACTIVE PHYSICAL LINKS

To be able to detect link loss, both the Central and the Peripheral shall use a
link supervision timer, T supervision . Upon reception of a valid
packet header with
one of the Peripheral's addresses (see Section 4.2) on the physical link, the
timer shall be reset. If at any time in Connection state, the timer reaches the
supervisionTO value, the connection shall be considered disconnected. The
same link supervision timer shall be used for SCO, eSCO, and ACL logical
transports.`

`CONNECTION TIMEOUT (0x08)
The Connection Timeout error code indicates that either the link supervision
timeout has expired for a given connection or that the synchronization timeout
has expired for a given broadcast`

So anything other than the error above shall _not_ be interpreted as Link Loss.

HOGP Reconnection Procedure is only meant to be used when a Link Loss
is detected:

`Link Loss Reconnection Procedure
When a connection is terminated due to link loss a HID Device should attempt to
reconnect to the HID Host by entering a GAP connectable mode using the
recommended advertising interval values shown in Table 5.2. The HID
Device may also
wait until it has data to transmit or until the next user activity is detected.`

This means that Reconnection Procedure shall _only_ be triggered when
Connection Timeout (0x08) is generated, otherwise it shall wait until
the user activity is detected.

>
>
> Kind regards,
>
>
> Clancy
>
>
>
>
>
> At 2023-12-26 23:40:07, "Luiz Augusto von Dentz" <[e-mail]> wrote:
> >Hi Clancy,
> >
> >On Tue, Dec 26, 2023 at 1:09 AM <[e-mail]> wrote:
> >>
> >> From: Clancy Shang <[e-mail]>
> >>
> >> when Bluetooth enters suspend, and disconnects everything with the
> >> disconnect reason code of 0x15, the hogp device could not into sleep
> >> and continued advertising. when use the disconnect reason code of 0x13,
> >> the hogp device going into sleep succeeded.
> >>
> >> Signed-off-by: Clancy Shang <[e-mail]>
> >> ---
> >>  net/bluetooth/hci_sync.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> >> index d85a7091a116..16b5420c32d0 100644
> >> --- a/net/bluetooth/hci_sync.c
> >> +++ b/net/bluetooth/hci_sync.c
> >> @@ -5927,7 +5927,7 @@ int hci_suspend_sync(struct hci_dev *hdev)
> >>
> >>         if (hci_conn_count(hdev)) {
> >>                 /* Soft disconnect everything (power off) */
> >> -               err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_POWER_OFF);
> >> +               err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_USER_TERM);
> >
> >So the device interpretes that it can reconnect on power off but not
> >on user termination error? And you think changing the error would make
> >this interoperate any better? Afaik user termination can be generated
> >by various reasons, including the non-clean termination ones, while
> >power off error is quite clear on the reason, so instead of using a
> >more generic error you probably should contact the device manufacturer
> >and have it fix this problem and until then Id probably disable wakeup
> >support since it doesn't behave properly when receiving
> >HCI_ERROR_REMOTE_POWER_OFF.
> >
> >>                 if (err) {
> >>                         /* Set state to BT_RUNNING so resume doesn't notify */
> >>                         hdev->suspend_state = BT_RUNNING;
> >> --
> >> 2.25.1
> >>
> >>
> >
> >
> >--
> >Luiz Augusto von Dentz

Patch

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index d85a7091a116..16b5420c32d0 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5927,7 +5927,7 @@  int hci_suspend_sync(struct hci_dev *hdev)
 
 	if (hci_conn_count(hdev)) {
 		/* Soft disconnect everything (power off) */
-		err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_POWER_OFF);
+		err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_USER_TERM);
 		if (err) {
 			/* Set state to BT_RUNNING so resume doesn't notify */
 			hdev->suspend_state = BT_RUNNING;