[v2,1/3] Bluetooth: hci_ll: add support for setting public address
diff mbox series

Message ID 1512701860-8321-2-git-send-email-david@lechnology.com
State New, archived
Headers show
Series
  • Bluetooth: hci_ll: Get BD address from NVMEM (was "bluetooth: hci_ll: Get MAC address from NVMEM")
Related show

Commit Message

David Lechner Dec. 8, 2017, 2:57 a.m. UTC
This adds support for setting the public address on Texas Instruments
Bluetooth chips using a vendor-specific command.

This has been tested on a CC2560A. The TI wiki also indicates that this
command should work on TI WL17xx/WL18xx Bluetooth chips.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:
* This is a new patch in v2

 drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Marcel Holtmann Dec. 8, 2017, 8:07 a.m. UTC | #1
Hi David,

> This adds support for setting the public address on Texas Instruments
> Bluetooth chips using a vendor-specific command.
> 
> This has been tested on a CC2560A. The TI wiki also indicates that this
> command should work on TI WL17xx/WL18xx Bluetooth chips.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> 
> v2 changes:
> * This is a new patch in v2
> 
> drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
> index 974a788..b732004 100644
> --- a/drivers/bluetooth/hci_ll.c
> +++ b/drivers/bluetooth/hci_ll.c
> @@ -57,6 +57,7 @@
> #include "hci_uart.h"
> 
> /* Vendor-specific HCI commands */
> +#define HCI_VS_WRITE_BD_ADDR			0xfc06
> #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
> 
> /* HCILL commands */
> @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)
> 	return err;
> }
> 
> +static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> +{
> +	bdaddr_t bdaddr_swapped;
> +	struct sk_buff *skb;
> +
> +	baswap(&bdaddr_swapped, bdaddr);
> +	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
> +			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
> +	if (!IS_ERR(skb))
> +		kfree_skb(skb);
> +	

You have a trailing whitespace here.

Does the HCI command really expect the BD_ADDR in the swapped order. The caller of hdev->set_bdaddr while provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.

So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think ll_set_bdaddr should function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.

Regards

Marcel
David Lechner Dec. 8, 2017, 7:21 p.m. UTC | #2
On 12/08/2017 02:07 AM, Marcel Holtmann wrote:
> Hi David,
> 
>> This adds support for setting the public address on Texas Instruments
>> Bluetooth chips using a vendor-specific command.
>>
>> This has been tested on a CC2560A. The TI wiki also indicates that this
>> command should work on TI WL17xx/WL18xx Bluetooth chips.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>
>> v2 changes:
>> * This is a new patch in v2
>>
>> drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
>> index 974a788..b732004 100644
>> --- a/drivers/bluetooth/hci_ll.c
>> +++ b/drivers/bluetooth/hci_ll.c
>> @@ -57,6 +57,7 @@
>> #include "hci_uart.h"
>>
>> /* Vendor-specific HCI commands */
>> +#define HCI_VS_WRITE_BD_ADDR			0xfc06
>> #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
>>
>> /* HCILL commands */
>> @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)
>> 	return err;
>> }
>>
>> +static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>> +{
>> +	bdaddr_t bdaddr_swapped;
>> +	struct sk_buff *skb;
>> +
>> +	baswap(&bdaddr_swapped, bdaddr);
>> +	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
>> +			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
>> +	if (!IS_ERR(skb))
>> +		kfree_skb(skb);
>> +	
> 
> You have a trailing whitespace here.
> 
> Does the HCI command really expect the BD_ADDR in the swapped order. The caller of hdev->set_bdaddr while provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.
> 
> So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think ll_set_bdaddr should function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.
> 

I did test using btmgmt public-address 00:11:22:33:44:55, which is how I 
found out that the order needed to be swapped. Like you, I was 
surprised. I couldn't find any documentation from TI saying what the 
order is supposed to be, so I can only assume that because this works, 
it is indeed correct as-is.
Marcel Holtmann Dec. 8, 2017, 7:24 p.m. UTC | #3
Hi David,

>>> This adds support for setting the public address on Texas Instruments
>>> Bluetooth chips using a vendor-specific command.
>>> 
>>> This has been tested on a CC2560A. The TI wiki also indicates that this
>>> command should work on TI WL17xx/WL18xx Bluetooth chips.
>>> 
>>> Signed-off-by: David Lechner <david@lechnology.com>
>>> ---
>>> 
>>> v2 changes:
>>> * This is a new patch in v2
>>> 
>>> drivers/bluetooth/hci_ll.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
>>> index 974a788..b732004 100644
>>> --- a/drivers/bluetooth/hci_ll.c
>>> +++ b/drivers/bluetooth/hci_ll.c
>>> @@ -57,6 +57,7 @@
>>> #include "hci_uart.h"
>>> 
>>> /* Vendor-specific HCI commands */
>>> +#define HCI_VS_WRITE_BD_ADDR			0xfc06
>>> #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
>>> 
>>> /* HCILL commands */
>>> @@ -662,6 +663,20 @@ static int download_firmware(struct ll_device *lldev)
>>> 	return err;
>>> }
>>> 
>>> +static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>> +{
>>> +	bdaddr_t bdaddr_swapped;
>>> +	struct sk_buff *skb;
>>> +
>>> +	baswap(&bdaddr_swapped, bdaddr);
>>> +	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
>>> +			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
>>> +	if (!IS_ERR(skb))
>>> +		kfree_skb(skb);
>>> +	
>> You have a trailing whitespace here.
>> Does the HCI command really expect the BD_ADDR in the swapped order. The caller of hdev->set_bdaddr while provide it in the same order as the HCI Read BD Address command and everything in HCI. So it seems odd that you have to swap it for the vendor command.
>> So have you actually tested this with btmgmt public-add <xx:xx..> and checked that the address comes out correctly. I think ll_set_bdaddr should function correctly for the mgmt interface. And if needed any other caller outside of mgmt has to do the swapping.
> 
> I did test using btmgmt public-address 00:11:22:33:44:55, which is how I found out that the order needed to be swapped. Like you, I was surprised. I couldn't find any documentation from TI saying what the order is supposed to be, so I can only assume that because this works, it is indeed correct as-is.

then please add a comment for that and I would appreciate to have the parts from btmon showing the public-addr command and the following HCI Read BD Address command as part of the commit message. Just for being able to dig this out at some later point if needed.

Regards

Marcel

Patch
diff mbox series

diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 974a788..b732004 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -57,6 +57,7 @@ 
 #include "hci_uart.h"
 
 /* Vendor-specific HCI commands */
+#define HCI_VS_WRITE_BD_ADDR			0xfc06
 #define HCI_VS_UPDATE_UART_HCI_BAUDRATE		0xff36
 
 /* HCILL commands */
@@ -662,6 +663,20 @@  static int download_firmware(struct ll_device *lldev)
 	return err;
 }
 
+static int ll_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
+{
+	bdaddr_t bdaddr_swapped;
+	struct sk_buff *skb;
+
+	baswap(&bdaddr_swapped, bdaddr);
+	skb = __hci_cmd_sync(hdev, HCI_VS_WRITE_BD_ADDR, sizeof(bdaddr_t),
+			     &bdaddr_swapped, HCI_INIT_TIMEOUT);
+	if (!IS_ERR(skb))
+		kfree_skb(skb);
+	
+	return PTR_ERR_OR_ZERO(skb);
+}
+
 static int ll_setup(struct hci_uart *hu)
 {
 	int err, retry = 3;
@@ -674,6 +689,8 @@  static int ll_setup(struct hci_uart *hu)
 
 	lldev = serdev_device_get_drvdata(serdev);
 
+	hu->hdev->set_bdaddr = ll_set_bdaddr;
+
 	serdev_device_set_flow_control(serdev, true);
 
 	do {