[PATCH 0/9] AMP interface and signal framework

[PATCH 0/9] AMP interface and signal framework

[Mat Martineau]

This patch set includes the AMP policy socket option that was
previously discussed and accepted in to bluetooth-testing, and the
signaling framework for AMP channel creation and AMP channel moves.

There is overlap with Andrei Emeltchenko's RFC ("L2CAP and HCI
preparation code for A2MP") earlier this week.  We (and Andrei) aren't
trying to have dueling patch sets - we very much want to cooperate and
get the AMP functionality upstreamed efficiently.  We were in touch
with him earlier in the week to see how far he was with the porting
effort.

Next steps:
 * L2CAP lockstep configuration
 * HCI layer changes for AMP HCI commands and physical/logical links
 * The rest of the create channel and move channel code
 * Move channel coordination with ERTM
 * AMP manager
 * ERTM optimizations

Mat Martineau (9):
  Bluetooth: Add BT_AMP_POLICY socket option
  Bluetooth: Add AMP policy information to l2cap_chan
  Bluetooth: Get/set AMP policy socket option
  Bluetooth: Add AMP-related data and structures for channel signals
  Bluetooth: Add signal handlers for channel creation
  Bluetooth: Add definitions for L2CAP fixed channels
  Bluetooth: Use symbolic values for the fixed channel map
  Bluetooth: Add AMP header file
  Bluetooth: Add signal handlers for channel moves

 include/net/bluetooth/amp.h       |   20 +++++
 include/net/bluetooth/bluetooth.h |   27 ++++++
 include/net/bluetooth/l2cap.h     |   65 ++++++++++++++
 net/bluetooth/l2cap_core.c        |  172 ++++++++++++++++++++++++++++++++++++-
 net/bluetooth/l2cap_sock.c        |   25 ++++++
 5 files changed, 308 insertions(+), 1 deletions(-)
 create mode 100644 include/net/bluetooth/amp.h

-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 1/9] Bluetooth: Add BT_AMP_POLICY socket option

[Mat Martineau]

Allow control of AMP functionality on L2CAP sockets. By default,
connections will be restricted to BR/EDR.  Manipulating the
BT_AMP_POLICY option allows for channels to be moved to or created
on AMP controllers.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index e727555..14ae418 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -77,6 +77,33 @@ struct bt_power {
 #define BT_POWER_FORCE_ACTIVE_OFF 0
 #define BT_POWER_FORCE_ACTIVE_ON  1
 
+#define BT_AMP_POLICY	10
+
+/* Require BR/EDR (default policy)
+ *   AMP controllers cannot be used.
+ *   Channel move requests from the remote device are denied.
+ *   If the L2CAP channel is currently using AMP, move the channel to BR/EDR.
+ */
+#define BT_AMP_POLICY_REQUIRE_BR_EDR   0
+
+/* Prefer AMP
+ *   Allow use of AMP controllers
+ *   If the L2CAP channel is currently on BR/EDR and AMP controller
+ *     resources are available, initiate a channel move to AMP.
+ *   Channel move requests from the remote device are allowed.
+ *   If the L2CAP socket has not been connected yet, try to create
+ *     and configure the channel directly on an AMP controller rather
+ *     than BR/EDR.
+ */
+#define BT_AMP_POLICY_PREFER_AMP       1
+
+/* Prefer BR/EDR
+ *   Allow use of AMP controllers.
+ *   If the L2CAP channel is currently on AMP, move it to BR/EDR.
+ *   Channel move requests from the remote device are allowed.
+ */
+#define BT_AMP_POLICY_PREFER_BR_EDR    2
+
 __attribute__((format (printf, 2, 3)))
 int bt_printk(const char *level, const char *fmt, ...);
 
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 2/9] Bluetooth: Add AMP policy information to l2cap_chan

[Mat Martineau]

Each channel has an active AMP policy to require BR/EDR (the default),
prefer AMP, or prefer BR/EDR.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 806b950..e77d39f 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -357,6 +357,7 @@ struct l2cap_chan {
 	__u8		num_conf_rsp;
 
 	__u8		fcs;
+	__u8		amp_policy;
 
 	__u16		tx_win;
 	__u8		max_tx;
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 3/9] Bluetooth: Get/set AMP policy socket option

[Mat Martineau]

Checks for valid policy value and L2CAP mode.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_sock.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 836d12e..9431e38 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -467,6 +467,11 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 
 		break;
 
+	case BT_AMP_POLICY:
+		if (put_user(chan->amp_policy, (u32 __user *) optval))
+			err = -EFAULT;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -690,6 +695,26 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 			clear_bit(FLAG_FORCE_ACTIVE, &chan->flags);
 		break;
 
+	case BT_AMP_POLICY:
+		if (get_user(opt, (u32 __user *) optval)) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (opt > BT_AMP_POLICY_PREFER_BR_EDR) {
+			err = -EINVAL;
+			break;
+		}
+
+		if (chan->mode != L2CAP_MODE_ERTM &&
+			 chan->mode != L2CAP_MODE_STREAMING) {
+			err = -EINVAL;
+			break;
+		}
+
+		chan->amp_policy = (u8) opt;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 4/9] Bluetooth: Add AMP-related data and structures for channel signals

[Mat Martineau]

AMP channel creation and channel moves are coordinated using the L2CAP
signaling channel.  These definitions cover the "create channel",
"move channel", and "move channel confirm" signals.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |   60 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index e77d39f..2ea0d15 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -89,6 +89,12 @@ struct l2cap_conninfo {
 #define L2CAP_ECHO_RSP		0x09
 #define L2CAP_INFO_REQ		0x0a
 #define L2CAP_INFO_RSP		0x0b
+#define L2CAP_CREATE_CHAN_REQ	0x0c
+#define L2CAP_CREATE_CHAN_RSP	0x0d
+#define L2CAP_MOVE_CHAN_REQ	0x0e
+#define L2CAP_MOVE_CHAN_RSP	0x0f
+#define L2CAP_MOVE_CHAN_CFM	0x10
+#define L2CAP_MOVE_CHAN_CFM_RSP	0x11
 #define L2CAP_CONN_PARAM_UPDATE_REQ	0x12
 #define L2CAP_CONN_PARAM_UPDATE_RSP	0x13
 
@@ -296,6 +302,60 @@ struct l2cap_info_rsp {
 	__u8        data[0];
 } __packed;
 
+struct l2cap_create_chan_req {
+	__le16      psm;
+	__le16      scid;
+	__u8        amp_id;
+} __attribute__ ((packed));
+
+struct l2cap_create_chan_rsp {
+	__le16      dcid;
+	__le16      scid;
+	__le16      result;
+	__le16      status;
+} __attribute__ ((packed));
+
+#define L2CAP_CREATE_CHAN_SUCCESS		0x0000
+#define L2CAP_CREATE_CHAN_PENDING		0x0001
+#define L2CAP_CREATE_CHAN_REFUSED_PSM		0x0002
+#define L2CAP_CREATE_CHAN_REFUSED_SECURITY	0x0003
+#define L2CAP_CREATE_CHAN_REFUSED_RESOURCES	0x0004
+#define L2CAP_CREATE_CHAN_REFUSED_CONTROLLER	0x0005
+
+#define L2CAP_CREATE_CHAN_STATUS_NONE		0x0000
+#define L2CAP_CREATE_CHAN_STATUS_AUTHENTICATION	0x0001
+#define L2CAP_CREATE_CHAN_STATUS_AUTHORIZATION	0x0002
+
+struct l2cap_move_chan_req {
+	__le16      icid;
+	__u8        dest_amp_id;
+} __attribute__ ((packed));
+
+struct l2cap_move_chan_rsp {
+	__le16      icid;
+	__le16      result;
+} __attribute__ ((packed));
+
+#define L2CAP_MOVE_CHAN_SUCCESS			0x0000
+#define L2CAP_MOVE_CHAN_PENDING			0x0001
+#define L2CAP_MOVE_CHAN_REFUSED_CONTROLLER	0x0002
+#define L2CAP_MOVE_CHAN_REFUSED_SAME_ID		0x0003
+#define L2CAP_MOVE_CHAN_REFUSED_CONFIG		0x0004
+#define L2CAP_MOVE_CHAN_REFUSED_COLLISION	0x0005
+#define L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED	0x0006
+
+struct l2cap_move_chan_cfm {
+	__le16      icid;
+	__le16      result;
+} __attribute__ ((packed));
+
+#define L2CAP_MOVE_CHAN_CONFIRMED	0x0000
+#define L2CAP_MOVE_CHAN_UNCONFIRMED	0x0001
+
+struct l2cap_move_chan_cfm_rsp {
+	__le16      icid;
+} __attribute__ ((packed));
+
 /* info type */
 #define L2CAP_IT_CL_MTU		0x0001
 #define L2CAP_IT_FEAT_MASK	0x0002
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 5/9] Bluetooth: Add signal handlers for channel creation

[Mat Martineau]

Handle both "create channel request" and "create channel response".

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_core.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 439e715..d023156 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2918,6 +2918,41 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn, struct l2cap_cm
 	return 0;
 }
 
+static inline int l2cap_create_channel_req(struct l2cap_conn *conn,
+					struct l2cap_cmd_hdr *cmd, u8 *data)
+{
+	struct l2cap_create_chan_req *req =
+		(struct l2cap_create_chan_req *) data;
+	struct l2cap_create_chan_rsp rsp;
+	u16 psm, scid;
+
+	psm = le16_to_cpu(req->psm);
+	scid = le16_to_cpu(req->scid);
+
+	BT_DBG("psm %d, scid %d, amp_id %d", (int) psm, (int) scid,
+		(int) req->amp_id);
+
+	/* Placeholder: Always reject */
+
+	rsp.dcid = 0;
+	rsp.scid = cpu_to_le16(scid);
+	rsp.result = L2CAP_CREATE_CHAN_REFUSED_CONTROLLER;
+	rsp.status = L2CAP_CREATE_CHAN_STATUS_NONE;
+
+	l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
+		       sizeof(rsp), &rsp);
+
+	return 0;
+}
+
+static inline int l2cap_create_channel_rsp(struct l2cap_conn *conn,
+					struct l2cap_cmd_hdr *cmd, u8 *data)
+{
+	BT_DBG("conn %p", conn);
+
+	return l2cap_connect_rsp(conn, cmd, data);
+}
+
 static inline int l2cap_check_conn_param(u16 min, u16 max, u16 latency,
 							u16 to_multiplier)
 {
@@ -3030,6 +3065,14 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 		err = l2cap_information_rsp(conn, cmd, data);
 		break;
 
+	case L2CAP_CREATE_CHAN_REQ:
+		err = l2cap_create_channel_req(conn, cmd, data);
+		break;
+
+	case L2CAP_CREATE_CHAN_RSP:
+		err = l2cap_create_channel_rsp(conn, cmd, data);
+		break;
+
 	default:
 		BT_ERR("Unknown BR/EDR signaling command 0x%2.2x", cmd->code);
 		err = -EINVAL;
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 6/9] Bluetooth: Add definitions for L2CAP fixed channels

[Mat Martineau]

Symbolic fixed channel IDs will be used instead of magic numbers.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 2ea0d15..e5d860e 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -114,6 +114,10 @@ struct l2cap_conninfo {
 #define L2CAP_FCS_NONE		0x00
 #define L2CAP_FCS_CRC16		0x01
 
+/* L2CAP fixed channels */
+#define L2CAP_FC_L2CAP		0x02
+#define L2CAP_FC_A2MP		0x08
+
 /* L2CAP Control Field bit masks */
 #define L2CAP_CTRL_SAR			0xC000
 #define L2CAP_CTRL_REQSEQ		0x3F00
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 7/9] Bluetooth: Use symbolic values for the fixed channel map

[Mat Martineau]

The A2MP fixed channel bit is only set when high-speed mode is enabled.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_core.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d023156..e38258b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -60,7 +60,7 @@ int disable_ertm;
 int enable_hs;
 
 static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
-static u8 l2cap_fixed_chan[8] = { 0x02, };
+static u8 l2cap_fixed_chan[8] = { L2CAP_FC_L2CAP, };
 
 static LIST_HEAD(chan_list);
 static DEFINE_RWLOCK(chan_list_lock);
@@ -2849,6 +2849,10 @@ static inline int l2cap_information_req(struct l2cap_conn *conn, struct l2cap_cm
 	} else if (type == L2CAP_IT_FIXED_CHAN) {
 		u8 buf[12];
 		struct l2cap_info_rsp *rsp = (struct l2cap_info_rsp *) buf;
+
+		if (enable_hs)
+			l2cap_fixed_chan[0] |= L2CAP_FC_A2MP;
+
 		rsp->type   = cpu_to_le16(L2CAP_IT_FIXED_CHAN);
 		rsp->result = cpu_to_le16(L2CAP_IR_SUCCESS);
 		memcpy(buf + 4, l2cap_fixed_chan, 8);
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 8/9] Bluetooth: Add AMP header file

[Mat Martineau]

This file will be home to all AMP- and A2MP-related declarations.  The
first macros map between HCI device indexes (for AMP controllers) and
AMP controller ids that are passed over the air.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/amp.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
 create mode 100644 include/net/bluetooth/amp.h

diff --git a/include/net/bluetooth/amp.h b/include/net/bluetooth/amp.h
new file mode 100644
index 0000000..b84ce53
--- /dev/null
+++ b/include/net/bluetooth/amp.h
@@ -0,0 +1,20 @@
+/*
+   Copyright (c) 2010-2011 Code Aurora Forum.  All rights reserved.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License version 2 and
+   only version 2 as published by the Free Software Foundation.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+*/
+
+#ifndef __AMP_H
+#define __AMP_H
+
+#define HCI_A2MP_ID(id)     ((id)+0x10)  /* convert HCI dev index to AMP ID */
+#define A2MP_HCI_ID(id)     ((id)-0x10)  /* convert AMP ID to HCI dev index */
+
+#endif /* __AMP_H */
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 9/9] Bluetooth: Add signal handlers for channel moves

[Mat Martineau]

AMP channels can be moved between BR/EDR and AMP controllers using a
sequence of signals. Every attempted channel move involves a series of
four signals:

   Move Initiator                 Move Responder
        |                                 |
        |       Move Channel Request      |
        |  ---------------------------->  |
        |                                 |
        |       Move Channel Response     |
        |  <----------------------------  |
        |                                 |
        |       Move Channel Confirm      |
        |  ---------------------------->  |
        |                                 |
        |  Move Channel Confirm Response  |
	|  <----------------------------  |

All four signals are sent even if the move fails.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_core.c |  123 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 123 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e38258b..ba64bab 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2957,6 +2957,112 @@ static inline int l2cap_create_channel_rsp(struct l2cap_conn *conn,
 	return l2cap_connect_rsp(conn, cmd, data);
 }
 
+static void l2cap_send_move_chan_rsp(struct l2cap_conn *conn, u8 ident,
+				u16 icid, u16 result)
+{
+	struct l2cap_move_chan_rsp rsp;
+
+	BT_DBG("icid %d, result %d", (int) icid, (int) result);
+
+	rsp.icid = cpu_to_le16(icid);
+	rsp.result = cpu_to_le16(result);
+
+	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_RSP, sizeof(rsp), &rsp);
+}
+
+static void l2cap_send_move_chan_cfm(struct l2cap_conn *conn,
+				struct l2cap_chan *chan, u16 icid, u16 result)
+{
+	struct l2cap_move_chan_cfm cfm;
+	u8 ident;
+
+	BT_DBG("icid %d, result %d", (int) icid, (int) result);
+
+	ident = l2cap_get_ident(conn);
+	if (chan)
+		chan->ident = ident;
+
+	cfm.icid = cpu_to_le16(icid);
+	cfm.result = cpu_to_le16(result);
+
+	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM, sizeof(cfm), &cfm);
+}
+
+static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
+					u16 icid)
+{
+	struct l2cap_move_chan_cfm_rsp rsp;
+
+	BT_DBG("icid %d", (int) icid);
+
+	rsp.icid = cpu_to_le16(icid);
+	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
+}
+
+static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
+					struct l2cap_cmd_hdr *cmd, u8 *data)
+{
+	struct l2cap_move_chan_req *req = (struct l2cap_move_chan_req *) data;
+	u16 icid = 0;
+	u16 result = L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED;
+
+	icid = le16_to_cpu(req->icid);
+
+	BT_DBG("icid %d, dest_amp_id %d", (int) icid, (int) req->dest_amp_id);
+
+	/* Placeholder: Always refuse */
+	l2cap_send_move_chan_rsp(conn, cmd->ident, icid, result);
+
+	return 0;
+}
+
+static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
+					struct l2cap_cmd_hdr *cmd, u8 *data)
+{
+	struct l2cap_move_chan_rsp *rsp = (struct l2cap_move_chan_rsp *) data;
+	u16 icid, result;
+
+	icid = le16_to_cpu(rsp->icid);
+	result = le16_to_cpu(rsp->result);
+
+	BT_DBG("icid %d, result %d", (int) icid, (int) result);
+
+	/* Placeholder: Always unconfirmed */
+	l2cap_send_move_chan_cfm(conn, NULL, icid, L2CAP_MOVE_CHAN_UNCONFIRMED);
+
+	return 0;
+}
+
+static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
+					struct l2cap_cmd_hdr *cmd, u8 *data)
+{
+	struct l2cap_move_chan_cfm *cfm = (struct l2cap_move_chan_cfm *) data;
+	u16 icid, result;
+
+	icid = le16_to_cpu(cfm->icid);
+	result = le16_to_cpu(cfm->result);
+
+	BT_DBG("icid %d, result %d", (int) icid, (int) result);
+
+	l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
+
+	return 0;
+}
+
+static inline int l2cap_move_channel_confirm_rsp(struct l2cap_conn *conn,
+					struct l2cap_cmd_hdr *cmd, u8 *data)
+{
+	struct l2cap_move_chan_cfm_rsp *rsp =
+		(struct l2cap_move_chan_cfm_rsp *) data;
+	u16 icid;
+
+	icid = le16_to_cpu(rsp->icid);
+
+	BT_DBG("icid %d", (int) icid);
+
+	return 0;
+}
+
 static inline int l2cap_check_conn_param(u16 min, u16 max, u16 latency,
 							u16 to_multiplier)
 {
@@ -3077,6 +3183,23 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
 		err = l2cap_create_channel_rsp(conn, cmd, data);
 		break;
 
+	case L2CAP_MOVE_CHAN_REQ:
+		err = l2cap_move_channel_req(conn, cmd, data);
+		break;
+
+	case L2CAP_MOVE_CHAN_RSP:
+		err = l2cap_move_channel_rsp(conn, cmd, data);
+		break;
+
+	case L2CAP_MOVE_CHAN_CFM:
+		err = l2cap_move_channel_confirm(conn, cmd, data);
+		break;
+
+	case L2CAP_MOVE_CHAN_CFM_RSP:
+		err = l2cap_move_channel_confirm_rsp(conn, cmd, data);
+		break;
+
+
 	default:
 		BT_ERR("Unknown BR/EDR signaling command 0x%2.2x", cmd->code);
 		err = -EINVAL;
-- 
1.7.7

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 3/9] Bluetooth: Get/set AMP policy socket option

[Marcel Holtmann]

Hi Mat,

> Checks for valid policy value and L2CAP mode.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_sock.c |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 836d12e..9431e38 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -467,6 +467,11 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
>  
>  		break;
>  
> +	case BT_AMP_POLICY:
> +		if (put_user(chan->amp_policy, (u32 __user *) optval))
> +			err = -EFAULT;

I prefer to not just add such an option just yet. We only want to add
socket option once they are functional. Otherwise we have problems to
detect if such an option is working or not. So enabling this option
should be the last patch after we have AMP implemented.

Regards

Marcel

Re: [PATCH 3/9] Bluetooth: Get/set AMP policy socket option

[Mat Martineau]

On Thu, 13 Oct 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> Checks for valid policy value and L2CAP mode.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/l2cap_sock.c |   25 +++++++++++++++++++++++++
>>  1 files changed, 25 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>> index 836d12e..9431e38 100644
>> --- a/net/bluetooth/l2cap_sock.c
>> +++ b/net/bluetooth/l2cap_sock.c
>> @@ -467,6 +467,11 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
>>
>>  		break;
>>
>> +	case BT_AMP_POLICY:
>> +		if (put_user(chan->amp_policy, (u32 __user *) optval))
>> +			err = -EFAULT;
>
> I prefer to not just add such an option just yet. We only want to add
> socket option once they are functional. Otherwise we have problems to
> detect if such an option is working or not. So enabling this option
> should be the last patch after we have AMP implemented.

Hi Marcel -

My plan was to add checks for enable_hs before adding any code that 
takes action based on amp_policy.  Would it be acceptable to add those 
enable_hs checks now so the code is in place, but disabled by default? 
It would be helpful for development and testing.


Regards,

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 3/9] Bluetooth: Get/set AMP policy socket option

[Marcel Holtmann]

Hi Mat,

> >> Checks for valid policy value and L2CAP mode.
> >>
> >> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >> ---
> >>  net/bluetooth/l2cap_sock.c |   25 +++++++++++++++++++++++++
> >>  1 files changed, 25 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> >> index 836d12e..9431e38 100644
> >> --- a/net/bluetooth/l2cap_sock.c
> >> +++ b/net/bluetooth/l2cap_sock.c
> >> @@ -467,6 +467,11 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
> >>
> >>  		break;
> >>
> >> +	case BT_AMP_POLICY:
> >> +		if (put_user(chan->amp_policy, (u32 __user *) optval))
> >> +			err = -EFAULT;
> >
> > I prefer to not just add such an option just yet. We only want to add
> > socket option once they are functional. Otherwise we have problems to
> > detect if such an option is working or not. So enabling this option
> > should be the last patch after we have AMP implemented.
>
> My plan was to add checks for enable_hs before adding any code that 
> takes action based on amp_policy.  Would it be acceptable to add those 
> enable_hs checks now so the code is in place, but disabled by default? 
> It would be helpful for development and testing.

if it is protected enable_hs, then this is fine with me.

Regards

Marcel

Re: [PATCH 1/9] Bluetooth: Add BT_AMP_POLICY socket option

[Marcel Holtmann]

Hi Mat,

> Allow control of AMP functionality on L2CAP sockets. By default,
> connections will be restricted to BR/EDR.  Manipulating the
> BT_AMP_POLICY option allows for channels to be moved to or created
> on AMP controllers.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index e727555..14ae418 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -77,6 +77,33 @@ struct bt_power {
>  #define BT_POWER_FORCE_ACTIVE_OFF 0
>  #define BT_POWER_FORCE_ACTIVE_ON  1
>  
> +#define BT_AMP_POLICY	10
> +
> +/* Require BR/EDR (default policy)
> + *   AMP controllers cannot be used.
> + *   Channel move requests from the remote device are denied.
> + *   If the L2CAP channel is currently using AMP, move the channel to BR/EDR.
> + */
> +#define BT_AMP_POLICY_REQUIRE_BR_EDR   0

so lets brainstorm a little bit with the names here. Do we really wanna
use the term AMP_POLICY? Is it really AMP specific? Or do we better make
ourselves a bit more future proof?

I am currently thinking a bit more generic. Maybe this is not the best
choice, but lets entertain this for a bit. So what about calling this
BT_CHANNEL_POLICY?

And instead of REQUIRE_BR_EDR I would call it BREDR_ONLY.

> +/* Prefer AMP
> + *   Allow use of AMP controllers
> + *   If the L2CAP channel is currently on BR/EDR and AMP controller
> + *     resources are available, initiate a channel move to AMP.
> + *   Channel move requests from the remote device are allowed.
> + *   If the L2CAP socket has not been connected yet, try to create
> + *     and configure the channel directly on an AMP controller rather
> + *     than BR/EDR.
> + */
> +#define BT_AMP_POLICY_PREFER_AMP       1
> +
> +/* Prefer BR/EDR
> + *   Allow use of AMP controllers.
> + *   If the L2CAP channel is currently on AMP, move it to BR/EDR.
> + *   Channel move requests from the remote device are allowed.
> + */
> +#define BT_AMP_POLICY_PREFER_BR_EDR    2

We might also wanna look into the option of having a SCM message in the
socket that tells us the current type. Or at least tells us when we
successfully switched to AMP or back to BR/EDR. Since obviously we are
not blocking on the setsockopt call.

Not sure if applications actually do care, but for OBEX it might be
interesting to know if you are on an AMP controller right now or not.

Regards

Marcel

Re: [PATCH 2/9] Bluetooth: Add AMP policy information to l2cap_chan

[Andrei Emeltchenko]

Hi Mat,

On Thu, Oct 13, 2011 at 03:00:40PM -0700, Mat Martineau wrote:
> Each channel has an active AMP policy to require BR/EDR (the default),
> prefer AMP, or prefer BR/EDR.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 806b950..e77d39f 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -357,6 +357,7 @@ struct l2cap_chan {
>  	__u8		num_conf_rsp;
>  
>  	__u8		fcs;
> +	__u8		amp_policy;

I would merge this patch with following one since this is only one line.

>  
>  	__u16		tx_win;
>  	__u8		max_tx;
> -- 
> 1.7.7
> 
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> --
> 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
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 4/9] Bluetooth: Add AMP-related data and structures for channel signals

[Andrei Emeltchenko]

Hi Mat,

On Thu, Oct 13, 2011 at 03:00:42PM -0700, Mat Martineau wrote:
> AMP channel creation and channel moves are coordinated using the L2CAP
> signaling channel.  These definitions cover the "create channel",
> "move channel", and "move channel confirm" signals.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |   60 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index e77d39f..2ea0d15 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -89,6 +89,12 @@ struct l2cap_conninfo {
>  #define L2CAP_ECHO_RSP		0x09
>  #define L2CAP_INFO_REQ		0x0a
>  #define L2CAP_INFO_RSP		0x0b
> +#define L2CAP_CREATE_CHAN_REQ	0x0c
> +#define L2CAP_CREATE_CHAN_RSP	0x0d
> +#define L2CAP_MOVE_CHAN_REQ	0x0e
> +#define L2CAP_MOVE_CHAN_RSP	0x0f
> +#define L2CAP_MOVE_CHAN_CFM	0x10

more tabs

> +#define L2CAP_MOVE_CHAN_CFM_RSP	0x11
>  #define L2CAP_CONN_PARAM_UPDATE_REQ	0x12
>  #define L2CAP_CONN_PARAM_UPDATE_RSP	0x13
>  
> @@ -296,6 +302,60 @@ struct l2cap_info_rsp {
>  	__u8        data[0];
>  } __packed;
>  
> +struct l2cap_create_chan_req {
> +	__le16      psm;
> +	__le16      scid;
> +	__u8        amp_id;
> +} __attribute__ ((packed));

I prefer tabs instead of spaces here and below. I will send patch cleaning
remaining spaces in l2cap.h

> +
> +struct l2cap_create_chan_rsp {
> +	__le16      dcid;
> +	__le16      scid;
> +	__le16      result;
> +	__le16      status;
> +} __attribute__ ((packed));
> +
> +#define L2CAP_CREATE_CHAN_SUCCESS		0x0000
> +#define L2CAP_CREATE_CHAN_PENDING		0x0001
> +#define L2CAP_CREATE_CHAN_REFUSED_PSM		0x0002
> +#define L2CAP_CREATE_CHAN_REFUSED_SECURITY	0x0003
> +#define L2CAP_CREATE_CHAN_REFUSED_RESOURCES	0x0004
> +#define L2CAP_CREATE_CHAN_REFUSED_CONTROLLER	0x0005
> +
> +#define L2CAP_CREATE_CHAN_STATUS_NONE		0x0000
> +#define L2CAP_CREATE_CHAN_STATUS_AUTHENTICATION	0x0001
> +#define L2CAP_CREATE_CHAN_STATUS_AUTHORIZATION	0x0002

Isn't it a little bit log? why not CR/CS as it is already done?

BTW: I've done it different way. Since STATUS and RESULT are almost the same for
connect/create my patch is:

<------8<-----------------------------------------
|  @@ -214,14 +216,15 @@ struct l2cap_conn_rsp {
|   #define L2CAP_CID_DYN_START    0x0040
|   #define L2CAP_CID_DYN_END      0xffff
|
|  -/* connect result */
|  +/* connect/create channel results */
|   #define L2CAP_CR_SUCCESS       0x0000
|   #define L2CAP_CR_PEND          0x0001
|   #define L2CAP_CR_BAD_PSM       0x0002
|   #define L2CAP_CR_SEC_BLOCK     0x0003
|   #define L2CAP_CR_NO_MEM                0x0004
|  +#define L2CAP_CR_BAD_AMP       0x0005
|
|  -/* connect status */
|  +/* connect/create channel status */
|   #define L2CAP_CS_NO_INFO       0x0000
|   #define L2CAP_CS_AUTHEN_PEND   0x0001
|   #define L2CAP_CS_AUTHOR_PEND   0x0002
|
<------8<-----------------------------------------

> +
> +struct l2cap_move_chan_req {
> +	__le16      icid;
> +	__u8        dest_amp_id;
> +} __attribute__ ((packed));
> +
> +struct l2cap_move_chan_rsp {
> +	__le16      icid;
> +	__le16      result;
> +} __attribute__ ((packed));
> +
> +#define L2CAP_MOVE_CHAN_SUCCESS			0x0000
> +#define L2CAP_MOVE_CHAN_PENDING			0x0001
> +#define L2CAP_MOVE_CHAN_REFUSED_CONTROLLER	0x0002
> +#define L2CAP_MOVE_CHAN_REFUSED_SAME_ID		0x0003
> +#define L2CAP_MOVE_CHAN_REFUSED_CONFIG		0x0004
> +#define L2CAP_MOVE_CHAN_REFUSED_COLLISION	0x0005
> +#define L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED	0x0006

Same here: use CR and shorter names

Best regards 
Andrei Emeltchenko 

> +
> +struct l2cap_move_chan_cfm {
> +	__le16      icid;
> +	__le16      result;
> +} __attribute__ ((packed));
> +
> +#define L2CAP_MOVE_CHAN_CONFIRMED	0x0000
> +#define L2CAP_MOVE_CHAN_UNCONFIRMED	0x0001
> +
> +struct l2cap_move_chan_cfm_rsp {
> +	__le16      icid;
> +} __attribute__ ((packed));
> +
>  /* info type */
>  #define L2CAP_IT_CL_MTU		0x0001
>  #define L2CAP_IT_FEAT_MASK	0x0002
> -- 
> 1.7.7
> 
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> --
> 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 7/9] Bluetooth: Use symbolic values for the fixed channel map

[Andrei Emeltchenko]

Hi Mat,

On Thu, Oct 13, 2011 at 03:00:45PM -0700, Mat Martineau wrote:
> The A2MP fixed channel bit is only set when high-speed mode is enabled.

It might make sense to merge previous patch with definitions of 
L2CAP_FC_L2CAP and L2CAP_FC_A2MP, otherwise these 2 patches are a bit
small.

> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index d023156..e38258b 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -60,7 +60,7 @@ int disable_ertm;
>  int enable_hs;
>  
>  static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
> -static u8 l2cap_fixed_chan[8] = { 0x02, };
> +static u8 l2cap_fixed_chan[8] = { L2CAP_FC_L2CAP, };

I personally do not like present approach with allocating 8-octet array
when we need only one octet _FOR_NOW_. (Also assigning is not clear enough).

Why not:

<------8<-----------------------------------------------
|  @@ -60,7 +60,7 @@ int disable_ertm;
|   int enable_hs;
|
|   static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
|  -static u8 l2cap_fixed_chan[8] = { 0x02, };
|  +static u8 l2cap_fixed_chan = L2CAP_FC_L2CAP;
|
|   static LIST_HEAD(chan_list);
|   static DEFINE_RWLOCK(chan_list_lock);
|
<------8<-----------------------------------------------

>  
>  static LIST_HEAD(chan_list);
>  static DEFINE_RWLOCK(chan_list_lock);
> @@ -2849,6 +2849,10 @@ static inline int l2cap_information_req(struct l2cap_conn *conn, struct l2cap_cm
>  	} else if (type == L2CAP_IT_FIXED_CHAN) {
>  		u8 buf[12];
>  		struct l2cap_info_rsp *rsp = (struct l2cap_info_rsp *) buf;
> +
> +		if (enable_hs)
> +			l2cap_fixed_chan[0] |= L2CAP_FC_A2MP;
> +
>  		rsp->type   = cpu_to_le16(L2CAP_IT_FIXED_CHAN);
>  		rsp->result = cpu_to_le16(L2CAP_IR_SUCCESS);
>  		memcpy(buf + 4, l2cap_fixed_chan, 8);

and then here:

<------8<--------------------------------------------------------------------------------------------------
|  @@ -2895,9 +2895,16 @@ static inline int l2cap_information_req(struct l2cap_conn *conn, struct l2cap_cm
|          } else if (type == L2CAP_IT_FIXED_CHAN) {
|                  u8 buf[12];
|                  struct l2cap_info_rsp *rsp = (struct l2cap_info_rsp *) buf;
|  +
|  +               memset(buf, 0, sizeof(buf));
|                  rsp->type   = cpu_to_le16(L2CAP_IT_FIXED_CHAN);
|                  rsp->result = cpu_to_le16(L2CAP_IR_SUCCESS);
|  -               memcpy(buf + 4, l2cap_fixed_chan, 8);
|  +
|  +               if (enable_hs)
|  +                       l2cap_fixed_chan |= L2CAP_FC_A2MP;
|  +
|  +               rsp->data[0] = l2cap_fixed_chan;
|  +
|                  l2cap_send_cmd(conn, cmd->ident,
|                                          L2CAP_INFO_RSP, sizeof(buf), buf);
|          } else {
|
<------8<--------------------------------------------------------------------------------------------------

See my patch here:

http://www.spinics.net/lists/linux-bluetooth/msg17023.html

Best regards 
Andrei Emeltchenko 

Re: [PATCH 8/9] Bluetooth: Add AMP header file

[Andrei Emeltchenko]

Hi Mat,

On Thu, Oct 13, 2011 at 03:00:46PM -0700, Mat Martineau wrote:
> This file will be home to all AMP- and A2MP-related declarations.  The
> first macros map between HCI device indexes (for AMP controllers) and
> AMP controller ids that are passed over the air.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/amp.h |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>  create mode 100644 include/net/bluetooth/amp.h
> 
> diff --git a/include/net/bluetooth/amp.h b/include/net/bluetooth/amp.h
> new file mode 100644
> index 0000000..b84ce53
> --- /dev/null
> +++ b/include/net/bluetooth/amp.h
> @@ -0,0 +1,20 @@
> +/*
> +   Copyright (c) 2010-2011 Code Aurora Forum.  All rights reserved.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License version 2 and
> +   only version 2 as published by the Free Software Foundation.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +*/
> +
> +#ifndef __AMP_H
> +#define __AMP_H
> +
> +#define HCI_A2MP_ID(id)     ((id)+0x10)  /* convert HCI dev index to AMP ID */
> +#define A2MP_HCI_ID(id)     ((id)-0x10)  /* convert AMP ID to HCI dev index */

Missing spaces? Can this be in hci.h ?

Best regards 
Andrei Emeltchenko 

Re: [PATCH 9/9] Bluetooth: Add signal handlers for channel moves

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2011-10-13 15:00:47 -0700]:

> AMP channels can be moved between BR/EDR and AMP controllers using a
> sequence of signals. Every attempted channel move involves a series of
> four signals:
> 
>    Move Initiator                 Move Responder
>         |                                 |
>         |       Move Channel Request      |
>         |  ---------------------------->  |
>         |                                 |
>         |       Move Channel Response     |
>         |  <----------------------------  |
>         |                                 |
>         |       Move Channel Confirm      |
>         |  ---------------------------->  |
>         |                                 |
>         |  Move Channel Confirm Response  |
> 	|  <----------------------------  |
> 
> All four signals are sent even if the move fails.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |  123 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 123 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index e38258b..ba64bab 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2957,6 +2957,112 @@ static inline int l2cap_create_channel_rsp(struct l2cap_conn *conn,
>  	return l2cap_connect_rsp(conn, cmd, data);
>  }
>  
> +static void l2cap_send_move_chan_rsp(struct l2cap_conn *conn, u8 ident,
> +				u16 icid, u16 result)
> +{
> +	struct l2cap_move_chan_rsp rsp;
> +
> +	BT_DBG("icid %d, result %d", (int) icid, (int) result);
> +
> +	rsp.icid = cpu_to_le16(icid);
> +	rsp.result = cpu_to_le16(result);
> +
> +	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_RSP, sizeof(rsp), &rsp);
> +}
> +
> +static void l2cap_send_move_chan_cfm(struct l2cap_conn *conn,
> +				struct l2cap_chan *chan, u16 icid, u16 result)
> +{
> +	struct l2cap_move_chan_cfm cfm;
> +	u8 ident;
> +
> +	BT_DBG("icid %d, result %d", (int) icid, (int) result);
> +
> +	ident = l2cap_get_ident(conn);
> +	if (chan)
> +		chan->ident = ident;
> +
> +	cfm.icid = cpu_to_le16(icid);
> +	cfm.result = cpu_to_le16(result);
> +
> +	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM, sizeof(cfm), &cfm);
> +}
> +
> +static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
> +					u16 icid)
> +{
> +	struct l2cap_move_chan_cfm_rsp rsp;
> +
> +	BT_DBG("icid %d", (int) icid);
> +
> +	rsp.icid = cpu_to_le16(icid);
> +	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
> +}
> +
> +static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> +					struct l2cap_cmd_hdr *cmd, u8 *data)
> +{
> +	struct l2cap_move_chan_req *req = (struct l2cap_move_chan_req *) data;
> +	u16 icid = 0;
> +	u16 result = L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED;
> +
> +	icid = le16_to_cpu(req->icid);
> +
> +	BT_DBG("icid %d, dest_amp_id %d", (int) icid, (int) req->dest_amp_id);
> +
> +	/* Placeholder: Always refuse */
> +	l2cap_send_move_chan_rsp(conn, cmd->ident, icid, result);
> +
> +	return 0;
> +}

It's a good idea protect the move channel request with enable_hs.

	Gustavo

Re: [PATCH 1/9] Bluetooth: Add BT_AMP_POLICY socket option

[Mat Martineau]

On Thu, 13 Oct 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> Allow control of AMP functionality on L2CAP sockets. By default,
>> connections will be restricted to BR/EDR.  Manipulating the
>> BT_AMP_POLICY option allows for channels to be moved to or created
>> on AMP controllers.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
>>  1 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index e727555..14ae418 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -77,6 +77,33 @@ struct bt_power {
>>  #define BT_POWER_FORCE_ACTIVE_OFF 0
>>  #define BT_POWER_FORCE_ACTIVE_ON  1
>>
>> +#define BT_AMP_POLICY	10
>> +
>> +/* Require BR/EDR (default policy)
>> + *   AMP controllers cannot be used.
>> + *   Channel move requests from the remote device are denied.
>> + *   If the L2CAP channel is currently using AMP, move the channel to BR/EDR.
>> + */
>> +#define BT_AMP_POLICY_REQUIRE_BR_EDR   0
>
> so lets brainstorm a little bit with the names here. Do we really wanna
> use the term AMP_POLICY? Is it really AMP specific? Or do we better make
> ourselves a bit more future proof?
>
> I am currently thinking a bit more generic. Maybe this is not the best
> choice, but lets entertain this for a bit. So what about calling this
> BT_CHANNEL_POLICY?
>
> And instead of REQUIRE_BR_EDR I would call it BREDR_ONLY.

It's no problem to fine-tune the names.

So, if BT_CHANNEL_POLICY is a good option name, how about these option 
values:

BT_CHANNEL_POLICY_BREDR_ONLY
BT_CHANNEL_POLICY_AMP_PREFERRED
BT_CHANNEL_POLICY_BREDR_PREFERRED


>> +/* Prefer AMP
>> + *   Allow use of AMP controllers
>> + *   If the L2CAP channel is currently on BR/EDR and AMP controller
>> + *     resources are available, initiate a channel move to AMP.
>> + *   Channel move requests from the remote device are allowed.
>> + *   If the L2CAP socket has not been connected yet, try to create
>> + *     and configure the channel directly on an AMP controller rather
>> + *     than BR/EDR.
>> + */
>> +#define BT_AMP_POLICY_PREFER_AMP       1
>> +
>> +/* Prefer BR/EDR
>> + *   Allow use of AMP controllers.
>> + *   If the L2CAP channel is currently on AMP, move it to BR/EDR.
>> + *   Channel move requests from the remote device are allowed.
>> + */
>> +#define BT_AMP_POLICY_PREFER_BR_EDR    2
>
> We might also wanna look into the option of having a SCM message in the
> socket that tells us the current type. Or at least tells us when we
> successfully switched to AMP or back to BR/EDR. Since obviously we are
> not blocking on the setsockopt call.
>
> Not sure if applications actually do care, but for OBEX it might be
> interesting to know if you are on an AMP controller right now or not.

Yes, that would be a nice feature.  OBEX seems to work well even 
without it, though.  The OBEX connection is made over BREDR, and the 
socket option is changed after the OBEX connection is made but before 
any gets or puts.  What you see in hcidump is that the move starts 
before the first get or put goes out over BREDR - those OBEX packets 
sit in the ERTM tx queue while the channel move happens.  When the 
move completes, the transfer immediatly proceeds on the AMP 
controller.


Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 2/9] Bluetooth: Add AMP policy information to l2cap_chan

[Mat Martineau]

On Fri, 14 Oct 2011, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Thu, Oct 13, 2011 at 03:00:40PM -0700, Mat Martineau wrote:
>> Each channel has an active AMP policy to require BR/EDR (the default),
>> prefer AMP, or prefer BR/EDR.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 806b950..e77d39f 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -357,6 +357,7 @@ struct l2cap_chan {
>>  	__u8		num_conf_rsp;
>>
>>  	__u8		fcs;
>> +	__u8		amp_policy;
>
> I would merge this patch with following one since this is only one line.

Sure.  I've seen a lot of requests to split up header changes in to 
separate commits, but I agree that doing it for one line is a little 
extreme.

>>
>>  	__u16		tx_win;
>>  	__u8		max_tx;
>> --
>> 1.7.7
>>

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 4/9] Bluetooth: Add AMP-related data and structures for channel signals

[Mat Martineau]

On Fri, 14 Oct 2011, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Thu, Oct 13, 2011 at 03:00:42PM -0700, Mat Martineau wrote:
>> AMP channel creation and channel moves are coordinated using the L2CAP
>> signaling channel.  These definitions cover the "create channel",
>> "move channel", and "move channel confirm" signals.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |   60 +++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 60 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index e77d39f..2ea0d15 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -89,6 +89,12 @@ struct l2cap_conninfo {
>>  #define L2CAP_ECHO_RSP		0x09
>>  #define L2CAP_INFO_REQ		0x0a
>>  #define L2CAP_INFO_RSP		0x0b
>> +#define L2CAP_CREATE_CHAN_REQ	0x0c
>> +#define L2CAP_CREATE_CHAN_RSP	0x0d
>> +#define L2CAP_MOVE_CHAN_REQ	0x0e
>> +#define L2CAP_MOVE_CHAN_RSP	0x0f
>> +#define L2CAP_MOVE_CHAN_CFM	0x10
>
> more tabs

Ok, I can line up the whole list.

>
>> +#define L2CAP_MOVE_CHAN_CFM_RSP	0x11
>>  #define L2CAP_CONN_PARAM_UPDATE_REQ	0x12
>>  #define L2CAP_CONN_PARAM_UPDATE_RSP	0x13
>>
>> @@ -296,6 +302,60 @@ struct l2cap_info_rsp {
>>  	__u8        data[0];
>>  } __packed;
>>
>> +struct l2cap_create_chan_req {
>> +	__le16      psm;
>> +	__le16      scid;
>> +	__u8        amp_id;
>> +} __attribute__ ((packed));
>
> I prefer tabs instead of spaces here and below. I will send patch cleaning
> remaining spaces in l2cap.h

Ok, it will be good to have them all the same style.

I also need to replace "__attribute__ ((packed))" with "__packed".

>
>> +
>> +struct l2cap_create_chan_rsp {
>> +	__le16      dcid;
>> +	__le16      scid;
>> +	__le16      result;
>> +	__le16      status;
>> +} __attribute__ ((packed));
>> +
>> +#define L2CAP_CREATE_CHAN_SUCCESS		0x0000
>> +#define L2CAP_CREATE_CHAN_PENDING		0x0001
>> +#define L2CAP_CREATE_CHAN_REFUSED_PSM		0x0002
>> +#define L2CAP_CREATE_CHAN_REFUSED_SECURITY	0x0003
>> +#define L2CAP_CREATE_CHAN_REFUSED_RESOURCES	0x0004
>> +#define L2CAP_CREATE_CHAN_REFUSED_CONTROLLER	0x0005
>> +
>> +#define L2CAP_CREATE_CHAN_STATUS_NONE		0x0000
>> +#define L2CAP_CREATE_CHAN_STATUS_AUTHENTICATION	0x0001
>> +#define L2CAP_CREATE_CHAN_STATUS_AUTHORIZATION	0x0002
>
> Isn't it a little bit log? why not CR/CS as it is already done?

I was staying close to the spec, but I admit it's verbose by Linux 
kernel standards.  I'll adopt your shorter names.


> BTW: I've done it different way. Since STATUS and RESULT are almost the same for
> connect/create my patch is:
>
> <------8<-----------------------------------------
> |  @@ -214,14 +216,15 @@ struct l2cap_conn_rsp {
> |   #define L2CAP_CID_DYN_START    0x0040
> |   #define L2CAP_CID_DYN_END      0xffff
> |
> |  -/* connect result */
> |  +/* connect/create channel results */
> |   #define L2CAP_CR_SUCCESS       0x0000
> |   #define L2CAP_CR_PEND          0x0001
> |   #define L2CAP_CR_BAD_PSM       0x0002
> |   #define L2CAP_CR_SEC_BLOCK     0x0003
> |   #define L2CAP_CR_NO_MEM                0x0004
> |  +#define L2CAP_CR_BAD_AMP       0x0005
> |
> |  -/* connect status */
> |  +/* connect/create channel status */
> |   #define L2CAP_CS_NO_INFO       0x0000
> |   #define L2CAP_CS_AUTHEN_PEND   0x0001
> |   #define L2CAP_CS_AUTHOR_PEND   0x0002
> |
> <------8<-----------------------------------------
>
>> +
>> +struct l2cap_move_chan_req {
>> +	__le16      icid;
>> +	__u8        dest_amp_id;
>> +} __attribute__ ((packed));
>> +
>> +struct l2cap_move_chan_rsp {
>> +	__le16      icid;
>> +	__le16      result;
>> +} __attribute__ ((packed));
>> +
>> +#define L2CAP_MOVE_CHAN_SUCCESS			0x0000
>> +#define L2CAP_MOVE_CHAN_PENDING			0x0001
>> +#define L2CAP_MOVE_CHAN_REFUSED_CONTROLLER	0x0002
>> +#define L2CAP_MOVE_CHAN_REFUSED_SAME_ID		0x0003
>> +#define L2CAP_MOVE_CHAN_REFUSED_CONFIG		0x0004
>> +#define L2CAP_MOVE_CHAN_REFUSED_COLLISION	0x0005
>> +#define L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED	0x0006
>
> Same here: use CR and shorter names

How about L2CAP_MC_SUCCESS, L2CAP_MC_PEND, etc.?  While create channel 
is very similar to the connect request, moving a channel is a whole 
different idea.


>> +
>> +struct l2cap_move_chan_cfm {
>> +	__le16      icid;
>> +	__le16      result;
>> +} __attribute__ ((packed));
>> +
>> +#define L2CAP_MOVE_CHAN_CONFIRMED	0x0000
>> +#define L2CAP_MOVE_CHAN_UNCONFIRMED	0x0001
>> +
>> +struct l2cap_move_chan_cfm_rsp {
>> +	__le16      icid;
>> +} __attribute__ ((packed));
>> +
>>  /* info type */
>>  #define L2CAP_IT_CL_MTU		0x0001
>>  #define L2CAP_IT_FEAT_MASK	0x0002
>> --
>> 1.7.7
>>

Thanks for the careful review!

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 7/9] Bluetooth: Use symbolic values for the fixed channel map

[Mat Martineau]

On Fri, 14 Oct 2011, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Thu, Oct 13, 2011 at 03:00:45PM -0700, Mat Martineau wrote:
>> The A2MP fixed channel bit is only set when high-speed mode is enabled.
>
> It might make sense to merge previous patch with definitions of
> L2CAP_FC_L2CAP and L2CAP_FC_A2MP, otherwise these 2 patches are a bit
> small.
>
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/l2cap_core.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index d023156..e38258b 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -60,7 +60,7 @@ int disable_ertm;
>>  int enable_hs;
>>
>>  static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
>> -static u8 l2cap_fixed_chan[8] = { 0x02, };
>> +static u8 l2cap_fixed_chan[8] = { L2CAP_FC_L2CAP, };
>
> I personally do not like present approach with allocating 8-octet array
> when we need only one octet _FOR_NOW_. (Also assigning is not clear enough).
>
> Why not:
>
> <------8<-----------------------------------------------
> |  @@ -60,7 +60,7 @@ int disable_ertm;
> |   int enable_hs;
> |
> |   static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
> |  -static u8 l2cap_fixed_chan[8] = { 0x02, };
> |  +static u8 l2cap_fixed_chan = L2CAP_FC_L2CAP;
> |
> |   static LIST_HEAD(chan_list);
> |   static DEFINE_RWLOCK(chan_list_lock);
> |
> <------8<-----------------------------------------------

Since the fixed channel map is 8 bytes, it makes some sense to have 
this data structure match what is used in the info request.  The 8th 
byte contains a bit that's defined for the AMP test manager too.

Gustavo, what do you think?

>>
>>  static LIST_HEAD(chan_list);
>>  static DEFINE_RWLOCK(chan_list_lock);
>> @@ -2849,6 +2849,10 @@ static inline int l2cap_information_req(struct l2cap_conn *conn, struct l2cap_cm
>>  	} else if (type == L2CAP_IT_FIXED_CHAN) {
>>  		u8 buf[12];
>>  		struct l2cap_info_rsp *rsp = (struct l2cap_info_rsp *) buf;
>> +
>> +		if (enable_hs)
>> +			l2cap_fixed_chan[0] |= L2CAP_FC_A2MP;
>> +
>>  		rsp->type   = cpu_to_le16(L2CAP_IT_FIXED_CHAN);
>>  		rsp->result = cpu_to_le16(L2CAP_IR_SUCCESS);
>>  		memcpy(buf + 4, l2cap_fixed_chan, 8);
>
> and then here:
>
> <------8<--------------------------------------------------------------------------------------------------
> |  @@ -2895,9 +2895,16 @@ static inline int l2cap_information_req(struct l2cap_conn *conn, struct l2cap_cm
> |          } else if (type == L2CAP_IT_FIXED_CHAN) {
> |                  u8 buf[12];
> |                  struct l2cap_info_rsp *rsp = (struct l2cap_info_rsp *) buf;
> |  +
> |  +               memset(buf, 0, sizeof(buf));
> |                  rsp->type   = cpu_to_le16(L2CAP_IT_FIXED_CHAN);
> |                  rsp->result = cpu_to_le16(L2CAP_IR_SUCCESS);
> |  -               memcpy(buf + 4, l2cap_fixed_chan, 8);
> |  +
> |  +               if (enable_hs)
> |  +                       l2cap_fixed_chan |= L2CAP_FC_A2MP;
> |  +
> |  +               rsp->data[0] = l2cap_fixed_chan;
> |  +
> |                  l2cap_send_cmd(conn, cmd->ident,
> |                                          L2CAP_INFO_RSP, sizeof(buf), buf);
> |          } else {
> |
> <------8<--------------------------------------------------------------------------------------------------
>
> See my patch here:
>
> http://www.spinics.net/lists/linux-bluetooth/msg17023.html


Thanks,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 8/9] Bluetooth: Add AMP header file

[Mat Martineau]

On Fri, 14 Oct 2011, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Thu, Oct 13, 2011 at 03:00:46PM -0700, Mat Martineau wrote:
>> This file will be home to all AMP- and A2MP-related declarations.  The
>> first macros map between HCI device indexes (for AMP controllers) and
>> AMP controller ids that are passed over the air.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/amp.h |   20 ++++++++++++++++++++
>>  1 files changed, 20 insertions(+), 0 deletions(-)
>>  create mode 100644 include/net/bluetooth/amp.h
>>
>> diff --git a/include/net/bluetooth/amp.h b/include/net/bluetooth/amp.h
>> new file mode 100644
>> index 0000000..b84ce53
>> --- /dev/null
>> +++ b/include/net/bluetooth/amp.h
>> @@ -0,0 +1,20 @@
>> +/*
>> +   Copyright (c) 2010-2011 Code Aurora Forum.  All rights reserved.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License version 2 and
>> +   only version 2 as published by the Free Software Foundation.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +*/
>> +
>> +#ifndef __AMP_H
>> +#define __AMP_H
>> +
>> +#define HCI_A2MP_ID(id)     ((id)+0x10)  /* convert HCI dev index to AMP ID */
>> +#define A2MP_HCI_ID(id)     ((id)-0x10)  /* convert AMP ID to HCI dev index */
>
> Missing spaces? Can this be in hci.h ?

After looking at this some more, I'm going to eliminate these macros 
altogether.  "0" is not a valid AMP id, so if AMP controllers are not 
registered as hci0, these macros go away and the code gets a lot 
cleaner.

Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 9/9] Bluetooth: Add signal handlers for channel moves

[Mat Martineau]

On Fri, 14 Oct 2011, Gustavo Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2011-10-13 15:00:47 -0700]:
>
>> AMP channels can be moved between BR/EDR and AMP controllers using a
>> sequence of signals. Every attempted channel move involves a series of
>> four signals:
>>
>>    Move Initiator                 Move Responder
>>         |                                 |
>>         |       Move Channel Request      |
>>         |  ---------------------------->  |
>>         |                                 |
>>         |       Move Channel Response     |
>>         |  <----------------------------  |
>>         |                                 |
>>         |       Move Channel Confirm      |
>>         |  ---------------------------->  |
>>         |                                 |
>>         |  Move Channel Confirm Response  |
>> 	|  <----------------------------  |
>>
>> All four signals are sent even if the move fails.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/l2cap_core.c |  123 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 123 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index e38258b..ba64bab 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -2957,6 +2957,112 @@ static inline int l2cap_create_channel_rsp(struct l2cap_conn *conn,
>>  	return l2cap_connect_rsp(conn, cmd, data);
>>  }
>>
>> +static void l2cap_send_move_chan_rsp(struct l2cap_conn *conn, u8 ident,
>> +				u16 icid, u16 result)
>> +{
>> +	struct l2cap_move_chan_rsp rsp;
>> +
>> +	BT_DBG("icid %d, result %d", (int) icid, (int) result);
>> +
>> +	rsp.icid = cpu_to_le16(icid);
>> +	rsp.result = cpu_to_le16(result);
>> +
>> +	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_RSP, sizeof(rsp), &rsp);
>> +}
>> +
>> +static void l2cap_send_move_chan_cfm(struct l2cap_conn *conn,
>> +				struct l2cap_chan *chan, u16 icid, u16 result)
>> +{
>> +	struct l2cap_move_chan_cfm cfm;
>> +	u8 ident;
>> +
>> +	BT_DBG("icid %d, result %d", (int) icid, (int) result);
>> +
>> +	ident = l2cap_get_ident(conn);
>> +	if (chan)
>> +		chan->ident = ident;
>> +
>> +	cfm.icid = cpu_to_le16(icid);
>> +	cfm.result = cpu_to_le16(result);
>> +
>> +	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM, sizeof(cfm), &cfm);
>> +}
>> +
>> +static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
>> +					u16 icid)
>> +{
>> +	struct l2cap_move_chan_cfm_rsp rsp;
>> +
>> +	BT_DBG("icid %d", (int) icid);
>> +
>> +	rsp.icid = cpu_to_le16(icid);
>> +	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
>> +}
>> +
>> +static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
>> +					struct l2cap_cmd_hdr *cmd, u8 *data)
>> +{
>> +	struct l2cap_move_chan_req *req = (struct l2cap_move_chan_req *) data;
>> +	u16 icid = 0;
>> +	u16 result = L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED;
>> +
>> +	icid = le16_to_cpu(req->icid);
>> +
>> +	BT_DBG("icid %d, dest_amp_id %d", (int) icid, (int) req->dest_amp_id);
>> +
>> +	/* Placeholder: Always refuse */
>> +	l2cap_send_move_chan_rsp(conn, cmd->ident, icid, result);
>> +
>> +	return 0;
>> +}
>
> It's a good idea protect the move channel request with enable_hs.

Yes, I'll require enable_hs to have the move channel do anything. 
When enable_hs is false, it seems like it should return a 
L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED response like it does now.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 7/9] Bluetooth: Use symbolic values for the fixed channel map

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2011-10-14 16:04:01 -0700]:

> 
> 
> On Fri, 14 Oct 2011, Andrei Emeltchenko wrote:
> 
> >Hi Mat,
> >
> >On Thu, Oct 13, 2011 at 03:00:45PM -0700, Mat Martineau wrote:
> >>The A2MP fixed channel bit is only set when high-speed mode is enabled.
> >
> >It might make sense to merge previous patch with definitions of
> >L2CAP_FC_L2CAP and L2CAP_FC_A2MP, otherwise these 2 patches are a bit
> >small.
> >
> >>
> >>Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >>---
> >> net/bluetooth/l2cap_core.c |    6 +++++-
> >> 1 files changed, 5 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >>index d023156..e38258b 100644
> >>--- a/net/bluetooth/l2cap_core.c
> >>+++ b/net/bluetooth/l2cap_core.c
> >>@@ -60,7 +60,7 @@ int disable_ertm;
> >> int enable_hs;
> >>
> >> static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
> >>-static u8 l2cap_fixed_chan[8] = { 0x02, };
> >>+static u8 l2cap_fixed_chan[8] = { L2CAP_FC_L2CAP, };
> >
> >I personally do not like present approach with allocating 8-octet array
> >when we need only one octet _FOR_NOW_. (Also assigning is not clear enough).
> >
> >Why not:
> >
> ><------8<-----------------------------------------------
> >|  @@ -60,7 +60,7 @@ int disable_ertm;
> >|   int enable_hs;
> >|
> >|   static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
> >|  -static u8 l2cap_fixed_chan[8] = { 0x02, };
> >|  +static u8 l2cap_fixed_chan = L2CAP_FC_L2CAP;
> >|
> >|   static LIST_HEAD(chan_list);
> >|   static DEFINE_RWLOCK(chan_list_lock);
> >|
> ><------8<-----------------------------------------------
> 
> Since the fixed channel map is 8 bytes, it makes some sense to have
> this data structure match what is used in the info request.  The 8th
> byte contains a bit that's defined for the AMP test manager too.

Yes, let's keep the 8 octet array. It make more sense as we use it on the
information response.  Btw, I fine with this patch.

	Gustavo

Re: [PATCH 1/9] Bluetooth: Add BT_AMP_POLICY socket option

[Marcel Holtmann]

Hi Mat,

> >> Allow control of AMP functionality on L2CAP sockets. By default,
> >> connections will be restricted to BR/EDR.  Manipulating the
> >> BT_AMP_POLICY option allows for channels to be moved to or created
> >> on AMP controllers.
> >>
> >> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >> ---
> >>  include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
> >>  1 files changed, 27 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> >> index e727555..14ae418 100644
> >> --- a/include/net/bluetooth/bluetooth.h
> >> +++ b/include/net/bluetooth/bluetooth.h
> >> @@ -77,6 +77,33 @@ struct bt_power {
> >>  #define BT_POWER_FORCE_ACTIVE_OFF 0
> >>  #define BT_POWER_FORCE_ACTIVE_ON  1
> >>
> >> +#define BT_AMP_POLICY	10
> >> +
> >> +/* Require BR/EDR (default policy)
> >> + *   AMP controllers cannot be used.
> >> + *   Channel move requests from the remote device are denied.
> >> + *   If the L2CAP channel is currently using AMP, move the channel to BR/EDR.
> >> + */
> >> +#define BT_AMP_POLICY_REQUIRE_BR_EDR   0
> >
> > so lets brainstorm a little bit with the names here. Do we really wanna
> > use the term AMP_POLICY? Is it really AMP specific? Or do we better make
> > ourselves a bit more future proof?
> >
> > I am currently thinking a bit more generic. Maybe this is not the best
> > choice, but lets entertain this for a bit. So what about calling this
> > BT_CHANNEL_POLICY?
> >
> > And instead of REQUIRE_BR_EDR I would call it BREDR_ONLY.
> 
> It's no problem to fine-tune the names.
> 
> So, if BT_CHANNEL_POLICY is a good option name, how about these option 
> values:
> 
> BT_CHANNEL_POLICY_BREDR_ONLY
> BT_CHANNEL_POLICY_AMP_PREFERRED
> BT_CHANNEL_POLICY_BREDR_PREFERRED

lets resort the list as

BT_CHANNEL_POLICY_BREDR_ONLY
BT_CHANNEL_POLICY_BREDR_PREFERRED
BT_CHANNEL_POLICY_AMP_PREFERRED

and I would think we are good. However any other opinion on the naming
would be welcome here as well. I was just brainstorming here.

> >> +/* Prefer AMP
> >> + *   Allow use of AMP controllers
> >> + *   If the L2CAP channel is currently on BR/EDR and AMP controller
> >> + *     resources are available, initiate a channel move to AMP.
> >> + *   Channel move requests from the remote device are allowed.
> >> + *   If the L2CAP socket has not been connected yet, try to create
> >> + *     and configure the channel directly on an AMP controller rather
> >> + *     than BR/EDR.
> >> + */
> >> +#define BT_AMP_POLICY_PREFER_AMP       1
> >> +
> >> +/* Prefer BR/EDR
> >> + *   Allow use of AMP controllers.
> >> + *   If the L2CAP channel is currently on AMP, move it to BR/EDR.
> >> + *   Channel move requests from the remote device are allowed.
> >> + */
> >> +#define BT_AMP_POLICY_PREFER_BR_EDR    2
> >
> > We might also wanna look into the option of having a SCM message in the
> > socket that tells us the current type. Or at least tells us when we
> > successfully switched to AMP or back to BR/EDR. Since obviously we are
> > not blocking on the setsockopt call.
> >
> > Not sure if applications actually do care, but for OBEX it might be
> > interesting to know if you are on an AMP controller right now or not.
> 
> Yes, that would be a nice feature.  OBEX seems to work well even 
> without it, though.  The OBEX connection is made over BREDR, and the 
> socket option is changed after the OBEX connection is made but before 
> any gets or puts.  What you see in hcidump is that the move starts 
> before the first get or put goes out over BREDR - those OBEX packets 
> sit in the ERTM tx queue while the channel move happens.  When the 
> move completes, the transfer immediatly proceeds on the AMP 
> controller.

If it works for fine for you guys right now, then we could do that later
and don't have to worry right now. However before we make enable_hs on
by default I like to have this figured out.

Regards

Marcel

Re: [PATCH 4/9] Bluetooth: Add AMP-related data and structures for channel signals

[Andrei Emeltchenko]

Hi Mat,

On Fri, Oct 14, 2011 at 03:58:27PM -0700, Mat Martineau wrote:
> >>+#define L2CAP_CREATE_CHAN_STATUS_NONE		0x0000
> >>+#define L2CAP_CREATE_CHAN_STATUS_AUTHENTICATION	0x0001
> >>+#define L2CAP_CREATE_CHAN_STATUS_AUTHORIZATION	0x0002
> >
> >Isn't it a little bit log? why not CR/CS as it is already done?
> 
> I was staying close to the spec, but I admit it's verbose by Linux
> kernel standards.  I'll adopt your shorter names.
> 
> 
> >BTW: I've done it different way. Since STATUS and RESULT are almost the same for
> >connect/create my patch is:
> >
> ><------8<-----------------------------------------
> >|  @@ -214,14 +216,15 @@ struct l2cap_conn_rsp {
> >|   #define L2CAP_CID_DYN_START    0x0040
> >|   #define L2CAP_CID_DYN_END      0xffff
> >|
> >|  -/* connect result */
> >|  +/* connect/create channel results */
> >|   #define L2CAP_CR_SUCCESS       0x0000
> >|   #define L2CAP_CR_PEND          0x0001
> >|   #define L2CAP_CR_BAD_PSM       0x0002
> >|   #define L2CAP_CR_SEC_BLOCK     0x0003
> >|   #define L2CAP_CR_NO_MEM                0x0004
> >|  +#define L2CAP_CR_BAD_AMP       0x0005
> >|
> >|  -/* connect status */
> >|  +/* connect/create channel status */
> >|   #define L2CAP_CS_NO_INFO       0x0000
> >|   #define L2CAP_CS_AUTHEN_PEND   0x0001
> >|   #define L2CAP_CS_AUTHOR_PEND   0x0002
> >|
> ><------8<-----------------------------------------
> >
> >>+
> >>+struct l2cap_move_chan_req {
> >>+	__le16      icid;
> >>+	__u8        dest_amp_id;
> >>+} __attribute__ ((packed));
> >>+
> >>+struct l2cap_move_chan_rsp {
> >>+	__le16      icid;
> >>+	__le16      result;
> >>+} __attribute__ ((packed));
> >>+
> >>+#define L2CAP_MOVE_CHAN_SUCCESS			0x0000
> >>+#define L2CAP_MOVE_CHAN_PENDING			0x0001
> >>+#define L2CAP_MOVE_CHAN_REFUSED_CONTROLLER	0x0002
> >>+#define L2CAP_MOVE_CHAN_REFUSED_SAME_ID		0x0003
> >>+#define L2CAP_MOVE_CHAN_REFUSED_CONFIG		0x0004
> >>+#define L2CAP_MOVE_CHAN_REFUSED_COLLISION	0x0005
> >>+#define L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED	0x0006
> >
> >Same here: use CR and shorter names
> 
> How about L2CAP_MC_SUCCESS, L2CAP_MC_PEND, etc.?  While create
> channel is very similar to the connect request, moving a channel is
> a whole different idea.

Maybe MR (Move Result, similar to CR) is better.

Best regards 
Andrei Emeltchenko 

Re: [PATCH 9/9] Bluetooth: Add signal handlers for channel moves

[Andrei Emeltchenko]

Hi Mat,

On Fri, Oct 14, 2011 at 04:19:53PM -0700, Mat Martineau wrote:
> >>AMP channels can be moved between BR/EDR and AMP controllers using a
> >>sequence of signals. Every attempted channel move involves a series of
> >>four signals:
> >>
> >>   Move Initiator                 Move Responder
> >>        |                                 |
> >>        |       Move Channel Request      |
> >>        |  ---------------------------->  |
> >>        |                                 |
> >>        |       Move Channel Response     |
> >>        |  <----------------------------  |
> >>        |                                 |
> >>        |       Move Channel Confirm      |
> >>        |  ---------------------------->  |
> >>        |                                 |
> >>        |  Move Channel Confirm Response  |
> >>	|  <----------------------------  |
> >>
> >>All four signals are sent even if the move fails.
> >>
> >>Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >>---
> >> net/bluetooth/l2cap_core.c |  123 ++++++++++++++++++++++++++++++++++++++++++++
> >> 1 files changed, 123 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >>index e38258b..ba64bab 100644
> >>--- a/net/bluetooth/l2cap_core.c
> >>+++ b/net/bluetooth/l2cap_core.c
> >>@@ -2957,6 +2957,112 @@ static inline int l2cap_create_channel_rsp(struct l2cap_conn *conn,
> >> 	return l2cap_connect_rsp(conn, cmd, data);
> >> }
> >>
> >>+static void l2cap_send_move_chan_rsp(struct l2cap_conn *conn, u8 ident,
> >>+				u16 icid, u16 result)
> >>+{
> >>+	struct l2cap_move_chan_rsp rsp;
> >>+
> >>+	BT_DBG("icid %d, result %d", (int) icid, (int) result);

BTW: why do you need type conversion here and below?

Best regards 
Andrei Emeltchenko 

> >>+
> >>+	rsp.icid = cpu_to_le16(icid);
> >>+	rsp.result = cpu_to_le16(result);
> >>+
> >>+	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_RSP, sizeof(rsp), &rsp);
> >>+}
> >>+
> >>+static void l2cap_send_move_chan_cfm(struct l2cap_conn *conn,
> >>+				struct l2cap_chan *chan, u16 icid, u16 result)
> >>+{
> >>+	struct l2cap_move_chan_cfm cfm;
> >>+	u8 ident;
> >>+
> >>+	BT_DBG("icid %d, result %d", (int) icid, (int) result);
> >>+
> >>+	ident = l2cap_get_ident(conn);
> >>+	if (chan)
> >>+		chan->ident = ident;
> >>+
> >>+	cfm.icid = cpu_to_le16(icid);
> >>+	cfm.result = cpu_to_le16(result);
> >>+
> >>+	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM, sizeof(cfm), &cfm);
> >>+}
> >>+
> >>+static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
> >>+					u16 icid)
> >>+{
> >>+	struct l2cap_move_chan_cfm_rsp rsp;
> >>+
> >>+	BT_DBG("icid %d", (int) icid);
> >>+
> >>+	rsp.icid = cpu_to_le16(icid);
> >>+	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
> >>+}
> >>+
> >>+static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> >>+					struct l2cap_cmd_hdr *cmd, u8 *data)
> >>+{
> >>+	struct l2cap_move_chan_req *req = (struct l2cap_move_chan_req *) data;
> >>+	u16 icid = 0;
> >>+	u16 result = L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED;
> >>+
> >>+	icid = le16_to_cpu(req->icid);
> >>+
> >>+	BT_DBG("icid %d, dest_amp_id %d", (int) icid, (int) req->dest_amp_id);
> >>+
> >>+	/* Placeholder: Always refuse */
> >>+	l2cap_send_move_chan_rsp(conn, cmd->ident, icid, result);
> >>+
> >>+	return 0;
> >>+}
> >
> >It's a good idea protect the move channel request with enable_hs.
> 
> Yes, I'll require enable_hs to have the move channel do anything.
> When enable_hs is false, it seems like it should return a
> L2CAP_MOVE_CHAN_REFUSED_NOT_ALLOWED response like it does now.

Re: [PATCH 1/9] Bluetooth: Add BT_AMP_POLICY socket option

[Luiz Augusto von Dentz]

Hi Mat,

On Sat, Oct 15, 2011 at 1:38 AM, Mat Martineau <mathewm@codeaurora.org> wro=
te:
>>
>> Not sure if applications actually do care, but for OBEX it might be
>> interesting to know if you are on an AMP controller right now or not.
>
> Yes, that would be a nice feature. =A0OBEX seems to work well even withou=
t it,
> though. =A0The OBEX connection is made over BREDR, and the socket option =
is
> changed after the OBEX connection is made but before any gets or puts. =
=A0What
> you see in hcidump is that the move starts before the first get or put go=
es
> out over BREDR - those OBEX packets sit in the ERTM tx queue while the
> channel move happens. =A0When the move completes, the transfer immediatly
> proceeds on the AMP controller.

I guess the is implementation dependent, for obexd we might want to
check if the transfer will take a considerable amount of time to
decide whether or not to use AMP, in some case it might not worth to
switch because it will take more time than transferring over BDEDR.

--=20
Luiz Augusto von Dentz

Re: [PATCH 1/9] Bluetooth: Add BT_AMP_POLICY socket option

[Marcel Holtmann]

Hi Luiz,

> >> Not sure if applications actually do care, but for OBEX it might be
> >> interesting to know if you are on an AMP controller right now or not.
> >
> > Yes, that would be a nice feature.  OBEX seems to work well even without it,
> > though.  The OBEX connection is made over BREDR, and the socket option is
> > changed after the OBEX connection is made but before any gets or puts.  What
> > you see in hcidump is that the move starts before the first get or put goes
> > out over BREDR - those OBEX packets sit in the ERTM tx queue while the
> > channel move happens.  When the move completes, the transfer immediatly
> > proceeds on the AMP controller.
> 
> I guess the is implementation dependent, for obexd we might want to
> check if the transfer will take a considerable amount of time to
> decide whether or not to use AMP, in some case it might not worth to
> switch because it will take more time than transferring over BDEDR.

there has been discussions that you should be waiting for the first
packet on BR/EDR and hope that it indicates the length of the data to
follow. And then make a decision to allow switching or not. All three
options would support this behavior right now.

Until we are collecting real data for the switching time with BlueZ, we
will have no idea anyway and everything will be a guess.

Regards

Marcel