Re: Re: [PATCH] Bluetooth: hci_sync: fix hogp device suspend bug

[Luiz Augusto von Dentz]

Hi Clancy,

On Tue, Dec 26, 2023 at 8:29 PM clancy_shang <clancy_shang@xxxxxxx> 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" <luiz.dentz@xxxxxxxxx> wrote:
> >Hi Clancy,
> >
> >On Tue, Dec 26, 2023 at 1:09 AM <clancy_shang@xxxxxxx> wrote:
> >>
> >> From: Clancy Shang <clancy.shang@xxxxxxxxxxx>
> >>
> >> 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 <clancy.shang@xxxxxxxxxxx>
> >> ---
> >>  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



-- 
Luiz Augusto von Dentz