[PATCH 1/2] Bluetooth: add public address configuration for Marvell USB devices

[PATCH 1/2] Bluetooth: add public address configuration for Marvell USB devices

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

Implemented .set_bdaddr handler provided by bluetooth stack for
Marvell devices for public address configuration.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
 drivers/bluetooth/btusb.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index ed7b33b..9846fc4 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -48,6 +48,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_INTEL		0x100
 #define BTUSB_INTEL_BOOT	0x200
 #define BTUSB_BCM_PATCHRAM	0x400
+#define BTUSB_MARVELL		0x800
 
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -242,6 +243,10 @@ static const struct usb_device_id blacklist_table[] = {
 	{ USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL },
 	{ USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL },
 
+	/* Marvell device */
+	{ USB_DEVICE(0x1286, 0x2044), .driver_info = BTUSB_MARVELL },
+	{ USB_DEVICE(0x1286, 0x2046), .driver_info = BTUSB_MARVELL },
+
 	{ }	/* Terminating entry */
 };
 
@@ -1455,6 +1460,29 @@ static int btusb_set_bdaddr_intel(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 	return 0;
 }
 
+static int btusb_set_bdaddr_marvell(struct hci_dev *hdev,
+				    const bdaddr_t *bdaddr)
+{
+	struct sk_buff *skb;
+	u8 buf[8];
+	long ret;
+
+	buf[0] = 0xfe;
+	buf[1] = sizeof(bdaddr_t);
+	bacpy((bdaddr_t *)&buf[2], bdaddr);
+
+	skb = __hci_cmd_sync(hdev, 0xfc22, sizeof(buf), buf, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		BT_ERR("%s: changing Marvell device address failed (%ld)",
+		       hdev->name, ret);
+		return ret;
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+
 #define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}})
 
 static int btusb_setup_bcm_patchram(struct hci_dev *hdev)
@@ -1766,6 +1794,9 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->set_bdaddr = btusb_set_bdaddr_intel;
 	}
 
+	if (id->driver_info & BTUSB_MARVELL)
+		hdev->set_bdaddr = btusb_set_bdaddr_marvell;
+
 	if (id->driver_info & BTUSB_INTEL_BOOT)
 		set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks);
 
-- 
1.8.2.3

[PATCH 2/2] Bluetooth: btmrvl: add public address configuration support

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

.set_bdaddr handler is implemented for public address configuration.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
 drivers/bluetooth/btmrvl_drv.h  |  1 +
 drivers/bluetooth/btmrvl_main.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index caf6841..38ad662 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -91,6 +91,7 @@ struct btmrvl_private {
 
 /* Vendor specific Bluetooth commands */
 #define BT_CMD_PSCAN_WIN_REPORT_ENABLE	0xFC03
+#define BT_CMD_SET_BDADDR		0xFC22
 #define BT_CMD_AUTO_SLEEP_MODE		0xFC23
 #define BT_CMD_HOST_SLEEP_CONFIG	0xFC59
 #define BT_CMD_HOST_SLEEP_ENABLE	0xFC5A
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index cc65fd2..1a325fe 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -539,6 +539,20 @@ static int btmrvl_setup(struct hci_dev *hdev)
 	return 0;
 }
 
+static int btmrvl_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
+{
+	struct btmrvl_private *priv = hci_get_drvdata(hdev);
+	long ret;
+	u8 buf[8];
+
+	buf[0] = MRVL_VENDOR_PKT;
+	buf[1] = sizeof(bdaddr_t);
+	bacpy((bdaddr_t *)&buf[2], bdaddr);
+	ret = btmrvl_send_sync_cmd(priv, BT_CMD_SET_BDADDR, buf, sizeof(buf));
+
+	return 0;
+}
+
 /*
  * This function handles the event generated by firmware, rx data
  * received from firmware, and tx data sent from kernel.
@@ -632,6 +646,7 @@ int btmrvl_register_hdev(struct btmrvl_private *priv)
 	hdev->flush = btmrvl_flush;
 	hdev->send  = btmrvl_send_frame;
 	hdev->setup = btmrvl_setup;
+	hdev->set_bdaddr = btmrvl_set_bdaddr;
 
 	hdev->dev_type = priv->btmrvl_dev.dev_type;
 
-- 
1.8.2.3

Re: [PATCH 1/2] Bluetooth: add public address configuration for Marvell USB devices

[Marcel Holtmann]

Hi Bing,

> Implemented .set_bdaddr handler provided by bluetooth stack for
> Marvell devices for public address configuration.
> 
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
> drivers/bluetooth/btusb.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index ed7b33b..9846fc4 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -48,6 +48,7 @@ static struct usb_driver btusb_driver;
> #define BTUSB_INTEL		0x100
> #define BTUSB_INTEL_BOOT	0x200
> #define BTUSB_BCM_PATCHRAM	0x400
> +#define BTUSB_MARVELL		0x800
> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -242,6 +243,10 @@ static const struct usb_device_id blacklist_table[] = {
> 	{ USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL },
> 	{ USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL },
> 
> +	/* Marvell device */
> +	{ USB_DEVICE(0x1286, 0x2044), .driver_info = BTUSB_MARVELL },
> +	{ USB_DEVICE(0x1286, 0x2046), .driver_info = BTUSB_MARVELL },
> +
> 	{ }	/* Terminating entry */
> };
> 
> @@ -1455,6 +1460,29 @@ static int btusb_set_bdaddr_intel(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> 	return 0;
> }
> 
> +static int btusb_set_bdaddr_marvell(struct hci_dev *hdev,
> +				    const bdaddr_t *bdaddr)
> +{
> +	struct sk_buff *skb;
> +	u8 buf[8];
> +	long ret;
> +
> +	buf[0] = 0xfe;
> +	buf[1] = sizeof(bdaddr_t);
> +	bacpy((bdaddr_t *)&buf[2], bdaddr);

you could define a packed struct for this or just use memcpy here instead.

> +
> +	skb = __hci_cmd_sync(hdev, 0xfc22, sizeof(buf), buf, HCI_INIT_TIMEOUT);

Is this opcode writing the address into flash and making it permanent or will a reboot restore it to its original address?

I am asking because for Ericsson chips this exact command with user_id 0xfe was actually making a permanent change of the BD_ADDR. What we want here is a volatile change of the address.

Regards

Marcel

RE: [PATCH 1/2] Bluetooth: add public address configuration for Marvell USB devices

[Bing Zhao]

Hi Marcel,

Thanks for your comments.

> > +static int btusb_set_bdaddr_marvell(struct hci_dev *hdev,
> > +				    const bdaddr_t *bdaddr)
> > +{
> > +	struct sk_buff *skb;
> > +	u8 buf[8];
> > +	long ret;
> > +
> > +	buf[0] =3D 0xfe;
> > +	buf[1] =3D sizeof(bdaddr_t);
> > +	bacpy((bdaddr_t *)&buf[2], bdaddr);
>=20
> you could define a packed struct for this or just use memcpy here instead=
.

In Amitkumar's original patch, it was like this:

+	memcpy((u8 *)buf + 2, bdaddr, sizeof(bdaddr_t));

I felt that it'd be better to use bacpy so I changed memcpy to bacpy while =
applying the patch.
I can change it back to memcpy if it looks ok to you. Otherwise I can defin=
e a packed struct.

>=20
> > +
> > +	skb =3D __hci_cmd_sync(hdev, 0xfc22, sizeof(buf), buf, HCI_INIT_TIMEO=
UT);
>=20
> Is this opcode writing the address into flash and making it permanent or =
will a reboot restore it to
> its original address?

A reboot restores it to its original address. I will add this information i=
n v2.

>=20
> I am asking because for Ericsson chips this exact command with user_id 0x=
fe was actually making a
> permanent change of the BD_ADDR. What we want here is a volatile change o=
f the address.

For Marvell devices it makes a temporary bdaddr change.

Thanks,
Bing

>=20
> Regards
>=20
> Marcel

Re: [PATCH 1/2] Bluetooth: add public address configuration for Marvell USB devices

[Marcel Holtmann]

Hi Bing,

>>> +static int btusb_set_bdaddr_marvell(struct hci_dev *hdev,
>>> +				    const bdaddr_t *bdaddr)
>>> +{
>>> +	struct sk_buff *skb;
>>> +	u8 buf[8];
>>> +	long ret;
>>> +
>>> +	buf[0] = 0xfe;
>>> +	buf[1] = sizeof(bdaddr_t);
>>> +	bacpy((bdaddr_t *)&buf[2], bdaddr);
>> 
>> you could define a packed struct for this or just use memcpy here instead.
> 
> In Amitkumar's original patch, it was like this:
> 
> +	memcpy((u8 *)buf + 2, bdaddr, sizeof(bdaddr_t));

you do not need the cast:

	memcpy(buf + 2, bdaddr, sizeof(bdaddr_t));

> 
> I felt that it'd be better to use bacpy so I changed memcpy to bacpy while applying the patch.
> I can change it back to memcpy if it looks ok to you. Otherwise I can define a packed struct.

I am fine with the memcpy actually.

>>> +
>>> +	skb = __hci_cmd_sync(hdev, 0xfc22, sizeof(buf), buf, HCI_INIT_TIMEOUT);
>> 
>> Is this opcode writing the address into flash and making it permanent or will a reboot restore it to
>> its original address?
> 
> A reboot restores it to its original address. I will add this information in v2.
> 
>> 
>> I am asking because for Ericsson chips this exact command with user_id 0xfe was actually making a
>> permanent change of the BD_ADDR. What we want here is a volatile change of the address.
> 
> For Marvell devices it makes a temporary bdaddr change.

Good.

I just know way too much details of all these vendor specific details of the original Bluetooth chips ;)

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: btmrvl: add public address configuration support

[Marcel Holtmann]

Hi Bing,

> .set_bdaddr handler is implemented for public address configuration.
> 
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
> drivers/bluetooth/btmrvl_drv.h  |  1 +
> drivers/bluetooth/btmrvl_main.c | 15 +++++++++++++++
> 2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
> index caf6841..38ad662 100644
> --- a/drivers/bluetooth/btmrvl_drv.h
> +++ b/drivers/bluetooth/btmrvl_drv.h
> @@ -91,6 +91,7 @@ struct btmrvl_private {
> 
> /* Vendor specific Bluetooth commands */
> #define BT_CMD_PSCAN_WIN_REPORT_ENABLE	0xFC03
> +#define BT_CMD_SET_BDADDR		0xFC22
> #define BT_CMD_AUTO_SLEEP_MODE		0xFC23
> #define BT_CMD_HOST_SLEEP_CONFIG	0xFC59
> #define BT_CMD_HOST_SLEEP_ENABLE	0xFC5A
> diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
> index cc65fd2..1a325fe 100644
> --- a/drivers/bluetooth/btmrvl_main.c
> +++ b/drivers/bluetooth/btmrvl_main.c
> @@ -539,6 +539,20 @@ static int btmrvl_setup(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static int btmrvl_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> +{
> +	struct btmrvl_private *priv = hci_get_drvdata(hdev);
> +	long ret;
> +	u8 buf[8];
> +
> +	buf[0] = MRVL_VENDOR_PKT;
> +	buf[1] = sizeof(bdaddr_t);
> +	bacpy((bdaddr_t *)&buf[2], bdaddr);

same thing here. Just use a memcpy.

> +	ret = btmrvl_send_sync_cmd(priv, BT_CMD_SET_BDADDR, buf, sizeof(buf));

So I keep wondering why you need the btmrvl_send_sync_cmd here. The HCI transport is up and running at this stage and sending vendor command should do the right thing. Similar to what you do in btusb driver.

You might have explained this to, but I must have clearly not understood it.

Regards

Marcel

RE: [PATCH 2/2] Bluetooth: btmrvl: add public address configuration support

[Bing Zhao]

Hi Marcel,

> > +	ret =3D btmrvl_send_sync_cmd(priv, BT_CMD_SET_BDADDR, buf, sizeof(buf=
));
>=20
> So I keep wondering why you need the btmrvl_send_sync_cmd here. The HCI t=
ransport is up and running
> at this stage and sending vendor command should do the right thing. Simil=
ar to what you do in btusb
> driver.
>=20
> You might have explained this to, but I must have clearly not understood =
it.

We will check if btmrvl_send_sync_cmd() can be replaced by __hci_cmd_sync()=
 and get back to you.

Thanks,
Bing

>=20
> Regards
>=20
> Marcel

RE: [PATCH 2/2] Bluetooth: btmrvl: add public address configuration support

[Bing Zhao]

Hi Marcel,

> > > +	ret =3D btmrvl_send_sync_cmd(priv, BT_CMD_SET_BDADDR, buf, sizeof(b=
uf));
> >
> > So I keep wondering why you need the btmrvl_send_sync_cmd here. The HCI=
 transport is up and running
> > at this stage and sending vendor command should do the right thing. Sim=
ilar to what you do in btusb
> > driver.
> >
> > You might have explained this to, but I must have clearly not understoo=
d it.
>=20
> We will check if btmrvl_send_sync_cmd() can be replaced by __hci_cmd_sync=
() and get back to you.

__hci_cmd_sync() sets skb->packet_type as 0x01 whereas btmrvl_send_sync_cmd=
() sets it as 0xfe.
But the firmware handles both 0x01 and 0xfe tx packets in similar way. Ther=
efore we can replace btmrvl_send_sync_cmd() with __hci_cmd_sync().

I will send a v2 for this patch shortly. Later on we will send out a cleanu=
p patch for other existing commands that are using btmrvl_send_sync_cmd().

Thanks,
Bing