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> <CABBYNZLEsUXFW_hrmcvSALRpeZwkmvk=c9SYPt+owO7tn+hMBw@mail.gmail.com>
In-Reply-To: <CABBYNZLEsUXFW_hrmcvSALRpeZwkmvk=c9SYPt+owO7tn+hMBw@mail.gmail.com>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Fri, 19 Jun 2020 16:57:47 -0400
Message-ID: <CALWDO_V2Hnc-HHYQQ5LfnW8a2FWzkTBOE1Cti0ZQzyw4CouAgw@mail.gmail.com>
Subject: Re: [PATCH v1] bluetooth: use configured params for ext adv
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.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 Luiz,

On Fri, Jun 19, 2020 at 2:42 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain, Marcel,
>
> On Fri, Jun 19, 2020 at 6:54 AM Alain Michaud <alainmichaud@google.com> w=
rote:
> >
> > Hi Marcel,
> >
> > On Fri, Jun 19, 2020 at 3:46 AM Marcel Holtmann <marcel@holtmann.org> w=
rote:
> > >
> > > 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=
 and
> > > > max interval of 0x8000 is used.  This patches fixes this issue by u=
sing
> > > > the configured min/max value.
> > > >
> > > > This was validated by setting min/max in main.conf and making sure =
the
> > > > right setting is applied:
> > > >
> > > > < HCI Command: LE Set Extended Advertising Parameters (0x08|0x0036)=
 plen
> > > > 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_reques=
t.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 h=
ci_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_mi=
n_interval);
> > > > +     const __le32 max_adv_interval =3D cpu_to_le32(hdev->le_adv_ma=
x_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 actuall=
y __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 l=
ist, 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 h=
ci_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_inte=
rval));
> > > > +     memcpy(cp.max_interval, &max_adv_interval, sizeof(cp.max_inte=
rval));
> > >
> > > This is dangerous and I think it actually break in case of unaligned =
access 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/byte=
order.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.
I like this. Would you put this in hci.h or keep to a lower scope?

static inline void hci_cpu_to_le24(__u32 val, __u8 dst[3])
{
dst[0] =3D val & 0xff;
dst[1] =3D (val & 0xff00) >> 8;
dst[2] =3D (val & 0xff0000) >> 16;
}

> > >
> > >
> > > 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
> > >
>
>
>
> --
> Luiz Augusto von Dentz