Linux bluetooth development
 help / color / mirror / Atom feed
* Re: [PATCH 5/7] Bluetooth: Refactor hci_connect_le
From: Anderson Lizardo @ 2013-10-02  0:05 UTC (permalink / raw)
  To: Andre Guedes; +Cc: BlueZ development
In-Reply-To: <1380668636-30654-6-git-send-email-andre.guedes@openbossa.org>

Hi Guedes,

On Tue, Oct 1, 2013 at 7:03 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> +       /* If already exists a hci_conn object for the following connection
> +        * attempt, we simply update pending_sec_level and auth_type fields
> +        * and return the object found.
> +        */

Small textual improvement: "If a hci_conn object already exists [...]"

>         le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> -       if (!le) {
> -               le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> -               if (le)
> -                       return ERR_PTR(-EBUSY);
> -
> -               le = hci_conn_add(hdev, LE_LINK, dst);
> -               if (!le)
> -                       return ERR_PTR(-ENOMEM);
> -
> -               le->dst_type = bdaddr_to_le(dst_type);
> -               le->state = BT_CONNECT;
> -               le->out = true;
> -               le->link_mode |= HCI_LM_MASTER;
> -               le->sec_level = BT_SECURITY_LOW;
> -
> -               err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> -               if (err) {
> -                       hci_conn_del(le);
> -                       return ERR_PTR(err);
> -               }
> +       if (le) {
> +               le->pending_sec_level = sec_level;
> +               le->auth_type = auth_type;
> +               goto out;
>         }
>
> -       le->pending_sec_level = sec_level;
> +       /* Since the controller supports only one LE connection attempt at the
> +        * time, we return busy if there is any connection attempt running.
> +        */

s/at the time/at a time/
s/busy/EBUSY/

> +       le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> +       if (le)
> +               return ERR_PTR(-EBUSY);
> +
> +       le = hci_conn_add(hdev, LE_LINK, dst);
> +       if (!le)
> +               return ERR_PTR(-ENOMEM);
> +
> +       le->dst_type = bdaddr_to_le(dst_type);
> +       le->state = BT_CONNECT;
> +       le->out = true;
> +       le->link_mode |= HCI_LM_MASTER;
> +       le->sec_level = BT_SECURITY_LOW;
> +       le->pending_sec_level = BT_SECURITY_LOW;

I think the previous statement should be:

le->pending_sec_level = sec_level;

Otherwise, we are changing semantics.

>         le->auth_type = auth_type;
>
> -       hci_conn_hold(le);
> +       err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> +       if (err) {
> +               hci_conn_del(le);
> +               return ERR_PTR(err);
> +       }
>
> +out:
> +       hci_conn_hold(le);
>         return le;
>  }

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply

* Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler
From: Anderson Lizardo @ 2013-10-01 23:44 UTC (permalink / raw)
  To: Andre Guedes; +Cc: BlueZ development
In-Reply-To: <1380668636-30654-5-git-send-email-andre.guedes@openbossa.org>

Hi Guedes,

On Tue, Oct 1, 2013 at 7:03 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> This patch removes the hci_cs_le_create_conn event handler since this
> handling is now done in create_le_connection_complete() callback in
> hci_conn.c.

Did you mean "... now done in initiate_le_connection_complete()
callback in hci_core.c" ?

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply

* [PATCH BlueZ] monitor: Fix EIR Data Type / AD Type assigned numbers
From: João Paulo Rechi Vita @ 2013-10-01 23:04 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: João Paulo Rechi Vita

From: João Paulo Rechi Vita <jprvita@openbossa.org>

The values for Public Target Address and Random Target Address were
swapped. This information can be verified in the Bluetooth SIG Assigned
numbers webpage:

https://www.bluetooth.org/en-us/specification/assigned-numbers/generic-access-profile
---
 monitor/packet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor/packet.c b/monitor/packet.c
index ddd3742..942d3de 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -2002,8 +2002,8 @@ static void print_fec(uint8_t fec)
 #define BT_EIR_SERVICE_UUID16		0x14
 #define BT_EIR_SERVICE_UUID128		0x15
 #define BT_EIR_SERVICE_DATA		0x16
-#define BT_EIR_RANDOM_ADDRESS		0x17
-#define BT_EIR_PUBLIC_ADDRESS		0x18
+#define BT_EIR_PUBLIC_ADDRESS		0x17
+#define BT_EIR_RANDOM_ADDRESS		0x18
 #define BT_EIR_GAP_APPEARANCE		0x19
 #define BT_EIR_MANUFACTURER_DATA	0xff
 
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH 7/7] Bluetooth: Locking in hci_le_conn_complete_evt
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380668636-30654-1-git-send-email-andre.guedes@openbossa.org>

This patch moves hci_dev_lock and hci_dev_unlock calls to where they
are really required, reducing the critical region in hci_le_conn_
complete_evt function. hdev->lock is required only in hci_conn_del
and hci_conn_add call to protect concurrent add and remove operations
in hci_conn_hash list.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0e4a9f4..ffd5186 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3442,19 +3442,20 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
 
-	hci_dev_lock(hdev);
-
 	if (ev->status) {
 		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
 		if (!conn)
-			goto unlock;
+			return;
 
 		mgmt_connect_failed(hdev, &conn->dst, conn->type,
 				    conn->dst_type, ev->status);
 		hci_proto_connect_cfm(conn, ev->status);
 		conn->state = BT_CLOSED;
+
+		hci_dev_lock(hdev);
 		hci_conn_del(conn);
-		goto unlock;
+		hci_dev_unlock(hdev);
+		return;
 	}
 
 	switch (ev->role) {
@@ -3466,10 +3467,13 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		 * object.
 		 */
 		if (!conn) {
+			hci_dev_lock(hdev);
 			conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
+			hci_dev_unlock(hdev);
+
 			if (!conn) {
 				BT_ERR("No memory for new connection");
-				goto unlock;
+				return;
 			}
 
 			conn->out = true;
@@ -3480,10 +3484,13 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		break;
 
 	case LE_CONN_ROLE_SLAVE:
+		hci_dev_lock(hdev);
 		conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
+		hci_dev_unlock(hdev);
+
 		if (!conn) {
 			BT_ERR("No memory for new connection");
-			goto unlock;
+			return;
 		}
 
 		conn->dst_type = ev->bdaddr_type;
@@ -3492,7 +3499,7 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	default:
 		BT_ERR("Used reserved Role parameter %d", ev->role);
-		goto unlock;
+		return;
 	}
 
 	if (!test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
@@ -3505,9 +3512,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_conn_add_sysfs(conn);
 
 	hci_proto_connect_cfm(conn, ev->status);
-
-unlock:
-	hci_dev_unlock(hdev);
 }
 
 static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
-- 
1.8.4


^ permalink raw reply related

* [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380668636-30654-1-git-send-email-andre.guedes@openbossa.org>

This patch does some code refactorig in LE Connection Complete HCI
event handler. It basically adds a switch statement to separate new
master connection code from new slave connection code.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_event.c   | 55 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7ede266..8c98f60 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
 
 /* Low energy meta events */
 #define LE_CONN_ROLE_MASTER	0x00
+#define LE_CONN_ROLE_SLAVE	0x01
 
 #define HCI_EV_LE_CONN_COMPLETE		0x01
 struct hci_ev_le_conn_complete {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1d1ffa6..0e4a9f4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_dev_lock(hdev);
 
-	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
-	if (!conn) {
+	if (ev->status) {
+		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+		if (!conn)
+			goto unlock;
+
+		mgmt_connect_failed(hdev, &conn->dst, conn->type,
+				    conn->dst_type, ev->status);
+		hci_proto_connect_cfm(conn, ev->status);
+		conn->state = BT_CLOSED;
+		hci_conn_del(conn);
+		goto unlock;
+	}
+
+	switch (ev->role) {
+	case LE_CONN_ROLE_MASTER:
+		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
+		/* If there is no hci_conn object with the given address, it
+		 * means this new connection was triggered through HCI socket
+		 * interface. For that case, we should create a new hci_conn
+		 * object.
+		 */
+		if (!conn) {
+			conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
+			if (!conn) {
+				BT_ERR("No memory for new connection");
+				goto unlock;
+			}
+
+			conn->out = true;
+			conn->link_mode |= HCI_LM_MASTER;
+			conn->sec_level = BT_SECURITY_LOW;
+			conn->dst_type = ev->bdaddr_type;
+		}
+		break;
+
+	case LE_CONN_ROLE_SLAVE:
 		conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
 		if (!conn) {
 			BT_ERR("No memory for new connection");
@@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		}
 
 		conn->dst_type = ev->bdaddr_type;
+		conn->sec_level = BT_SECURITY_LOW;
+		break;
 
-		if (ev->role == LE_CONN_ROLE_MASTER) {
-			conn->out = true;
-			conn->link_mode |= HCI_LM_MASTER;
-		}
-	}
-
-	if (ev->status) {
-		mgmt_connect_failed(hdev, &conn->dst, conn->type,
-				    conn->dst_type, ev->status);
-		hci_proto_connect_cfm(conn, ev->status);
-		conn->state = BT_CLOSED;
-		hci_conn_del(conn);
+	default:
+		BT_ERR("Used reserved Role parameter %d", ev->role);
 		goto unlock;
 	}
 
@@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
 				      conn->dst_type, 0, NULL, 0, NULL);
 
-	conn->sec_level = BT_SECURITY_LOW;
 	conn->handle = __le16_to_cpu(ev->handle);
 	conn->state = BT_CONNECTED;
 
-- 
1.8.4


^ permalink raw reply related

* [PATCH 5/7] Bluetooth: Refactor hci_connect_le
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380668636-30654-1-git-send-email-andre.guedes@openbossa.org>

This patch organizes hci_connect_le() logic and adds extra comments
to improve code's readability.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c | 54 ++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b89522f..d2409dc 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -531,34 +531,44 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
 		return ERR_PTR(-ENOTSUPP);
 
+	/* If already exists a hci_conn object for the following connection
+	 * attempt, we simply update pending_sec_level and auth_type fields
+	 * and return the object found.
+	 */
 	le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
-	if (!le) {
-		le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
-		if (le)
-			return ERR_PTR(-EBUSY);
-
-		le = hci_conn_add(hdev, LE_LINK, dst);
-		if (!le)
-			return ERR_PTR(-ENOMEM);
-
-		le->dst_type = bdaddr_to_le(dst_type);
-		le->state = BT_CONNECT;
-		le->out = true;
-		le->link_mode |= HCI_LM_MASTER;
-		le->sec_level = BT_SECURITY_LOW;
-
-		err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
-		if (err) {
-			hci_conn_del(le);
-			return ERR_PTR(err);
-		}
+	if (le) {
+		le->pending_sec_level = sec_level;
+		le->auth_type = auth_type;
+		goto out;
 	}
 
-	le->pending_sec_level = sec_level;
+	/* Since the controller supports only one LE connection attempt at the
+	 * time, we return busy if there is any connection attempt running.
+	 */
+	le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+	if (le)
+		return ERR_PTR(-EBUSY);
+
+	le = hci_conn_add(hdev, LE_LINK, dst);
+	if (!le)
+		return ERR_PTR(-ENOMEM);
+
+	le->dst_type = bdaddr_to_le(dst_type);
+	le->state = BT_CONNECT;
+	le->out = true;
+	le->link_mode |= HCI_LM_MASTER;
+	le->sec_level = BT_SECURITY_LOW;
+	le->pending_sec_level = BT_SECURITY_LOW;
 	le->auth_type = auth_type;
 
-	hci_conn_hold(le);
+	err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
+	if (err) {
+		hci_conn_del(le);
+		return ERR_PTR(err);
+	}
 
+out:
+	hci_conn_hold(le);
 	return le;
 }
 
-- 
1.8.4


^ permalink raw reply related

* [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380668636-30654-1-git-send-email-andre.guedes@openbossa.org>

This patch removes the hci_cs_le_create_conn event handler since this
handling is now done in create_le_connection_complete() callback in
hci_conn.c.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d171c04b..1d1ffa6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1465,33 +1465,6 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
 	hci_dev_unlock(hdev);
 }
 
-static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
-{
-	struct hci_conn *conn;
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	if (status) {
-		hci_dev_lock(hdev);
-
-		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
-		if (!conn) {
-			hci_dev_unlock(hdev);
-			return;
-		}
-
-		BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, conn);
-
-		conn->state = BT_CLOSED;
-		mgmt_connect_failed(hdev, &conn->dst, conn->type,
-				    conn->dst_type, status);
-		hci_proto_connect_cfm(conn, status);
-		hci_conn_del(conn);
-
-		hci_dev_unlock(hdev);
-	}
-}
-
 static void hci_cs_create_phylink(struct hci_dev *hdev, u8 status)
 {
 	struct hci_cp_create_phy_link *cp;
@@ -2342,10 +2315,6 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cs_disconnect(hdev, ev->status);
 		break;
 
-	case HCI_OP_LE_CREATE_CONN:
-		hci_cs_le_create_conn(hdev, ev->status);
-		break;
-
 	case HCI_OP_CREATE_PHY_LINK:
 		hci_cs_create_phylink(hdev, ev->status);
 		break;
-- 
1.8.4


^ permalink raw reply related

* [PATCH 3/7] Bluetooth: Remove hci_le_create_connection
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380668636-30654-1-git-send-email-andre.guedes@openbossa.org>

Since we now use hci_initiate_le_connection() helper for creating new
LE connections, we can safely remove hci_le_create_connection().

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 24d1a0a..b89522f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -49,25 +49,6 @@ static const struct sco_param sco_param_wideband[] = {
 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008 }, /* T1 */
 };
 
-static void hci_le_create_connection(struct hci_conn *conn)
-{
-	struct hci_dev *hdev = conn->hdev;
-	struct hci_cp_le_create_conn cp;
-
-	memset(&cp, 0, sizeof(cp));
-	cp.scan_interval = __constant_cpu_to_le16(0x0060);
-	cp.scan_window = __constant_cpu_to_le16(0x0030);
-	bacpy(&cp.peer_addr, &conn->dst);
-	cp.peer_addr_type = conn->dst_type;
-	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
-	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
-	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
-	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
-	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
-
-	hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
-}
-
 static void hci_le_create_connection_cancel(struct hci_conn *conn)
 {
 	hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
-- 
1.8.4


^ permalink raw reply related

* [PATCH 2/7] Bluetooth: Use HCI request for LE connection
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380668636-30654-1-git-send-email-andre.guedes@openbossa.org>

This patch adds a new helper for initiating LE conneciton which uses
the HCI request framework. This patch also changes the hci_connect_le()
so it uses the new helper instead of the old hci_le_create_connection().

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_conn.c         |  7 +++++-
 net/bluetooth/hci_core.c         | 46 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 26cc9f7..6aa172c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1216,6 +1216,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 
 u8 bdaddr_to_le(u8 bdaddr_type);
 
+int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type);
+
 #define SCO_AIRMODE_MASK       0x0003
 #define SCO_AIRMODE_CVSD       0x0000
 #define SCO_AIRMODE_TRANSP     0x0003
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index f473605..24d1a0a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -545,6 +545,7 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 				    u8 dst_type, u8 sec_level, u8 auth_type)
 {
 	struct hci_conn *le;
+	int err;
 
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
 		return ERR_PTR(-ENOTSUPP);
@@ -565,7 +566,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 		le->link_mode |= HCI_LM_MASTER;
 		le->sec_level = BT_SECURITY_LOW;
 
-		hci_le_create_connection(le);
+		err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
+		if (err) {
+			hci_conn_del(le);
+			return ERR_PTR(err);
+		}
 	}
 
 	le->pending_sec_level = sec_level;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4549b5c..51c1796 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3631,3 +3631,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
 		return ADDR_LE_DEV_RANDOM;
 	}
 }
+
+static void initiate_le_connection_complete(struct hci_dev *hdev, u8 status)
+{
+	struct hci_conn *conn;
+
+	if (status == 0)
+		return;
+
+	BT_ERR("HCI request failed to initiate LE connection: status 0x%2.2x",
+	       status);
+
+	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+	if (!conn)
+		return;
+
+	mgmt_connect_failed(hdev, &conn->dst, conn->type, conn->dst_type,
+			    status);
+
+	hci_proto_connect_cfm(conn, status);
+
+	hci_dev_lock(hdev);
+	hci_conn_del(conn);
+	hci_dev_unlock(hdev);
+}
+
+int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
+{
+	struct hci_cp_le_create_conn cp;
+	struct hci_request req;
+
+	hci_req_init(&req, hdev);
+
+	memset(&cp, 0, sizeof(cp));
+	cp.scan_interval = __constant_cpu_to_le16(0x0060);
+	cp.scan_window = __constant_cpu_to_le16(0x0030);
+	bacpy(&cp.peer_addr, addr);
+	cp.peer_addr_type = type;
+	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
+	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
+	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
+	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
+	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
+	hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
+
+	return hci_req_run(&req, initiate_le_connection_complete);
+}
-- 
1.8.4


^ permalink raw reply related

* [PATCH 1/7] Bluetooth: Initialize hci_conn fields in hci_connect_le
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380668636-30654-1-git-send-email-andre.guedes@openbossa.org>

This patch moves some hci_conn fields initialization from hci_le_
create_connection() to hci_connect_le(). It makes more sense to
initialize these fields within the function that creates the hci_
conn object.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d2380e0..f473605 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -54,11 +54,6 @@ static void hci_le_create_connection(struct hci_conn *conn)
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_le_create_conn cp;
 
-	conn->state = BT_CONNECT;
-	conn->out = true;
-	conn->link_mode |= HCI_LM_MASTER;
-	conn->sec_level = BT_SECURITY_LOW;
-
 	memset(&cp, 0, sizeof(cp));
 	cp.scan_interval = __constant_cpu_to_le16(0x0060);
 	cp.scan_window = __constant_cpu_to_le16(0x0030);
@@ -565,6 +560,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 			return ERR_PTR(-ENOMEM);
 
 		le->dst_type = bdaddr_to_le(dst_type);
+		le->state = BT_CONNECT;
+		le->out = true;
+		le->link_mode |= HCI_LM_MASTER;
+		le->sec_level = BT_SECURITY_LOW;
+
 		hci_le_create_connection(le);
 	}
 
-- 
1.8.4


^ permalink raw reply related

* [PATCH 0/7] LE connection refactoring and fixes
From: Andre Guedes @ 2013-10-01 23:03 UTC (permalink / raw)
  To: linux-bluetooth

Hi all,

This patchset contains several refactoring and fixes for LE connection code.
These patches are part of the bigger LE auto connection patchset. As discussed
in New Orleans, I'm sending these patches since they can already be applied to
bluetooth-next.

Also, I'm still implementing the suggestions collected during the Wireless
Summit and I'll send the LE auto connection patchset soon.

Regards,

Andre


Andre Guedes (7):
  Bluetooth: Initialize hci_conn fields in hci_connect_le
  Bluetooth: Use HCI request for LE connection
  Bluetooth: Remove hci_le_create_connection
  Bluetooth: Remove hci_cs_le_create_conn event handler
  Bluetooth: Refactor hci_connect_le
  Bluetooth: Refactor LE Connection Complete HCI event handler
  Bluetooth: Locking in hci_le_conn_complete_evt

 include/net/bluetooth/hci.h      |   1 +
 include/net/bluetooth/hci_core.h |   2 +
 net/bluetooth/hci_conn.c         |  70 +++++++++++++--------------
 net/bluetooth/hci_core.c         |  46 ++++++++++++++++++
 net/bluetooth/hci_event.c        | 102 +++++++++++++++++++--------------------
 5 files changed, 132 insertions(+), 89 deletions(-)

-- 
1.8.4


^ permalink raw reply

* [PATCH v2 2/2] Bluetooth: Fix workqueue synchronization in hci_dev_open
From: johan.hedberg @ 2013-10-01 19:44 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380656690-22246-1-git-send-email-johan.hedberg@gmail.com>

From: Johan Hedberg <johan.hedberg@intel.com>

When hci_sock.c calls hci_dev_open it needs to ensure that there isn't
pending work in progress, such as that which is scheduled for the
initial setup procedure or the one for automatically powering off after
the setup procedure. This adds the necessary calls to ensure that any
previously scheduled work is completed before attempting to call
hci_dev_do_open.

This patch fixes a race with old user space versions where we might
receive a HCIDEVUP ioctl before the setup procedure has been completed.
When that happens the setup procedures callback may fail early and leave
the device in an inconsistent state, causing e.g. the setup callback to
be (incorrectly) called more than once.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fc63e78..a573aa2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1227,6 +1227,16 @@ int hci_dev_open(__u16 dev)
 	if (!hdev)
 		return -ENODEV;
 
+	/* We need to ensure that no other power on/off work is pending
+	 * before proceeding to call hci_dev_do_open. This is
+	 * particularly important if the setup procedure has not yet
+	 * completed.
+	 */
+	if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags))
+		cancel_delayed_work(&hdev->power_off);
+
+	flush_workqueue(hdev->req_workqueue);
+
 	err = hci_dev_do_open(hdev);
 
 	hci_dev_put(hdev);
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH v2 1/2] Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function
From: johan.hedberg @ 2013-10-01 19:44 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1380656690-22246-1-git-send-email-johan.hedberg@gmail.com>

From: Johan Hedberg <johan.hedberg@intel.com>

The requirements of an external call to hci_dev_open from hci_sock.c are
different to that from within hci_core.c. In the former case we want to
flush any pending work in hdev->req_workqueue whereas in the latter we
don't (since there we are already calling from within the workqueue
itself). This patch does the necessary refactoring to a separate
hci_dev_do_open function (analogous to hci_dev_do_close) but does not
yet introduce the synchronizations relating to the workqueue usage.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/hci_core.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 1b66547..fc63e78 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1126,17 +1126,10 @@ void hci_update_ad(struct hci_request *req)
 	hci_req_add(req, HCI_OP_LE_SET_ADV_DATA, sizeof(cp), &cp);
 }
 
-/* ---- HCI ioctl helpers ---- */
-
-int hci_dev_open(__u16 dev)
+static int hci_dev_do_open(struct hci_dev *hdev)
 {
-	struct hci_dev *hdev;
 	int ret = 0;
 
-	hdev = hci_dev_get(dev);
-	if (!hdev)
-		return -ENODEV;
-
 	BT_DBG("%s %p", hdev->name, hdev);
 
 	hci_req_lock(hdev);
@@ -1220,10 +1213,27 @@ int hci_dev_open(__u16 dev)
 
 done:
 	hci_req_unlock(hdev);
-	hci_dev_put(hdev);
 	return ret;
 }
 
+/* ---- HCI ioctl helpers ---- */
+
+int hci_dev_open(__u16 dev)
+{
+	struct hci_dev *hdev;
+	int err;
+
+	hdev = hci_dev_get(dev);
+	if (!hdev)
+		return -ENODEV;
+
+	err = hci_dev_do_open(hdev);
+
+	hci_dev_put(hdev);
+
+	return err;
+}
+
 static int hci_dev_do_close(struct hci_dev *hdev)
 {
 	BT_DBG("%s %p", hdev->name, hdev);
@@ -1592,7 +1602,7 @@ static void hci_power_on(struct work_struct *work)
 
 	BT_DBG("%s", hdev->name);
 
-	err = hci_dev_open(hdev->id);
+	err = hci_dev_do_open(hdev);
 	if (err < 0) {
 		mgmt_set_powered_failed(hdev, err);
 		return;
-- 
1.8.3.1


^ permalink raw reply related

* [PATCH v2 0/2] Bluetooth: Fix hci_dev_open race condition
From: johan.hedberg @ 2013-10-01 19:44 UTC (permalink / raw)
  To: linux-bluetooth

Hi,

Here's an updated set including fixes based on feedback. The patches
should go to bluetooth.git, but I'm not sure how important it is to try
to get them to the stable tree since we haven't had this issue reported
with the Intel setup procedure which (afaik) should be the only one that
could be affected in older kernels.

Johan

----------------------------------------------------------------
Johan Hedberg (2):
      Bluetooth: Refactor hci_dev_open to a separate hci_dev_do_open function
      Bluetooth: Fix workqueue synchronization in hci_dev_open

 net/bluetooth/hci_core.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)


^ permalink raw reply

* [PATCH v6 4/4] Bluetooth: btmrvl: add calibration data download support
From: Bing Zhao @ 2013-10-01 19:19 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, linux-wireless,
	Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar, Bing Zhao
In-Reply-To: <1380655155-10007-1-git-send-email-bzhao@marvell.com>

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)
v6: Remove redundant label 'done' and 'cfg' check, and a new line character check
    (Mike Frysinger)
    Use btmrvl_send_sync_cmd() for downloading calibration data.
    (Marcel Holtmann)

 drivers/bluetooth/btmrvl_drv.h  |   8 +++
 drivers/bluetooth/btmrvl_main.c | 116 ++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btmrvl_sdio.c |   9 +++-
 drivers/bluetooth/btmrvl_sdio.h |   2 +
 4 files changed, 134 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index 42f7028..f9d1833 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;
 
@@ -91,6 +95,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
@@ -116,6 +121,9 @@ 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_event {
 	u8 ec;		/* event counter */
 	u8 length;
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index e0ae1f4..6e7bd4e 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -432,12 +432,128 @@ 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++;
+			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)
+{
+	int i, ret;
+	u8 data[BT_CMD_DATA_SIZE];
+
+	data[0] = 0x00;
+	data[1] = 0x00;
+	data[2] = 0x00;
+	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++)
+		data[i] = config_data[(i / 4) * 8 - 1 - i];
+
+	print_hex_dump_bytes("Calibration data: ",
+			     DUMP_PREFIX_OFFSET, data, BT_CMD_DATA_SIZE);
+
+	ret = btmrvl_send_sync_cmd(priv, BT_CMD_LOAD_CONFIG_DATA, data,
+				   BT_CMD_DATA_SIZE);
+	if (ret)
+		BT_ERR("Failed to download caibration data\n");
+
+	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);
+		return 0;
+	}
+
+	ret = btmrvl_process_cal_cfg(priv, (u8 *)cfg->data, cfg->size);
+	release_firmware(cfg);
+	return ret;
+}
+
 static int btmrvl_setup(struct hci_dev *hdev)
 {
 	struct btmrvl_private *priv = hci_get_drvdata(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 5b70bcb..332475e 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.8.0

^ permalink raw reply related

* [PATCH v6 3/4] Bluetooth: btmrvl: add setup handler
From: Bing Zhao @ 2013-10-01 19:19 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, linux-wireless,
	Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar, Bing Zhao
In-Reply-To: <1380655155-10007-1-git-send-email-bzhao@marvell.com>

From: Amitkumar Karwar <akarwar@marvell.com>

Move initialization code to hdev's setup handler.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
v6: remove setup_done variable (Marcel Holtmann)
    This change requires a fix in hci_core for hci_setup.
v5: make use of hdev's setup handler (Marcel Holtmann)

 drivers/bluetooth/btmrvl_main.c | 18 ++++++++++++++++--
 drivers/bluetooth/btmrvl_sdio.c |  6 ------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index a4da7c8..e0ae1f4 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -432,6 +432,21 @@ 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);
+
+	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);
+
+	return 0;
+}
+
 /*
  * This function handles the event generated by firmware, rx data
  * received from firmware, and tx data sent from kernel.
@@ -525,8 +540,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 00da6df..5b70bcb 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.8.0

^ permalink raw reply related

* [PATCH v6 2/4] Bluetooth: btmrvl: get rid of struct btmrvl_cmd
From: Bing Zhao @ 2013-10-01 19:19 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, linux-wireless,
	Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar, Bing Zhao
In-Reply-To: <1380655155-10007-1-git-send-email-bzhao@marvell.com>

From: Amitkumar Karwar <akarwar@marvell.com>

Replace this proprietary structure with the standard one
(struct hci_command_hdr).

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
v6: remove proprietary struct btmrvl_cmd

 drivers/bluetooth/btmrvl_drv.h  |  6 ------
 drivers/bluetooth/btmrvl_main.c | 12 ++++++------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index 27068d1..42f7028 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -116,12 +116,6 @@ struct btmrvl_private {
 #define PS_SLEEP			0x01
 #define PS_AWAKE			0x00
 
-struct btmrvl_cmd {
-	__le16 ocf_ogf;
-	u8 length;
-	u8 data[4];
-} __packed;
-
 struct btmrvl_event {
 	u8 ec;		/* event counter */
 	u8 length;
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index d9d4229..a4da7c8 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -170,20 +170,20 @@ static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
 				const void *param, u8 len)
 {
 	struct sk_buff *skb;
-	struct btmrvl_cmd *cmd;
+	struct hci_command_hdr *hdr;
 
-	skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
+	skb = bt_skb_alloc(HCI_COMMAND_HDR_SIZE + len, GFP_ATOMIC);
 	if (skb == NULL) {
 		BT_ERR("No free skb");
 		return -ENOMEM;
 	}
 
-	cmd = (struct btmrvl_cmd *) skb_put(skb, sizeof(*cmd));
-	cmd->ocf_ogf = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
-	cmd->length = len;
+	hdr = (struct hci_command_hdr *)skb_put(skb, HCI_COMMAND_HDR_SIZE);
+	hdr->opcode = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
+	hdr->plen = len;
 
 	if (len)
-		memcpy(cmd->data, param, len);
+		memcpy(skb_put(skb, len), param, len);
 
 	bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
 
-- 
1.8.0

^ permalink raw reply related

* [PATCH v6 1/4] Bluetooth: btmrvl: add btmrvl_send_sync_cmd() function
From: Bing Zhao @ 2013-10-01 19:19 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, linux-wireless,
	Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar, Bing Zhao
In-Reply-To: <1380655155-10007-1-git-send-email-bzhao@marvell.com>

From: Amitkumar Karwar <akarwar@marvell.com>

Command preparation code is used multiple times. This patch
separate out this common code and create btmrvl_send_sync_cmd()
function.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
v6: separate out common code

 drivers/bluetooth/btmrvl_main.c | 129 +++++++++++++---------------------------
 1 file changed, 41 insertions(+), 88 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 9a9f518..d9d4229 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -57,8 +57,7 @@ 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 (priv->btmrvl_dev.sendcmdflag) {
 			priv->btmrvl_dev.sendcmdflag = false;
 			priv->adapter->cmd_complete = true;
 			wake_up_interruptible(&priv->adapter->cmd_wait_q);
@@ -116,7 +115,6 @@ int btmrvl_process_event(struct btmrvl_private *priv, struct sk_buff *skb)
 			adapter->hs_state = HS_ACTIVATED;
 			if (adapter->psmode)
 				adapter->ps_state = PS_SLEEP;
-			wake_up_interruptible(&adapter->cmd_wait_q);
 			BT_DBG("HS ACTIVATED!");
 		} else {
 			BT_DBG("HS Enable failed");
@@ -168,11 +166,11 @@ exit:
 }
 EXPORT_SYMBOL_GPL(btmrvl_process_event);
 
-int btmrvl_send_module_cfg_cmd(struct btmrvl_private *priv, int subcmd)
+static int btmrvl_send_sync_cmd(struct btmrvl_private *priv, u16 cmd_no,
+				const void *param, u8 len)
 {
 	struct sk_buff *skb;
 	struct btmrvl_cmd *cmd;
-	int ret = 0;
 
 	skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
 	if (skb == NULL) {
@@ -181,9 +179,11 @@ int btmrvl_send_module_cfg_cmd(struct btmrvl_private *priv, int subcmd)
 	}
 
 	cmd = (struct btmrvl_cmd *) skb_put(skb, sizeof(*cmd));
-	cmd->ocf_ogf = cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_MODULE_CFG_REQ));
-	cmd->length = 1;
-	cmd->data[0] = subcmd;
+	cmd->ocf_ogf = cpu_to_le16(hci_opcode_pack(OGF, cmd_no));
+	cmd->length = len;
+
+	if (len)
+		memcpy(cmd->data, param, len);
 
 	bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
 
@@ -194,19 +194,23 @@ int btmrvl_send_module_cfg_cmd(struct btmrvl_private *priv, int subcmd)
 
 	priv->adapter->cmd_complete = false;
 
-	BT_DBG("Queue module cfg Command");
-
 	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))) {
-		ret = -ETIMEDOUT;
-		BT_ERR("module_cfg_cmd(%x): timeout: %d",
-					subcmd, priv->btmrvl_dev.sendcmdflag);
-	}
+				msecs_to_jiffies(WAIT_UNTIL_CMD_RESP)))
+		return -ETIMEDOUT;
 
-	BT_DBG("module cfg Command done");
+	return 0;
+}
+
+int btmrvl_send_module_cfg_cmd(struct btmrvl_private *priv, int subcmd)
+{
+	int ret;
+
+	ret = btmrvl_send_sync_cmd(priv, BT_CMD_MODULE_CFG_REQ, &subcmd, 1);
+	if (ret)
+		BT_ERR("module_cfg_cmd(%x) failed\n", subcmd);
 
 	return ret;
 }
@@ -214,61 +218,36 @@ EXPORT_SYMBOL_GPL(btmrvl_send_module_cfg_cmd);
 
 int btmrvl_send_hscfg_cmd(struct btmrvl_private *priv)
 {
-	struct sk_buff *skb;
-	struct btmrvl_cmd *cmd;
-
-	skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
-	if (!skb) {
-		BT_ERR("No free skb");
-		return -ENOMEM;
-	}
-
-	cmd = (struct btmrvl_cmd *) skb_put(skb, sizeof(*cmd));
-	cmd->ocf_ogf = cpu_to_le16(hci_opcode_pack(OGF,
-						   BT_CMD_HOST_SLEEP_CONFIG));
-	cmd->length = 2;
-	cmd->data[0] = (priv->btmrvl_dev.gpio_gap & 0xff00) >> 8;
-	cmd->data[1] = (u8) (priv->btmrvl_dev.gpio_gap & 0x00ff);
+	int ret;
+	u8 param[2];
 
-	bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
+	param[0] = (priv->btmrvl_dev.gpio_gap & 0xff00) >> 8;
+	param[1] = (u8) (priv->btmrvl_dev.gpio_gap & 0x00ff);
 
-	skb->dev = (void *) priv->btmrvl_dev.hcidev;
-	skb_queue_head(&priv->adapter->tx_queue, skb);
+	BT_DBG("Sending HSCFG Command, gpio=0x%x, gap=0x%x",
+	       param[0], param[1]);
 
-	BT_DBG("Queue HSCFG Command, gpio=0x%x, gap=0x%x", cmd->data[0],
-	       cmd->data[1]);
+	ret = btmrvl_send_sync_cmd(priv, BT_CMD_HOST_SLEEP_CONFIG, param, 2);
+	if (ret)
+		BT_ERR("HSCFG command failed\n");
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(btmrvl_send_hscfg_cmd);
 
 int btmrvl_enable_ps(struct btmrvl_private *priv)
 {
-	struct sk_buff *skb;
-	struct btmrvl_cmd *cmd;
-
-	skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
-	if (skb == NULL) {
-		BT_ERR("No free skb");
-		return -ENOMEM;
-	}
-
-	cmd = (struct btmrvl_cmd *) skb_put(skb, sizeof(*cmd));
-	cmd->ocf_ogf = cpu_to_le16(hci_opcode_pack(OGF,
-					BT_CMD_AUTO_SLEEP_MODE));
-	cmd->length = 1;
+	int ret;
+	u8 param;
 
 	if (priv->btmrvl_dev.psmode)
-		cmd->data[0] = BT_PS_ENABLE;
+		param = BT_PS_ENABLE;
 	else
-		cmd->data[0] = BT_PS_DISABLE;
-
-	bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
-
-	skb->dev = (void *) priv->btmrvl_dev.hcidev;
-	skb_queue_head(&priv->adapter->tx_queue, skb);
+		param = BT_PS_DISABLE;
 
-	BT_DBG("Queue PSMODE Command:%d", cmd->data[0]);
+	ret = btmrvl_send_sync_cmd(priv, BT_CMD_AUTO_SLEEP_MODE, &param, 1);
+	if (ret)
+		BT_ERR("PSMODE command failed\n");
 
 	return 0;
 }
@@ -276,37 +255,11 @@ EXPORT_SYMBOL_GPL(btmrvl_enable_ps);
 
 int btmrvl_enable_hs(struct btmrvl_private *priv)
 {
-	struct sk_buff *skb;
-	struct btmrvl_cmd *cmd;
-	int ret = 0;
-
-	skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
-	if (skb == NULL) {
-		BT_ERR("No free skb");
-		return -ENOMEM;
-	}
-
-	cmd = (struct btmrvl_cmd *) skb_put(skb, sizeof(*cmd));
-	cmd->ocf_ogf = cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_HOST_SLEEP_ENABLE));
-	cmd->length = 0;
-
-	bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
-
-	skb->dev = (void *) priv->btmrvl_dev.hcidev;
-	skb_queue_head(&priv->adapter->tx_queue, skb);
-
-	BT_DBG("Queue hs enable Command");
-
-	wake_up_interruptible(&priv->main_thread.wait_q);
+	int ret;
 
-	if (!wait_event_interruptible_timeout(priv->adapter->cmd_wait_q,
-			priv->adapter->hs_state,
-			msecs_to_jiffies(WAIT_UNTIL_HS_STATE_CHANGED))) {
-		ret = -ETIMEDOUT;
-		BT_ERR("timeout: %d, %d,%d", priv->adapter->hs_state,
-						priv->adapter->ps_state,
-						priv->adapter->wakeup_tries);
-	}
+	ret = btmrvl_send_sync_cmd(priv, BT_CMD_HOST_SLEEP_ENABLE, NULL, 0);
+	if (ret)
+		BT_ERR("Host sleep enable command failed\n");
 
 	return ret;
 }
-- 
1.8.0

^ permalink raw reply related

* [PATCH v6 0/4] Bluetooth: btmrvl cal data downloading
From: Bing Zhao @ 2013-10-01 19:19 UTC (permalink / raw)
  To: linux-bluetooth
  Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, linux-wireless,
	Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar, Bing Zhao

This series adds the calibration data downloading support
along with improvements in sending commands and setup handler.

Amitkumar Karwar (4):
  Bluetooth: btmrvl: add btmrvl_send_sync_cmd() function
  Bluetooth: btmrvl: get rid of struct btmrvl_cmd
  Bluetooth: btmrvl: add setup handler
  Bluetooth: btmrvl: add calibration data download support

 drivers/bluetooth/btmrvl_drv.h  |  12 +-
 drivers/bluetooth/btmrvl_main.c | 269 ++++++++++++++++++++++++++--------------
 drivers/bluetooth/btmrvl_sdio.c |  15 +--
 drivers/bluetooth/btmrvl_sdio.h |   2 +
 4 files changed, 193 insertions(+), 105 deletions(-)

-- 
1.8.0

^ permalink raw reply

* RE: [PATCH v5 1/2] Bluetooth: btmrvl: add setup handler
From: Bing Zhao @ 2013-10-01 16:23 UTC (permalink / raw)
  To: Johan Hedberg
  Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org development,
	Gustavo F. Padovan, linux-wireless@vger.kernel.org Wireless,
	Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <20131001111309.GA19641@x220.p-661hnu-f1>

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

^ permalink raw reply

* Re: [RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command
From: Marcel Holtmann @ 2013-10-01 16:08 UTC (permalink / raw)
  To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380640922-18647-4-git-send-email-johan.hedberg@gmail.com>

Hi Johan,

> This patch introduces a new mgmt command for enabling/disabling BR/EDR
> functionality. This can be convenient when one wants to make a dual-mode
> controller behave like a single-mode one. The command is only available
> for dual-mode controllers and requires that LE is enabled before using
> it.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/mgmt.h |   2 +
> net/bluetooth/mgmt.c         | 122 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 124 insertions(+)
> 
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 421d763..7347df8 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -354,6 +354,8 @@ struct mgmt_cp_set_device_id {
> 
> #define MGMT_OP_SET_ADVERTISING		0x0029
> 
> +#define MGMT_OP_SET_BREDR		0x002A
> +
> #define MGMT_EV_CMD_COMPLETE		0x0001
> struct mgmt_ev_cmd_complete {
> 	__le16	opcode;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index eea0b97..6405406 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -75,6 +75,7 @@ static const u16 mgmt_commands[] = {
> 	MGMT_OP_UNBLOCK_DEVICE,
> 	MGMT_OP_SET_DEVICE_ID,
> 	MGMT_OP_SET_ADVERTISING,
> +	MGMT_OP_SET_BREDR,
> };
> 
> static const u16 mgmt_events[] = {
> @@ -3259,6 +3260,126 @@ unlock:
> 	return err;
> }
> 
> +static void set_no_scan(struct hci_request *req)
> +{
> +	struct hci_dev *hdev = req->hdev;
> +	u8 scan = 0x00;
> +
> +	if (!test_bit(HCI_ISCAN, &hdev->flags) &&
> +	    !test_bit(HCI_PSCAN, &hdev->flags))
> +		return;

this one needs a comment on why we do it this way. I had to read it twice to get it.

> +
> +	hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, sizeof(scan), &scan);
> +}
> +
> +static void set_bredr_complete(struct hci_dev *hdev, u8 status)
> +{
> +	struct pending_cmd *cmd;
> +
> +	BT_DBG("status 0x%02x", status);
> +
> +	hci_dev_lock(hdev);
> +
> +	cmd = mgmt_pending_find(MGMT_OP_SET_BREDR, hdev);
> +	if (!cmd)
> +		goto unlock;
> +
> +	if (status) {
> +		u8 mgmt_err = mgmt_status(status);
> +
> +		/* We need to restore the flag if related HCI commands
> +		 * failed.
> +		 */
> +		change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +		cmd_status(cmd->sk, cmd->index, cmd->opcode, mgmt_err);
> +	} else {
> +		send_settings_rsp(cmd->sk, MGMT_OP_SET_BREDR, hdev);
> +	}

Who is sending the new settings event when this succeeds?

> +
> +	mgmt_pending_remove(cmd);
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +}
> +
> +static int set_bredr(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> +{
> +	struct mgmt_mode *cp = data;
> +	struct pending_cmd *cmd;
> +	struct hci_request req;
> +	u8 val, enabled;
> +	int err;
> +
> +	BT_DBG("request for %s", hdev->name);
> +
> +	if (!lmp_bredr_capable(hdev) || !lmp_le_capable(hdev))
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +				  MGMT_STATUS_REJECTED);
> +
> +	if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +				  MGMT_STATUS_REJECTED);
> +
> +	if (cp->val != 0x00 && cp->val != 0x01)
> +		return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +				  MGMT_STATUS_INVALID_PARAMS);

As already pointed out, copy & paste mistake here.

In addition we might either want to reject this command when HCI_HS_ENABLED or maybe even better also disable HS at the same time. I prefer just disabling HS since we do a similar thing with discoverable when turning off connectable.

The same applies to connectable and advertising and fast connectable. I think these settings need to be cleared as well. Essentially all settings that require BR/EDR need to be cleared.

> +
> +	hci_dev_lock(hdev);
> +
> +	val = !!cp->val;
> +	enabled = test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +	if (!hdev_is_powered(hdev) || val == enabled) {
> +		bool changed = false;
> +
> +		if (val != test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
> +			change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +			changed = true;
> +		}
> +
> +		err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
> +		if (err < 0)
> +			goto unlock;
> +
> +		if (changed)
> +			err = new_settings(hdev, sk);
> +
> +		goto unlock;
> +	}

I keep seeing this snippet over and over again. And actually the check if changed is done twice here. First we check if val == enabled and then we check it again and set changed = true. This looks a bit redundant.

What we should do is run the check for if the value actually changed and if not, just send the response and exit. And only then do the check for the case when the controller is not powered.

> +
> +	if (mgmt_pending_find(MGMT_OP_SET_BREDR, hdev)) {
> +		err = cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
> +				 MGMT_STATUS_BUSY);
> +		goto unlock;
> +	}
> +
> +	cmd = mgmt_pending_add(sk, MGMT_OP_SET_BREDR, hdev, data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +	hci_req_init(&req, hdev);
> +
> +	if (val)
> +		set_bredr_scan(&req);
> +	else
> +		set_no_scan(&req);
> +
> +	hci_update_ad(&req);
> +
> +	err = hci_req_run(&req, set_bredr_complete);
> +	if (err < 0)
> +		mgmt_pending_remove(cmd);
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +	return err;
> +}
> +
> static void fast_connectable_complete(struct hci_dev *hdev, u8 status)
> {
> 	struct pending_cmd *cmd;

On a total unrelated note, why is the fast connectable code at the bottom. I think that should be rearranged to be in the same order as the opcodes.

> @@ -3472,6 +3593,7 @@ static const struct mgmt_handler {
> 	{ unblock_device,         false, MGMT_UNBLOCK_DEVICE_SIZE },
> 	{ set_device_id,          false, MGMT_SET_DEVICE_ID_SIZE },
> 	{ set_advertising,        false, MGMT_SETTING_SIZE },
> +	{ set_bredr,              false, MGMT_SETTING_SIZE },
> };

Regards

Marcel


^ permalink raw reply

* Re: [RFC 1/3] Bluetooth: Introduce a new HCI_BREDR_ENABLED flag
From: Marcel Holtmann @ 2013-10-01 15:59 UTC (permalink / raw)
  To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380640922-18647-2-git-send-email-johan.hedberg@gmail.com>

Hi Johan,

> To allow treating dual-mode (BR/EDR/LE) controllers as single-mode ones
> (LE-only) we want to introduce a new HCI_BREDR_ENABLED flag to track
> whether BR/EDR is enabled or not (previously we simply looked at the
> feature bit with lmp_bredr_enabled).
> 
> This patch add the new flag and updates the relevant places to test
> against it instead of using lmp_bredr_enabled. The flag is by default
> enabled when registering an adapter and only cleared if necessary once
> the local features have been read during the HCI init procedure.
> 
> We cannot completely block BR/EDR usage in case user space uses raw HCI
> sockets but the patch tries to block this in places where possible, such
> as the various BR/EDR specific ioctls.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci.h |  1 +
> net/bluetooth/hci_conn.c    |  3 +++
> net/bluetooth/hci_core.c    | 21 +++++++++++++++++++--
> net/bluetooth/mgmt.c        | 24 +++++++++++++-----------
> 4 files changed, 36 insertions(+), 13 deletions(-)

unless you missed a spot, this patch looks pretty much correct.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel


^ permalink raw reply

* Re: [RFC 2/3] Bluetooth: Move set_bredr_scan() to a more convenient location
From: Marcel Holtmann @ 2013-10-01 15:53 UTC (permalink / raw)
  To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1380640922-18647-3-git-send-email-johan.hedberg@gmail.com>

Hi Johan,

> The static set_bredr_scan function will soon be reused by a new
> set_bredr method. This trivial patch moves the function to a new
> location within the mgmt.c file where it can be reused without requiring
> forward declarations.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 40 ++++++++++++++++++++--------------------
> 1 file changed, 20 insertions(+), 20 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel


^ permalink raw reply

* Re: [RFC 3/3] Bluetooth: Add a new mgmt_set_bredr command
From: Anderson Lizardo @ 2013-10-01 15:46 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: BlueZ development
In-Reply-To: <1380640922-18647-4-git-send-email-johan.hedberg@gmail.com>

Hi Johan,

On Tue, Oct 1, 2013 at 11:22 AM,  <johan.hedberg@gmail.com> wrote:
> +       if (!lmp_bredr_capable(hdev) || !lmp_le_capable(hdev))
> +               return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +                                 MGMT_STATUS_REJECTED);
> +
> +       if (!test_bit(HCI_LE_ENABLED, &hdev->dev_flags))
> +               return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +                                 MGMT_STATUS_REJECTED);
> +
> +       if (cp->val != 0x00 && cp->val != 0x01)
> +               return cmd_status(sk, hdev->id, MGMT_OP_SET_ADVERTISING,
> +                                 MGMT_STATUS_INVALID_PARAMS);

Looks like the above 3 cmd_status() should use MGMT_OP_SET_BREDR.

> +
> +       hci_dev_lock(hdev);
> +
> +       val = !!cp->val;
> +       enabled = test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +
> +       if (!hdev_is_powered(hdev) || val == enabled) {
> +               bool changed = false;
> +
> +               if (val != test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
> +                       change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> +                       changed = true;
> +               }

IMHO the trick used in another patch from Marcel (sent a while ago for
enabling/disabling HS) is easier to follow, something like (may
require adaptation if the logic is not the same than MGMT_OP_SET_HS):

if (cp->val)
    changed = !test_and_set_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
else
    changed = test_and_clear_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);


> +
> +               err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
> +               if (err < 0)
> +                       goto unlock;
> +
> +               if (changed)
> +                       err = new_settings(hdev, sk);
> +
> +               goto unlock;
> +       }

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply

* [PATCH BlueZ] tools: Fix update_compids.sh to avoid non-ASCII output
From: Anderson Lizardo @ 2013-10-01 15:46 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Anderson Lizardo

Some distros have html2text patches that may generate non-ASCII output
even when -ascii is used. This patch adds another case (seen in Fedora)
where HTML entity &#160; (non-breaking space) is converted into a
multibyte whitespace.

Also add a sanity check to make sure non-ASCII text is not introduced in
lib/bluetooth.c.
---
 tools/update_compids.sh |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/update_compids.sh b/tools/update_compids.sh
index 332fb16..38d1fff 100755
--- a/tools/update_compids.sh
+++ b/tools/update_compids.sh
@@ -21,13 +21,16 @@ cd $tmpdir
 
 path=en-us/specification/assigned-numbers/company-identifiers
 # Use "iconv -c" to strip unwanted unicode characters
-# Also strip <input> tags of type checkbox because html2text generates UTF-8
-# for them in some distros even when using -ascii (e.g. Fedora 18)
+# Fixups:
+# - strip <input> tags of type "checkbox" because html2text generates UTF-8 for
+#   them in some distros even when using -ascii (e.g. Fedora)
+# - replace "&#160;" (non-breaking space) with whitespace manually, because
+#   some versions incorrectly convert it into "\xC2\xA0"
 curl https://www.bluetooth.org/$path | iconv -c -f utf8 -t ascii | \
-    sed '/<input.*type="checkbox"/d' | \
+    sed '/<input.*type="checkbox"/d; s/&#160;/ /g' | \
     html2text -ascii -o identifiers.txt >/dev/null
 
-# Some versions of html2text do not replace &amp; (e.g. Fedora 18)
+# Some versions of html2text do not replace &amp; (e.g. Fedora)
 sed -i 's/&amp;/\&/g' identifiers.txt
 
 sed -n '/^const char \*bt_compidtostr(int compid)/,/^}/p' \
@@ -41,6 +44,12 @@ if ! grep -q "return \"" new.c; then
     echo "ERROR: could not parse company IDs from bluetooth.org" >&2
     exit 1
 fi
+if [ -n "$(tr -d '[:print:]\t\n' < new.c)" ]; then
+    echo -n "ERROR: invalid non-ASCII characters found while parsing" >&2
+    echo -n " company IDs. Please identify offending sequence and fix" >&2
+    echo " tools/update_compids.sh accordingly." >&2
+    exit 1
+fi
 echo -e '\tcase 65535:\n\t\treturn "internal use";' >> new.c
 echo -e '\tdefault:\n\t\treturn "not assigned";\n\t}\n}' >> new.c
 
-- 
1.7.9.5


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox