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.

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

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




[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