[PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

Move initialization code to hdev's setup handler. New flag
setup_done is added to make sure that initialization is done only
during driver load time. Our firmware doesn't expect
re-initialization later when interface is re-enabled.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
v5: make use of hdev's setup handler (Marcel Holtmann)

 drivers/bluetooth/btmrvl_drv.h  |    1 +
 drivers/bluetooth/btmrvl_main.c |   23 +++++++++++++++++++++--
 drivers/bluetooth/btmrvl_sdio.c |    6 ------
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index 27068d1..e776b8b 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -68,6 +68,7 @@ struct btmrvl_adapter {
 	wait_queue_head_t cmd_wait_q;
 	u8 cmd_complete;
 	bool is_suspended;
+	bool setup_done;
 };
 
 struct btmrvl_private {
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 9a9f518..e352f8e 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -479,6 +479,27 @@ static int btmrvl_open(struct hci_dev *hdev)
 	return 0;
 }
 
+static int btmrvl_setup(struct hci_dev *hdev)
+{
+	struct btmrvl_private *priv = hci_get_drvdata(hdev);
+	struct btmrvl_adapter *adapter = priv->adapter;
+
+	if (adapter->setup_done)
+		return 0;
+
+	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
+
+	priv->btmrvl_dev.psmode = 1;
+	btmrvl_enable_ps(priv);
+
+	priv->btmrvl_dev.gpio_gap = 0xffff;
+	btmrvl_send_hscfg_cmd(priv);
+
+	adapter->setup_done = true;
+
+	return 0;
+}
+
 /*
  * This function handles the event generated by firmware, rx data
  * received from firmware, and tx data sent from kernel.
@@ -572,8 +592,7 @@ int btmrvl_register_hdev(struct btmrvl_private *priv)
 	hdev->flush = btmrvl_flush;
 	hdev->send = btmrvl_send_frame;
 	hdev->ioctl = btmrvl_ioctl;
-
-	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
+	hdev->setup = btmrvl_setup;
 
 	hdev->dev_type = priv->btmrvl_dev.dev_type;
 
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 75c2626..c526915 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -1046,12 +1046,6 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
 		goto disable_host_int;
 	}
 
-	priv->btmrvl_dev.psmode = 1;
-	btmrvl_enable_ps(priv);
-
-	priv->btmrvl_dev.gpio_gap = 0xffff;
-	btmrvl_send_hscfg_cmd(priv);
-
 	return 0;
 
 disable_host_int:
-- 
1.7.3.4

[PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Bing Zhao]

From: Amitkumar Karwar <akarwar@marvell.com>

A text file containing calibration data in hex format can
be provided at following path:

/lib/firmware/mrvl/sd8797_caldata.conf

The data will be downloaded to firmware during initialization.

Reviewed-by: Mike Frysinger <vapier@chromium.org>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
Signed-off-by: Hyuckjoo Lee <hyuckjoo.lee@samsung.com>
---
v2: Remove module parameter. The calibration data will be downloaded
    only when the device speicific data file is provided.
    (Marcel Holtmann)
v3: Fix crash (misaligned memory access) on ARM
v4: Simplify white space parsing and save some CPU cycles (Mike Frysinger)
v5: Improvements in cal data parsing logic. Add explanatory comments.
    Replace GFP_ATOMIC flag with GFP_KERNEL (Mike Frysinger)

 drivers/bluetooth/btmrvl_drv.h  |   10 +++-
 drivers/bluetooth/btmrvl_main.c |  144 ++++++++++++++++++++++++++++++++++++++-
 drivers/bluetooth/btmrvl_sdio.c |    9 ++-
 drivers/bluetooth/btmrvl_sdio.h |    2 +
 4 files changed, 161 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index e776b8b..dcd3468 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -23,6 +23,8 @@
 #include <linux/bitops.h>
 #include <linux/slab.h>
 #include <net/bluetooth/bluetooth.h>
+#include <linux/ctype.h>
+#include <linux/firmware.h>
 
 #define BTM_HEADER_LEN			4
 #define BTM_UPLD_SIZE			2312
@@ -41,6 +43,8 @@ struct btmrvl_thread {
 struct btmrvl_device {
 	void *card;
 	struct hci_dev *hcidev;
+	struct device *dev;
+	const char *cal_data;
 
 	u8 dev_type;
 
@@ -92,6 +96,7 @@ struct btmrvl_private {
 #define BT_CMD_HOST_SLEEP_CONFIG	0x59
 #define BT_CMD_HOST_SLEEP_ENABLE	0x5A
 #define BT_CMD_MODULE_CFG_REQ		0x5B
+#define BT_CMD_LOAD_CONFIG_DATA		0x61
 
 /* Sub-commands: Module Bringup/Shutdown Request/Response */
 #define MODULE_BRINGUP_REQ		0xF1
@@ -117,10 +122,13 @@ struct btmrvl_private {
 #define PS_SLEEP			0x01
 #define PS_AWAKE			0x00
 
+#define BT_CMD_DATA_SIZE		32
+#define BT_CAL_DATA_SIZE		28
+
 struct btmrvl_cmd {
 	__le16 ocf_ogf;
 	u8 length;
-	u8 data[4];
+	u8 data[BT_CMD_DATA_SIZE];
 } __packed;
 
 struct btmrvl_event {
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index e352f8e..6eea188 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -57,8 +57,9 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
 		ocf = hci_opcode_ocf(opcode);
 		ogf = hci_opcode_ogf(opcode);
 
-		if (ocf == BT_CMD_MODULE_CFG_REQ &&
-					priv->btmrvl_dev.sendcmdflag) {
+		if ((ocf == BT_CMD_MODULE_CFG_REQ ||
+		     ocf == BT_CMD_LOAD_CONFIG_DATA) &&
+		    priv->btmrvl_dev.sendcmdflag) {
 			priv->btmrvl_dev.sendcmdflag = false;
 			priv->adapter->cmd_complete = true;
 			wake_up_interruptible(&priv->adapter->cmd_wait_q);
@@ -479,6 +480,142 @@ static int btmrvl_open(struct hci_dev *hdev)
 	return 0;
 }
 
+/*
+ * This function parses provided calibration data input. It should contain
+ * hex bytes separated by space or new line character. Here is an example.
+ * 00 1C 01 37 FF FF FF FF 02 04 7F 01
+ * CE BA 00 00 00 2D C6 C0 00 00 00 00
+ * 00 F0 00 00
+ */
+static int btmrvl_parse_cal_cfg(const u8 *src, u32 len, u8 *dst, u32 dst_size)
+{
+	const u8 *s = src;
+	u8 *d = dst;
+	int ret;
+	u8 tmp[3];
+
+	tmp[2] = '\0';
+	while ((s - src) <= len - 2) {
+		if (isspace(*s) || *s == '\n') {
+			s++;
+			continue;
+		}
+
+		if (isxdigit(*s)) {
+			if ((d - dst) >= dst_size) {
+				BT_ERR("calibration data file too big!!!");
+				return -EINVAL;
+			}
+
+			memcpy(tmp, s, 2);
+
+			ret = kstrtou8(tmp, 16, d++);
+			if (ret < 0)
+				return ret;
+
+			s += 2;
+		} else {
+			return -EINVAL;
+		}
+	}
+	if (d == dst)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int btmrvl_load_cal_data(struct btmrvl_private *priv,
+				u8 *config_data)
+{
+	struct sk_buff *skb;
+	struct btmrvl_cmd *cmd;
+	int i;
+
+	skb = bt_skb_alloc(sizeof(*cmd), GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	cmd = (struct btmrvl_cmd *)skb->data;
+	cmd->ocf_ogf =
+		cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_LOAD_CONFIG_DATA));
+	cmd->length = BT_CMD_DATA_SIZE;
+	cmd->data[0] = 0x00;
+	cmd->data[1] = 0x00;
+	cmd->data[2] = 0x00;
+	cmd->data[3] = BT_CMD_DATA_SIZE - 4;
+
+	/* Swap cal-data bytes. Each four bytes are swapped. Considering 4
+	 * byte SDIO header offset, mapping of input and output bytes will be
+	 * {3, 2, 1, 0} -> {0+4, 1+4, 2+4, 3+4},
+	 * {7, 6, 5, 4} -> {4+4, 5+4, 6+4, 7+4} */
+	for (i = 4; i < BT_CMD_DATA_SIZE; i++)
+		cmd->data[i] = config_data[(i / 4) * 8 - 1 - i];
+
+	bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
+	skb_put(skb, sizeof(*cmd));
+	skb->dev = (void *)priv->btmrvl_dev.hcidev;
+	skb_queue_head(&priv->adapter->tx_queue, skb);
+	priv->btmrvl_dev.sendcmdflag = true;
+	priv->adapter->cmd_complete = false;
+
+	print_hex_dump_bytes("Calibration data: ",
+			     DUMP_PREFIX_OFFSET, cmd->data, BT_CMD_DATA_SIZE);
+
+	wake_up_interruptible(&priv->main_thread.wait_q);
+	if (!wait_event_interruptible_timeout(priv->adapter->cmd_wait_q,
+					      priv->adapter->cmd_complete,
+				       msecs_to_jiffies(WAIT_UNTIL_CMD_RESP))) {
+		BT_ERR("Timeout while loading calibration data");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int
+btmrvl_process_cal_cfg(struct btmrvl_private *priv, u8 *data, u32 size)
+{
+	u8 cal_data[BT_CAL_DATA_SIZE];
+	int ret;
+
+	ret = btmrvl_parse_cal_cfg(data, size, cal_data, sizeof(cal_data));
+	if (ret)
+		return ret;
+
+	ret = btmrvl_load_cal_data(priv, cal_data);
+	if (ret) {
+		BT_ERR("Fail to load calibrate data");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int btmrvl_cal_data_config(struct btmrvl_private *priv)
+{
+	const struct firmware *cfg;
+	int ret;
+	const char *cal_data = priv->btmrvl_dev.cal_data;
+
+	if (!cal_data)
+		return 0;
+
+	ret = request_firmware(&cfg, cal_data, priv->btmrvl_dev.dev);
+	if (ret < 0) {
+		BT_DBG("Failed to get %s file, skipping cal data download",
+		       cal_data);
+		ret = 0;
+		goto done;
+	}
+
+	ret = btmrvl_process_cal_cfg(priv, (u8 *)cfg->data, cfg->size);
+done:
+	if (cfg)
+		release_firmware(cfg);
+
+	return ret;
+}
+
 static int btmrvl_setup(struct hci_dev *hdev)
 {
 	struct btmrvl_private *priv = hci_get_drvdata(hdev);
@@ -489,6 +626,9 @@ static int btmrvl_setup(struct hci_dev *hdev)
 
 	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
 
+	if (btmrvl_cal_data_config(priv))
+		BT_ERR("Set cal data failed");
+
 	priv->btmrvl_dev.psmode = 1;
 	btmrvl_enable_ps(priv);
 
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index c526915..51e95ed 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -18,7 +18,6 @@
  * this warranty disclaimer.
  **/
 
-#include <linux/firmware.h>
 #include <linux/slab.h>
 
 #include <linux/mmc/sdio_ids.h>
@@ -102,6 +101,7 @@ static const struct btmrvl_sdio_card_reg btmrvl_reg_88xx = {
 static const struct btmrvl_sdio_device btmrvl_sdio_sd8688 = {
 	.helper		= "mrvl/sd8688_helper.bin",
 	.firmware	= "mrvl/sd8688.bin",
+	.cal_data	= NULL,
 	.reg		= &btmrvl_reg_8688,
 	.sd_blksz_fw_dl	= 64,
 };
@@ -109,6 +109,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8688 = {
 static const struct btmrvl_sdio_device btmrvl_sdio_sd8787 = {
 	.helper		= NULL,
 	.firmware	= "mrvl/sd8787_uapsta.bin",
+	.cal_data	= NULL,
 	.reg		= &btmrvl_reg_87xx,
 	.sd_blksz_fw_dl	= 256,
 };
@@ -116,6 +117,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8787 = {
 static const struct btmrvl_sdio_device btmrvl_sdio_sd8797 = {
 	.helper		= NULL,
 	.firmware	= "mrvl/sd8797_uapsta.bin",
+	.cal_data	= "mrvl/sd8797_caldata.conf",
 	.reg		= &btmrvl_reg_87xx,
 	.sd_blksz_fw_dl	= 256,
 };
@@ -123,6 +125,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8797 = {
 static const struct btmrvl_sdio_device btmrvl_sdio_sd8897 = {
 	.helper		= NULL,
 	.firmware	= "mrvl/sd8897_uapsta.bin",
+	.cal_data	= NULL,
 	.reg		= &btmrvl_reg_88xx,
 	.sd_blksz_fw_dl	= 256,
 };
@@ -1006,6 +1009,7 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
 		struct btmrvl_sdio_device *data = (void *) id->driver_data;
 		card->helper = data->helper;
 		card->firmware = data->firmware;
+		card->cal_data = data->cal_data;
 		card->reg = data->reg;
 		card->sd_blksz_fw_dl = data->sd_blksz_fw_dl;
 	}
@@ -1034,6 +1038,8 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
 	}
 
 	card->priv = priv;
+	priv->btmrvl_dev.dev = &card->func->dev;
+	priv->btmrvl_dev.cal_data = card->cal_data;
 
 	/* Initialize the interface specific function pointers */
 	priv->hw_host_to_card = btmrvl_sdio_host_to_card;
@@ -1216,4 +1222,5 @@ MODULE_FIRMWARE("mrvl/sd8688_helper.bin");
 MODULE_FIRMWARE("mrvl/sd8688.bin");
 MODULE_FIRMWARE("mrvl/sd8787_uapsta.bin");
 MODULE_FIRMWARE("mrvl/sd8797_uapsta.bin");
+MODULE_FIRMWARE("mrvl/sd8797_caldata.conf");
 MODULE_FIRMWARE("mrvl/sd8897_uapsta.bin");
diff --git a/drivers/bluetooth/btmrvl_sdio.h b/drivers/bluetooth/btmrvl_sdio.h
index 43d35a6..6872d9e 100644
--- a/drivers/bluetooth/btmrvl_sdio.h
+++ b/drivers/bluetooth/btmrvl_sdio.h
@@ -85,6 +85,7 @@ struct btmrvl_sdio_card {
 	u32 ioport;
 	const char *helper;
 	const char *firmware;
+	const char *cal_data;
 	const struct btmrvl_sdio_card_reg *reg;
 	u16 sd_blksz_fw_dl;
 	u8 rx_unit;
@@ -94,6 +95,7 @@ struct btmrvl_sdio_card {
 struct btmrvl_sdio_device {
 	const char *helper;
 	const char *firmware;
+	const char *cal_data;
 	const struct btmrvl_sdio_card_reg *reg;
 	u16 sd_blksz_fw_dl;
 };
-- 
1.7.3.4

Re: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Marcel Holtmann]

Hi Bing,

> Move initialization code to hdev's setup handler. New flag
> setup_done is added to make sure that initialization is done only
> during driver load time. Our firmware doesn't expect
> re-initialization later when interface is re-enabled.
> 
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> ---
> v5: make use of hdev's setup handler (Marcel Holtmann)
> 
> drivers/bluetooth/btmrvl_drv.h  |    1 +
> drivers/bluetooth/btmrvl_main.c |   23 +++++++++++++++++++++--
> drivers/bluetooth/btmrvl_sdio.c |    6 ------
> 3 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
> index 27068d1..e776b8b 100644
> --- a/drivers/bluetooth/btmrvl_drv.h
> +++ b/drivers/bluetooth/btmrvl_drv.h
> @@ -68,6 +68,7 @@ struct btmrvl_adapter {
> 	wait_queue_head_t cmd_wait_q;
> 	u8 cmd_complete;
> 	bool is_suspended;
> +	bool setup_done;
> };
> 
> struct btmrvl_private {
> diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
> index 9a9f518..e352f8e 100644
> --- a/drivers/bluetooth/btmrvl_main.c
> +++ b/drivers/bluetooth/btmrvl_main.c
> @@ -479,6 +479,27 @@ static int btmrvl_open(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static int btmrvl_setup(struct hci_dev *hdev)
> +{
> +	struct btmrvl_private *priv = hci_get_drvdata(hdev);
> +	struct btmrvl_adapter *adapter = priv->adapter;
> +
> +	if (adapter->setup_done)
> +		return 0;
> +
> +	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
> +
> +	priv->btmrvl_dev.psmode = 1;
> +	btmrvl_enable_ps(priv);
> +
> +	priv->btmrvl_dev.gpio_gap = 0xffff;
> +	btmrvl_send_hscfg_cmd(priv);
> +
> +	adapter->setup_done = true;
> +
> +	return 0;
> +}
> +
> /*
>  * This function handles the event generated by firmware, rx data
>  * received from firmware, and tx data sent from kernel.
> @@ -572,8 +592,7 @@ int btmrvl_register_hdev(struct btmrvl_private *priv)
> 	hdev->flush = btmrvl_flush;
> 	hdev->send = btmrvl_send_frame;
> 	hdev->ioctl = btmrvl_ioctl;
> -
> -	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
> +	hdev->setup = btmrvl_setup;

just to make sure you guys understand how ->setup() works. It is only called once. Bringing the adapter down and up again does not call ->setup() a second time. So do you guys need this setup_done variable and if so, then you need to be a bit more verbose and help me understand why.

Regards

Marcel

Re: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Marcel Holtmann]

Hi Bing,

> A text file containing calibration data in hex format can
> be provided at following path:
> 
> /lib/firmware/mrvl/sd8797_caldata.conf
> 
> The data will be downloaded to firmware during initialization.
> 
> Reviewed-by: Mike Frysinger <vapier@chromium.org>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> Signed-off-by: Hyuckjoo Lee <hyuckjoo.lee@samsung.com>
> ---
> v2: Remove module parameter. The calibration data will be downloaded
>    only when the device speicific data file is provided.
>    (Marcel Holtmann)
> v3: Fix crash (misaligned memory access) on ARM
> v4: Simplify white space parsing and save some CPU cycles (Mike Frysinger)
> v5: Improvements in cal data parsing logic. Add explanatory comments.
>    Replace GFP_ATOMIC flag with GFP_KERNEL (Mike Frysinger)
> 
> drivers/bluetooth/btmrvl_drv.h  |   10 +++-
> drivers/bluetooth/btmrvl_main.c |  144 ++++++++++++++++++++++++++++++++++++++-
> drivers/bluetooth/btmrvl_sdio.c |    9 ++-
> drivers/bluetooth/btmrvl_sdio.h |    2 +
> 4 files changed, 161 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
> index e776b8b..dcd3468 100644
> --- a/drivers/bluetooth/btmrvl_drv.h
> +++ b/drivers/bluetooth/btmrvl_drv.h
> @@ -23,6 +23,8 @@
> #include <linux/bitops.h>
> #include <linux/slab.h>
> #include <net/bluetooth/bluetooth.h>
> +#include <linux/ctype.h>
> +#include <linux/firmware.h>
> 
> #define BTM_HEADER_LEN			4
> #define BTM_UPLD_SIZE			2312
> @@ -41,6 +43,8 @@ struct btmrvl_thread {
> struct btmrvl_device {
> 	void *card;
> 	struct hci_dev *hcidev;
> +	struct device *dev;
> +	const char *cal_data;
> 
> 	u8 dev_type;
> 
> @@ -92,6 +96,7 @@ struct btmrvl_private {
> #define BT_CMD_HOST_SLEEP_CONFIG	0x59
> #define BT_CMD_HOST_SLEEP_ENABLE	0x5A
> #define BT_CMD_MODULE_CFG_REQ		0x5B
> +#define BT_CMD_LOAD_CONFIG_DATA		0x61
> 
> /* Sub-commands: Module Bringup/Shutdown Request/Response */
> #define MODULE_BRINGUP_REQ		0xF1
> @@ -117,10 +122,13 @@ struct btmrvl_private {
> #define PS_SLEEP			0x01
> #define PS_AWAKE			0x00
> 
> +#define BT_CMD_DATA_SIZE		32
> +#define BT_CAL_DATA_SIZE		28
> +
> struct btmrvl_cmd {
> 	__le16 ocf_ogf;
> 	u8 length;
> -	u8 data[4];
> +	u8 data[BT_CMD_DATA_SIZE];
> } __packed;
> 
> struct btmrvl_event {
> diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
> index e352f8e..6eea188 100644
> --- a/drivers/bluetooth/btmrvl_main.c
> +++ b/drivers/bluetooth/btmrvl_main.c
> @@ -57,8 +57,9 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
> 		ocf = hci_opcode_ocf(opcode);
> 		ogf = hci_opcode_ogf(opcode);
> 
> -		if (ocf == BT_CMD_MODULE_CFG_REQ &&
> -					priv->btmrvl_dev.sendcmdflag) {
> +		if ((ocf == BT_CMD_MODULE_CFG_REQ ||
> +		     ocf == BT_CMD_LOAD_CONFIG_DATA) &&
> +		    priv->btmrvl_dev.sendcmdflag) {
> 			priv->btmrvl_dev.sendcmdflag = false;
> 			priv->adapter->cmd_complete = true;
> 			wake_up_interruptible(&priv->adapter->cmd_wait_q);
> @@ -479,6 +480,142 @@ static int btmrvl_open(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +/*
> + * This function parses provided calibration data input. It should contain
> + * hex bytes separated by space or new line character. Here is an example.
> + * 00 1C 01 37 FF FF FF FF 02 04 7F 01
> + * CE BA 00 00 00 2D C6 C0 00 00 00 00
> + * 00 F0 00 00
> + */
> +static int btmrvl_parse_cal_cfg(const u8 *src, u32 len, u8 *dst, u32 dst_size)
> +{
> +	const u8 *s = src;
> +	u8 *d = dst;
> +	int ret;
> +	u8 tmp[3];
> +
> +	tmp[2] = '\0';
> +	while ((s - src) <= len - 2) {
> +		if (isspace(*s) || *s == '\n') {
> +			s++;
> +			continue;
> +		}
> +
> +		if (isxdigit(*s)) {
> +			if ((d - dst) >= dst_size) {
> +				BT_ERR("calibration data file too big!!!");
> +				return -EINVAL;
> +			}
> +
> +			memcpy(tmp, s, 2);
> +
> +			ret = kstrtou8(tmp, 16, d++);
> +			if (ret < 0)
> +				return ret;
> +
> +			s += 2;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +	if (d == dst)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int btmrvl_load_cal_data(struct btmrvl_private *priv,
> +				u8 *config_data)
> +{
> +	struct sk_buff *skb;
> +	struct btmrvl_cmd *cmd;
> +	int i;
> +
> +	skb = bt_skb_alloc(sizeof(*cmd), GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	cmd = (struct btmrvl_cmd *)skb->data;
> +	cmd->ocf_ogf =
> +		cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_LOAD_CONFIG_DATA));
> +	cmd->length = BT_CMD_DATA_SIZE;
> +	cmd->data[0] = 0x00;
> +	cmd->data[1] = 0x00;
> +	cmd->data[2] = 0x00;
> +	cmd->data[3] = BT_CMD_DATA_SIZE - 4;

why not use __hci_cmd_sync() here. It is designed to be used from ->setup() where it is guaranteed that the HCI request lock is held. And it is guaranteed that ->setup() is executed in a workqueue.

Regards

Marcel

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Marcel,

> > -	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
> > +	hdev->setup = btmrvl_setup;
> 
> just to make sure you guys understand how ->setup() works. It is only called once. Bringing the
> adapter down and up again does not call ->setup() a second time. So do you guys need this setup_done
> variable and if so, then you need to be a bit more verbose and help me understand why.

It's observed that sometimes the setup handler is called twice when Bluetooth daemon is running in background. We will rebase to latest commit on bluetooth-next tree and test again. If the issue is gone with the latest code in -next tree we will remove the setup_done flag.

Thanks,
Bing

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Marcel,

> > -	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
> > +	hdev->setup =3D btmrvl_setup;
>=20
> just to make sure you guys understand how ->setup() works. It is only cal=
led once. Bringing the
> adapter down and up again does not call ->setup() a second time. So do yo=
u guys need this setup_done
> variable and if so, then you need to be a bit more verbose and help me un=
derstand why.

It's observed that sometimes the setup handler is called twice when Bluetoo=
th daemon is running in background. We will rebase to latest commit on blue=
tooth-next tree and test again. If the issue is gone with the latest code i=
n -next tree we will remove the setup_done flag.

Thanks,
Bing

RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Bing Zhao]

Hi Marcel,

> > +	cmd = (struct btmrvl_cmd *)skb->data;
> > +	cmd->ocf_ogf =
> > +		cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_LOAD_CONFIG_DATA));
> > +	cmd->length = BT_CMD_DATA_SIZE;
> > +	cmd->data[0] = 0x00;
> > +	cmd->data[1] = 0x00;
> > +	cmd->data[2] = 0x00;
> > +	cmd->data[3] = BT_CMD_DATA_SIZE - 4;
> 
> why not use __hci_cmd_sync() here. It is designed to be used from ->setup() where it is guaranteed
> that the HCI request lock is held. And it is guaranteed that ->setup() is executed in a workqueue.

The reason of not using __hci_cmd_sync() is that we are sending vendor specific command here (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_COMMAND_PKT only.
Please let us know if you have any suggestion to solve this problem.

Thanks,
Bing

RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Bing Zhao]

Hi Marcel,

> > +	cmd =3D (struct btmrvl_cmd *)skb->data;
> > +	cmd->ocf_ogf =3D
> > +		cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_LOAD_CONFIG_DATA));
> > +	cmd->length =3D BT_CMD_DATA_SIZE;
> > +	cmd->data[0] =3D 0x00;
> > +	cmd->data[1] =3D 0x00;
> > +	cmd->data[2] =3D 0x00;
> > +	cmd->data[3] =3D BT_CMD_DATA_SIZE - 4;
>=20
> why not use __hci_cmd_sync() here. It is designed to be used from ->setup=
() where it is guaranteed
> that the HCI request lock is held. And it is guaranteed that ->setup() is=
 executed in a workqueue.

The reason of not using __hci_cmd_sync() is that we are sending vendor spec=
ific command here (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_CO=
MMAND_PKT only.
Please let us know if you have any suggestion to solve this problem.

Thanks,
Bing

Re: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Marcel Holtmann]

Hi Bing,

>>> +	cmd = (struct btmrvl_cmd *)skb->data;
>>> +	cmd->ocf_ogf =
>>> +		cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_LOAD_CONFIG_DATA));
>>> +	cmd->length = BT_CMD_DATA_SIZE;
>>> +	cmd->data[0] = 0x00;
>>> +	cmd->data[1] = 0x00;
>>> +	cmd->data[2] = 0x00;
>>> +	cmd->data[3] = BT_CMD_DATA_SIZE - 4;
>> 
>> why not use __hci_cmd_sync() here. It is designed to be used from ->setup() where it is guaranteed
>> that the HCI request lock is held. And it is guaranteed that ->setup() is executed in a workqueue.
> 
> The reason of not using __hci_cmd_sync() is that we are sending vendor specific command here (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_COMMAND_PKT only.
> Please let us know if you have any suggestion to solve this problem.

what is a MRVL_VENDOR_PKT actually?

If you guys are not using standard HCI command/event for vendor operation, then this obviously does not fit. However a similar model might make sense instead of manually building packets all the time.

Regards

Marcel

Re: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Marcel Holtmann]

Hi Bing,

>>> -	btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
>>> +	hdev->setup = btmrvl_setup;
>> 
>> just to make sure you guys understand how ->setup() works. It is only called once. Bringing the
>> adapter down and up again does not call ->setup() a second time. So do you guys need this setup_done
>> variable and if so, then you need to be a bit more verbose and help me understand why.
> 
> It's observed that sometimes the setup handler is called twice when Bluetooth daemon is running in background. We will rebase to latest commit on bluetooth-next tree and test again. If the issue is gone with the latest code in -next tree we will remove the setup_done flag.

that is a bug. It should only be ever called once. Could this be due to RFKILL issue we had? Please re-test with Johan's patches applied and check if it makes a difference. Otherwise please send some logs since we want to get this fixed.

Regards

Marcel

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Hi Marcel,

> > It's observed that sometimes the setup handler is called twice when Bluetooth daemon is running in
> background. We will rebase to latest commit on bluetooth-next tree and test again. If the issue is
> gone with the latest code in -next tree we will remove the setup_done flag.
> 
> that is a bug. It should only be ever called once. Could this be due to RFKILL issue we had? Please
> re-test with Johan's patches applied and check if it makes a difference. Otherwise please send some
> logs since we want to get this fixed.

Amitkumar Karwar has tested it with latest code on bluetooth-next tree but the result is the same.
Apparently two threads race to call hci_dev_open(). If the thread from hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP hdev flag in hci_power_on(). This results that the setup handler gets called again when user brings up the interface later.

Attached are the debug logs and the patch used to generate them.

I checked the bluetooth-next tree, the following two patches (by Johan) are not present in this tree.

bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag

They are in bluetooth.git tree. So, I'm not certain if Amitkumar has applied them manually or not. Anyway we will re-test with Johan's patches applied and confirm if they fix the race or not.

Thanks,
Bing


[-- Attachment #2: success.log --]
[-- Type: application/octet-stream, Size: 1097 bytes --]

[ 1717.100628] mmc1: new SDIO card at address 0001
[ 1717.100862] Bluetooth: vendor=0x2df, device=0x912a, class=255, fn=2
[ 1718.381306] BT_DBG: enter: hci_register_dev
[ 1718.381668] BT_DBG: in hci_register_dev queueing work power_on
[ 1718.381676] BT_DBG: exit: hci_register_dev
[ 1718.382658] BT_DBG: enter: hci_power_on line=1673 pid=3686
[ 1718.382664] BT_DBG: hci_dev_open line=1186 pid=3686
[ 1718.382669] BT_DBG: hci_dev_open line=1200 pid=3686
[ 1718.382673] BT_DBG: hci_dev_open line=1206 pid=3686
[ 1718.382677] BT_DBG: hci_dev_open line=1212 pid=3686
[ 1718.382681] BT_DBG: hci_dev_open line=1217 pid=3686
[ 1718.382684] btmrvl: enter btmrvl_setup()
[ 1718.383231] BT_DBG: calling hci_dev_open from hci_sock.c pid=3689
[ 1718.383236] BT_DBG: hci_dev_open line=1186 pid=3689
[ 1718.489930] BT_DBG: hci_dev_open line=1225 pid=3686
[ 1718.635973] BT_DBG: hci_power_on line=1684 updating HCI_SETUP hdev flag pid=3686
[ 1718.635984] BT_DBG: exit: hci_power_on line=1687 pid=3686
[ 1718.636282] BT_DBG: hci_dev_open line=1200 pid=3689
[ 1718.636288] BT_DBG: hci_dev_open line=1206 pid=3689


[-- Attachment #3: failure.log --]
[-- Type: application/octet-stream, Size: 951 bytes --]

[  132.180560] mmc1: new SDIO card at address 0001
[  132.415282] Bluetooth: vendor=0x2df, device=0x912a, class=255, fn=2
[  133.784309] BT_DBG: enter: hci_register_dev
[  133.784694] BT_DBG: in hci_register_dev queueing work power_on
[  133.784703] BT_DBG: exit: hci_register_dev
[  133.786616] BT_DBG: calling hci_dev_open from hci_sock.c pid=3287
[  133.786624] BT_DBG: hci_dev_open line=1186 pid=3287
[  133.786628] BT_DBG: hci_dev_open line=1200 pid=3287
[  133.786633] BT_DBG: hci_dev_open line=1206 pid=3287
[  133.786636] BT_DBG: hci_dev_open line=1212 pid=3287
[  133.786640] BT_DBG: hci_dev_open line=1217 pid=3287
[  133.786644] btmrvl: enter btmrvl_setup()
[  133.789683] BT_DBG: enter: hci_power_on line=1673 pid=3283
[  133.789691] BT_DBG: hci_dev_open line=1186 pid=3283
[  133.954749] BT_DBG: hci_dev_open line=1225 pid=3287
[  134.101863] BT_DBG: hci_dev_open line=1200 pid=3283
[  134.101871] BT_DBG: hci_dev_open line=1206 pid=3283

[-- Attachment #4: bt_debug.diff --]
[-- Type: application/octet-stream, Size: 3517 bytes --]

diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 6eea188..b474dde 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -621,6 +621,7 @@ static int btmrvl_setup(struct hci_dev *hdev)
 	struct btmrvl_private *priv = hci_get_drvdata(hdev);
 	struct btmrvl_adapter *adapter = priv->adapter;
 
+	printk("btmrvl: enter btmrvl_setup()\n");
 	if (adapter->setup_done)
 		return 0;
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3d9f02b..d0795e9 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1183,6 +1183,7 @@ int hci_dev_open(__u16 dev)
 	struct hci_dev *hdev;
 	int ret = 0;
 
+	printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
 	hdev = hci_dev_get(dev);
 	if (!hdev)
 		return -ENODEV;
@@ -1196,20 +1197,24 @@ int hci_dev_open(__u16 dev)
 		goto done;
 	}
 
+	printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
 	if (hdev->rfkill && rfkill_blocked(hdev->rfkill)) {
 		ret = -ERFKILL;
 		goto done;
 	}
 
+	printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
 	if (test_bit(HCI_UP, &hdev->flags)) {
 		ret = -EALREADY;
 		goto done;
 	}
 
+	printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
 	if (hdev->open(hdev)) {
 		ret = -EIO;
 		goto done;
 	}
+	printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
 
 	atomic_set(&hdev->cmd_cnt, 1);
 	set_bit(HCI_INIT, &hdev->flags);
@@ -1217,6 +1222,7 @@ int hci_dev_open(__u16 dev)
 	if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
 		ret = hdev->setup(hdev);
 
+	printk("BT_DBG: hci_dev_open line=%d pid=%d\n", __LINE__, current->pid);
 	if (!ret) {
 		/* Treat all non BR/EDR controllers as raw devices if
 		 * enable_hs is not set.
@@ -1664,6 +1670,7 @@ static void hci_power_on(struct work_struct *work)
 
 	BT_DBG("%s", hdev->name);
 
+	printk("BT_DBG: enter: hci_power_on line=%d pid=%d\n", __LINE__, current->pid);
 	err = hci_dev_open(hdev->id);
 	if (err < 0) {
 		mgmt_set_powered_failed(hdev, err);
@@ -1674,8 +1681,10 @@ static void hci_power_on(struct work_struct *work)
 		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
 				   HCI_AUTO_OFF_TIMEOUT);
 
+	printk("BT_DBG: hci_power_on line=%d updating HCI_SETUP hdev flag pid=%d\n", __LINE__, current->pid);
 	if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags))
 		mgmt_index_added(hdev);
+	printk("BT_DBG: exit: hci_power_on line=%d pid=%d\n", __LINE__, current->pid);
 }
 
 static void hci_power_off(struct work_struct *work)
@@ -2234,6 +2243,7 @@ int hci_register_dev(struct hci_dev *hdev)
 {
 	int id, error;
 
+	printk("BT_DBG: enter: hci_register_dev\n");
 	if (!hdev->open || !hdev->close)
 		return -EINVAL;
 
@@ -2300,7 +2310,9 @@ int hci_register_dev(struct hci_dev *hdev)
 	hci_notify(hdev, HCI_DEV_REG);
 	hci_dev_hold(hdev);
 
+	printk("BT_DBG: in hci_register_dev queueing work power_on\n");
 	queue_work(hdev->req_workqueue, &hdev->power_on);
+	printk("BT_DBG: exit: hci_register_dev\n");
 
 	return id;
 
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index c09e976..ddefcbe 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -587,6 +587,7 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
 	case HCIDEVUP:
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
+		printk("BT_DBG: calling hci_dev_open from hci_sock.c pid=%d\n", current->pid);
 		return hci_dev_open(arg);
 
 	case HCIDEVDOWN:

RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Bing Zhao]

Hi Marcel,

> > The reason of not using __hci_cmd_sync() is that we are sending vendor specific command here
> (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_COMMAND_PKT only.
> > Please let us know if you have any suggestion to solve this problem.
> 
> what is a MRVL_VENDOR_PKT actually?

It's defined as 0xfe in our driver. The firmware doesn't understand 0xff (HCI_VENDOR_PKT).

> 
> If you guys are not using standard HCI command/event for vendor operation, then this obviously does
> not fit. However a similar model might make sense instead of manually building packets all the time.

We can extend __hci_cmd_sync() function with a new parameter 'type'.
This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other drivers will pass in HCI_COMMAND_PKT.

Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion before downloading the frame to firmware. And the MRVL_VENDOR_PKT frame from firmware will be replaced with HCI_VENDOR_PKT while uploading the frame to stack.

Please let us know if this approach works for you or not.

Thanks,
Bing

RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Bing Zhao]

Hi Marcel,

> > The reason of not using __hci_cmd_sync() is that we are sending vendor =
specific command here
> (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_COMMAND_PKT only.
> > Please let us know if you have any suggestion to solve this problem.
>=20
> what is a MRVL_VENDOR_PKT actually?

It's defined as 0xfe in our driver. The firmware doesn't understand 0xff (H=
CI_VENDOR_PKT).

>=20
> If you guys are not using standard HCI command/event for vendor operation=
, then this obviously does
> not fit. However a similar model might make sense instead of manually bui=
lding packets all the time.

We can extend __hci_cmd_sync() function with a new parameter 'type'.
This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other driv=
ers will pass in HCI_COMMAND_PKT.

Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion before do=
wnloading the frame to firmware. And the MRVL_VENDOR_PKT frame from firmwar=
e will be replaced with HCI_VENDOR_PKT while uploading the frame to stack.

Please let us know if this approach works for you or not.

Thanks,
Bing

Re: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

Hi Bing,

On Tue, Sep 24, 2013, Bing Zhao wrote:
> > that is a bug. It should only be ever called once. Could this be due
> > to RFKILL issue we had? Please re-test with Johan's patches applied
> > and check if it makes a difference. Otherwise please send some logs
> > since we want to get this fixed.
> 
> Amitkumar Karwar has tested it with latest code on bluetooth-next tree
> but the result is the same.
> Apparently two threads race to call hci_dev_open(). If the thread from
> hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP
> hdev flag in hci_power_on(). This results that the setup handler gets
> called again when user brings up the interface later.

Let's see if I understood this right: the only hci_dev_open call in
hci_sock.c is the one for the HCIDEVUP ioctl. So what you're doing is
having user space call the HCIDEVUP ioctl before our own hci_power_on
callback gets called to initialize the adapter?

You're right that we're missing the clearing of the HCI_SETUP flag for
such a scenario. Could you try the attached patch. It should fix the
issue. One problem that it does have is that if the HCIDEVUP ioctl path
goes through before hci_power_on gets called we will never notify mgmt
of the adapter. However, that might be acceptable here since if you're
using HCIDEVUP like this it seems it's not a mgmt based system anyway.

> I checked the bluetooth-next tree, the following two patches (by
> Johan) are not present in this tree.
> 
> bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
> 5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag
> 
> They are in bluetooth.git tree. So, I'm not certain if Amitkumar has
> applied them manually or not. Anyway we will re-test with Johan's
> patches applied and confirm if they fix the race or not.

I don't think these patches will help you in this case.

Johan

[-- Attachment #2: hci-setup.patch --]
[-- Type: text/plain, Size: 1114 bytes --]

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 634deba..c48bf1a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1164,7 +1164,7 @@ int hci_dev_open(__u16 dev)
 	atomic_set(&hdev->cmd_cnt, 1);
 	set_bit(HCI_INIT, &hdev->flags);
 
-	if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags))
+	if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags) && hdev->setup)
 		ret = hdev->setup(hdev);
 
 	if (!ret) {
@@ -1581,10 +1581,13 @@ static const struct rfkill_ops hci_rfkill_ops = {
 static void hci_power_on(struct work_struct *work)
 {
 	struct hci_dev *hdev = container_of(work, struct hci_dev, power_on);
+	bool setup;
 	int err;
 
 	BT_DBG("%s", hdev->name);
 
+	setup = test_bit(HCI_SETUP, &hdev->dev_flags);
+
 	err = hci_dev_open(hdev->id);
 	if (err < 0) {
 		mgmt_set_powered_failed(hdev, err);
@@ -1595,7 +1598,7 @@ static void hci_power_on(struct work_struct *work)
 		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
 				   HCI_AUTO_OFF_TIMEOUT);
 
-	if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags))
+	if (setup)
 		mgmt_index_added(hdev);
 }
 

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Johan,

> Hi Bing,
> 
> On Tue, Sep 24, 2013, Bing Zhao wrote:
> > > that is a bug. It should only be ever called once. Could this be due
> > > to RFKILL issue we had? Please re-test with Johan's patches applied
> > > and check if it makes a difference. Otherwise please send some logs
> > > since we want to get this fixed.
> >
> > Amitkumar Karwar has tested it with latest code on bluetooth-next tree
> > but the result is the same.
> > Apparently two threads race to call hci_dev_open(). If the thread from
> > hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP
> > hdev flag in hci_power_on(). This results that the setup handler gets
> > called again when user brings up the interface later.
> 
> Let's see if I understood this right: the only hci_dev_open call in
> hci_sock.c is the one for the HCIDEVUP ioctl. So what you're doing is
> having user space call the HCIDEVUP ioctl before our own hci_power_on
> callback gets called to initialize the adapter?

That's right. The ioctl is initiated by the Bluetooth daemon.
Amitkumar has a setup that can reproduce this corner case easily.
I tested it on my Ubuntu but I couldn't replicate it.

> 
> You're right that we're missing the clearing of the HCI_SETUP flag for
> such a scenario. Could you try the attached patch. It should fix the
> issue. One problem that it does have is that if the HCIDEVUP ioctl path
> goes through before hci_power_on gets called we will never notify mgmt
> of the adapter. However, that might be acceptable here since if you're
> using HCIDEVUP like this it seems it's not a mgmt based system anyway.
> 
> > I checked the bluetooth-next tree, the following two patches (by
> > Johan) are not present in this tree.
> >
> > bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
> > 5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag
> >
> > They are in bluetooth.git tree. So, I'm not certain if Amitkumar has
> > applied them manually or not. Anyway we will re-test with Johan's
> > patches applied and confirm if they fix the race or not.
> 
> I don't think these patches will help you in this case.

OK, we will test your patch instead.

Thanks,
Bing

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Johan,

> Hi Bing,
>=20
> On Tue, Sep 24, 2013, Bing Zhao wrote:
> > > that is a bug. It should only be ever called once. Could this be due
> > > to RFKILL issue we had? Please re-test with Johan's patches applied
> > > and check if it makes a difference. Otherwise please send some logs
> > > since we want to get this fixed.
> >
> > Amitkumar Karwar has tested it with latest code on bluetooth-next tree
> > but the result is the same.
> > Apparently two threads race to call hci_dev_open(). If the thread from
> > hci_sock calls hci_dev_open earlier, it ends up not updating HCI_SETUP
> > hdev flag in hci_power_on(). This results that the setup handler gets
> > called again when user brings up the interface later.
>=20
> Let's see if I understood this right: the only hci_dev_open call in
> hci_sock.c is the one for the HCIDEVUP ioctl. So what you're doing is
> having user space call the HCIDEVUP ioctl before our own hci_power_on
> callback gets called to initialize the adapter?

That's right. The ioctl is initiated by the Bluetooth daemon.
Amitkumar has a setup that can reproduce this corner case easily.
I tested it on my Ubuntu but I couldn't replicate it.

>=20
> You're right that we're missing the clearing of the HCI_SETUP flag for
> such a scenario. Could you try the attached patch. It should fix the
> issue. One problem that it does have is that if the HCIDEVUP ioctl path
> goes through before hci_power_on gets called we will never notify mgmt
> of the adapter. However, that might be acceptable here since if you're
> using HCIDEVUP like this it seems it's not a mgmt based system anyway.
>=20
> > I checked the bluetooth-next tree, the following two patches (by
> > Johan) are not present in this tree.
> >
> > bf54303 Bluetooth: Fix rfkill functionality during the HCI setup stage
> > 5e13036 Bluetooth: Introduce a new HCI_RFKILLED flag
> >
> > They are in bluetooth.git tree. So, I'm not certain if Amitkumar has
> > applied them manually or not. Anyway we will re-test with Johan's
> > patches applied and confirm if they fix the race or not.
>=20
> I don't think these patches will help you in this case.

OK, we will test your patch instead.

Thanks,
Bing

Re: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Marcel Holtmann]

Hi Bing,

>>> The reason of not using __hci_cmd_sync() is that we are sending vendor specific command here
>> (MRVL_VENDOR_PKT). The __hci_cmd_sync seems handle HCI_COMMAND_PKT only.
>>> Please let us know if you have any suggestion to solve this problem.
>> 
>> what is a MRVL_VENDOR_PKT actually?
> 
> It's defined as 0xfe in our driver. The firmware doesn't understand 0xff (HCI_VENDOR_PKT).

so it is actually out-of-channel vendor packet.

>> 
>> If you guys are not using standard HCI command/event for vendor operation, then this obviously does
>> not fit. However a similar model might make sense instead of manually building packets all the time.
> 
> We can extend __hci_cmd_sync() function with a new parameter 'type'.
> This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other drivers will pass in HCI_COMMAND_PKT.

That will actually not work. And I also do not want to do that. The __hci_cmd_sync() is for real HCI packets. That means types 0x01 and 0x04 only. They need to adhere to the HCI flow control mechanism for commands.

> Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion before downloading the frame to firmware. And the MRVL_VENDOR_PKT frame from firmware will be replaced with HCI_VENDOR_PKT while uploading the frame to stack.
> 
> Please let us know if this approach works for you or not.

I think this is best kept inside the driver. However you might consider building something like __hci_cmd_sync() that is specific to your driver, but allows for a similar flow within ->setup().

Regards

Marcel

RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Bing Zhao]

Hi Marcel,

> > We can extend __hci_cmd_sync() function with a new parameter 'type'.
> > This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other drivers will pass in
> HCI_COMMAND_PKT.
> 
> That will actually not work. And I also do not want to do that. The __hci_cmd_sync() is for real HCI
> packets. That means types 0x01 and 0x04 only. They need to adhere to the HCI flow control mechanism
> for commands.

I see.

> 
> > Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion before downloading the frame to
> firmware. And the MRVL_VENDOR_PKT frame from firmware will be replaced with HCI_VENDOR_PKT while
> uploading the frame to stack.
> >
> > Please let us know if this approach works for you or not.
> 
> I think this is best kept inside the driver. However you might consider building something like
> __hci_cmd_sync() that is specific to your driver, but allows for a similar flow within ->setup().

Sure, we will consider building a function in the driver to handle this.

Thanks,
Bing

RE: [PATCH v5 2/2] Bluetooth: btmrvl: add calibration data download support

[Bing Zhao]

Hi Marcel,

> > We can extend __hci_cmd_sync() function with a new parameter 'type'.
> > This way we can pass HCI_VENDOR_PKT into __hci_cmd_sync(), while other =
drivers will pass in
> HCI_COMMAND_PKT.
>=20
> That will actually not work. And I also do not want to do that. The __hci=
_cmd_sync() is for real HCI
> packets. That means types 0x01 and 0x04 only. They need to adhere to the =
HCI flow control mechanism
> for commands.

I see.

>=20
> > Our driver will make HCI_VENDOR_PKT -> MRVL_VENDOR_PKT conversion befor=
e downloading the frame to
> firmware. And the MRVL_VENDOR_PKT frame from firmware will be replaced wi=
th HCI_VENDOR_PKT while
> uploading the frame to stack.
> >
> > Please let us know if this approach works for you or not.
>=20
> I think this is best kept inside the driver. However you might consider b=
uilding something like
> __hci_cmd_sync() that is specific to your driver, but allows for a simila=
r flow within ->setup().

Sure, we will consider building a function in the driver to handle this.

Thanks,
Bing

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Johan,

> You're right that we're missing the clearing of the HCI_SETUP flag for
> such a scenario. Could you try the attached patch. It should fix the

We have tested your patch. Yes, it fixes the problem. Thanks!

> issue. One problem that it does have is that if the HCIDEVUP ioctl path
> goes through before hci_power_on gets called we will never notify mgmt
> of the adapter. However, that might be acceptable here since if you're
> using HCIDEVUP like this it seems it's not a mgmt based system anyway.

Do you think the following change helps?

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3d9f02b..24814b0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1665,7 +1665,7 @@ static void hci_power_on(struct work_struct *work)
 	BT_DBG("%s", hdev->name);
 
 	err = hci_dev_open(hdev->id);
-	if (err < 0) {
+	if (err < 0 && err != -EALREADY) {
 		mgmt_set_powered_failed(hdev, err);
 		return;
 	}

Thanks,
Bing

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Johan,

> You're right that we're missing the clearing of the HCI_SETUP flag for
> such a scenario. Could you try the attached patch. It should fix the

We have tested your patch. Yes, it fixes the problem. Thanks!

> issue. One problem that it does have is that if the HCIDEVUP ioctl path
> goes through before hci_power_on gets called we will never notify mgmt
> of the adapter. However, that might be acceptable here since if you're
> using HCIDEVUP like this it seems it's not a mgmt based system anyway.

Do you think the following change helps?

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3d9f02b..24814b0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1665,7 +1665,7 @@ static void hci_power_on(struct work_struct *work)
 	BT_DBG("%s", hdev->name);
=20
 	err =3D hci_dev_open(hdev->id);
-	if (err < 0) {
+	if (err < 0 && err !=3D -EALREADY) {
 		mgmt_set_powered_failed(hdev, err);
 		return;
 	}

Thanks,
Bing

Re: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Marcel Holtmann]

Hi Bing,

>> You're right that we're missing the clearing of the HCI_SETUP flag for
>> such a scenario. Could you try the attached patch. It should fix the
> 
> We have tested your patch. Yes, it fixes the problem. Thanks!

then lets get a proper version with full commit message explaining the issue merged upstream. As I said, this is a real bug we need to fix.

>> issue. One problem that it does have is that if the HCIDEVUP ioctl path
>> goes through before hci_power_on gets called we will never notify mgmt
>> of the adapter. However, that might be acceptable here since if you're
>> using HCIDEVUP like this it seems it's not a mgmt based system anyway.
> 
> Do you think the following change helps?
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 3d9f02b..24814b0 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1665,7 +1665,7 @@ static void hci_power_on(struct work_struct *work)
> 	BT_DBG("%s", hdev->name);
> 
> 	err = hci_dev_open(hdev->id);
> -	if (err < 0) {
> +	if (err < 0 && err != -EALREADY) {
> 		mgmt_set_powered_failed(hdev, err);
> 		return;
> 	}

I am more and more convinced that we might just need to disable certain ioctl in case there is a mgmt socket/client present and HCI_MGMT is set. Trying to be graceful with legacy ioctl that BlueZ 5 will never use anymore seems to come at a too high cost. I rather fail on the legacy commands with a proper error than having mgmt interface behave inconsistent.

We might actually be able to keep HCIDEVUP and HCIDEVDOWN around, but then they need to behave like mgmt set powered command. So both commands need to go through the same mgmt engine for this.

Regards

Marcel

Re: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Johan Hedberg]

Hi Marcel & Bing,

On Thu, Sep 26, 2013, Marcel Holtmann wrote:
> >> You're right that we're missing the clearing of the HCI_SETUP flag for
> >> such a scenario. Could you try the attached patch. It should fix the
> > 
> > We have tested your patch. Yes, it fixes the problem. Thanks!
> 
> then lets get a proper version with full commit message explaining the
> issue merged upstream. As I said, this is a real bug we need to fix.

I've just sent a new patch set titled "[PATCH 0/2] Bluetooth: Fix
hci_dev_open race condition". Bing, could you please test this with your
original setup so we ensure that the issue is still properly handled.

Johan

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Johan,

> Hi Marcel & Bing,
> 
> On Thu, Sep 26, 2013, Marcel Holtmann wrote:
> > >> You're right that we're missing the clearing of the HCI_SETUP flag
> > >> for such a scenario. Could you try the attached patch. It should
> > >> fix the
> > >
> > > We have tested your patch. Yes, it fixes the problem. Thanks!
> >
> > then lets get a proper version with full commit message explaining the
> > issue merged upstream. As I said, this is a real bug we need to fix.
> 
> I've just sent a new patch set titled "[PATCH 0/2] Bluetooth: Fix
> hci_dev_open race condition". Bing, could you please test this with your
> original setup so we ensure that the issue is still properly handled.

We tested this new patch set with our original setup and the issue is not seen.

Thanks,
Bing

RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler

[Bing Zhao]

Hi Johan,

> Hi Marcel & Bing,
>=20
> On Thu, Sep 26, 2013, Marcel Holtmann wrote:
> > >> You're right that we're missing the clearing of the HCI_SETUP flag
> > >> for such a scenario. Could you try the attached patch. It should
> > >> fix the
> > >
> > > We have tested your patch. Yes, it fixes the problem. Thanks!
> >
> > then lets get a proper version with full commit message explaining the
> > issue merged upstream. As I said, this is a real bug we need to fix.
>=20
> I've just sent a new patch set titled "[PATCH 0/2] Bluetooth: Fix
> hci_dev_open race condition". Bing, could you please test this with your
> original setup so we ensure that the issue is still properly handled.

We tested this new patch set with our original setup and the issue is not s=
een.

Thanks,
Bing