Re: [PATCH v1] bluetooth: use configured params for ext adv

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 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




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux