[PATCH 0/7] Update mgmt interface to latest specification

[PATCH 0/7] Update mgmt interface to latest specification

[johan.hedberg]

Hi,

This patch set updates the kernel side implementation of the managment
interface to match the latest specification for it. This will break
support with any older user-space versions and you will need to use the
very latest user-space git to test it (where I just a minute ago pushed
matching patches).

Johan

[PATCH 1/7] Bluetooth: Update mgmt_read_info and related mgmt messages

[johan.hedberg]

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

This patch updates the mgmt_read_info and related messages to the latest
management API which uses a bitfield of settings instead of individual
boolean values.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci.h  |    1 +
 include/net/bluetooth/mgmt.h |   29 +++++---
 net/bluetooth/mgmt.c         |  146 +++++++++++++++++++++++++++---------------
 3 files changed, 113 insertions(+), 63 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 67ad984..c9ad56f 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -210,6 +210,7 @@ enum {
 
 #define LMP_EV4		0x01
 #define LMP_EV5		0x02
+#define LMP_NO_BREDR	0x20
 #define LMP_LE		0x40
 
 #define LMP_SNIFF_SUBR	0x02
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 3b68806..85e9c6e 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -61,22 +61,29 @@ struct mgmt_rp_read_index_list {
 /* Reserve one extra byte for names in management messages so that they
  * are always guaranteed to be nul-terminated */
 #define MGMT_MAX_NAME_LENGTH		(HCI_MAX_NAME_LENGTH + 1)
+#define MGMT_MAX_SHORT_NAME_LENGTH	(10 + 1)
+
+#define MGMT_SETTING_POWERED		0x00000001
+#define MGMT_SETTING_CONNECTABLE	0x00000002
+#define MGMT_SETTING_FAST_CONNECTABLE	0x00000004
+#define MGMT_SETTING_DISCOVERABLE	0x00000008
+#define MGMT_SETTING_PAIRABLE		0x00000010
+#define MGMT_SETTING_LINK_SECURITY	0x00000020
+#define MGMT_SETTING_SSP		0x00000040
+#define MGMT_SETTING_BREDR		0x00000080
+#define MGMT_SETTING_HS			0x00000100
+#define MGMT_SETTING_LE			0x00000200
 
 #define MGMT_OP_READ_INFO		0x0004
 struct mgmt_rp_read_info {
-	__u8 type;
-	__u8 powered;
-	__u8 connectable;
-	__u8 discoverable;
-	__u8 pairable;
-	__u8 sec_mode;
 	bdaddr_t bdaddr;
+	__u8 version;
+	__le16 manufacturer;
+	__le32 supported_settings;
+	__le32 current_settings;
 	__u8 dev_class[3];
-	__u8 features[8];
-	__u16 manufacturer;
-	__u8 hci_ver;
-	__u16 hci_rev;
 	__u8 name[MGMT_MAX_NAME_LENGTH];
+	__u8 short_name[MGMT_MAX_SHORT_NAME_LENGTH];
 } __packed;
 
 struct mgmt_mode {
@@ -285,7 +292,7 @@ struct mgmt_ev_controller_error {
 
 #define MGMT_EV_INDEX_REMOVED		0x0005
 
-#define MGMT_EV_POWERED			0x0006
+#define MGMT_EV_NEW_SETTINGS		0x0006
 
 #define MGMT_EV_DISCOVERABLE		0x0007
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7a23f21..629570c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -242,6 +242,63 @@ static int read_index_list(struct sock *sk)
 	return err;
 }
 
+static u32 get_supported_settings(struct hci_dev *hdev)
+{
+	u32 settings = 0;
+
+	settings |= MGMT_SETTING_POWERED;
+	settings |= MGMT_SETTING_CONNECTABLE;
+	settings |= MGMT_SETTING_FAST_CONNECTABLE;
+	settings |= MGMT_SETTING_DISCOVERABLE;
+	settings |= MGMT_SETTING_PAIRABLE;
+
+	if (hdev->features[6] & LMP_SIMPLE_PAIR)
+		settings |= MGMT_SETTING_SSP;
+
+	if (!(hdev->features[4] & LMP_NO_BREDR)) {
+		settings |= MGMT_SETTING_BREDR;
+		settings |= MGMT_SETTING_LINK_SECURITY;
+	}
+
+	if (hdev->features[4] & LMP_LE)
+		settings |= MGMT_SETTING_LE;
+
+	return settings;
+}
+
+static u32 get_current_settings(struct hci_dev *hdev)
+{
+	u32 settings = 0;
+
+	if (test_bit(HCI_UP, &hdev->flags))
+		settings |= MGMT_SETTING_POWERED;
+	else
+		return settings;
+
+	if (test_bit(HCI_PSCAN, &hdev->flags))
+		settings |= MGMT_SETTING_CONNECTABLE;
+
+	if (test_bit(HCI_ISCAN, &hdev->flags))
+		settings |= MGMT_SETTING_DISCOVERABLE;
+
+	if (test_bit(HCI_PAIRABLE, &hdev->flags))
+		settings |= MGMT_SETTING_PAIRABLE;
+
+	if (!(hdev->features[4] & LMP_NO_BREDR))
+		settings |= MGMT_SETTING_BREDR;
+
+	if (hdev->extfeatures[0] & LMP_HOST_LE)
+		settings |= MGMT_SETTING_LE;
+
+	if (test_bit(HCI_AUTH, &hdev->flags))
+		settings |= MGMT_SETTING_LINK_SECURITY;
+
+	if (hdev->ssp_mode > 0)
+		settings |= MGMT_SETTING_SSP;
+
+	return settings;
+}
+
 static int read_controller_info(struct sock *sk, u16 index)
 {
 	struct mgmt_rp_read_info rp;
@@ -263,26 +320,16 @@ static int read_controller_info(struct sock *sk, u16 index)
 
 	memset(&rp, 0, sizeof(rp));
 
-	rp.type = hdev->dev_type;
+	bacpy(&rp.bdaddr, &hdev->bdaddr);
 
-	rp.powered = test_bit(HCI_UP, &hdev->flags);
-	rp.connectable = test_bit(HCI_PSCAN, &hdev->flags);
-	rp.discoverable = test_bit(HCI_ISCAN, &hdev->flags);
-	rp.pairable = test_bit(HCI_PSCAN, &hdev->flags);
+	rp.version = hdev->hci_ver;
 
-	if (test_bit(HCI_AUTH, &hdev->flags))
-		rp.sec_mode = 3;
-	else if (hdev->ssp_mode > 0)
-		rp.sec_mode = 4;
-	else
-		rp.sec_mode = 2;
+	put_unaligned_le16(hdev->manufacturer, &rp.manufacturer);
+
+	rp.supported_settings = cpu_to_le32(get_supported_settings(hdev));
+	rp.current_settings = cpu_to_le32(get_current_settings(hdev));
 
-	bacpy(&rp.bdaddr, &hdev->bdaddr);
-	memcpy(rp.features, hdev->features, 8);
 	memcpy(rp.dev_class, hdev->dev_class, 3);
-	put_unaligned_le16(hdev->manufacturer, &rp.manufacturer);
-	rp.hci_ver = hdev->hci_ver;
-	put_unaligned_le16(hdev->hci_rev, &rp.hci_rev);
 
 	memcpy(rp.name, hdev->dev_name, sizeof(hdev->dev_name));
 
@@ -365,13 +412,11 @@ static void mgmt_pending_remove(struct pending_cmd *cmd)
 	mgmt_pending_free(cmd);
 }
 
-static int send_mode_rsp(struct sock *sk, u16 opcode, u16 index, u8 val)
+static int send_settings_rsp(struct sock *sk, u16 opcode, struct hci_dev *hdev)
 {
-	struct mgmt_mode rp;
+	__le32 settings = cpu_to_le32(get_current_settings(hdev));
 
-	rp.val = val;
-
-	return cmd_complete(sk, index, opcode, &rp, sizeof(rp));
+	return cmd_complete(sk, hdev->id, opcode, &settings, sizeof(settings));
 }
 
 static int set_powered(struct sock *sk, u16 index, unsigned char *data, u16 len)
@@ -398,7 +443,7 @@ static int set_powered(struct sock *sk, u16 index, unsigned char *data, u16 len)
 
 	up = test_bit(HCI_UP, &hdev->flags);
 	if ((cp->val && up) || (!cp->val && !up)) {
-		err = send_mode_rsp(sk, index, MGMT_OP_SET_POWERED, cp->val);
+		err = send_settings_rsp(sk, MGMT_OP_SET_POWERED, hdev);
 		goto failed;
 	}
 
@@ -466,8 +511,7 @@ static int set_discoverable(struct sock *sk, u16 index, unsigned char *data,
 
 	if (cp->val == test_bit(HCI_ISCAN, &hdev->flags) &&
 					test_bit(HCI_PSCAN, &hdev->flags)) {
-		err = send_mode_rsp(sk, index, MGMT_OP_SET_DISCOVERABLE,
-								cp->val);
+		err = send_settings_rsp(sk, MGMT_OP_SET_DISCOVERABLE, hdev);
 		goto failed;
 	}
 
@@ -536,8 +580,7 @@ static int set_connectable(struct sock *sk, u16 index, unsigned char *data,
 	}
 
 	if (cp->val == test_bit(HCI_PSCAN, &hdev->flags)) {
-		err = send_mode_rsp(sk, index, MGMT_OP_SET_CONNECTABLE,
-								cp->val);
+		err = send_settings_rsp(sk, MGMT_OP_SET_CONNECTABLE, hdev);
 		goto failed;
 	}
 
@@ -595,8 +638,9 @@ static int mgmt_event(u16 event, struct hci_dev *hdev, void *data,
 static int set_pairable(struct sock *sk, u16 index, unsigned char *data,
 									u16 len)
 {
-	struct mgmt_mode *cp, ev;
+	struct mgmt_mode *cp;
 	struct hci_dev *hdev;
+	__le32 ev;
 	int err;
 
 	cp = (void *) data;
@@ -619,13 +663,13 @@ static int set_pairable(struct sock *sk, u16 index, unsigned char *data,
 	else
 		clear_bit(HCI_PAIRABLE, &hdev->flags);
 
-	err = send_mode_rsp(sk, MGMT_OP_SET_PAIRABLE, index, cp->val);
+	err = send_settings_rsp(sk, MGMT_OP_SET_PAIRABLE, hdev);
 	if (err < 0)
 		goto failed;
 
-	ev.val = cp->val;
+	ev = cpu_to_le32(get_current_settings(hdev));
 
-	err = mgmt_event(MGMT_EV_PAIRABLE, hdev, &ev, sizeof(ev), sk);
+	err = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev), sk);
 
 failed:
 	hci_dev_unlock_bh(hdev);
@@ -2234,17 +2278,14 @@ int mgmt_index_removed(struct hci_dev *hdev)
 struct cmd_lookup {
 	u8 val;
 	struct sock *sk;
+	struct hci_dev *hdev;
 };
 
-static void mode_rsp(struct pending_cmd *cmd, void *data)
+static void settings_rsp(struct pending_cmd *cmd, void *data)
 {
-	struct mgmt_mode *cp = cmd->param;
 	struct cmd_lookup *match = data;
 
-	if (cp->val != match->val)
-		return;
-
-	send_mode_rsp(cmd->sk, cmd->opcode, cmd->index, cp->val);
+	send_settings_rsp(cmd->sk, cmd->opcode, match->hdev);
 
 	list_del(&cmd->list);
 
@@ -2258,20 +2299,21 @@ static void mode_rsp(struct pending_cmd *cmd, void *data)
 
 int mgmt_powered(struct hci_dev *hdev, u8 powered)
 {
-	struct mgmt_mode ev;
-	struct cmd_lookup match = { powered, NULL };
+	struct cmd_lookup match = { powered, NULL, hdev };
+	__le32 ev;
 	int ret;
 
-	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, mode_rsp, &match);
+	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
 
 	if (!powered) {
 		u8 status = ENETDOWN;
 		mgmt_pending_foreach(0, hdev, cmd_status_rsp, &status);
 	}
 
-	ev.val = powered;
+	ev = cpu_to_le32(get_current_settings(hdev));
 
-	ret = mgmt_event(MGMT_EV_POWERED, hdev, &ev, sizeof(ev), match.sk);
+	ret = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev),
+								match.sk);
 
 	if (match.sk)
 		sock_put(match.sk);
@@ -2281,17 +2323,16 @@ int mgmt_powered(struct hci_dev *hdev, u8 powered)
 
 int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
 {
-	struct mgmt_mode ev;
-	struct cmd_lookup match = { discoverable, NULL };
+	struct cmd_lookup match = { discoverable, NULL, hdev };
+	__le32 ev;
 	int ret;
 
-	mgmt_pending_foreach(MGMT_OP_SET_DISCOVERABLE, hdev, mode_rsp, &match);
+	mgmt_pending_foreach(MGMT_OP_SET_DISCOVERABLE, hdev, settings_rsp, &match);
 
-	ev.val = discoverable;
+	ev = cpu_to_le32(get_current_settings(hdev));
 
-	ret = mgmt_event(MGMT_EV_DISCOVERABLE, hdev, &ev, sizeof(ev),
+	ret = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev),
 								match.sk);
-
 	if (match.sk)
 		sock_put(match.sk);
 
@@ -2300,15 +2341,16 @@ int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
 
 int mgmt_connectable(struct hci_dev *hdev, u8 connectable)
 {
-	struct mgmt_mode ev;
-	struct cmd_lookup match = { connectable, NULL };
+	__le32 ev;
+	struct cmd_lookup match = { connectable, NULL, hdev };
 	int ret;
 
-	mgmt_pending_foreach(MGMT_OP_SET_CONNECTABLE, hdev, mode_rsp, &match);
+	mgmt_pending_foreach(MGMT_OP_SET_CONNECTABLE, hdev, settings_rsp,
+								&match);
 
-	ev.val = connectable;
+	ev = cpu_to_le32(get_current_settings(hdev));
 
-	ret = mgmt_event(MGMT_EV_CONNECTABLE, hdev, &ev, sizeof(ev), match.sk);
+	ret = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev), match.sk);
 
 	if (match.sk)
 		sock_put(match.sk);
-- 
1.7.7.3

Re: [PATCH 1/7] Bluetooth: Update mgmt_read_info and related mgmt messages

[Marcel Holtmann]

Hi Johan,

> This patch updates the mgmt_read_info and related messages to the latest
> management API which uses a bitfield of settings instead of individual
> boolean values.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci.h  |    1 +
>  include/net/bluetooth/mgmt.h |   29 +++++---
>  net/bluetooth/mgmt.c         |  146 +++++++++++++++++++++++++++---------------
>  3 files changed, 113 insertions(+), 63 deletions(-)

I would have split the hci.h patch into a separate one, but since it is
so tiny, this is fine with me as well.

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

Regards

Marcel

Re: [PATCH 1/7] Bluetooth: Update mgmt_read_info and related mgmt messages

[Andrei Emeltchenko]

Hi Johan,

On Wed, Dec 14, 2011 at 11:51:58PM +0200, johan.hedberg@gmail.com wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> This patch updates the mgmt_read_info and related messages to the latest
> management API which uses a bitfield of settings instead of individual
> boolean values.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci.h  |    1 +
>  include/net/bluetooth/mgmt.h |   29 +++++---
>  net/bluetooth/mgmt.c         |  146 +++++++++++++++++++++++++++---------------
>  3 files changed, 113 insertions(+), 63 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 67ad984..c9ad56f 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -210,6 +210,7 @@ enum {
>  
>  #define LMP_EV4		0x01
>  #define LMP_EV5		0x02
> +#define LMP_NO_BREDR	0x20
>  #define LMP_LE		0x40
>  
>  #define LMP_SNIFF_SUBR	0x02
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 3b68806..85e9c6e 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -61,22 +61,29 @@ struct mgmt_rp_read_index_list {
>  /* Reserve one extra byte for names in management messages so that they
>   * are always guaranteed to be nul-terminated */
>  #define MGMT_MAX_NAME_LENGTH		(HCI_MAX_NAME_LENGTH + 1)
> +#define MGMT_MAX_SHORT_NAME_LENGTH	(10 + 1)
> +
> +#define MGMT_SETTING_POWERED		0x00000001
> +#define MGMT_SETTING_CONNECTABLE	0x00000002
> +#define MGMT_SETTING_FAST_CONNECTABLE	0x00000004
> +#define MGMT_SETTING_DISCOVERABLE	0x00000008
> +#define MGMT_SETTING_PAIRABLE		0x00000010
> +#define MGMT_SETTING_LINK_SECURITY	0x00000020
> +#define MGMT_SETTING_SSP		0x00000040
> +#define MGMT_SETTING_BREDR		0x00000080
> +#define MGMT_SETTING_HS			0x00000100
> +#define MGMT_SETTING_LE			0x00000200

Just a minor comment.
Can we use set_bit style for the defines above?

Best regards 
Andrei Emeltchenko 

>  
>  #define MGMT_OP_READ_INFO		0x0004
>  struct mgmt_rp_read_info {
> -	__u8 type;
> -	__u8 powered;
> -	__u8 connectable;
> -	__u8 discoverable;
> -	__u8 pairable;
> -	__u8 sec_mode;
>  	bdaddr_t bdaddr;
> +	__u8 version;
> +	__le16 manufacturer;
> +	__le32 supported_settings;
> +	__le32 current_settings;
>  	__u8 dev_class[3];
> -	__u8 features[8];
> -	__u16 manufacturer;
> -	__u8 hci_ver;
> -	__u16 hci_rev;
>  	__u8 name[MGMT_MAX_NAME_LENGTH];
> +	__u8 short_name[MGMT_MAX_SHORT_NAME_LENGTH];
>  } __packed;
>  
>  struct mgmt_mode {
> @@ -285,7 +292,7 @@ struct mgmt_ev_controller_error {
>  
>  #define MGMT_EV_INDEX_REMOVED		0x0005
>  
> -#define MGMT_EV_POWERED			0x0006
> +#define MGMT_EV_NEW_SETTINGS		0x0006
>  
>  #define MGMT_EV_DISCOVERABLE		0x0007
>  
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 7a23f21..629570c 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -242,6 +242,63 @@ static int read_index_list(struct sock *sk)
>  	return err;
>  }
>  
> +static u32 get_supported_settings(struct hci_dev *hdev)
> +{
> +	u32 settings = 0;
> +
> +	settings |= MGMT_SETTING_POWERED;
> +	settings |= MGMT_SETTING_CONNECTABLE;
> +	settings |= MGMT_SETTING_FAST_CONNECTABLE;
> +	settings |= MGMT_SETTING_DISCOVERABLE;
> +	settings |= MGMT_SETTING_PAIRABLE;
> +
> +	if (hdev->features[6] & LMP_SIMPLE_PAIR)
> +		settings |= MGMT_SETTING_SSP;
> +
> +	if (!(hdev->features[4] & LMP_NO_BREDR)) {
> +		settings |= MGMT_SETTING_BREDR;
> +		settings |= MGMT_SETTING_LINK_SECURITY;
> +	}
> +
> +	if (hdev->features[4] & LMP_LE)
> +		settings |= MGMT_SETTING_LE;
> +
> +	return settings;
> +}
> +
> +static u32 get_current_settings(struct hci_dev *hdev)
> +{
> +	u32 settings = 0;
> +
> +	if (test_bit(HCI_UP, &hdev->flags))
> +		settings |= MGMT_SETTING_POWERED;
> +	else
> +		return settings;
> +
> +	if (test_bit(HCI_PSCAN, &hdev->flags))
> +		settings |= MGMT_SETTING_CONNECTABLE;
> +
> +	if (test_bit(HCI_ISCAN, &hdev->flags))
> +		settings |= MGMT_SETTING_DISCOVERABLE;
> +
> +	if (test_bit(HCI_PAIRABLE, &hdev->flags))
> +		settings |= MGMT_SETTING_PAIRABLE;
> +
> +	if (!(hdev->features[4] & LMP_NO_BREDR))
> +		settings |= MGMT_SETTING_BREDR;
> +
> +	if (hdev->extfeatures[0] & LMP_HOST_LE)
> +		settings |= MGMT_SETTING_LE;
> +
> +	if (test_bit(HCI_AUTH, &hdev->flags))
> +		settings |= MGMT_SETTING_LINK_SECURITY;
> +
> +	if (hdev->ssp_mode > 0)
> +		settings |= MGMT_SETTING_SSP;
> +
> +	return settings;
> +}
> +
>  static int read_controller_info(struct sock *sk, u16 index)
>  {
>  	struct mgmt_rp_read_info rp;
> @@ -263,26 +320,16 @@ static int read_controller_info(struct sock *sk, u16 index)
>  
>  	memset(&rp, 0, sizeof(rp));
>  
> -	rp.type = hdev->dev_type;
> +	bacpy(&rp.bdaddr, &hdev->bdaddr);
>  
> -	rp.powered = test_bit(HCI_UP, &hdev->flags);
> -	rp.connectable = test_bit(HCI_PSCAN, &hdev->flags);
> -	rp.discoverable = test_bit(HCI_ISCAN, &hdev->flags);
> -	rp.pairable = test_bit(HCI_PSCAN, &hdev->flags);
> +	rp.version = hdev->hci_ver;
>  
> -	if (test_bit(HCI_AUTH, &hdev->flags))
> -		rp.sec_mode = 3;
> -	else if (hdev->ssp_mode > 0)
> -		rp.sec_mode = 4;
> -	else
> -		rp.sec_mode = 2;
> +	put_unaligned_le16(hdev->manufacturer, &rp.manufacturer);
> +
> +	rp.supported_settings = cpu_to_le32(get_supported_settings(hdev));
> +	rp.current_settings = cpu_to_le32(get_current_settings(hdev));
>  
> -	bacpy(&rp.bdaddr, &hdev->bdaddr);
> -	memcpy(rp.features, hdev->features, 8);
>  	memcpy(rp.dev_class, hdev->dev_class, 3);
> -	put_unaligned_le16(hdev->manufacturer, &rp.manufacturer);
> -	rp.hci_ver = hdev->hci_ver;
> -	put_unaligned_le16(hdev->hci_rev, &rp.hci_rev);
>  
>  	memcpy(rp.name, hdev->dev_name, sizeof(hdev->dev_name));
>  
> @@ -365,13 +412,11 @@ static void mgmt_pending_remove(struct pending_cmd *cmd)
>  	mgmt_pending_free(cmd);
>  }
>  
> -static int send_mode_rsp(struct sock *sk, u16 opcode, u16 index, u8 val)
> +static int send_settings_rsp(struct sock *sk, u16 opcode, struct hci_dev *hdev)
>  {
> -	struct mgmt_mode rp;
> +	__le32 settings = cpu_to_le32(get_current_settings(hdev));
>  
> -	rp.val = val;
> -
> -	return cmd_complete(sk, index, opcode, &rp, sizeof(rp));
> +	return cmd_complete(sk, hdev->id, opcode, &settings, sizeof(settings));
>  }
>  
>  static int set_powered(struct sock *sk, u16 index, unsigned char *data, u16 len)
> @@ -398,7 +443,7 @@ static int set_powered(struct sock *sk, u16 index, unsigned char *data, u16 len)
>  
>  	up = test_bit(HCI_UP, &hdev->flags);
>  	if ((cp->val && up) || (!cp->val && !up)) {
> -		err = send_mode_rsp(sk, index, MGMT_OP_SET_POWERED, cp->val);
> +		err = send_settings_rsp(sk, MGMT_OP_SET_POWERED, hdev);
>  		goto failed;
>  	}
>  
> @@ -466,8 +511,7 @@ static int set_discoverable(struct sock *sk, u16 index, unsigned char *data,
>  
>  	if (cp->val == test_bit(HCI_ISCAN, &hdev->flags) &&
>  					test_bit(HCI_PSCAN, &hdev->flags)) {
> -		err = send_mode_rsp(sk, index, MGMT_OP_SET_DISCOVERABLE,
> -								cp->val);
> +		err = send_settings_rsp(sk, MGMT_OP_SET_DISCOVERABLE, hdev);
>  		goto failed;
>  	}
>  
> @@ -536,8 +580,7 @@ static int set_connectable(struct sock *sk, u16 index, unsigned char *data,
>  	}
>  
>  	if (cp->val == test_bit(HCI_PSCAN, &hdev->flags)) {
> -		err = send_mode_rsp(sk, index, MGMT_OP_SET_CONNECTABLE,
> -								cp->val);
> +		err = send_settings_rsp(sk, MGMT_OP_SET_CONNECTABLE, hdev);
>  		goto failed;
>  	}
>  
> @@ -595,8 +638,9 @@ static int mgmt_event(u16 event, struct hci_dev *hdev, void *data,
>  static int set_pairable(struct sock *sk, u16 index, unsigned char *data,
>  									u16 len)
>  {
> -	struct mgmt_mode *cp, ev;
> +	struct mgmt_mode *cp;
>  	struct hci_dev *hdev;
> +	__le32 ev;
>  	int err;
>  
>  	cp = (void *) data;
> @@ -619,13 +663,13 @@ static int set_pairable(struct sock *sk, u16 index, unsigned char *data,
>  	else
>  		clear_bit(HCI_PAIRABLE, &hdev->flags);
>  
> -	err = send_mode_rsp(sk, MGMT_OP_SET_PAIRABLE, index, cp->val);
> +	err = send_settings_rsp(sk, MGMT_OP_SET_PAIRABLE, hdev);
>  	if (err < 0)
>  		goto failed;
>  
> -	ev.val = cp->val;
> +	ev = cpu_to_le32(get_current_settings(hdev));
>  
> -	err = mgmt_event(MGMT_EV_PAIRABLE, hdev, &ev, sizeof(ev), sk);
> +	err = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev), sk);
>  
>  failed:
>  	hci_dev_unlock_bh(hdev);
> @@ -2234,17 +2278,14 @@ int mgmt_index_removed(struct hci_dev *hdev)
>  struct cmd_lookup {
>  	u8 val;
>  	struct sock *sk;
> +	struct hci_dev *hdev;
>  };
>  
> -static void mode_rsp(struct pending_cmd *cmd, void *data)
> +static void settings_rsp(struct pending_cmd *cmd, void *data)
>  {
> -	struct mgmt_mode *cp = cmd->param;
>  	struct cmd_lookup *match = data;
>  
> -	if (cp->val != match->val)
> -		return;
> -
> -	send_mode_rsp(cmd->sk, cmd->opcode, cmd->index, cp->val);
> +	send_settings_rsp(cmd->sk, cmd->opcode, match->hdev);
>  
>  	list_del(&cmd->list);
>  
> @@ -2258,20 +2299,21 @@ static void mode_rsp(struct pending_cmd *cmd, void *data)
>  
>  int mgmt_powered(struct hci_dev *hdev, u8 powered)
>  {
> -	struct mgmt_mode ev;
> -	struct cmd_lookup match = { powered, NULL };
> +	struct cmd_lookup match = { powered, NULL, hdev };
> +	__le32 ev;
>  	int ret;
>  
> -	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, mode_rsp, &match);
> +	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
>  
>  	if (!powered) {
>  		u8 status = ENETDOWN;
>  		mgmt_pending_foreach(0, hdev, cmd_status_rsp, &status);
>  	}
>  
> -	ev.val = powered;
> +	ev = cpu_to_le32(get_current_settings(hdev));
>  
> -	ret = mgmt_event(MGMT_EV_POWERED, hdev, &ev, sizeof(ev), match.sk);
> +	ret = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev),
> +								match.sk);
>  
>  	if (match.sk)
>  		sock_put(match.sk);
> @@ -2281,17 +2323,16 @@ int mgmt_powered(struct hci_dev *hdev, u8 powered)
>  
>  int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
>  {
> -	struct mgmt_mode ev;
> -	struct cmd_lookup match = { discoverable, NULL };
> +	struct cmd_lookup match = { discoverable, NULL, hdev };
> +	__le32 ev;
>  	int ret;
>  
> -	mgmt_pending_foreach(MGMT_OP_SET_DISCOVERABLE, hdev, mode_rsp, &match);
> +	mgmt_pending_foreach(MGMT_OP_SET_DISCOVERABLE, hdev, settings_rsp, &match);
>  
> -	ev.val = discoverable;
> +	ev = cpu_to_le32(get_current_settings(hdev));
>  
> -	ret = mgmt_event(MGMT_EV_DISCOVERABLE, hdev, &ev, sizeof(ev),
> +	ret = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev),
>  								match.sk);
> -
>  	if (match.sk)
>  		sock_put(match.sk);
>  
> @@ -2300,15 +2341,16 @@ int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
>  
>  int mgmt_connectable(struct hci_dev *hdev, u8 connectable)
>  {
> -	struct mgmt_mode ev;
> -	struct cmd_lookup match = { connectable, NULL };
> +	__le32 ev;
> +	struct cmd_lookup match = { connectable, NULL, hdev };
>  	int ret;
>  
> -	mgmt_pending_foreach(MGMT_OP_SET_CONNECTABLE, hdev, mode_rsp, &match);
> +	mgmt_pending_foreach(MGMT_OP_SET_CONNECTABLE, hdev, settings_rsp,
> +								&match);
>  
> -	ev.val = connectable;
> +	ev = cpu_to_le32(get_current_settings(hdev));
>  
> -	ret = mgmt_event(MGMT_EV_CONNECTABLE, hdev, &ev, sizeof(ev), match.sk);
> +	ret = mgmt_event(MGMT_EV_NEW_SETTINGS, hdev, &ev, sizeof(ev), match.sk);
>  
>  	if (match.sk)
>  		sock_put(match.sk);
> -- 
> 1.7.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] Bluetooth: Update mgmt_read_info and related mgmt messages

[Johan Hedberg]

Hi Andrei,

On Thu, Dec 15, 2011, Andrei Emeltchenko wrote:
> > +#define MGMT_SETTING_POWERED		0x00000001
> > +#define MGMT_SETTING_CONNECTABLE	0x00000002
> > +#define MGMT_SETTING_FAST_CONNECTABLE	0x00000004
> > +#define MGMT_SETTING_DISCOVERABLE	0x00000008
> > +#define MGMT_SETTING_PAIRABLE		0x00000010
> > +#define MGMT_SETTING_LINK_SECURITY	0x00000020
> > +#define MGMT_SETTING_SSP		0x00000040
> > +#define MGMT_SETTING_BREDR		0x00000080
> > +#define MGMT_SETTING_HS			0x00000100
> > +#define MGMT_SETTING_LE			0x00000200
> 
> Just a minor comment.
> Can we use set_bit style for the defines above?

I was considering it, but the set_bit/test_bit functions expect pointers
to unsigned long whereas we're dealing with a fixed-size u32 in the mgmt
protocol. In the best case that'd require adding type-casts to avoid
compiler warnings/errors and in the worst case it could even cause bugs
on some architectures.

Johan

[PATCH 2/7] Bluetooth: Move mgmt_set_fast_connectable to the right location

[johan.hedberg]

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

Fast connetable is logically after the connectable property so that's
where it should show up in the code as well (it's also after connectable
in the settings bitfield).

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/mgmt.h |    7 ++-----
 net/bluetooth/mgmt.c         |   12 ++++++------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 85e9c6e..bf217cc 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -100,6 +100,8 @@ struct mgmt_cp_set_discoverable {
 
 #define MGMT_OP_SET_CONNECTABLE		0x0007
 
+#define MGMT_OP_SET_FAST_CONNECTABLE	0x001F
+
 #define MGMT_OP_SET_PAIRABLE		0x0008
 
 #define MGMT_OP_ADD_UUID		0x0009
@@ -255,11 +257,6 @@ struct mgmt_cp_unblock_device {
 	bdaddr_t bdaddr;
 } __packed;
 
-#define MGMT_OP_SET_FAST_CONNECTABLE	0x001F
-struct mgmt_cp_set_fast_connectable {
-	__u8 enable;
-} __packed;
-
 #define MGMT_OP_USER_PASSKEY_REPLY	0x0020
 struct mgmt_cp_user_passkey_reply {
 	bdaddr_t bdaddr;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 629570c..54092c2 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2052,7 +2052,7 @@ static int set_fast_connectable(struct sock *sk, u16 index,
 					unsigned char *data, u16 len)
 {
 	struct hci_dev *hdev;
-	struct mgmt_cp_set_fast_connectable *cp = (void *) data;
+	struct mgmt_mode *cp = (void *) data;
 	struct hci_cp_write_page_scan_activity acp;
 	u8 type;
 	int err;
@@ -2070,7 +2070,7 @@ static int set_fast_connectable(struct sock *sk, u16 index,
 
 	hci_dev_lock(hdev);
 
-	if (cp->enable) {
+	if (cp->val) {
 		type = PAGE_SCAN_TYPE_INTERLACED;
 		acp.interval = 0x0024;	/* 22.5 msec page scan interval */
 	} else {
@@ -2154,6 +2154,10 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 	case MGMT_OP_SET_CONNECTABLE:
 		err = set_connectable(sk, index, buf + sizeof(*hdr), len);
 		break;
+	case MGMT_OP_SET_FAST_CONNECTABLE:
+		err = set_fast_connectable(sk, index, buf + sizeof(*hdr),
+								len);
+		break;
 	case MGMT_OP_SET_PAIRABLE:
 		err = set_pairable(sk, index, buf + sizeof(*hdr), len);
 		break;
@@ -2232,10 +2236,6 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 	case MGMT_OP_UNBLOCK_DEVICE:
 		err = unblock_device(sk, index, buf + sizeof(*hdr), len);
 		break;
-	case MGMT_OP_SET_FAST_CONNECTABLE:
-		err = set_fast_connectable(sk, index, buf + sizeof(*hdr),
-								len);
-		break;
 	default:
 		BT_DBG("Unknown op %u", opcode);
 		err = cmd_status(sk, index, opcode,
-- 
1.7.7.3

Re: [PATCH 2/7] Bluetooth: Move mgmt_set_fast_connectable to the right location

[Marcel Holtmann]

Hi Johan,

> Fast connetable is logically after the connectable property so that's
> where it should show up in the code as well (it's also after connectable
> in the settings bitfield).
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/mgmt.h |    7 ++-----
>  net/bluetooth/mgmt.c         |   12 ++++++------
>  2 files changed, 8 insertions(+), 11 deletions(-)

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

However changing the hci_dev_lock in hci_dev_lock_bh would have been
nice as well, but that becomes obsolete anyway when moving to a
workqueue. So not worth bothering.

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: Move mgmt_set_fast_connectable to the right location

[Hemant Gupta]

Hi Johan,

On Thu, Dec 15, 2011 at 3:21 AM,  <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> Fast connetable is logically after the connectable property so that's
connetable  -> connectable

> where it should show up in the code as well (it's also after connectable
> in the settings bitfield).
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/mgmt.h |    7 ++-----
>  net/bluetooth/mgmt.c         |   12 ++++++------
>  2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 85e9c6e..bf217cc 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -100,6 +100,8 @@ struct mgmt_cp_set_discoverable {
>
>  #define MGMT_OP_SET_CONNECTABLE                0x0007
>
> +#define MGMT_OP_SET_FAST_CONNECTABLE   0x001F
> +
>  #define MGMT_OP_SET_PAIRABLE           0x0008
>
>  #define MGMT_OP_ADD_UUID               0x0009
> @@ -255,11 +257,6 @@ struct mgmt_cp_unblock_device {
>        bdaddr_t bdaddr;
>  } __packed;
>
> -#define MGMT_OP_SET_FAST_CONNECTABLE   0x001F
> -struct mgmt_cp_set_fast_connectable {
> -       __u8 enable;
> -} __packed;
> -
>  #define MGMT_OP_USER_PASSKEY_REPLY     0x0020
>  struct mgmt_cp_user_passkey_reply {
>        bdaddr_t bdaddr;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 629570c..54092c2 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2052,7 +2052,7 @@ static int set_fast_connectable(struct sock *sk, u16 index,
>                                        unsigned char *data, u16 len)
>  {
>        struct hci_dev *hdev;
> -       struct mgmt_cp_set_fast_connectable *cp = (void *) data;
> +       struct mgmt_mode *cp = (void *) data;
>        struct hci_cp_write_page_scan_activity acp;
>        u8 type;
>        int err;
> @@ -2070,7 +2070,7 @@ static int set_fast_connectable(struct sock *sk, u16 index,
>
>        hci_dev_lock(hdev);
>
> -       if (cp->enable) {
> +       if (cp->val) {
>                type = PAGE_SCAN_TYPE_INTERLACED;
>                acp.interval = 0x0024;  /* 22.5 msec page scan interval */
>        } else {
> @@ -2154,6 +2154,10 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
>        case MGMT_OP_SET_CONNECTABLE:
>                err = set_connectable(sk, index, buf + sizeof(*hdr), len);
>                break;
> +       case MGMT_OP_SET_FAST_CONNECTABLE:
> +               err = set_fast_connectable(sk, index, buf + sizeof(*hdr),
> +                                                               len);
> +               break;
>        case MGMT_OP_SET_PAIRABLE:
>                err = set_pairable(sk, index, buf + sizeof(*hdr), len);
>                break;
> @@ -2232,10 +2236,6 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
>        case MGMT_OP_UNBLOCK_DEVICE:
>                err = unblock_device(sk, index, buf + sizeof(*hdr), len);
>                break;
> -       case MGMT_OP_SET_FAST_CONNECTABLE:
> -               err = set_fast_connectable(sk, index, buf + sizeof(*hdr),
> -                                                               len);
> -               break;
>        default:
>                BT_DBG("Unknown op %u", opcode);
>                err = cmd_status(sk, index, opcode,
> --
> 1.7.7.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Hemant Gupta
ST-Ericsson India

[PATCH 3/7] Bluetooth: Remove mgmt_set_service_cache

[johan.hedberg]

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

Instead of having an explicit service cache command we can make the mgmt
API simpler by implicitly enabling the cache when mgmt_read_info is
called for the first time and disabiling it when mgmt_set_dev_class is
called.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    4 +++
 include/net/bluetooth/mgmt.h     |    5 ---
 net/bluetooth/hci_sock.c         |    7 +++-
 net/bluetooth/mgmt.c             |   56 +++++---------------------------------
 4 files changed, 16 insertions(+), 56 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e34cd71..4e67bb8 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -958,12 +958,16 @@ int mgmt_device_unblocked(struct hci_dev *hdev, bdaddr_t *bdaddr);
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
 
+/* HCI socket flags */
+#define HCI_PI_MGMT_INIT	0
+
 struct hci_pinfo {
 	struct bt_sock    bt;
 	struct hci_dev    *hdev;
 	struct hci_filter filter;
 	__u32             cmsg_mask;
 	unsigned short   channel;
+	unsigned long     flags;
 };
 
 /* HCI security filter */
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index bf217cc..bdb0a58 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -121,11 +121,6 @@ struct mgmt_cp_set_dev_class {
 	__u8 minor;
 } __packed;
 
-#define MGMT_OP_SET_SERVICE_CACHE	0x000C
-struct mgmt_cp_set_service_cache {
-	__u8 enable;
-} __packed;
-
 struct mgmt_link_key_info {
 	bdaddr_t bdaddr;
 	u8 type;
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index f6afe3d..47febe8 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -343,8 +343,11 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 	if (haddr.hci_channel > HCI_CHANNEL_CONTROL)
 		return -EINVAL;
 
-	if (haddr.hci_channel == HCI_CHANNEL_CONTROL && !enable_mgmt)
-		return -EINVAL;
+	if (haddr.hci_channel == HCI_CHANNEL_CONTROL) {
+		if (!enable_mgmt)
+			return -EINVAL;
+		set_bit(HCI_PI_MGMT_INIT, &hci_pi(sk)->flags);
+	}
 
 	lock_sock(sk);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 54092c2..25e7bf9 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -316,7 +316,10 @@ static int read_controller_info(struct sock *sk, u16 index)
 
 	hci_dev_lock_bh(hdev);
 
-	set_bit(HCI_MGMT, &hdev->flags);
+	if (test_and_clear_bit(HCI_PI_MGMT_INIT, &hci_pi(sk)->flags)) {
+		set_bit(HCI_MGMT, &hdev->flags);
+		set_bit(HCI_SERVICE_CACHE, &hdev->flags);
+	}
 
 	memset(&rp, 0, sizeof(rp));
 
@@ -989,6 +992,9 @@ static int set_dev_class(struct sock *sk, u16 index, unsigned char *data,
 	hdev->major_class = cp->major;
 	hdev->minor_class = cp->minor;
 
+	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
+		update_eir(hdev);
+
 	err = update_class(hdev);
 
 	if (err == 0)
@@ -1000,51 +1006,6 @@ static int set_dev_class(struct sock *sk, u16 index, unsigned char *data,
 	return err;
 }
 
-static int set_service_cache(struct sock *sk, u16 index,  unsigned char *data,
-									u16 len)
-{
-	struct hci_dev *hdev;
-	struct mgmt_cp_set_service_cache *cp;
-	int err;
-
-	cp = (void *) data;
-
-	if (len != sizeof(*cp))
-		return cmd_status(sk, index, MGMT_OP_SET_SERVICE_CACHE,
-						MGMT_STATUS_INVALID_PARAMS);
-
-	hdev = hci_dev_get(index);
-	if (!hdev)
-		return cmd_status(sk, index, MGMT_OP_SET_SERVICE_CACHE,
-						MGMT_STATUS_INVALID_PARAMS);
-
-	hci_dev_lock_bh(hdev);
-
-	BT_DBG("hci%u enable %d", index, cp->enable);
-
-	if (cp->enable) {
-		set_bit(HCI_SERVICE_CACHE, &hdev->flags);
-		err = 0;
-	} else {
-		clear_bit(HCI_SERVICE_CACHE, &hdev->flags);
-		err = update_class(hdev);
-		if (err == 0)
-			err = update_eir(hdev);
-	}
-
-	if (err == 0)
-		err = cmd_complete(sk, index, MGMT_OP_SET_SERVICE_CACHE, NULL,
-									0);
-	else
-		cmd_status(sk, index, MGMT_OP_SET_SERVICE_CACHE, -err);
-
-
-	hci_dev_unlock_bh(hdev);
-	hci_dev_put(hdev);
-
-	return err;
-}
-
 static int load_link_keys(struct sock *sk, u16 index, unsigned char *data,
 								u16 len)
 {
@@ -2170,9 +2131,6 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 	case MGMT_OP_SET_DEV_CLASS:
 		err = set_dev_class(sk, index, buf + sizeof(*hdr), len);
 		break;
-	case MGMT_OP_SET_SERVICE_CACHE:
-		err = set_service_cache(sk, index, buf + sizeof(*hdr), len);
-		break;
 	case MGMT_OP_LOAD_LINK_KEYS:
 		err = load_link_keys(sk, index, buf + sizeof(*hdr), len);
 		break;
-- 
1.7.7.3

Re: [PATCH 3/7] Bluetooth: Remove mgmt_set_service_cache

[Marcel Holtmann]

Hi Johan,

> Instead of having an explicit service cache command we can make the mgmt
> API simpler by implicitly enabling the cache when mgmt_read_info is
> called for the first time and disabiling it when mgmt_set_dev_class is

it is "disabling".

> called.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    4 +++
>  include/net/bluetooth/mgmt.h     |    5 ---
>  net/bluetooth/hci_sock.c         |    7 +++-
>  net/bluetooth/mgmt.c             |   56 +++++---------------------------------
>  4 files changed, 16 insertions(+), 56 deletions(-)

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

Regards

Marcel

[PATCH 4/7] Bluetooth: Move EIR and CoD update functions to a better position

[johan.hedberg]

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

Due to the upcoming addition of a service cache timer the functions to
update the EIR and CoD need to be higher up in mgmt.c in order to avoid
unnecessary forward-declarations. This patch simply moves code around
without any other changes in order to make subsequent patches more
readable.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |  346 +++++++++++++++++++++++++-------------------------
 1 files changed, 173 insertions(+), 173 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 25e7bf9..696e585 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -299,6 +299,179 @@ static u32 get_current_settings(struct hci_dev *hdev)
 	return settings;
 }
 
+#define EIR_FLAGS		0x01 /* flags */
+#define EIR_UUID16_SOME		0x02 /* 16-bit UUID, more available */
+#define EIR_UUID16_ALL		0x03 /* 16-bit UUID, all listed */
+#define EIR_UUID32_SOME		0x04 /* 32-bit UUID, more available */
+#define EIR_UUID32_ALL		0x05 /* 32-bit UUID, all listed */
+#define EIR_UUID128_SOME	0x06 /* 128-bit UUID, more available */
+#define EIR_UUID128_ALL		0x07 /* 128-bit UUID, all listed */
+#define EIR_NAME_SHORT		0x08 /* shortened local name */
+#define EIR_NAME_COMPLETE	0x09 /* complete local name */
+#define EIR_TX_POWER		0x0A /* transmit power level */
+#define EIR_DEVICE_ID		0x10 /* device ID */
+
+#define PNP_INFO_SVCLASS_ID		0x1200
+
+static u8 bluetooth_base_uuid[] = {
+			0xFB, 0x34, 0x9B, 0x5F, 0x80, 0x00, 0x00, 0x80,
+			0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+};
+
+static u16 get_uuid16(u8 *uuid128)
+{
+	u32 val;
+	int i;
+
+	for (i = 0; i < 12; i++) {
+		if (bluetooth_base_uuid[i] != uuid128[i])
+			return 0;
+	}
+
+	memcpy(&val, &uuid128[12], 4);
+
+	val = le32_to_cpu(val);
+	if (val > 0xffff)
+		return 0;
+
+	return (u16) val;
+}
+
+static void create_eir(struct hci_dev *hdev, u8 *data)
+{
+	u8 *ptr = data;
+	u16 eir_len = 0;
+	u16 uuid16_list[HCI_MAX_EIR_LENGTH / sizeof(u16)];
+	int i, truncated = 0;
+	struct bt_uuid *uuid;
+	size_t name_len;
+
+	name_len = strlen(hdev->dev_name);
+
+	if (name_len > 0) {
+		/* EIR Data type */
+		if (name_len > 48) {
+			name_len = 48;
+			ptr[1] = EIR_NAME_SHORT;
+		} else
+			ptr[1] = EIR_NAME_COMPLETE;
+
+		/* EIR Data length */
+		ptr[0] = name_len + 1;
+
+		memcpy(ptr + 2, hdev->dev_name, name_len);
+
+		eir_len += (name_len + 2);
+		ptr += (name_len + 2);
+	}
+
+	memset(uuid16_list, 0, sizeof(uuid16_list));
+
+	/* Group all UUID16 types */
+	list_for_each_entry(uuid, &hdev->uuids, list) {
+		u16 uuid16;
+
+		uuid16 = get_uuid16(uuid->uuid);
+		if (uuid16 == 0)
+			return;
+
+		if (uuid16 < 0x1100)
+			continue;
+
+		if (uuid16 == PNP_INFO_SVCLASS_ID)
+			continue;
+
+		/* Stop if not enough space to put next UUID */
+		if (eir_len + 2 + sizeof(u16) > HCI_MAX_EIR_LENGTH) {
+			truncated = 1;
+			break;
+		}
+
+		/* Check for duplicates */
+		for (i = 0; uuid16_list[i] != 0; i++)
+			if (uuid16_list[i] == uuid16)
+				break;
+
+		if (uuid16_list[i] == 0) {
+			uuid16_list[i] = uuid16;
+			eir_len += sizeof(u16);
+		}
+	}
+
+	if (uuid16_list[0] != 0) {
+		u8 *length = ptr;
+
+		/* EIR Data type */
+		ptr[1] = truncated ? EIR_UUID16_SOME : EIR_UUID16_ALL;
+
+		ptr += 2;
+		eir_len += 2;
+
+		for (i = 0; uuid16_list[i] != 0; i++) {
+			*ptr++ = (uuid16_list[i] & 0x00ff);
+			*ptr++ = (uuid16_list[i] & 0xff00) >> 8;
+		}
+
+		/* EIR Data length */
+		*length = (i * sizeof(u16)) + 1;
+	}
+}
+
+static int update_eir(struct hci_dev *hdev)
+{
+	struct hci_cp_write_eir cp;
+
+	if (!(hdev->features[6] & LMP_EXT_INQ))
+		return 0;
+
+	if (hdev->ssp_mode == 0)
+		return 0;
+
+	if (test_bit(HCI_SERVICE_CACHE, &hdev->flags))
+		return 0;
+
+	memset(&cp, 0, sizeof(cp));
+
+	create_eir(hdev, cp.data);
+
+	if (memcmp(cp.data, hdev->eir, sizeof(cp.data)) == 0)
+		return 0;
+
+	memcpy(hdev->eir, cp.data, sizeof(cp.data));
+
+	return hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
+}
+
+static u8 get_service_classes(struct hci_dev *hdev)
+{
+	struct bt_uuid *uuid;
+	u8 val = 0;
+
+	list_for_each_entry(uuid, &hdev->uuids, list)
+		val |= uuid->svc_hint;
+
+	return val;
+}
+
+static int update_class(struct hci_dev *hdev)
+{
+	u8 cod[3];
+
+	BT_DBG("%s", hdev->name);
+
+	if (test_bit(HCI_SERVICE_CACHE, &hdev->flags))
+		return 0;
+
+	cod[0] = hdev->minor_class;
+	cod[1] = hdev->major_class;
+	cod[2] = get_service_classes(hdev);
+
+	if (memcmp(cod, hdev->dev_class, 3) == 0)
+		return 0;
+
+	return hci_send_cmd(hdev, HCI_OP_WRITE_CLASS_OF_DEV, sizeof(cod), cod);
+}
+
 static int read_controller_info(struct sock *sk, u16 index)
 {
 	struct mgmt_rp_read_info rp;
@@ -681,179 +854,6 @@ failed:
 	return err;
 }
 
-#define EIR_FLAGS		0x01 /* flags */
-#define EIR_UUID16_SOME		0x02 /* 16-bit UUID, more available */
-#define EIR_UUID16_ALL		0x03 /* 16-bit UUID, all listed */
-#define EIR_UUID32_SOME		0x04 /* 32-bit UUID, more available */
-#define EIR_UUID32_ALL		0x05 /* 32-bit UUID, all listed */
-#define EIR_UUID128_SOME	0x06 /* 128-bit UUID, more available */
-#define EIR_UUID128_ALL		0x07 /* 128-bit UUID, all listed */
-#define EIR_NAME_SHORT		0x08 /* shortened local name */
-#define EIR_NAME_COMPLETE	0x09 /* complete local name */
-#define EIR_TX_POWER		0x0A /* transmit power level */
-#define EIR_DEVICE_ID		0x10 /* device ID */
-
-#define PNP_INFO_SVCLASS_ID		0x1200
-
-static u8 bluetooth_base_uuid[] = {
-			0xFB, 0x34, 0x9B, 0x5F, 0x80, 0x00, 0x00, 0x80,
-			0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-};
-
-static u16 get_uuid16(u8 *uuid128)
-{
-	u32 val;
-	int i;
-
-	for (i = 0; i < 12; i++) {
-		if (bluetooth_base_uuid[i] != uuid128[i])
-			return 0;
-	}
-
-	memcpy(&val, &uuid128[12], 4);
-
-	val = le32_to_cpu(val);
-	if (val > 0xffff)
-		return 0;
-
-	return (u16) val;
-}
-
-static void create_eir(struct hci_dev *hdev, u8 *data)
-{
-	u8 *ptr = data;
-	u16 eir_len = 0;
-	u16 uuid16_list[HCI_MAX_EIR_LENGTH / sizeof(u16)];
-	int i, truncated = 0;
-	struct bt_uuid *uuid;
-	size_t name_len;
-
-	name_len = strlen(hdev->dev_name);
-
-	if (name_len > 0) {
-		/* EIR Data type */
-		if (name_len > 48) {
-			name_len = 48;
-			ptr[1] = EIR_NAME_SHORT;
-		} else
-			ptr[1] = EIR_NAME_COMPLETE;
-
-		/* EIR Data length */
-		ptr[0] = name_len + 1;
-
-		memcpy(ptr + 2, hdev->dev_name, name_len);
-
-		eir_len += (name_len + 2);
-		ptr += (name_len + 2);
-	}
-
-	memset(uuid16_list, 0, sizeof(uuid16_list));
-
-	/* Group all UUID16 types */
-	list_for_each_entry(uuid, &hdev->uuids, list) {
-		u16 uuid16;
-
-		uuid16 = get_uuid16(uuid->uuid);
-		if (uuid16 == 0)
-			return;
-
-		if (uuid16 < 0x1100)
-			continue;
-
-		if (uuid16 == PNP_INFO_SVCLASS_ID)
-			continue;
-
-		/* Stop if not enough space to put next UUID */
-		if (eir_len + 2 + sizeof(u16) > HCI_MAX_EIR_LENGTH) {
-			truncated = 1;
-			break;
-		}
-
-		/* Check for duplicates */
-		for (i = 0; uuid16_list[i] != 0; i++)
-			if (uuid16_list[i] == uuid16)
-				break;
-
-		if (uuid16_list[i] == 0) {
-			uuid16_list[i] = uuid16;
-			eir_len += sizeof(u16);
-		}
-	}
-
-	if (uuid16_list[0] != 0) {
-		u8 *length = ptr;
-
-		/* EIR Data type */
-		ptr[1] = truncated ? EIR_UUID16_SOME : EIR_UUID16_ALL;
-
-		ptr += 2;
-		eir_len += 2;
-
-		for (i = 0; uuid16_list[i] != 0; i++) {
-			*ptr++ = (uuid16_list[i] & 0x00ff);
-			*ptr++ = (uuid16_list[i] & 0xff00) >> 8;
-		}
-
-		/* EIR Data length */
-		*length = (i * sizeof(u16)) + 1;
-	}
-}
-
-static int update_eir(struct hci_dev *hdev)
-{
-	struct hci_cp_write_eir cp;
-
-	if (!(hdev->features[6] & LMP_EXT_INQ))
-		return 0;
-
-	if (hdev->ssp_mode == 0)
-		return 0;
-
-	if (test_bit(HCI_SERVICE_CACHE, &hdev->flags))
-		return 0;
-
-	memset(&cp, 0, sizeof(cp));
-
-	create_eir(hdev, cp.data);
-
-	if (memcmp(cp.data, hdev->eir, sizeof(cp.data)) == 0)
-		return 0;
-
-	memcpy(hdev->eir, cp.data, sizeof(cp.data));
-
-	return hci_send_cmd(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp);
-}
-
-static u8 get_service_classes(struct hci_dev *hdev)
-{
-	struct bt_uuid *uuid;
-	u8 val = 0;
-
-	list_for_each_entry(uuid, &hdev->uuids, list)
-		val |= uuid->svc_hint;
-
-	return val;
-}
-
-static int update_class(struct hci_dev *hdev)
-{
-	u8 cod[3];
-
-	BT_DBG("%s", hdev->name);
-
-	if (test_bit(HCI_SERVICE_CACHE, &hdev->flags))
-		return 0;
-
-	cod[0] = hdev->minor_class;
-	cod[1] = hdev->major_class;
-	cod[2] = get_service_classes(hdev);
-
-	if (memcmp(cod, hdev->dev_class, 3) == 0)
-		return 0;
-
-	return hci_send_cmd(hdev, HCI_OP_WRITE_CLASS_OF_DEV, sizeof(cod), cod);
-}
-
 static int add_uuid(struct sock *sk, u16 index, unsigned char *data, u16 len)
 {
 	struct mgmt_cp_add_uuid *cp;
-- 
1.7.7.3

Re: [PATCH 4/7] Bluetooth: Move EIR and CoD update functions to a better position

[Marcel Holtmann]

Hi Johan,

> Due to the upcoming addition of a service cache timer the functions to
> update the EIR and CoD need to be higher up in mgmt.c in order to avoid
> unnecessary forward-declarations. This patch simply moves code around
> without any other changes in order to make subsequent patches more
> readable.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |  346 +++++++++++++++++++++++++-------------------------
>  1 files changed, 173 insertions(+), 173 deletions(-)

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

Regards

Marcel

[PATCH 5/7] Bluetooth: Add timer for automatically disabling the service cache

[johan.hedberg]

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

We do not want the service cache to be enabled indefinitely after
mgmt_read_info is called. The solve this a timer is added which will
automaticall disable the cache if mgmt_set_dev_class isn't called within
5 seconds of calling mgmt_read_info.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_core.c         |    3 ++
 net/bluetooth/mgmt.c             |   40 +++++++++++++++++++++++++++++++++----
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4e67bb8..a5d7e42 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -200,6 +200,8 @@ struct hci_dev {
 	__u16			discov_timeout;
 	struct delayed_work	discov_off;
 
+	struct delayed_work	service_cache;
+
 	struct timer_list	cmd_timer;
 	struct tasklet_struct	cmd_task;
 	struct tasklet_struct	rx_task;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ce3727e..323be52 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -597,6 +597,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 	if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->flags))
 		cancel_delayed_work(&hdev->power_off);
 
+	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
+		cancel_delayed_work(&hdev->service_cache);
+
 	hci_dev_lock_bh(hdev);
 	inquiry_cache_flush(hdev);
 	hci_conn_hash_flush(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 696e585..4d531b9 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -35,6 +35,8 @@
 
 #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
 
+#define SERVICE_CACHE_TIMEOUT (5 * 1000)
+
 struct pending_cmd {
 	struct list_head list;
 	u16 opcode;
@@ -472,6 +474,32 @@ static int update_class(struct hci_dev *hdev)
 	return hci_send_cmd(hdev, HCI_OP_WRITE_CLASS_OF_DEV, sizeof(cod), cod);
 }
 
+static void service_cache_off(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev,
+							service_cache.work);
+
+	if (!test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
+		return;
+
+	hci_dev_lock_bh(hdev);
+
+	update_eir(hdev);
+	update_class(hdev);
+
+	hci_dev_unlock_bh(hdev);
+}
+
+static void mgmt_init_hdev(struct hci_dev *hdev)
+{
+	if (!test_and_set_bit(HCI_MGMT, &hdev->flags))
+		INIT_DELAYED_WORK(&hdev->service_cache, service_cache_off);
+
+	if (!test_and_set_bit(HCI_SERVICE_CACHE, &hdev->flags))
+		queue_delayed_work(hdev->workqueue, &hdev->service_cache,
+				msecs_to_jiffies(SERVICE_CACHE_TIMEOUT));
+}
+
 static int read_controller_info(struct sock *sk, u16 index)
 {
 	struct mgmt_rp_read_info rp;
@@ -489,10 +517,8 @@ static int read_controller_info(struct sock *sk, u16 index)
 
 	hci_dev_lock_bh(hdev);
 
-	if (test_and_clear_bit(HCI_PI_MGMT_INIT, &hci_pi(sk)->flags)) {
-		set_bit(HCI_MGMT, &hdev->flags);
-		set_bit(HCI_SERVICE_CACHE, &hdev->flags);
-	}
+	if (test_and_clear_bit(HCI_PI_MGMT_INIT, &hci_pi(sk)->flags))
+		mgmt_init_hdev(hdev);
 
 	memset(&rp, 0, sizeof(rp));
 
@@ -992,8 +1018,12 @@ static int set_dev_class(struct sock *sk, u16 index, unsigned char *data,
 	hdev->major_class = cp->major;
 	hdev->minor_class = cp->minor;
 
-	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
+	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags)) {
+		hci_dev_unlock_bh(hdev);
+		cancel_delayed_work_sync(&hdev->service_cache);
+		hci_dev_lock_bh(hdev);
 		update_eir(hdev);
+	}
 
 	err = update_class(hdev);
 
-- 
1.7.7.3

Re: [PATCH 5/7] Bluetooth: Add timer for automatically disabling the service cache

[Marcel Holtmann]

Hi Johan,

> We do not want the service cache to be enabled indefinitely after
> mgmt_read_info is called. The solve this a timer is added which will
> automaticall disable the cache if mgmt_set_dev_class isn't called within

it is "automatically".

> 5 seconds of calling mgmt_read_info.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    2 +
>  net/bluetooth/hci_core.c         |    3 ++
>  net/bluetooth/mgmt.c             |   40 +++++++++++++++++++++++++++++++++----
>  3 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 4e67bb8..a5d7e42 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -200,6 +200,8 @@ struct hci_dev {
>  	__u16			discov_timeout;
>  	struct delayed_work	discov_off;
>  
> +	struct delayed_work	service_cache;
> +
>  	struct timer_list	cmd_timer;
>  	struct tasklet_struct	cmd_task;
>  	struct tasklet_struct	rx_task;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index ce3727e..323be52 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -597,6 +597,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>  	if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->flags))
>  		cancel_delayed_work(&hdev->power_off);
>  
> +	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
> +		cancel_delayed_work(&hdev->service_cache);
> +
>  	hci_dev_lock_bh(hdev);
>  	inquiry_cache_flush(hdev);
>  	hci_conn_hash_flush(hdev);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 696e585..4d531b9 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -35,6 +35,8 @@
>  
>  #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>  
> +#define SERVICE_CACHE_TIMEOUT (5 * 1000)
> +
>  struct pending_cmd {
>  	struct list_head list;
>  	u16 opcode;
> @@ -472,6 +474,32 @@ static int update_class(struct hci_dev *hdev)
>  	return hci_send_cmd(hdev, HCI_OP_WRITE_CLASS_OF_DEV, sizeof(cod), cod);
>  }
>  
> +static void service_cache_off(struct work_struct *work)
> +{
> +	struct hci_dev *hdev = container_of(work, struct hci_dev,
> +							service_cache.work);
> +
> +	if (!test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
> +		return;
> +
> +	hci_dev_lock_bh(hdev);
> +
> +	update_eir(hdev);
> +	update_class(hdev);
> +
> +	hci_dev_unlock_bh(hdev);
> +}
> +
> +static void mgmt_init_hdev(struct hci_dev *hdev)
> +{
> +	if (!test_and_set_bit(HCI_MGMT, &hdev->flags))
> +		INIT_DELAYED_WORK(&hdev->service_cache, service_cache_off);

Why does this depend on MGMT being enabled? Why do we bother here? Why
not just always initialize it when registering the controller.

> +	if (!test_and_set_bit(HCI_SERVICE_CACHE, &hdev->flags))
> +		queue_delayed_work(hdev->workqueue, &hdev->service_cache,
> +				msecs_to_jiffies(SERVICE_CACHE_TIMEOUT));
> +}
> +
>  static int read_controller_info(struct sock *sk, u16 index)
>  {
>  	struct mgmt_rp_read_info rp;
> @@ -489,10 +517,8 @@ static int read_controller_info(struct sock *sk, u16 index)
>  
>  	hci_dev_lock_bh(hdev);
>  
> -	if (test_and_clear_bit(HCI_PI_MGMT_INIT, &hci_pi(sk)->flags)) {
> -		set_bit(HCI_MGMT, &hdev->flags);
> -		set_bit(HCI_SERVICE_CACHE, &hdev->flags);
> -	}
> +	if (test_and_clear_bit(HCI_PI_MGMT_INIT, &hci_pi(sk)->flags))
> +		mgmt_init_hdev(hdev);
>  
>  	memset(&rp, 0, sizeof(rp));
>  
> @@ -992,8 +1018,12 @@ static int set_dev_class(struct sock *sk, u16 index, unsigned char *data,
>  	hdev->major_class = cp->major;
>  	hdev->minor_class = cp->minor;
>  
> -	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
> +	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags)) {
> +		hci_dev_unlock_bh(hdev);
> +		cancel_delayed_work_sync(&hdev->service_cache);
> +		hci_dev_lock_bh(hdev);
>  		update_eir(hdev);
> +	}

We have to be a bit careful here since essentially the service cache
(actually UUID cache to be precise) is a per mgmt socket feature and not
a per hdev feature.

I have nothing against merging this first, but keep in mind that this
needs to be updated since we can have two concurrent applications
opening a mgmt socket.

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

Regards

Marcel

Re: [PATCH 5/7] Bluetooth: Add timer for automatically disabling the service cache

[Johan Hedberg]

Hi Marcel,

On Wed, Dec 14, 2011, Marcel Holtmann wrote:
> > +static void service_cache_off(struct work_struct *work)
> > +{
> > +	struct hci_dev *hdev = container_of(work, struct hci_dev,
> > +							service_cache.work);
> > +
> > +	if (!test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
> > +		return;
> > +
> > +	hci_dev_lock_bh(hdev);
> > +
> > +	update_eir(hdev);
> > +	update_class(hdev);
> > +
> > +	hci_dev_unlock_bh(hdev);
> > +}
> > +
> > +static void mgmt_init_hdev(struct hci_dev *hdev)
> > +{
> > +	if (!test_and_set_bit(HCI_MGMT, &hdev->flags))
> > +		INIT_DELAYED_WORK(&hdev->service_cache, service_cache_off);
> 
> Why does this depend on MGMT being enabled? Why do we bother here? Why
> not just always initialize it when registering the controller.

Because the timer callback function is static and resides within mgmt.c
(since it needs to call update_class & update_eir which are also static
within mgmt.c).

> > -	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
> > +	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags)) {
> > +		hci_dev_unlock_bh(hdev);
> > +		cancel_delayed_work_sync(&hdev->service_cache);
> > +		hci_dev_lock_bh(hdev);
> >  		update_eir(hdev);
> > +	}
> 
> We have to be a bit careful here since essentially the service cache
> (actually UUID cache to be precise) is a per mgmt socket feature and not
> a per hdev feature.
> 
> I have nothing against merging this first, but keep in mind that this
> needs to be updated since we can have two concurrent applications
> opening a mgmt socket.

I understand. It does get a bit tricky though since the UUID list is per
HCI device. I.e. do we keep our own list for each socket and hide it
from struct hci_dev until the (per-socket) cache gets disabled?

Johan

Re: [PATCH 5/7] Bluetooth: Add timer for automatically disabling the service cache

[Marcel Holtmann]

Hi Johan,

> > > -	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags))
> > > +	if (test_and_clear_bit(HCI_SERVICE_CACHE, &hdev->flags)) {
> > > +		hci_dev_unlock_bh(hdev);
> > > +		cancel_delayed_work_sync(&hdev->service_cache);
> > > +		hci_dev_lock_bh(hdev);
> > >  		update_eir(hdev);
> > > +	}
> > 
> > We have to be a bit careful here since essentially the service cache
> > (actually UUID cache to be precise) is a per mgmt socket feature and not
> > a per hdev feature.
> > 
> > I have nothing against merging this first, but keep in mind that this
> > needs to be updated since we can have two concurrent applications
> > opening a mgmt socket.
> 
> I understand. It does get a bit tricky though since the UUID list is per
> HCI device. I.e. do we keep our own list for each socket and hide it
> from struct hci_dev until the (per-socket) cache gets disabled?

that might be a good idea. Every mgmt socket should keep its list of
added UUIDs and then maybe just call an update function that runs over
all mgmt sockets and picks the union of it. If a socket closes, the list
gets flushed and we update again. That way we do not have to play global
reference counting tricks.

Regards

Marcel

[PATCH 6/7] Bluetooth: Update ordering and opcodes of mgmt messages

[johan.hedberg]

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

This patch updates the ordering and opcodes of mgmt messages to match
the latest API specification.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/mgmt.h |  147 +++++++++++++++++++++++-------------------
 1 files changed, 81 insertions(+), 66 deletions(-)

diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index bdb0a58..2b1059d 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -100,27 +100,40 @@ struct mgmt_cp_set_discoverable {
 
 #define MGMT_OP_SET_CONNECTABLE		0x0007
 
-#define MGMT_OP_SET_FAST_CONNECTABLE	0x001F
+#define MGMT_OP_SET_FAST_CONNECTABLE	0x0008
 
-#define MGMT_OP_SET_PAIRABLE		0x0008
+#define MGMT_OP_SET_PAIRABLE		0x0009
 
-#define MGMT_OP_ADD_UUID		0x0009
+#define MGMT_OP_SET_LINK_SECURITY	0x000A
+
+#define MGMT_OP_SET_SSP			0x000B
+
+#define MGMT_OP_SET_HS			0x000C
+
+#define MGMT_OP_SET_LE			0x000D
+
+#define MGMT_OP_SET_DEV_CLASS		0x000E
+struct mgmt_cp_set_dev_class {
+	__u8 major;
+	__u8 minor;
+} __packed;
+
+#define MGMT_OP_SET_LOCAL_NAME		0x000F
+struct mgmt_cp_set_local_name {
+	__u8 name[MGMT_MAX_NAME_LENGTH];
+} __packed;
+
+#define MGMT_OP_ADD_UUID		0x0010
 struct mgmt_cp_add_uuid {
 	__u8 uuid[16];
 	__u8 svc_hint;
 } __packed;
 
-#define MGMT_OP_REMOVE_UUID		0x000A
+#define MGMT_OP_REMOVE_UUID		0x0011
 struct mgmt_cp_remove_uuid {
 	__u8 uuid[16];
 } __packed;
 
-#define MGMT_OP_SET_DEV_CLASS		0x000B
-struct mgmt_cp_set_dev_class {
-	__u8 major;
-	__u8 minor;
-} __packed;
-
 struct mgmt_link_key_info {
 	bdaddr_t bdaddr;
 	u8 type;
@@ -128,14 +141,14 @@ struct mgmt_link_key_info {
 	u8 pin_len;
 } __packed;
 
-#define MGMT_OP_LOAD_LINK_KEYS		0x000D
+#define MGMT_OP_LOAD_LINK_KEYS		0x0012
 struct mgmt_cp_load_link_keys {
 	__u8 debug_keys;
 	__le16 key_count;
 	struct mgmt_link_key_info keys[0];
 } __packed;
 
-#define MGMT_OP_REMOVE_KEYS		0x000E
+#define MGMT_OP_REMOVE_KEYS		0x0013
 struct mgmt_cp_remove_keys {
 	bdaddr_t bdaddr;
 	__u8 disconnect;
@@ -145,7 +158,7 @@ struct mgmt_rp_remove_keys {
 	__u8 status;
 };
 
-#define MGMT_OP_DISCONNECT		0x000F
+#define MGMT_OP_DISCONNECT		0x0014
 struct mgmt_cp_disconnect {
 	bdaddr_t bdaddr;
 } __packed;
@@ -164,13 +177,13 @@ struct mgmt_addr_info {
 	__u8 type;
 } __packed;
 
-#define MGMT_OP_GET_CONNECTIONS		0x0010
+#define MGMT_OP_GET_CONNECTIONS		0x0015
 struct mgmt_rp_get_connections {
 	__le16 conn_count;
 	struct mgmt_addr_info addr[0];
 } __packed;
 
-#define MGMT_OP_PIN_CODE_REPLY		0x0011
+#define MGMT_OP_PIN_CODE_REPLY		0x0016
 struct mgmt_cp_pin_code_reply {
 	bdaddr_t bdaddr;
 	__u8 pin_len;
@@ -181,17 +194,17 @@ struct mgmt_rp_pin_code_reply {
 	uint8_t status;
 } __packed;
 
-#define MGMT_OP_PIN_CODE_NEG_REPLY	0x0012
+#define MGMT_OP_PIN_CODE_NEG_REPLY	0x0017
 struct mgmt_cp_pin_code_neg_reply {
 	bdaddr_t bdaddr;
 } __packed;
 
-#define MGMT_OP_SET_IO_CAPABILITY	0x0013
+#define MGMT_OP_SET_IO_CAPABILITY	0x0018
 struct mgmt_cp_set_io_capability {
 	__u8 io_capability;
 } __packed;
 
-#define MGMT_OP_PAIR_DEVICE		0x0014
+#define MGMT_OP_PAIR_DEVICE		0x0019
 struct mgmt_cp_pair_device {
 	struct mgmt_addr_info addr;
 	__u8 io_cap;
@@ -201,7 +214,7 @@ struct mgmt_rp_pair_device {
 	__u8 status;
 } __packed;
 
-#define MGMT_OP_USER_CONFIRM_REPLY	0x0015
+#define MGMT_OP_USER_CONFIRM_REPLY	0x001A
 struct mgmt_cp_user_confirm_reply {
 	bdaddr_t bdaddr;
 } __packed;
@@ -210,59 +223,61 @@ struct mgmt_rp_user_confirm_reply {
 	__u8 status;
 } __packed;
 
-#define MGMT_OP_USER_CONFIRM_NEG_REPLY	0x0016
+#define MGMT_OP_USER_CONFIRM_NEG_REPLY	0x001B
+struct mgmt_cp_user_confirm_neg_reply {
+	bdaddr_t bdaddr;
+} __packed;
 
-#define MGMT_OP_SET_LOCAL_NAME		0x0017
-struct mgmt_cp_set_local_name {
-	__u8 name[MGMT_MAX_NAME_LENGTH];
+#define MGMT_OP_USER_PASSKEY_REPLY	0x001C
+struct mgmt_cp_user_passkey_reply {
+	bdaddr_t bdaddr;
+	__le32 passkey;
+} __packed;
+struct mgmt_rp_user_passkey_reply {
+	bdaddr_t bdaddr;
+	__u8 status;
+} __packed;
+
+#define MGMT_OP_USER_PASSKEY_NEG_REPLY	0x001D
+struct mgmt_cp_user_passkey_neg_reply {
+	bdaddr_t bdaddr;
 } __packed;
 
-#define MGMT_OP_READ_LOCAL_OOB_DATA	0x0018
+#define MGMT_OP_READ_LOCAL_OOB_DATA	0x001E
 struct mgmt_rp_read_local_oob_data {
 	__u8 hash[16];
 	__u8 randomizer[16];
 } __packed;
 
-#define MGMT_OP_ADD_REMOTE_OOB_DATA	0x0019
+#define MGMT_OP_ADD_REMOTE_OOB_DATA	0x001F
 struct mgmt_cp_add_remote_oob_data {
 	bdaddr_t bdaddr;
 	__u8 hash[16];
 	__u8 randomizer[16];
 } __packed;
 
-#define MGMT_OP_REMOVE_REMOTE_OOB_DATA	0x001A
+#define MGMT_OP_REMOVE_REMOTE_OOB_DATA	0x0020
 struct mgmt_cp_remove_remote_oob_data {
 	bdaddr_t bdaddr;
 } __packed;
 
-#define MGMT_OP_START_DISCOVERY		0x001B
+#define MGMT_OP_START_DISCOVERY		0x0021
 struct mgmt_cp_start_discovery {
 	__u8 type;
 } __packed;
 
-#define MGMT_OP_STOP_DISCOVERY		0x001C
+#define MGMT_OP_STOP_DISCOVERY		0x0022
 
-#define MGMT_OP_BLOCK_DEVICE		0x001D
+#define MGMT_OP_BLOCK_DEVICE		0x0023
 struct mgmt_cp_block_device {
 	bdaddr_t bdaddr;
 } __packed;
 
-#define MGMT_OP_UNBLOCK_DEVICE		0x001E
+#define MGMT_OP_UNBLOCK_DEVICE		0x0024
 struct mgmt_cp_unblock_device {
 	bdaddr_t bdaddr;
 } __packed;
 
-#define MGMT_OP_USER_PASSKEY_REPLY	0x0020
-struct mgmt_cp_user_passkey_reply {
-	bdaddr_t bdaddr;
-	__le32 passkey;
-} __packed;
-
-#define MGMT_OP_USER_PASSKEY_NEG_REPLY	0x0021
-struct mgmt_cp_user_passkey_neg_reply {
-	bdaddr_t bdaddr;
-} __packed;
-
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16 opcode;
@@ -286,53 +301,58 @@ struct mgmt_ev_controller_error {
 
 #define MGMT_EV_NEW_SETTINGS		0x0006
 
-#define MGMT_EV_DISCOVERABLE		0x0007
-
-#define MGMT_EV_CONNECTABLE		0x0008
+#define MGMT_EV_CLASS_OF_DEV_CHANGED	0x0007
+struct mgmt_ev_class_of_dev_changed {
+	__u8 dev_class[3];
+};
 
-#define MGMT_EV_PAIRABLE		0x0009
+#define MGMT_EV_LOCAL_NAME_CHANGED	0x0008
+struct mgmt_ev_local_name_changed {
+	__u8 name[MGMT_MAX_NAME_LENGTH];
+	__u8 short_name[MGMT_MAX_SHORT_NAME_LENGTH];
+} __packed;
 
-#define MGMT_EV_NEW_LINK_KEY		0x000A
+#define MGMT_EV_NEW_LINK_KEY		0x0009
 struct mgmt_ev_new_link_key {
 	__u8 store_hint;
 	struct mgmt_link_key_info key;
 } __packed;
 
-#define MGMT_EV_CONNECTED		0x000B
+#define MGMT_EV_CONNECTED		0x000A
 
-#define MGMT_EV_DISCONNECTED		0x000C
+#define MGMT_EV_DISCONNECTED		0x000B
 
-#define MGMT_EV_CONNECT_FAILED		0x000D
+#define MGMT_EV_CONNECT_FAILED		0x000C
 struct mgmt_ev_connect_failed {
 	struct mgmt_addr_info addr;
 	__u8 status;
 } __packed;
 
-#define MGMT_EV_PIN_CODE_REQUEST	0x000E
+#define MGMT_EV_PIN_CODE_REQUEST	0x000D
 struct mgmt_ev_pin_code_request {
 	bdaddr_t bdaddr;
 	__u8 secure;
 } __packed;
 
-#define MGMT_EV_USER_CONFIRM_REQUEST	0x000F
+#define MGMT_EV_USER_CONFIRM_REQUEST	0x000E
 struct mgmt_ev_user_confirm_request {
 	bdaddr_t bdaddr;
 	__u8 confirm_hint;
 	__le32 value;
 } __packed;
 
+#define MGMT_EV_USER_PASSKEY_REQUEST	0x000F
+struct mgmt_ev_user_passkey_request {
+	bdaddr_t bdaddr;
+} __packed;
+
 #define MGMT_EV_AUTH_FAILED		0x0010
 struct mgmt_ev_auth_failed {
 	bdaddr_t bdaddr;
 	__u8 status;
 } __packed;
 
-#define MGMT_EV_LOCAL_NAME_CHANGED	0x0011
-struct mgmt_ev_local_name_changed {
-	__u8 name[MGMT_MAX_NAME_LENGTH];
-} __packed;
-
-#define MGMT_EV_DEVICE_FOUND		0x0012
+#define MGMT_EV_DEVICE_FOUND		0x0011
 struct mgmt_ev_device_found {
 	struct mgmt_addr_info addr;
 	__u8 dev_class[3];
@@ -340,25 +360,20 @@ struct mgmt_ev_device_found {
 	__u8 eir[HCI_MAX_EIR_LENGTH];
 } __packed;
 
-#define MGMT_EV_REMOTE_NAME		0x0013
+#define MGMT_EV_REMOTE_NAME		0x0012
 struct mgmt_ev_remote_name {
 	bdaddr_t bdaddr;
 	__u8 name[MGMT_MAX_NAME_LENGTH];
 } __packed;
 
-#define MGMT_EV_DISCOVERING		0x0014
+#define MGMT_EV_DISCOVERING		0x0013
 
-#define MGMT_EV_DEVICE_BLOCKED		0x0015
+#define MGMT_EV_DEVICE_BLOCKED		0x0014
 struct mgmt_ev_device_blocked {
 	bdaddr_t bdaddr;
 } __packed;
 
-#define MGMT_EV_DEVICE_UNBLOCKED	0x0016
+#define MGMT_EV_DEVICE_UNBLOCKED	0x0015
 struct mgmt_ev_device_unblocked {
 	bdaddr_t bdaddr;
 } __packed;
-
-#define MGMT_EV_USER_PASSKEY_REQUEST	0x0017
-struct mgmt_ev_user_passkey_request {
-	bdaddr_t bdaddr;
-} __packed;
-- 
1.7.7.3

Re: [PATCH 6/7] Bluetooth: Update ordering and opcodes of mgmt messages

[Marcel Holtmann]

Hi Johan,

> This patch updates the ordering and opcodes of mgmt messages to match
> the latest API specification.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/mgmt.h |  147 +++++++++++++++++++++++-------------------
>  1 files changed, 81 insertions(+), 66 deletions(-)

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

Regards

Marcel

[PATCH 7/7] Bluetooth: Use correct struct for user_confirm_neg_reply

[johan.hedberg]

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

This patch fixes user_confirm_neg_reply to use the appropriate struct
for accessing the call parameters.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 4d531b9..054229f 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1690,7 +1690,7 @@ static int user_confirm_reply(struct sock *sk, u16 index, void *data, u16 len)
 static int user_confirm_neg_reply(struct sock *sk, u16 index, void *data,
 									u16 len)
 {
-	struct mgmt_cp_user_confirm_reply *cp = (void *) data;
+	struct mgmt_cp_user_confirm_neg_reply *cp = (void *) data;
 
 	BT_DBG("");
 
-- 
1.7.7.3

Re: [PATCH 7/7] Bluetooth: Use correct struct for user_confirm_neg_reply

[Marcel Holtmann]

Hi Johan,

> This patch fixes user_confirm_neg_reply to use the appropriate struct
> for accessing the call parameters.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/mgmt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 4d531b9..054229f 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1690,7 +1690,7 @@ static int user_confirm_reply(struct sock *sk, u16 index, void *data, u16 len)
>  static int user_confirm_neg_reply(struct sock *sk, u16 index, void *data,
>  									u16 len)
>  {
> -	struct mgmt_cp_user_confirm_reply *cp = (void *) data;
> +	struct mgmt_cp_user_confirm_neg_reply *cp = (void *) data;

the cast here seems pointless. Just remove that altogether. Otherwise
fine with me.

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

Regards

Marcel