[PATCHv2 0/9] AMP interface and signal framework

[PATCHv2 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.

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_CHANNEL_POLICY socket option
  Bluetooth: Change scope of the enable_hs module parameter
  Bluetooth: Add channel policy to getsockopt/setsockopt
  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 signal handlers for channel moves
  Bluetooth: Guarantee BR-EDR device will be registered as hci0

 include/net/bluetooth/bluetooth.h |   27 ++++++
 include/net/bluetooth/l2cap.h     |   60 ++++++++++++-
 net/bluetooth/hci_core.c          |    4 +-
 net/bluetooth/l2cap_core.c        |  176 ++++++++++++++++++++++++++++++++++++-
 net/bluetooth/l2cap_sock.c        |   35 +++++++
 5 files changed, 298 insertions(+), 4 deletions(-)

-- 
1.7.7

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

[PATCHv2 1/9] Bluetooth: Add BT_CHANNEL_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_CHANNEL_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 fb1acb3..38cd3da 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_CHANNEL_POLICY	10
+
+/* BR/EDR only (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_CHANNEL_POLICY_BREDR_ONLY		0
+
+/* BR/EDR Preferred
+ *   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_CHANNEL_POLICY_BREDR_PREFERRED	1
+
+/* AMP Preferred
+ *   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_CHANNEL_POLICY_AMP_PREFERRED		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

Re: [PATCHv2 1/9] Bluetooth: Add BT_CHANNEL_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_CHANNEL_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(-)

so until someone else speaks up about naming or the policy itself, then
I am fine with this right now.

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

Regards

Marcel

[PATCHv2 2/9] Bluetooth: Change scope of the enable_hs module parameter

[Mat Martineau]

This variable is currently only accessible within l2cap_core.c, but
it is also needed in l2cap_sock.c

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 fddc82a..fd090db 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -730,6 +730,7 @@ static inline __u8 __ctrl_size(struct l2cap_chan *chan)
 }
 
 extern int disable_ertm;
+extern int enable_hs;
 
 int l2cap_init_sockets(void);
 void l2cap_cleanup_sockets(void);
-- 
1.7.7

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

Re: [PATCHv2 2/9] Bluetooth: Change scope of the enable_hs module parameter

[Marcel Holtmann]

Hi Mat,

> This variable is currently only accessible within l2cap_core.c, but
> it is also needed in l2cap_sock.c
> 
> 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 fddc82a..fd090db 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -730,6 +730,7 @@ static inline __u8 __ctrl_size(struct l2cap_chan *chan)
>  }
>  
>  extern int disable_ertm;
> +extern int enable_hs;
>  
>  int l2cap_init_sockets(void);
>  void l2cap_cleanup_sockets(void);

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

Regards

Marcel

[PATCHv2 3/9] Bluetooth: Add channel policy to getsockopt/setsockopt

[Mat Martineau]

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

Check for valid policy value and L2CAP mode.

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

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index fd090db..b0c9345 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -366,6 +366,7 @@ struct l2cap_chan {
 	__u16		flush_to;
 	__u8		mode;
 	__u8		chan_type;
+	__u8		chan_policy;
 
 	__le16		sport;
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 836d12e..1230e6e 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -467,6 +467,16 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 
 		break;
 
+	case BT_CHANNEL_POLICY:
+		if (!enable_hs) {
+			err = -ENOPROTOOPT;
+			break;
+		}
+
+		if (put_user(chan->chan_policy, (u32 __user *) optval))
+			err = -EFAULT;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -690,6 +700,31 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 			clear_bit(FLAG_FORCE_ACTIVE, &chan->flags);
 		break;
 
+	case BT_CHANNEL_POLICY:
+		if (!enable_hs) {
+			err = -ENOPROTOOPT;
+			break;
+		}
+
+		if (get_user(opt, (u32 __user *) optval)) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (opt > BT_CHANNEL_POLICY_AMP_PREFERRED) {
+			err = -EINVAL;
+			break;
+		}
+
+		if (chan->mode != L2CAP_MODE_ERTM &&
+			 chan->mode != L2CAP_MODE_STREAMING) {
+			err = -EINVAL;
+			break;
+		}
+
+		chan->chan_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

Re: [PATCHv2 3/9] Bluetooth: Add channel policy to getsockopt/setsockopt

[Marcel Holtmann]

Hi Mat,

> Each channel has a policy to require BR/EDR (the default),
> prefer BR/EDR, or prefer AMP.
> 
> Check for valid policy value and L2CAP mode.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    1 +
>  net/bluetooth/l2cap_sock.c    |   35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index fd090db..b0c9345 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -366,6 +366,7 @@ struct l2cap_chan {
>  	__u16		flush_to;
>  	__u8		mode;
>  	__u8		chan_type;
> +	__u8		chan_policy;
>  
>  	__le16		sport;
>  
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 836d12e..1230e6e 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -467,6 +467,16 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
>  
>  		break;
>  
> +	case BT_CHANNEL_POLICY:
> +		if (!enable_hs) {
> +			err = -ENOPROTOOPT;
> +			break;
> +		}
> +
> +		if (put_user(chan->chan_policy, (u32 __user *) optval))
> +			err = -EFAULT;
> +		break;
> +
>  	default:
>  		err = -ENOPROTOOPT;
>  		break;
> @@ -690,6 +700,31 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>  			clear_bit(FLAG_FORCE_ACTIVE, &chan->flags);
>  		break;
>  
> +	case BT_CHANNEL_POLICY:
> +		if (!enable_hs) {
> +			err = -ENOPROTOOPT;
> +			break;
> +		}
> +
> +		if (get_user(opt, (u32 __user *) optval)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +
> +		if (opt > BT_CHANNEL_POLICY_AMP_PREFERRED) {
> +			err = -EINVAL;
> +			break;
> +		}
> +
> +		if (chan->mode != L2CAP_MODE_ERTM &&
> +			 chan->mode != L2CAP_MODE_STREAMING) {

use another tab here instead of the single space.

> +			err = -EINVAL;
> +			break;
> +		}

And do we wanna keep EINVAL or better do EOPNOTSUPP. I think the later
is better here. The value is essentially supported, but not valid on
this specific transport.

> +
> +		chan->chan_policy = (u8) opt;
> +		break;
> +
>  	default:
>  		err = -ENOPROTOOPT;
>  		break;

Regards

Marcel

Re: [PATCHv2 3/9] Bluetooth: Add channel policy to getsockopt/setsockopt

[Mat Martineau]

On Wed, 19 Oct 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> Each channel has a policy to require BR/EDR (the default),
>> prefer BR/EDR, or prefer AMP.
>>
>> Check for valid policy value and L2CAP mode.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |    1 +
>>  net/bluetooth/l2cap_sock.c    |   35 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index fd090db..b0c9345 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -366,6 +366,7 @@ struct l2cap_chan {
>>  	__u16		flush_to;
>>  	__u8		mode;
>>  	__u8		chan_type;
>> +	__u8		chan_policy;
>>
>>  	__le16		sport;
>>
>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>> index 836d12e..1230e6e 100644
>> --- a/net/bluetooth/l2cap_sock.c
>> +++ b/net/bluetooth/l2cap_sock.c
>> @@ -467,6 +467,16 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
>>
>>  		break;
>>
>> +	case BT_CHANNEL_POLICY:
>> +		if (!enable_hs) {
>> +			err = -ENOPROTOOPT;
>> +			break;
>> +		}
>> +
>> +		if (put_user(chan->chan_policy, (u32 __user *) optval))
>> +			err = -EFAULT;
>> +		break;
>> +
>>  	default:
>>  		err = -ENOPROTOOPT;
>>  		break;
>> @@ -690,6 +700,31 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>>  			clear_bit(FLAG_FORCE_ACTIVE, &chan->flags);
>>  		break;
>>
>> +	case BT_CHANNEL_POLICY:
>> +		if (!enable_hs) {
>> +			err = -ENOPROTOOPT;
>> +			break;
>> +		}
>> +
>> +		if (get_user(opt, (u32 __user *) optval)) {
>> +			err = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		if (opt > BT_CHANNEL_POLICY_AMP_PREFERRED) {
>> +			err = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		if (chan->mode != L2CAP_MODE_ERTM &&
>> +			 chan->mode != L2CAP_MODE_STREAMING) {
>
> use another tab here instead of the single space.

Ok

>> +			err = -EINVAL;
>> +			break;
>> +		}
>
> And do we wanna keep EINVAL or better do EOPNOTSUPP. I think the later
> is better here. The value is essentially supported, but not valid on
> this specific transport.

I'll change this one to EOPNOTSUPP.

>> +
>> +		chan->chan_policy = (u8) opt;
>> +		break;
>> +
>>  	default:
>>  		err = -ENOPROTOOPT;
>>  		break;

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

[PATCHv2 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 |   54 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index b0c9345..4f4f318 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -94,6 +94,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
 
@@ -217,14 +223,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
@@ -318,6 +325,49 @@ struct l2cap_info_rsp {
 	__u8        data[0];
 } __packed;
 
+struct l2cap_create_chan_req {
+	__le16      psm;
+	__le16      scid;
+	__u8        amp_id;
+} __packed;
+
+struct l2cap_create_chan_rsp {
+	__le16      dcid;
+	__le16      scid;
+	__le16      result;
+	__le16      status;
+} __packed;
+
+struct l2cap_move_chan_req {
+	__le16      icid;
+	__u8        dest_amp_id;
+} __packed;
+
+struct l2cap_move_chan_rsp {
+	__le16      icid;
+	__le16      result;
+} __packed;
+
+#define L2CAP_MR_SUCCESS	0x0000
+#define L2CAP_MR_PEND		0x0001
+#define L2CAP_MR_BAD_ID		0x0002
+#define L2CAP_MR_SAME_ID	0x0003
+#define L2CAP_MR_NOT_SUPP	0x0004
+#define L2CAP_MR_COLLISION	0x0005
+#define L2CAP_MR_NOT_ALLOWED	0x0006
+
+struct l2cap_move_chan_cfm {
+	__le16      icid;
+	__le16      result;
+} __packed;
+
+#define L2CAP_MC_CONFIRMED	0x0000
+#define L2CAP_MC_UNCONFIRMED	0x0001
+
+struct l2cap_move_chan_cfm_rsp {
+	__le16      icid;
+} __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

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

[Marcel Holtmann]

Hi Mat,

> 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 |   54 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 52 insertions(+), 2 deletions(-)

looks all fine to me.

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

Regards

Marcel

[PATCHv2 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 |   45 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index bda6da7..67f0ab6 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3044,6 +3044,43 @@ 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);
+
+	if (!enable_hs)
+		return -EINVAL;
+
+	/* Placeholder: Always reject */
+	rsp.dcid = 0;
+	rsp.scid = cpu_to_le16(scid);
+	rsp.result = L2CAP_CR_NO_MEM;
+	rsp.status = L2CAP_CS_NO_INFO;
+
+	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)
 {
@@ -3156,6 +3193,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

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

[Marcel Holtmann]

Hi Mat,

> Handle both "create channel request" and "create channel response".
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 45 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index bda6da7..67f0ab6 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3044,6 +3044,43 @@ 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)

so I just question myself why we keep doing u8 *data here and not just
fix everything to be a void *data.

> +{
> +	struct l2cap_create_chan_req *req =
> +		(struct l2cap_create_chan_req *) data;

Then these casting stuff would go away. And I bet it is just some
leftover from the original L2CAP code.

Or does anybody else have an idea why we keep on insisting on u8 *data?

> +	struct l2cap_create_chan_rsp rsp;
> +	u16 psm, scid;

I think we might need to have a length check here first to ensure that
the header packet is really full present.

> +
> +	psm = le16_to_cpu(req->psm);
> +	scid = le16_to_cpu(req->scid);

Otherwise this just accesses some random memory.

> +
> +	BT_DBG("psm %d, scid %d, amp_id %d", (int) psm, (int) scid,
> +		(int) req->amp_id);

Why are we casting to (int) here?

> +
> +	if (!enable_hs)
> +		return -EINVAL;
> +
> +	/* Placeholder: Always reject */
> +	rsp.dcid = 0;
> +	rsp.scid = cpu_to_le16(scid);
> +	rsp.result = L2CAP_CR_NO_MEM;
> +	rsp.status = L2CAP_CS_NO_INFO;
> +
> +	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)
>  {
> @@ -3156,6 +3193,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;

Regards

Marcel

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

[Mat Martineau]

On Wed, 19 Oct 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> Handle both "create channel request" and "create channel response".
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/l2cap_core.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 45 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index bda6da7..67f0ab6 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -3044,6 +3044,43 @@ 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)
>
> so I just question myself why we keep doing u8 *data here and not just
> fix everything to be a void *data.
>
>> +{
>> +	struct l2cap_create_chan_req *req =
>> +		(struct l2cap_create_chan_req *) data;
>
> Then these casting stuff would go away. And I bet it is just some
> leftover from the original L2CAP code.
>
> Or does anybody else have an idea why we keep on insisting on u8 *data?

I assume it traces back to "u8 *data = skb->data;" in 
l2cap_sig_channel(), but I see no reason to hang on to that type 
information either if it's just going to be cast away.

I'll change it for these new signal handlers.  The others can be in a 
separate cleanup patch.

>> +	struct l2cap_create_chan_rsp rsp;
>> +	u16 psm, scid;
>
> I think we might need to have a length check here first to ensure that
> the header packet is really full present.

Will add a length check.

>> +
>> +	psm = le16_to_cpu(req->psm);
>> +	scid = le16_to_cpu(req->scid);
>
> Otherwise this just accesses some random memory.
>
>> +
>> +	BT_DBG("psm %d, scid %d, amp_id %d", (int) psm, (int) scid,
>> +		(int) req->amp_id);
>
> Why are we casting to (int) here?

I'll remove them.

>> +
>> +	if (!enable_hs)
>> +		return -EINVAL;
>> +
>> +	/* Placeholder: Always reject */
>> +	rsp.dcid = 0;
>> +	rsp.scid = cpu_to_le16(scid);
>> +	rsp.result = L2CAP_CR_NO_MEM;
>> +	rsp.status = L2CAP_CS_NO_INFO;
>> +
>> +	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)
>>  {
>> @@ -3156,6 +3193,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;


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

[PATCHv2 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 4f4f318..b8b25f6 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -119,6 +119,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

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

[Marcel Holtmann]

Hi Mat,

> 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 4f4f318..b8b25f6 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -119,6 +119,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
> +

while you are at it, please add the known fixed channels for SMP and LE
signaling here. They should be in use already somewhere ;)

>  /* L2CAP Control Field bit masks */
>  #define L2CAP_CTRL_SAR			0xC000
>  #define L2CAP_CTRL_REQSEQ		0x3F00

Regards

Marcel

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

[Mat Martineau]

On Wed, 19 Oct 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> 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 4f4f318..b8b25f6 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -119,6 +119,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
>> +
>
> while you are at it, please add the known fixed channels for SMP and LE
> signaling here. They should be in use already somewhere ;)

The channel mask wasn't a problem at the recent UPF, since the LE 
fixed channels are only used on LE connections and the info 
request/response are only relevant for BR/EDR.  It seems like these LE 
fixed channel bits should not be set when sending an info response or 
checked when processing an info response.

Do you still want me to add them?

>
>>  /* L2CAP Control Field bit masks */
>>  #define L2CAP_CTRL_SAR			0xC000
>>  #define L2CAP_CTRL_REQSEQ		0x3F00

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

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

[Marcel Holtmann]

Hi Mat,

> >> 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 4f4f318..b8b25f6 100644
> >> --- a/include/net/bluetooth/l2cap.h
> >> +++ b/include/net/bluetooth/l2cap.h
> >> @@ -119,6 +119,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
> >> +
> >
> > while you are at it, please add the known fixed channels for SMP and LE
> > signaling here. They should be in use already somewhere ;)
> 
> The channel mask wasn't a problem at the recent UPF, since the LE 
> fixed channels are only used on LE connections and the info 
> request/response are only relevant for BR/EDR.  It seems like these LE 
> fixed channel bits should not be set when sending an info response or 
> checked when processing an info response.
> 
> Do you still want me to add them?

I have not looked at all LE patches and this might need some
coordination, but in general yes, I like to have a proper set of fixed
channel value here. A good implementation would check these first. Even
if with LE, the LE link manager connection implies LE support, I rather
have a proper fixed channel mask here.

That said, if you just wanna send a patch that updates this later on, I
am fine with that as well.

Regards

Marcel

[PATCHv2 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 67f0ab6..f250392 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);
@@ -2975,6 +2975,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

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

[Marcel Holtmann]

Hi Mat,

> 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 67f0ab6..f250392 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);
> @@ -2975,6 +2975,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);

doing it this way is a bit sneaky actually. Since it does not really
allow disabling HS once you enabled it.

Maybe doing feature enabling/disabling (and also for LE) is better
handled via /sys/kernel/debug/bluetooth/enable_hs.

Regards

Marcel

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

[Mat Martineau]

On Wed, 19 Oct 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> 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 67f0ab6..f250392 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);
>> @@ -2975,6 +2975,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);
>
> doing it this way is a bit sneaky actually. Since it does not really
> allow disabling HS once you enabled it.
>
> Maybe doing feature enabling/disabling (and also for LE) is better
> handled via /sys/kernel/debug/bluetooth/enable_hs.

This is really the only place where anything other than the enable_hs 
value itself needs to be manipulated.  It would be less code to just 
add an "else" and clear the feature mask bit.  l2cap_fixed_chan[] is 
not used anywhere outside this function.

If we want to move enable_hs to debugfs, I'll drop this change from 
patch set and work on debugfs support for the next patch set.

Any other opinions?


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

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

[Marcel Holtmann]

Hi Mat,

> >> 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 67f0ab6..f250392 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);
> >> @@ -2975,6 +2975,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);
> >
> > doing it this way is a bit sneaky actually. Since it does not really
> > allow disabling HS once you enabled it.
> >
> > Maybe doing feature enabling/disabling (and also for LE) is better
> > handled via /sys/kernel/debug/bluetooth/enable_hs.
> 
> This is really the only place where anything other than the enable_hs 
> value itself needs to be manipulated.  It would be less code to just 
> add an "else" and clear the feature mask bit.  l2cap_fixed_chan[] is 
> not used anywhere outside this function.
> 
> If we want to move enable_hs to debugfs, I'll drop this change from 
> patch set and work on debugfs support for the next patch set.
> 
> Any other opinions?

right now, I would be fine with an else statement to clear the bit.
However we better add a note to get this merged over to debugfs. In
general it seems a way better place anyway.

Regards

Marcel

[PATCHv2 8/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 |  129 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 127 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f250392..fe8aaf7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3059,8 +3059,7 @@ static inline int l2cap_create_channel_req(struct l2cap_conn *conn,
 	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);
+	BT_DBG("psm %d, scid %d, amp_id %d", psm, scid, req->amp_id);
 
 	if (!enable_hs)
 		return -EINVAL;
@@ -3085,6 +3084,115 @@ 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", icid, 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", icid, 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", 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_MR_NOT_ALLOWED;
+
+	icid = le16_to_cpu(req->icid);
+
+	BT_DBG("icid %d, dest_amp_id %d", icid, req->dest_amp_id);
+
+	if (!enable_hs)
+		return -EINVAL;
+
+	/* 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", icid, result);
+
+	/* Placeholder: Always unconfirmed */
+	l2cap_send_move_chan_cfm(conn, NULL, icid, L2CAP_MC_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", icid, 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", icid);
+
+	return 0;
+}
+
 static inline int l2cap_check_conn_param(u16 min, u16 max, u16 latency,
 							u16 to_multiplier)
 {
@@ -3205,6 +3313,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: [PATCHv2 8/9] Bluetooth: Add signal handlers for channel moves

[Marcel Holtmann]

Hi Mat,

> 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 |  129 +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 127 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index f250392..fe8aaf7 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3059,8 +3059,7 @@ static inline int l2cap_create_channel_req(struct l2cap_conn *conn,
>  	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);
> +	BT_DBG("psm %d, scid %d, amp_id %d", psm, scid, req->amp_id);

you need some git foo magic to not add something first and then fix it
up later ;)
 
>  	if (!enable_hs)
>  		return -EINVAL;
> @@ -3085,6 +3084,115 @@ 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", icid, 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", icid, 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", 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_MR_NOT_ALLOWED;
> +
> +	icid = le16_to_cpu(req->icid);
> +
> +	BT_DBG("icid %d, dest_amp_id %d", icid, req->dest_amp_id);
> +
> +	if (!enable_hs)
> +		return -EINVAL;
> +
> +	/* 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", icid, result);
> +
> +	/* Placeholder: Always unconfirmed */
> +	l2cap_send_move_chan_cfm(conn, NULL, icid, L2CAP_MC_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", icid, 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", icid);
> +
> +	return 0;
> +}
> +
>  static inline int l2cap_check_conn_param(u16 min, u16 max, u16 latency,
>  							u16 to_multiplier)
>  {
> @@ -3205,6 +3313,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;

so besides my comment about u8 *data and the on above, this looks fine
to me.

Regards

Marcel

[PATCHv2 9/9] Bluetooth: Guarantee BR-EDR device will be registered as hci0

[Mat Martineau]

It's convenient to use the HCI device index the AMP controller id, but
the spec requires that an AMP controller never has id 0.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fdcbf8f..78e6b05 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1426,7 +1426,7 @@ int hci_add_adv_entry(struct hci_dev *hdev,
 int hci_register_dev(struct hci_dev *hdev)
 {
 	struct list_head *head = &hci_dev_list, *p;
-	int i, id = 0, error;
+	int i, id, error;
 
 	BT_DBG("%p name %s bus %d owner %p", hdev, hdev->name,
 						hdev->bus, hdev->owner);
@@ -1434,6 +1434,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	if (!hdev->open || !hdev->close || !hdev->destruct)
 		return -EINVAL;
 
+	id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
+
 	write_lock_bh(&hci_dev_list_lock);
 
 	/* Find first available device id */
-- 
1.7.7

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

Re: [PATCHv2 9/9] Bluetooth: Guarantee BR-EDR device will be registered as hci0

[Marcel Holtmann]

Hi Mat,

> It's convenient to use the HCI device index the AMP controller id, but
> the spec requires that an AMP controller never has id 0.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/hci_core.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index fdcbf8f..78e6b05 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1426,7 +1426,7 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>  int hci_register_dev(struct hci_dev *hdev)
>  {
>  	struct list_head *head = &hci_dev_list, *p;
> -	int i, id = 0, error;
> +	int i, id, error;
>  	BT_DBG("%p name %s bus %d owner %p", hdev, hdev->name,
>  						hdev->bus, hdev->owner);
> @@ -1434,6 +1434,8 @@ int hci_register_dev(struct hci_dev *hdev)
>  	if (!hdev->open || !hdev->close || !hdev->destruct)
>  		return -EINVAL;
>  
> +	id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
> +

this is a pretty nasty hack. Especially since we can have as many BR/EDR
controllers as we want and then our list of AMPs can have empty spots in
their list. Also it can happen that the first AMP controller has id 2 or
higher.

In general I am fine with this, but please add an extra comment about
the AMP not allowed to be id 0 above this change (similar to what your
commit message says).

Regards

Marcel