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: <20200618210659.142284-1-alainm@chromium.org> <C5D30F5E-B50F-4DE7-A909-F48F6C555661@holtmann.org>
 <CALWDO_XLRELJhe1n9AVq2w9fQYkgGLC+bdXQ-1-M4f-cUfEwGQ@mail.gmail.com>
In-Reply-To: <CALWDO_XLRELJhe1n9AVq2w9fQYkgGLC+bdXQ-1-M4f-cUfEwGQ@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 19 Jun 2020 11:41:53 -0700
Message-ID: <CABBYNZLEsUXFW_hrmcvSALRpeZwkmvk=c9SYPt+owO7tn+hMBw@mail.gmail.com>
Subject: Re: [PATCH v1] bluetooth: use configured params for ext adv
To:     Alain Michaud <alainmichaud@google.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Alain Michaud <alainm@chromium.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        Daniel Winkler <danielwinkler@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Alain, Marcel,

On Fri, Jun 19, 2020 at 6:54 AM Alain Michaud <alainmichaud@google.com> wro=
te:
>
> Hi Marcel,
>
> On Fri, Jun 19, 2020 at 3:46 AM Marcel Holtmann <marcel@holtmann.org> wro=
te:
> >
> > Hi Alain,
> >
> > please use =E2=80=9CBluetooth: =E2=80=9C prefix for the subject.
>
> ack.
> >
> >
> > > When the extended advertisement feature is enabled, a hardcoded min a=
nd
> > > max interval of 0x8000 is used.  This patches fixes this issue by usi=
ng
> > > the configured min/max value.
> > >
> > > This was validated by setting min/max in main.conf and making sure th=
e
> > > right setting is applied:
> > >
> > > < HCI Command: LE Set Extended Advertising Parameters (0x08|0x0036) p=
len
> > > 25                                          #93 [hci0] 10.953011
> > > =E2=80=A6
> > > Min advertising interval: 181.250 msec (0x0122)
> > > Max advertising interval: 181.250 msec (0x0122)
> > > =E2=80=A6
> > >
> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > Reviewed-by: Daniel Winkler <danielwinkler@google.com>
> > >
> > > Signed-off-by: Alain Michaud <alainm@chromium.org>
> >
> > The Reviewed-by lines go after your Signed-off-by.
>
> ack.
> >
> >
> > > ---
> > >
> > > net/bluetooth/hci_request.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.=
c
> > > index 29decd7e8051..08818b9bf89f 100644
> > > --- a/net/bluetooth/hci_request.c
> > > +++ b/net/bluetooth/hci_request.c
> > > @@ -1799,8 +1799,9 @@ int __hci_req_setup_ext_adv_instance(struct hci=
_request *req, u8 instance)
> > >       int err;
> > >       struct adv_info *adv_instance;
> > >       bool secondary_adv;
> > > -     /* In ext adv set param interval is 3 octets */
> > > -     const u8 adv_interval[3] =3D { 0x00, 0x08, 0x00 };
> > > +     /* In ext adv set param interval is 3 octets in le format */
> > > +     const __le32 min_adv_interval =3D cpu_to_le32(hdev->le_adv_min_=
interval);
> > > +     const __le32 max_adv_interval =3D cpu_to_le32(hdev->le_adv_max_=
interval);
> >
> > Scrap the const here.
>
> I'd like to understand why it isn't prefered to use const when you
> don't intend to modify it in the code.
> >
> >
> > And it is wrong since your hdev->le_adv_{min,max}_interval is actually =
__u16. So that first needs to be extended to a __u16 value.
>
> The macro actually leads to a function call that has a __u32 as a
> parameter so the __u16 gets upcasted to a __u32 already.
> >
> >
> > That said, if we have this in the Load Default System Configuration lis=
t, we should extended it to __le32 there as well.
>
> I agree, this means the range of default system configuration may not
> be sufficient to accept all possible values that the newer command
> supports, although I think this is a separate issue from what this
> patch is trying to solve.
> >
> >
> > >       if (instance > 0) {
> > >               adv_instance =3D hci_find_adv_instance(hdev, instance);
> > > @@ -1833,8 +1834,9 @@ int __hci_req_setup_ext_adv_instance(struct hci=
_request *req, u8 instance)
> > >
> > >       memset(&cp, 0, sizeof(cp));
> > >
> > > -     memcpy(cp.min_interval, adv_interval, sizeof(cp.min_interval));
> > > -     memcpy(cp.max_interval, adv_interval, sizeof(cp.max_interval));
> > > +     /* take least significant 3 bytes */
> > > +     memcpy(cp.min_interval, &min_adv_interval, sizeof(cp.min_interv=
al));
> > > +     memcpy(cp.max_interval, &max_adv_interval, sizeof(cp.max_interv=
al));
> >
> > This is dangerous and I think it actually break in case of unaligned ac=
cess platforms.
>
> Since it is in le format already and the 3 bytes from the cmd struct
> are raw, I'm not sure how this can be dangerous.  It effectively
> yields the exact same results as your suggestions below.

In zephyr we end up doing helper functions for 24 bits:

https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/byteor=
der.h#L316

I guess that is safer in terms of alignment access and it would work
independent of the host order which apparently was not the case in the
code above since it doesn't do the conversion to le32 (or perhaps the
intervals are already in le32), anyway having something like that is
probably much simpler to maintain given that most intervals use for
things like ISO are also 24 bits long.

> >
> >
> > In this case I prefer to actually do this manually.
> >
> >                 /* In ext adv min interval is 3 octets */
> >                 cp.min_interval[0] =3D cp.min_interval & 0xff;
> >                 cp.min_interval[1] =3D (cp.min_interval & 0xff00) >> 8;
> >                 cp.min_interval[2] =3D (cp.min_interval & 0xff0000) >> =
12;
> >
> > Regards
> >
> > Marcel
> >


--=20
Luiz Augusto von Dentz