Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210429111523.Bluez.v1.1.Ic00ed950add081b346d6c8ced590bb7b2eb6e9f7@changeid>
 <44A0B714-12C5-4A43-9829-CAE3D8BF374B@holtmann.org>
In-Reply-To: <44A0B714-12C5-4A43-9829-CAE3D8BF374B@holtmann.org>
From:   Yu Liu <yudiliu@google.com>
Date:   Fri, 7 May 2021 09:43:59 -0700
Message-ID: <CAHC-ybycimdB4q13Dd4omTDu83NhZQJ0jR7_bXi0fKDJpu3jFg@mail.gmail.com>
Subject: Re: [Bluez PATCH v1] doc/mgmt-api - Add a new error code for HCI
 status 0x3e
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marcel,

We have a couple of bugs where the controller failed on the Read
Remote Feature command after initial connection with the 0x3E error,
guy.damary@intel made some suggestions after root caused the issues,
you can see his detailed suggestions here:
https://buganizer.corp.google.com/issues/174806913#comment98. To
summarize: he suggests us to retry up to 3 times if 0x3E is
encountered during LE pairing.

Regarding this change: I see MGMT_STATUS_CONNECT_FAILED could be
returned under 4 different scenarios, 3 in the mgmt_status_table table
and 1 in pair_device as a fallback error to cover all the other cases,
so I decided to introduce a new error code to make sure we don't retry
in the cases where we shouldn't. If you think this cae be avoided by
checking other flags at the same time, I can then drop the change from
the kernel and only change the user space.

Thanks


On Fri, May 7, 2021 at 1:19 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Yu,
>
> > We want to retry the pairing when HCI status 0x3e (Connection failed to
> > established/Synchronization timeout) is returned from the controller.
> > This is to add a new MGMT error code so that we can catch this 0x3e
> > failure and issue a retry in the user space.
> >
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v1:
> > - Initial change
> >
> > doc/mgmt-api.txt | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> > index 5355fedb0..f7cbf7ab2 100644
> > --- a/doc/mgmt-api.txt
> > +++ b/doc/mgmt-api.txt
> > @@ -200,6 +200,7 @@ and Command Complete events:
> > 0x12  RFKilled
> > 0x13  Already Paired
> > 0x14  Permission Denied
> > +0x15 Connection Not Established
> >
> > As a general rule all commands generate the events as specified below,
> > however invalid lengths or unknown commands will always generate a
> > @@ -1112,6 +1113,7 @@ Pair Device Command
> >                               Not Powered
> >                               Invalid Index
> >                               Already Paired
> > +                             Connection Not Established
>
> I really dislike the naming. And even more so, I request the motive here.
>
> So looking at our code, we have 3 cases where we use the previous status:
>
>         MGMT_STATUS_CONNECT_FAILED, /* Page Timeout */
>         MGMT_STATUS_CONNECT_FAILED, /* Connection Establishment Failed */
>         MGMT_STATUS_CONNECT_FAILED, /* MAC Connection Failed */
>
> And they do map to the 3 available transports, either via BR/EDR or LE or AMP. That means if you call Pair Device you already know well today when it fails to establish the link and can retry it.
>
> My question, what are you trying to fix here.
>
> Regards
>
> Marcel
>