Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-774-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20231226060818.2446327-1-clancy_shang@163.com>
 <CABBYNZJQFD_=VcFZxK=AugixVMgc=Zw4WPgQn+ax28cNGWBGUQ@mail.gmail.com> <6358e9a9.6c6.18ca8e3e1cf.Coremail.clancy_shang@163.com>
In-Reply-To: <6358e9a9.6c6.18ca8e3e1cf.Coremail.clancy_shang@163.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Wed, 27 Dec 2023 11:47:46 -0500
Message-ID: <CABBYNZK782JSRsxmNGFFSiEevrdzAomWUYnUYX0tFirNFBCqHg@mail.gmail.com>
Subject: Re: Re: [PATCH] Bluetooth: hci_sync: fix hogp device suspend bug
To: clancy_shang <clancy_shang@163.com>
Cc: marcel@holtmann.org, johan.hedberg@gmail.com, 
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org, 
	zhongjun.yu@quectel.com, Clancy Shang <clancy.shang@quectel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Clancy,

On Tue, Dec 26, 2023 at 8:29=E2=80=AFPM clancy_shang <clancy_shang@163.com>=
 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 h=
ost 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 L=
oss:

`3.1 LINK SUPERVISION FOR ACTIVE PHYSICAL LINKS

To be able to detect link loss, both the Central and the Peripheral shall u=
se 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, t=
he
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 supervisio=
n
timeout has expired for a given connection or that the synchronization time=
out
has expired for a given broadcast`

So anything other than the error above shall _not_ be interpreted as Link L=
oss.

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 attemp=
t 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 detec=
ted.`

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@gmail.com> w=
rote:
> >Hi Clancy,
> >
> >On Tue, Dec 26, 2023 at 1:09=E2=80=AFAM <clancy_shang@163.com> wrote:
> >>
> >> From: Clancy Shang <clancy.shang@quectel.com>
> >>
> >> 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@quectel.com>
> >> ---
> >>  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 =3D hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE=
_POWER_OFF);
> >> +               err =3D 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 n=
otify */
> >>                         hdev->suspend_state =3D BT_RUNNING;
> >> --
> >> 2.25.1
> >>
> >>
> >
> >
> >--
> >Luiz Augusto von Dentz


--=20
Luiz Augusto von Dentz