Hi Marcel,
On Fri, Jun 19, 2020 at 3:46 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Alain,
>
> please use “Bluetooth: “ 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) plen
> > 25 #93 [hci0] 10.953011
> > …
> > Min advertising interval: 181.250 msec (0x0122)
> > Max advertising interval: 181.250 msec (0x0122)
> > …
> >
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > Reviewed-by: Daniel Winkler <danielwinkler@xxxxxxxxxx>
> >
> > Signed-off-by: Alain Michaud <alainm@xxxxxxxxxxxx>
>
> 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] = { 0x00, 0x08, 0x00 };
> > + /* In ext adv set param interval is 3 octets in le format */
> > + const __le32 min_adv_interval = cpu_to_le32(hdev->le_adv_min_interval);
> > + const __le32 max_adv_interval = 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 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 = 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_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 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 this case I prefer to actually do this manually.
>
> /* In ext adv min interval is 3 octets */
> cp.min_interval[0] = cp.min_interval & 0xff;
> cp.min_interval[1] = (cp.min_interval & 0xff00) >> 8;
> cp.min_interval[2] = (cp.min_interval & 0xff0000) >> 12;
>
> Regards
>
> Marcel
>
