MIME-Version: 1.0
References: <20230214145609.kernel.v1.1.Ibe4d3a42683381c1e78b8c3aa67b53fc74437ae9@changeid>
 <CABBYNZKVVo4T_pbEdozhNvgiykC7NiLQKEnJi3q5gZpHunGrbA@mail.gmail.com>
 <CAB4PzUo+EuapOr+O7eWZH2xiVVAUd98m_DmEK-337=CvfUDeoA@mail.gmail.com> <CABBYNZJJhNTrH85VuqvAQbk6JyNhQ5atXzxb+rV7JcrhkgFWpQ@mail.gmail.com>
In-Reply-To: <CABBYNZJJhNTrH85VuqvAQbk6JyNhQ5atXzxb+rV7JcrhkgFWpQ@mail.gmail.com>
From:   Zhengping Jiang <jiangzp@google.com>
Date:   Tue, 14 Feb 2023 17:19:54 -0800
Message-ID: <CAB4PzUoj2=QNH0SqrSe8LbT74Z7DZr-K6Qw=b71k20a=1aLuSg@mail.gmail.com>
Subject: Re: [kernel PATCH v1] Bluetooth: hci_sync: Resume adv with no RPA
 when active scan
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.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 Luiz,

> Where is that mentioned?
It is just below the command on "7.8.44 LE Set Address Resolution
Enable command".

On "4.7 RESOLVING LIST", there is another note:
> Note: The Controller may generate Resolvable Private Addresses even when =
address resolution is disabled.

If this is the case, then the comment in the kernel
(hci_active_scan_sync) is not accurate.
> /* Pause advertising since active scanning disables address resolution
> * which advertising depend on in order to generate its RPAs.
> */

> I think it may be related to the fact that it only affects the addr
> resolution of remote devices, that said if you are active scanning
> that probably means the user wants to setup a new device thus why we
> don't enable any filtering like accept list, etc, so it is not really
> useful to keep address resolution active either way.
That makes sense. When the local privacy is enabled, I assume the host
RPA will change
when advertising. I haven't tested that scenario, but if RPA
generation is not related to disable/enable
address resolution, why should the advertising be paused when active scan?

Thanks,
Zhengping

>
> > Thanks,
> > Zhengping
> >
> > On Tue, Feb 14, 2023 at 4:09 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > 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 sca=
n without RPA")
> > > > Signed-off-by: Zhengping Jiang <jiangzp@google.com>
> > > > ---
> > > >
> > > > Changes in v1:
> > > > - Always pause advertising when active scan, but resume the adverti=
sing if 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_dev *hdev)
> > > >         u8 filter_policy;
> > > >         int err;
> > > >
> > > > -       /* Pause advertising if resolving list can be used as contr=
ollers are
> > > > +       /* Pause advertising if resolving list can be used as contr=
ollers
> > > >          * cannot accept resolving list modifications while adverti=
sing.
> > > >          */
> > > >         if (use_ll_privacy(hdev)) {
> > > > @@ -5397,7 +5397,7 @@ static int hci_active_scan_sync(struct hci_de=
v *hdev, uint16_t interval)
> > > >         /* Pause advertising since active scanning disables address=
 resolution
> > > >          * which advertising depend on in order to generate its RPA=
s.
> > > >          */
> > > > -       if (use_ll_privacy(hdev) && hci_dev_test_flag(hdev, HCI_PRI=
VACY)) {
> > > > +       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_d=
ev *hdev, 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_PR=
IVACY))
> > > > +               hci_resume_advertising_sync(hdev);
> > > > +
> > > >         /* All active scans will be done with either a resolvable p=
rivate
> > > >          * address (when privacy feature has been enabled) or non-r=
esolvable
> > > >          * 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 t=
o
> > > have proper checks for these conditions on the emulator:
> > >
> > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btde=
v.c#n4090
> > >
> > > 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.
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz