MIME-Version: 1.0
References: <20230214145609.kernel.v1.1.Ibe4d3a42683381c1e78b8c3aa67b53fc74437ae9@changeid>
In-Reply-To: <20230214145609.kernel.v1.1.Ibe4d3a42683381c1e78b8c3aa67b53fc74437ae9@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 14 Feb 2023 16:09:45 -0800
Message-ID: <CABBYNZKVVo4T_pbEdozhNvgiykC7NiLQKEnJi3q5gZpHunGrbA@mail.gmail.com>
Subject: Re: [kernel PATCH v1] Bluetooth: hci_sync: Resume adv with no RPA
 when active scan
To:     Zhengping Jiang <jiangzp@google.com>
Cc:     linux-bluetooth@vger.kernel.org, marcel@holtmann.org,
        chromeos-bluetooth-upstreaming@chromium.org,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Zhengping,

On Tue, Feb 14, 2023 at 2:56 PM Zhengping Jiang <jiangzp@google.com> wrote:
>
> The address resolution should be disabled during the active scan,
> so all the advertisements can reach the host. The advertising
> has to be paused before disabling the address resolution,
> because the advertising will prevent any changes to the resolving
> list and the address resolution status. Skipping this will cause
> the hci error and the discovery failure.

It is probably a good idea to quote the spec saying:

7.8.44 LE Set Address Resolution Enable command

This command shall not be used when:
=E2=80=A2 Advertising (other than periodic advertising) is enabled,

> If the host is using RPA, the controller needs to generate RPA for
> the advertising, so the advertising must remain paused during the
> active scan.
>
> If the host is not using RPA, the advertising can be resumed after
> disabling the address resolution.
>
> Fixes: 9afc675edeeb ("Bluetooth: hci_sync: allow advertise when scan with=
out RPA")
> Signed-off-by: Zhengping Jiang <jiangzp@google.com>
> ---
>
> Changes in v1:
> - Always pause advertising when active scan, but resume the advertising i=
f the host is not using RPA
>
>  net/bluetooth/hci_sync.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 117eedb6f709..edbf9faf7fa1 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -2402,7 +2402,7 @@ static u8 hci_update_accept_list_sync(struct hci_de=
v *hdev)
>         u8 filter_policy;
>         int err;
>
> -       /* Pause advertising if resolving list can be used as controllers=
 are
> +       /* Pause advertising if resolving list can be used as controllers
>          * cannot accept resolving list modifications while advertising.
>          */
>         if (use_ll_privacy(hdev)) {
> @@ -5397,7 +5397,7 @@ static int hci_active_scan_sync(struct hci_dev *hde=
v, uint16_t interval)
>         /* Pause advertising since active scanning disables address resol=
ution
>          * which advertising depend on in order to generate its RPAs.
>          */
> -       if (use_ll_privacy(hdev) && hci_dev_test_flag(hdev, HCI_PRIVACY))=
 {
> +       if (use_ll_privacy(hdev)) {
>                 err =3D hci_pause_advertising_sync(hdev);
>                 if (err) {
>                         bt_dev_err(hdev, "pause advertising failed: %d", =
err);
> @@ -5416,6 +5416,10 @@ static int hci_active_scan_sync(struct hci_dev *hd=
ev, uint16_t interval)
>                 goto failed;
>         }
>
> +       // Resume paused advertising if the host is not using RPA
> +       if (use_ll_privacy(hdev) && !hci_dev_test_flag(hdev, HCI_PRIVACY)=
)
> +               hci_resume_advertising_sync(hdev);
> +
>         /* All active scans will be done with either a resolvable private
>          * address (when privacy feature has been enabled) or non-resolva=
ble
>          * private address.
> --
> 2.39.1.581.gbfd45094c4-goog

I think it is better that we add something like
hci_pause_addr_resolution so we can make it check all the conditions,
such as pausing advertising and resuming if needed. Btw, we do seem to
have proper checks for these conditions on the emulator:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n4=
090

But perhaps there is no test which attempts to enable LL Privacy
without enabling Local Privacy, so it would be great if you could
update mgmt-tester adding a test that emulates such behavior.

--=20
Luiz Augusto von Dentz