| Message ID | 20200618210659.142284-1-alainm@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] bluetooth: use configured params for ext adv | expand |
Hi Alain, please use “Bluetooth: “ prefix for the subject. > 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@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. > --- > > 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. 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. That said, if we have this in the Load Default System Configuration list, we should extended it to __le32 there as well. > 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. 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
Hi Marcel, On Fri, Jun 19, 2020 at 3:46 AM Marcel Holtmann <marcel@holtmann.org> 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@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] = { 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 >
Hi Alain, Marcel, On Fri, Jun 19, 2020 at 6:54 AM Alain Michaud <alainmichaud@google.com> wrote: > > Hi Marcel, > > On Fri, Jun 19, 2020 at 3:46 AM Marcel Holtmann <marcel@holtmann.org> 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@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] = { 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 zephyr we end up doing helper functions for 24 bits: https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/byteorder.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] = cp.min_interval & 0xff; > > cp.min_interval[1] = (cp.min_interval & 0xff00) >> 8; > > cp.min_interval[2] = (cp.min_interval & 0xff0000) >> 12; > > > > Regards > > > > Marcel > >
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> wrote: > > > > Hi Marcel, > > > > On Fri, Jun 19, 2020 at 3:46 AM Marcel Holtmann <marcel@holtmann.org> 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@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] = { 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 zephyr we end up doing helper functions for 24 bits: > > https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/byteorder.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] = val & 0xff; dst[1] = (val & 0xff00) >> 8; dst[2] = (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] = cp.min_interval & 0xff; > > > cp.min_interval[1] = (cp.min_interval & 0xff00) >> 8; > > > cp.min_interval[2] = (cp.min_interval & 0xff0000) >> 12; > > > > > > Regards > > > > > > Marcel > > > > > > > -- > Luiz Augusto von Dentz
Hi Alain, >>> 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. does the const make a difference for integer values? For me this the difference between adding value and extra noise. >> >> >> 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 is true, but lets be clean and use a proper storage size in the first place. Eventually we want to use the larger intervals at some point. And then you end up hunting that root cause for a complete day ;) >> >> >> 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. Separate issue and separate patch, but needs to be addressed. >> >> >>> 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. It is only fine on architectures that do the unaligned access correctly. In case they don’t this will result in a funny value. Regards Marcel
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@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] = { 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 zephyr we end up doing helper functions for 24 bits: >> >> https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/byteorder.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] = val & 0xff; > dst[1] = (val & 0xff00) >> 8; > dst[2] = (val & 0xff0000) >> 16; > } hmmm, how many 24-bit fields do we have in Bluetooth HCI spec? If it is just one, then lets keep it close to the usage, if not, I have also no object to put it in a higher level. Regards Marcel
Hi Marcel, On Mon, Jun 22, 2020 at 12:14 AM Marcel Holtmann <marcel@holtmann.org> 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@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] = { 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 zephyr we end up doing helper functions for 24 bits: > >> > >> https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/byteorder.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] = val & 0xff; > > dst[1] = (val & 0xff00) >> 8; > > dst[2] = (val & 0xff0000) >> 16; > > } > > hmmm, how many 24-bit fields do we have in Bluetooth HCI spec? If it is just one, then lets keep it close to the usage, if not, I have also no object to put it in a higher level. These are the instances of 24-bit fields: include/net/bluetooth/hci.h: __u8 min_interval[3]; include/net/bluetooth/hci.h: __u8 max_interval[3]; include/net/bluetooth/hci.h: __u8 m_interval[3]; include/net/bluetooth/hci.h: __u8 s_interval[3]; include/net/bluetooth/hci.h: __u8 cig_sync_delay[3]; include/net/bluetooth/hci.h: __u8 cis_sync_delay[3]; include/net/bluetooth/hci.h: __u8 m_latency[3]; include/net/bluetooth/hci.h: __u8 s_latency[3]; I guess they all could benefit from having such a helper so we don't have to resort in cpu_to_32 + memcpy. > Regards > > Marcel >
Hi Luiz, >>>>>> 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@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] = { 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 zephyr we end up doing helper functions for 24 bits: >>>> >>>> https://github.com/zephyrproject-rtos/zephyr/blob/master/include/sys/byteorder.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] = val & 0xff; >>> dst[1] = (val & 0xff00) >> 8; >>> dst[2] = (val & 0xff0000) >> 16; >>> } >> >> hmmm, how many 24-bit fields do we have in Bluetooth HCI spec? If it is just one, then lets keep it close to the usage, if not, I have also no object to put it in a higher level. > > These are the instances of 24-bit fields: > > include/net/bluetooth/hci.h: __u8 min_interval[3]; > include/net/bluetooth/hci.h: __u8 max_interval[3]; > include/net/bluetooth/hci.h: __u8 m_interval[3]; > include/net/bluetooth/hci.h: __u8 s_interval[3]; > include/net/bluetooth/hci.h: __u8 cig_sync_delay[3]; > include/net/bluetooth/hci.h: __u8 cis_sync_delay[3]; > include/net/bluetooth/hci.h: __u8 m_latency[3]; > include/net/bluetooth/hci.h: __u8 s_latency[3]; > > I guess they all could benefit from having such a helper so we don't > have to resort in cpu_to_32 + memcpy. I see, the new ISO channel support also used 24-bit variables heavily. Regards Marcel
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); 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)); secondary_adv = (flags & MGMT_ADV_FLAG_SEC_MASK);