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>
In-Reply-To: <C5D30F5E-B50F-4DE7-A909-F48F6C555661@holtmann.org>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Fri, 19 Jun 2020 09:50:30 -0400
Message-ID: <CALWDO_XLRELJhe1n9AVq2w9fQYkgGLC+bdXQ-1-M4f-cUfEwGQ@mail.gmail.com>
Subject: Re: [PATCH v1] bluetooth: use configured params for ext adv
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     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 Marcel,

On Fri, Jun 19, 2020 at 3:46 AM Marcel Holtmann <marcel@holtmann.org> wrote=
:
>
> 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 using
> > 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) ple=
n
> > 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_r=
equest *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_in=
terval);
> > +     const __le32 max_adv_interval =3D cpu_to_le32(hdev->le_adv_max_in=
terval);
>
> 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 list,=
 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_r=
equest *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_interval=
));
> > +     memcpy(cp.max_interval, &max_adv_interval, sizeof(cp.max_interval=
));
>
> This is dangerous and I think it actually break in case of unaligned acce=
ss 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 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
>