* [PATCHv5 09/18] Bluetooth: Move channel response
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
The move response command includes a result code indicationg
"pending", "success", or "failure" status. A pending result is
received when the remote address is still setting up a physical link,
and will be followed by success or failure. On success, logical link
setup will proceed. On failure, the move is stopped. The receiver of
a move channel response must always follow up by sending a move
channel confirm command.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/l2cap.h | 2 +
net/bluetooth/l2cap_core.c | 183 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 170 insertions(+), 15 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 6d3615e..b4c3c65 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -52,6 +52,8 @@
#define L2CAP_ENC_TIMEOUT msecs_to_jiffies(5000)
#define L2CAP_CONN_TIMEOUT msecs_to_jiffies(40000)
#define L2CAP_INFO_TIMEOUT msecs_to_jiffies(4000)
+#define L2CAP_MOVE_TIMEOUT msecs_to_jiffies(4000)
+#define L2CAP_MOVE_ERTX_TIMEOUT msecs_to_jiffies(60000)
#define L2CAP_A2MP_DEFAULT_MTU 670
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index fef0394..2277ed5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -129,6 +129,20 @@ static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn,
return NULL;
}
+static struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn,
+ u8 ident)
+{
+ struct l2cap_chan *c;
+
+ mutex_lock(&conn->chan_lock);
+ c = __l2cap_get_chan_by_ident(conn, ident);
+ if (c)
+ l2cap_chan_lock(c);
+ mutex_unlock(&conn->chan_lock);
+
+ return c;
+}
+
static struct l2cap_chan *__l2cap_global_chan_by_addr(__le16 psm, bdaddr_t *src)
{
struct l2cap_chan *c;
@@ -4185,23 +4199,34 @@ static void l2cap_send_move_chan_rsp(struct l2cap_conn *conn, u8 ident,
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)
+static void l2cap_send_move_chan_cfm(struct l2cap_chan *chan, u16 result)
{
struct l2cap_move_chan_cfm cfm;
- u8 ident;
- BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
+ BT_DBG("chan %p, result 0x%4.4x", chan, result);
- ident = l2cap_get_ident(conn);
- if (chan)
- chan->ident = ident;
+ chan->ident = l2cap_get_ident(chan->conn);
- cfm.icid = cpu_to_le16(icid);
+ cfm.icid = cpu_to_le16(chan->scid);
cfm.result = cpu_to_le16(result);
- l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM, sizeof(cfm), &cfm);
+ l2cap_send_cmd(chan->conn, chan->ident, L2CAP_MOVE_CHAN_CFM,
+ sizeof(cfm), &cfm);
+
+ __set_chan_timer(chan, L2CAP_MOVE_TIMEOUT);
+}
+
+static void l2cap_send_move_chan_cfm_icid(struct l2cap_conn *conn, u16 icid)
+{
+ struct l2cap_move_chan_cfm cfm;
+
+ BT_DBG("conn %p, icid 0x%4.4x", conn, icid);
+
+ cfm.icid = cpu_to_le16(icid);
+ cfm.result = __constant_cpu_to_le16(L2CAP_MC_UNCONFIRMED);
+
+ l2cap_send_cmd(conn, l2cap_get_ident(conn), L2CAP_MOVE_CHAN_CFM,
+ sizeof(cfm), &cfm);
}
static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
@@ -4223,6 +4248,13 @@ static void __release_logical_link(struct l2cap_chan *chan)
/* Placeholder - release the logical link */
}
+static void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
+ u8 status)
+{
+ /* Placeholder */
+ return;
+}
+
static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
struct l2cap_cmd_hdr *cmd,
u16 cmd_len, void *data)
@@ -4317,9 +4349,128 @@ send_move_response:
return 0;
}
-static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
- struct l2cap_cmd_hdr *cmd,
- u16 cmd_len, void *data)
+static void l2cap_move_continue(struct l2cap_conn *conn, u16 icid, u16 result)
+{
+ struct l2cap_chan *chan;
+ struct hci_chan *hchan = NULL;
+
+ chan = l2cap_get_chan_by_scid(conn, icid);
+ if (!chan) {
+ l2cap_send_move_chan_cfm_icid(conn, icid);
+ return;
+ }
+
+ __clear_chan_timer(chan);
+ if (result == L2CAP_MR_PEND)
+ __set_chan_timer(chan, L2CAP_MOVE_ERTX_TIMEOUT);
+
+ switch (chan->move_state) {
+ case L2CAP_MOVE_WAIT_LOGICAL_COMP:
+ /* Move confirm will be sent when logical link
+ * is complete.
+ */
+ chan->move_state = L2CAP_MOVE_WAIT_LOGICAL_CFM;
+ break;
+ case L2CAP_MOVE_WAIT_RSP_SUCCESS:
+ if (result == L2CAP_MR_PEND) {
+ break;
+ } else if (test_bit(CONN_LOCAL_BUSY,
+ &chan->conn_state)) {
+ chan->move_state = L2CAP_MOVE_WAIT_LOCAL_BUSY;
+ } else {
+ /* Logical link is up or moving to BR/EDR,
+ * proceed with move
+ */
+ chan->move_state = L2CAP_MOVE_WAIT_CONFIRM_RSP;
+ l2cap_send_move_chan_cfm(chan, L2CAP_MC_CONFIRMED);
+ }
+ break;
+ case L2CAP_MOVE_WAIT_RSP:
+ /* Moving to AMP */
+ if (result == L2CAP_MR_SUCCESS) {
+ /* Remote is ready, send confirm immediately
+ * after logical link is ready
+ */
+ chan->move_state = L2CAP_MOVE_WAIT_LOGICAL_CFM;
+ } else {
+ /* Both logical link and move success
+ * are required to confirm
+ */
+ chan->move_state = L2CAP_MOVE_WAIT_LOGICAL_COMP;
+ }
+
+ /* Placeholder - get hci_chan for logical link */
+ if (!hchan) {
+ /* Logical link not available */
+ l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
+ break;
+ }
+
+ /* If the logical link is not yet connected, do not
+ * send confirmation.
+ */
+ if (hchan->state != BT_CONNECTED)
+ break;
+
+ /* Logical link is already ready to go */
+
+ chan->hs_hcon = hchan->conn;
+ chan->hs_hcon->l2cap_data = chan->conn;
+
+ if (result == L2CAP_MR_SUCCESS) {
+ /* Can confirm now */
+ l2cap_send_move_chan_cfm(chan, L2CAP_MC_CONFIRMED);
+ } else {
+ /* Now only need move success
+ * to confirm
+ */
+ chan->move_state = L2CAP_MOVE_WAIT_RSP_SUCCESS;
+ }
+
+ l2cap_logical_cfm(chan, hchan, L2CAP_MR_SUCCESS);
+ break;
+ default:
+ /* Any other amp move state means the move failed. */
+ chan->move_id = chan->local_amp_id;
+ l2cap_move_done(chan);
+ l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
+ }
+
+ l2cap_chan_unlock(chan);
+}
+
+static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
+ u16 result)
+{
+ struct l2cap_chan *chan;
+
+ chan = l2cap_get_chan_by_ident(conn, ident);
+ if (!chan) {
+ /* Could not locate channel, icid is best guess */
+ l2cap_send_move_chan_cfm_icid(conn, icid);
+ return;
+ }
+
+ __clear_chan_timer(chan);
+
+ if (chan->move_role == L2CAP_MOVE_ROLE_INITIATOR) {
+ if (result == L2CAP_MR_COLLISION) {
+ chan->move_role = L2CAP_MOVE_ROLE_RESPONDER;
+ } else {
+ /* Cleanup - cancel move */
+ chan->move_id = chan->local_amp_id;
+ l2cap_move_done(chan);
+ }
+ }
+
+ l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
+
+ l2cap_chan_unlock(chan);
+}
+
+static int l2cap_move_channel_rsp(struct l2cap_conn *conn,
+ struct l2cap_cmd_hdr *cmd,
+ u16 cmd_len, void *data)
{
struct l2cap_move_chan_rsp *rsp = data;
u16 icid, result;
@@ -4332,8 +4483,10 @@ static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
- /* Placeholder: Always unconfirmed */
- l2cap_send_move_chan_cfm(conn, NULL, icid, L2CAP_MC_UNCONFIRMED);
+ if (result == L2CAP_MR_SUCCESS || result == L2CAP_MR_PEND)
+ l2cap_move_continue(conn, icid, result);
+ else
+ l2cap_move_fail(conn, cmd->ident, icid, result);
return 0;
}
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 08/18] Bluetooth: Add state to hci_chan
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
On an AMP controller, hci_chan maps to a logical link. When a channel
is being moved, the logical link may or may not be connected already.
The hci_chan->state is used to determine the existance of a useable
logical link so the link can be either used or requested.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_conn.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9fe8e2d..00abc52 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -355,6 +355,7 @@ struct hci_chan {
struct hci_conn *conn;
struct sk_buff_head data_q;
unsigned int sent;
+ __u8 state;
};
extern struct list_head hci_dev_list;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index fe64621..6dcf452 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -959,6 +959,7 @@ struct hci_chan *hci_chan_create(struct hci_conn *conn)
chan->conn = conn;
skb_queue_head_init(&chan->data_q);
+ chan->state = BT_CONNECTED;
list_add_rcu(&chan->list, &conn->chan_list);
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 07/18] Bluetooth: Add move channel confirm handling
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
After sending a move channel response, a move responder waits for a
move channel confirm command. If the received command has a
"confirmed" result the move is proceeding, and "unconfirmed" means the
move has failed and the channel will not change controllers.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 58 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 55 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b9a91bf..fef0394 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1037,6 +1037,28 @@ static void l2cap_move_setup(struct l2cap_chan *chan)
set_bit(CONN_REMOTE_BUSY, &chan->conn_state);
}
+static void l2cap_move_done(struct l2cap_chan *chan)
+{
+ u8 move_role = chan->move_role;
+ BT_DBG("chan %p", chan);
+
+ chan->move_state = L2CAP_MOVE_STABLE;
+ chan->move_role = L2CAP_MOVE_ROLE_NONE;
+
+ if (chan->mode != L2CAP_MODE_ERTM)
+ return;
+
+ switch (move_role) {
+ case L2CAP_MOVE_ROLE_INITIATOR:
+ l2cap_tx(chan, NULL, NULL, L2CAP_EV_EXPLICIT_POLL);
+ chan->rx_state = L2CAP_RX_STATE_WAIT_F;
+ break;
+ case L2CAP_MOVE_ROLE_RESPONDER:
+ chan->rx_state = L2CAP_RX_STATE_WAIT_P;
+ break;
+ }
+}
+
static void l2cap_chan_ready(struct l2cap_chan *chan)
{
/* This clears all conf flags, including CONF_NOT_COMPLETE */
@@ -4193,6 +4215,14 @@ static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
}
+static void __release_logical_link(struct l2cap_chan *chan)
+{
+ chan->hs_hchan = NULL;
+ chan->hs_hcon = NULL;
+
+ /* Placeholder - release the logical link */
+}
+
static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
struct l2cap_cmd_hdr *cmd,
u16 cmd_len, void *data)
@@ -4308,11 +4338,12 @@ static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
return 0;
}
-static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
- struct l2cap_cmd_hdr *cmd,
- u16 cmd_len, void *data)
+static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
+ struct l2cap_cmd_hdr *cmd,
+ u16 cmd_len, void *data)
{
struct l2cap_move_chan_cfm *cfm = data;
+ struct l2cap_chan *chan;
u16 icid, result;
if (cmd_len != sizeof(*cfm))
@@ -4323,8 +4354,29 @@ static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
+ chan = l2cap_get_chan_by_dcid(conn, icid);
+ if (!chan) {
+ /* Spec requires a response even if the icid was not found */
+ l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
+ return 0;
+ }
+
+ if (chan->move_state == L2CAP_MOVE_WAIT_CONFIRM) {
+ if (result == L2CAP_MC_CONFIRMED) {
+ chan->local_amp_id = chan->move_id;
+ if (!chan->local_amp_id)
+ __release_logical_link(chan);
+ } else {
+ chan->move_id = chan->local_amp_id;
+ }
+
+ l2cap_move_done(chan);
+ }
+
l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
+ l2cap_chan_unlock(chan);
+
return 0;
}
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 06/18] Bluetooth: Add new ERTM receive states for channel move
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
Two new states are required to implement channel moves with the ERTM
receive state machine.
The "WAIT_P" state is used by a move responder to wait for a "poll"
flag after a move is completed (success or failure). "WAIT_F" is
similarly used by a move initiator to wait for a "final" flag when the
move is completing. In either state, the reqseq value in the
poll/final frame tells the state machine exactly which frame should be
expected next.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/l2cap_core.c | 102 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 102 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 24729f5..b9a91bf 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4713,6 +4713,12 @@ static int l2cap_reassemble_sdu(struct l2cap_chan *chan, struct sk_buff *skb,
return err;
}
+static int l2cap_resegment(struct l2cap_chan *chan)
+{
+ /* Placeholder */
+ return 0;
+}
+
void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
{
u8 event;
@@ -5218,6 +5224,96 @@ static int l2cap_rx_state_srej_sent(struct l2cap_chan *chan,
return err;
}
+static int l2cap_finish_move(struct l2cap_chan *chan)
+{
+ BT_DBG("chan %p", chan);
+
+ chan->rx_state = L2CAP_RX_STATE_RECV;
+
+ if (chan->hs_hcon)
+ chan->conn->mtu = chan->hs_hcon->hdev->block_mtu;
+ else
+ chan->conn->mtu = chan->conn->hcon->hdev->acl_mtu;
+
+ return l2cap_resegment(chan);
+}
+
+static int l2cap_rx_state_wait_p(struct l2cap_chan *chan,
+ struct l2cap_ctrl *control,
+ struct sk_buff *skb, u8 event)
+{
+ int err;
+
+ BT_DBG("chan %p, control %p, skb %p, event %d", chan, control, skb,
+ event);
+
+ if (!control->poll)
+ return -EPROTO;
+
+ l2cap_process_reqseq(chan, control->reqseq);
+
+ if (!skb_queue_empty(&chan->tx_q))
+ chan->tx_send_head = skb_peek(&chan->tx_q);
+ else
+ chan->tx_send_head = NULL;
+
+ /* Rewind next_tx_seq to the point expected
+ * by the receiver.
+ */
+ chan->next_tx_seq = control->reqseq;
+ chan->unacked_frames = 0;
+
+ err = l2cap_finish_move(chan);
+ if (err)
+ return err;
+
+ set_bit(CONN_SEND_FBIT, &chan->conn_state);
+ l2cap_send_i_or_rr_or_rnr(chan);
+
+ if (event == L2CAP_EV_RECV_IFRAME)
+ return -EPROTO;
+
+ return l2cap_rx_state_recv(chan, control, NULL, event);
+}
+
+static int l2cap_rx_state_wait_f(struct l2cap_chan *chan,
+ struct l2cap_ctrl *control,
+ struct sk_buff *skb, u8 event)
+{
+ int err;
+
+ if (!control->final)
+ return -EPROTO;
+
+ clear_bit(CONN_REMOTE_BUSY, &chan->conn_state);
+
+ chan->rx_state = L2CAP_RX_STATE_RECV;
+ l2cap_process_reqseq(chan, control->reqseq);
+
+ if (!skb_queue_empty(&chan->tx_q))
+ chan->tx_send_head = skb_peek(&chan->tx_q);
+ else
+ chan->tx_send_head = NULL;
+
+ /* Rewind next_tx_seq to the point expected
+ * by the receiver.
+ */
+ chan->next_tx_seq = control->reqseq;
+ chan->unacked_frames = 0;
+
+ if (chan->hs_hcon)
+ chan->conn->mtu = chan->hs_hcon->hdev->block_mtu;
+ else
+ chan->conn->mtu = chan->conn->hcon->hdev->acl_mtu;
+
+ err = l2cap_resegment(chan);
+
+ if (!err)
+ err = l2cap_rx_state_recv(chan, control, skb, event);
+
+ return err;
+}
+
static bool __valid_reqseq(struct l2cap_chan *chan, u16 reqseq)
{
/* Make sure reqseq is for a packet that has been sent but not acked */
@@ -5244,6 +5340,12 @@ static int l2cap_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
err = l2cap_rx_state_srej_sent(chan, control, skb,
event);
break;
+ case L2CAP_RX_STATE_WAIT_P:
+ err = l2cap_rx_state_wait_p(chan, control, skb, event);
+ break;
+ case L2CAP_RX_STATE_WAIT_F:
+ err = l2cap_rx_state_wait_f(chan, control, skb, event);
+ break;
default:
/* shut it down */
break;
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 05/18] Bluetooth: Channel move request handling
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
On receipt of a channel move request, the request must be validated
based on the L2CAP mode, connection state, and controller
capabilities. ERTM channels must have their state machines cleared
and transmission paused while the channel move takes place.
If the channel is being moved to an AMP controller then
an AMP physical link must be prepared. Moving the channel back to
BR/EDR proceeds immediately.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/l2cap_core.c | 113 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 112 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 03daae8..24729f5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -735,6 +735,12 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len,
hci_send_acl(conn->hchan, skb, flags);
}
+static bool __chan_is_moving(struct l2cap_chan *chan)
+{
+ return chan->move_state != L2CAP_MOVE_STABLE &&
+ chan->move_state != L2CAP_MOVE_WAIT_PREPARE;
+}
+
static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
{
struct hci_conn *hcon = chan->conn->hcon;
@@ -996,6 +1002,41 @@ void l2cap_send_conn_req(struct l2cap_chan *chan)
l2cap_send_cmd(conn, chan->ident, L2CAP_CONN_REQ, sizeof(req), &req);
}
+static void l2cap_move_setup(struct l2cap_chan *chan)
+{
+ struct sk_buff *skb;
+
+ BT_DBG("chan %p", chan);
+
+ if (chan->mode != L2CAP_MODE_ERTM)
+ return;
+
+ __clear_retrans_timer(chan);
+ __clear_monitor_timer(chan);
+ __clear_ack_timer(chan);
+
+ chan->retry_count = 0;
+ skb_queue_walk(&chan->tx_q, skb) {
+ if (bt_cb(skb)->control.retries)
+ bt_cb(skb)->control.retries = 1;
+ else
+ break;
+ }
+
+ chan->expected_tx_seq = chan->buffer_seq;
+
+ clear_bit(CONN_REJ_ACT, &chan->conn_state);
+ clear_bit(CONN_SREJ_ACT, &chan->conn_state);
+ l2cap_seq_list_clear(&chan->retrans_list);
+ l2cap_seq_list_clear(&chan->srej_list);
+ skb_queue_purge(&chan->srej_q);
+
+ chan->tx_state = L2CAP_TX_STATE_XMIT;
+ chan->rx_state = L2CAP_RX_STATE_MOVE;
+
+ set_bit(CONN_REMOTE_BUSY, &chan->conn_state);
+}
+
static void l2cap_chan_ready(struct l2cap_chan *chan)
{
/* This clears all conf flags, including CONF_NOT_COMPLETE */
@@ -4157,6 +4198,7 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
u16 cmd_len, void *data)
{
struct l2cap_move_chan_req *req = data;
+ struct l2cap_chan *chan;
u16 icid = 0;
u16 result = L2CAP_MR_NOT_ALLOWED;
@@ -4170,9 +4212,78 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
if (!enable_hs)
return -EINVAL;
- /* Placeholder: Always refuse */
+ chan = l2cap_get_chan_by_dcid(conn, icid);
+ if (!chan) {
+ l2cap_send_move_chan_rsp(conn, cmd->ident, icid,
+ L2CAP_MR_NOT_ALLOWED);
+ return 0;
+ }
+
+ if (chan->scid < L2CAP_CID_DYN_START ||
+ chan->chan_policy == BT_CHANNEL_POLICY_BREDR_ONLY ||
+ (chan->mode != L2CAP_MODE_ERTM &&
+ chan->mode != L2CAP_MODE_STREAMING)) {
+ result = L2CAP_MR_NOT_ALLOWED;
+ goto send_move_response;
+ }
+
+ if (chan->local_amp_id == req->dest_amp_id) {
+ result = L2CAP_MR_SAME_ID;
+ goto send_move_response;
+ }
+
+ if (req->dest_amp_id) {
+ struct hci_dev *hdev;
+ hdev = hci_dev_get(req->dest_amp_id);
+ if (!hdev || hdev->dev_type != HCI_AMP ||
+ !test_bit(HCI_UP, &hdev->flags)) {
+ if (hdev)
+ hci_dev_put(hdev);
+
+ result = L2CAP_MR_BAD_ID;
+ goto send_move_response;
+ }
+ hci_dev_put(hdev);
+ }
+
+ /* Detect a move collision. Only send a collision response
+ * if this side has "lost", otherwise proceed with the move.
+ * The winner has the larger bd_addr.
+ */
+ if ((__chan_is_moving(chan) ||
+ chan->move_role != L2CAP_MOVE_ROLE_NONE) &&
+ bacmp(conn->src, conn->dst) > 0) {
+ result = L2CAP_MR_COLLISION;
+ goto send_move_response;
+ }
+
+ chan->ident = cmd->ident;
+ chan->move_role = L2CAP_MOVE_ROLE_RESPONDER;
+ l2cap_move_setup(chan);
+ chan->move_id = req->dest_amp_id;
+ icid = chan->dcid;
+
+ if (!req->dest_amp_id) {
+ /* Moving to BR/EDR */
+ if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
+ chan->move_state = L2CAP_MOVE_WAIT_LOCAL_BUSY;
+ result = L2CAP_MR_PEND;
+ } else {
+ chan->move_state = L2CAP_MOVE_WAIT_CONFIRM;
+ result = L2CAP_MR_SUCCESS;
+ }
+ } else {
+ chan->move_state = L2CAP_MOVE_WAIT_PREPARE;
+ /* Placeholder - uncomment when amp functions are available */
+ /*amp_accept_physical(chan, req->dest_amp_id);*/
+ result = L2CAP_MR_PEND;
+ }
+
+send_move_response:
l2cap_send_move_chan_rsp(conn, cmd->ident, icid, result);
+ l2cap_chan_unlock(chan);
+
return 0;
}
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 04/18] Bluetooth: Lookup channel structure based on DCID
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
Processing a move channel request involves getting the channel
structure using the destination channel ID. Previous code could only
look up using the source channel ID.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/l2cap_core.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 3175094..03daae8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -100,6 +100,23 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
return c;
}
+/* Find channel with given DCID.
+ * Returns locked channel.
+ */
+static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
+ u16 cid)
+{
+ struct l2cap_chan *c;
+
+ mutex_lock(&conn->chan_lock);
+ c = __l2cap_get_chan_by_dcid(conn, cid);
+ if (c)
+ l2cap_chan_lock(c);
+ mutex_unlock(&conn->chan_lock);
+
+ return c;
+}
+
static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn,
u8 ident)
{
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 03/18] Bluetooth: Remove unnecessary intermediate function
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
Resolves a conflict resolution issue in "Bluetooth: Fix L2CAP coding
style". The remaining connect and create channel response handler is
renamed to better reflect its use for both response types.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/l2cap_core.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2b3eef7..3175094 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3538,7 +3538,7 @@ static int l2cap_connect_req(struct l2cap_conn *conn,
return 0;
}
-static inline int l2cap_connect_rsp(struct l2cap_conn *conn,
+static int l2cap_connect_create_rsp(struct l2cap_conn *conn,
struct l2cap_cmd_hdr *cmd, u8 *data)
{
struct l2cap_conn_rsp *rsp = (struct l2cap_conn_rsp *) data;
@@ -4092,15 +4092,6 @@ static int l2cap_create_channel_req(struct l2cap_conn *conn,
return 0;
}
-static inline int l2cap_create_channel_rsp(struct l2cap_conn *conn,
- struct l2cap_cmd_hdr *cmd,
- void *data)
-{
- BT_DBG("conn %p", 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)
{
@@ -4307,7 +4298,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
case L2CAP_CONN_RSP:
case L2CAP_CREATE_CHAN_RSP:
- err = l2cap_connect_rsp(conn, cmd, data);
+ err = l2cap_connect_create_rsp(conn, cmd, data);
break;
case L2CAP_CONF_REQ:
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 02/18] Bluetooth: Add L2CAP create channel request handling
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo
Cc: sunnyk, marcel, andrei.emeltchenko.news, Andrei Emeltchenko
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
The L2CAP create channel request is very similar to an L2CAP connect
request, but it has an additional parameter for the controller ID. If
the controller id is 0, the channel is set up on the BR/EDR controller
(just like a connect request). Using a valid high speed controller ID
will cause the channel to be initially created on that high speed
controller. While the L2CAP data will be initially routed over the
AMP controller, the L2CAP fixed signaling channel only uses BR/EDR.
When a create channel request is received for a high speed controller,
a pending response is always sent first. After the high speed
physical and logical links are complete a success response will be
sent.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 63 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c1b169f..2b3eef7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3400,8 +3400,9 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn,
return 0;
}
-static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd,
- u8 *data, u8 rsp_code, u8 amp_id)
+static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
+ struct l2cap_cmd_hdr *cmd,
+ u8 *data, u8 rsp_code, u8 amp_id)
{
struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
struct l2cap_conn_rsp rsp;
@@ -3452,6 +3453,7 @@ static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd,
bacpy(&bt_sk(sk)->dst, conn->dst);
chan->psm = psm;
chan->dcid = scid;
+ chan->local_amp_id = amp_id;
__l2cap_chan_add(conn, chan);
@@ -3469,8 +3471,17 @@ static void l2cap_connect(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd,
status = L2CAP_CS_AUTHOR_PEND;
chan->ops->defer(chan);
} else {
- __l2cap_state_change(chan, BT_CONFIG);
- result = L2CAP_CR_SUCCESS;
+ /* Force pending result for AMP controllers.
+ * The connection will succeed after the
+ * physical link is up.
+ */
+ if (amp_id) {
+ __l2cap_state_change(chan, BT_CONNECT2);
+ result = L2CAP_CR_PEND;
+ } else {
+ __l2cap_state_change(chan, BT_CONFIG);
+ result = L2CAP_CR_SUCCESS;
+ }
status = L2CAP_CS_NO_INFO;
}
} else {
@@ -3516,6 +3527,8 @@ sendresp:
l2cap_build_conf_req(chan, buf), buf);
chan->num_conf_req++;
}
+
+ return chan;
}
static int l2cap_connect_req(struct l2cap_conn *conn,
@@ -4028,12 +4041,12 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn,
return 0;
}
-static inline int l2cap_create_channel_req(struct l2cap_conn *conn,
- struct l2cap_cmd_hdr *cmd,
- u16 cmd_len, void *data)
+static int l2cap_create_channel_req(struct l2cap_conn *conn,
+ struct l2cap_cmd_hdr *cmd,
+ u16 cmd_len, void *data)
{
struct l2cap_create_chan_req *req = data;
- struct l2cap_create_chan_rsp rsp;
+ struct l2cap_chan *chan;
u16 psm, scid;
if (cmd_len != sizeof(*req))
@@ -4047,14 +4060,34 @@ static inline int l2cap_create_channel_req(struct l2cap_conn *conn,
BT_DBG("psm 0x%2.2x, scid 0x%4.4x, amp_id %d", psm, scid, req->amp_id);
- /* Placeholder: Always reject */
- rsp.dcid = 0;
- rsp.scid = cpu_to_le16(scid);
- rsp.result = __constant_cpu_to_le16(L2CAP_CR_NO_MEM);
- rsp.status = __constant_cpu_to_le16(L2CAP_CS_NO_INFO);
+ if (req->amp_id) {
+ struct hci_dev *hdev;
- l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
- sizeof(rsp), &rsp);
+ /* Validate AMP controller id */
+ hdev = hci_dev_get(req->amp_id);
+ if (!hdev || hdev->dev_type != HCI_AMP ||
+ !test_bit(HCI_UP, &hdev->flags)) {
+ struct l2cap_create_chan_rsp rsp;
+
+ rsp.dcid = 0;
+ rsp.scid = cpu_to_le16(scid);
+ rsp.result = __constant_cpu_to_le16(L2CAP_CR_BAD_AMP);
+ rsp.status = __constant_cpu_to_le16(L2CAP_CS_NO_INFO);
+
+ l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
+ sizeof(rsp), &rsp);
+
+ if (hdev)
+ hci_dev_put(hdev);
+
+ return 0;
+ }
+
+ hci_dev_put(hdev);
+ }
+
+ chan = l2cap_connect(conn, cmd, data, L2CAP_CREATE_CHAN_RSP,
+ req->amp_id);
return 0;
}
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 01/18] Bluetooth: Add new l2cap_chan struct members for high speed channels
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo
Cc: sunnyk, marcel, andrei.emeltchenko.news, Andrei Emeltchenko
In-Reply-To: <1351031063-11791-1-git-send-email-mathewm@codeaurora.org>
An L2CAP channel using high speed continues to be associated with a
BR/EDR l2cap_conn, while also tracking an additional hci_conn
(representing a physical link on a high speed controller) and hci_chan
(representing a logical link). There may only be one physical link
between two high speed controllers. Each physical link may contain
several logical links, with each logical link representing a channel
with specific quality of service.
During a channel move, the destination channel id, current move state,
and role (initiator vs. responder) are tracked and used by the channel
move state machine. The ident value associated with a move request
must also be stored in order to use it in later move responses.
The active channel is stored in local_amp_id.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/l2cap.h | 29 +++++++++++++++++++++++++++++
net/bluetooth/l2cap_core.c | 5 +++++
2 files changed, 34 insertions(+)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 6e23afd..6d3615e 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -434,6 +434,8 @@ struct l2cap_chan {
struct sock *sk;
struct l2cap_conn *conn;
+ struct hci_conn *hs_hcon;
+ struct hci_chan *hs_hchan;
struct kref kref;
__u8 state;
@@ -477,6 +479,11 @@ struct l2cap_chan {
unsigned long conn_state;
unsigned long flags;
+ __u8 local_amp_id;
+ __u8 move_id;
+ __u8 move_state;
+ __u8 move_role;
+
__u16 next_tx_seq;
__u16 expected_ack_seq;
__u16 expected_tx_seq;
@@ -644,6 +651,9 @@ enum {
enum {
L2CAP_RX_STATE_RECV,
L2CAP_RX_STATE_SREJ_SENT,
+ L2CAP_RX_STATE_MOVE,
+ L2CAP_RX_STATE_WAIT_P,
+ L2CAP_RX_STATE_WAIT_F,
};
enum {
@@ -674,6 +684,25 @@ enum {
L2CAP_EV_RECV_FRAME,
};
+enum {
+ L2CAP_MOVE_ROLE_NONE,
+ L2CAP_MOVE_ROLE_INITIATOR,
+ L2CAP_MOVE_ROLE_RESPONDER,
+};
+
+enum {
+ L2CAP_MOVE_STABLE,
+ L2CAP_MOVE_WAIT_REQ,
+ L2CAP_MOVE_WAIT_RSP,
+ L2CAP_MOVE_WAIT_RSP_SUCCESS,
+ L2CAP_MOVE_WAIT_CONFIRM,
+ L2CAP_MOVE_WAIT_CONFIRM_RSP,
+ L2CAP_MOVE_WAIT_LOGICAL_COMP,
+ L2CAP_MOVE_WAIT_LOGICAL_CFM,
+ L2CAP_MOVE_WAIT_LOCAL_BUSY,
+ L2CAP_MOVE_WAIT_PREPARE,
+};
+
void l2cap_chan_hold(struct l2cap_chan *c);
void l2cap_chan_put(struct l2cap_chan *c);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 08efc25..c1b169f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2788,6 +2788,11 @@ int l2cap_ertm_init(struct l2cap_chan *chan)
skb_queue_head_init(&chan->tx_q);
+ chan->local_amp_id = 0;
+ chan->move_id = 0;
+ chan->move_state = L2CAP_MOVE_STABLE;
+ chan->move_role = L2CAP_MOVE_ROLE_NONE;
+
if (chan->mode != L2CAP_MODE_ERTM)
return 0;
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCHv5 00/18] L2CAP signaling for AMP channel create/move
From: Mat Martineau @ 2012-10-23 22:24 UTC (permalink / raw)
To: linux-bluetooth, gustavo; +Cc: sunnyk, marcel, andrei.emeltchenko.news
Here are the changes that process commands on the L2CAP signaling
channel for setting up AMP channels. There's still a lot of
integration to do as other AMP functionality is implemented. I've
marked places that require this integration with "Placeholder"
comments (look for that string).
Changes:
* PATCHv5 - Additional style updates to patch 10. Refactored
l2cap_move_channel_req in patch 5 to remove conditional
unlock. Everything has been acked by Marcel except
patch 10.
* PATCHv4 - More acks, several code organization fixups to patch 10.
send_move_chan_* helper functions are now more helpful.
* PATCHv3 - Dropped patch 17, fixed style issues in 5, 7, and 9.
Split up a large function in patch 10.
* PATCHv2 - Fixed formatting and style issues, added acks
* PATCHv1 - Rebased after AMP physical link patch set, incorporated
mailing list feedback.
* RFCv1 - Finished commit messages, fixed formatting/style issues
* RFCv0 - Initial post
Mat Martineau (18):
Bluetooth: Add new l2cap_chan struct members for high speed channels
Bluetooth: Add L2CAP create channel request handling
Bluetooth: Remove unnecessary intermediate function
Bluetooth: Lookup channel structure based on DCID
Bluetooth: Channel move request handling
Bluetooth: Add new ERTM receive states for channel move
Bluetooth: Add move channel confirm handling
Bluetooth: Add state to hci_chan
Bluetooth: Move channel response
Bluetooth: Add logical link confirm
Bluetooth: Add move confirm response handling
Bluetooth: Handle physical link completion
Bluetooth: Flag ACL frames as complete for AMP controllers
Bluetooth: Do not send data during channel move
Bluetooth: Configure appropriate timeouts for AMP controllers
Bluetooth: Ignore BR/EDR packet size constraints when fragmenting for
AMP
Bluetooth: Do not retransmit data during a channel move
Bluetooth: Start channel move when socket option is changed
include/net/bluetooth/hci_core.h | 1 +
include/net/bluetooth/l2cap.h | 32 ++
net/bluetooth/hci_conn.c | 1 +
net/bluetooth/l2cap_core.c | 947 ++++++++++++++++++++++++++++++++++++---
net/bluetooth/l2cap_sock.c | 5 +
5 files changed, 930 insertions(+), 56 deletions(-)
--
1.8.0
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCHv4 10/18] Bluetooth: Add logical link confirm
From: Marcel Holtmann @ 2012-10-23 21:51 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, sunnyk, andrei.emeltchenko.news
In-Reply-To: <alpine.DEB.2.02.1210231158350.26626@mathewm-linux>
Hi Mat,
> >> The logical link confirm callback is executed when the AMP controller
> >> completes its logical link setup. During a channel move, a newly
> >> formed logical link allows a move responder to send a move channel
> >> response. A move initiator will send a move channel confirm. A
> >> failed logical link will end the channel move and send an appropriate
> >> response or confirm command indicating a failure.
> >>
> >> If the channel is being created on an AMP controller, L2CAP
> >> configuration is completed after the logical link is set up.
> >>
> >> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >> ---
> >> net/bluetooth/l2cap_core.c | 124 ++++++++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 116 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >> index 69d43c9..0edc955 100644
> >> --- a/net/bluetooth/l2cap_core.c
> >> +++ b/net/bluetooth/l2cap_core.c
> >> @@ -3799,6 +3799,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> >> goto unlock;
> >> }
> >>
> >> + chan->ident = cmd->ident;
> >> l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP, len, rsp);
> >> chan->num_conf_rsp++;
> >>
> >> @@ -4198,17 +4199,17 @@ static int l2cap_create_channel_req(struct l2cap_conn *conn,
> >> return 0;
> >> }
> >>
> >> -static void l2cap_send_move_chan_rsp(struct l2cap_conn *conn, u8 ident,
> >> - u16 icid, u16 result)
> >> +static void l2cap_send_move_chan_rsp(struct l2cap_chan *chan, u16 result)
> >> {
> >> struct l2cap_move_chan_rsp rsp;
> >>
> >> - BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
> >> + BT_DBG("chan %p, result 0x%4.4x", chan, result);
> >>
> >> - rsp.icid = cpu_to_le16(icid);
> >> + rsp.icid = cpu_to_le16(chan->dcid);
> >> rsp.result = cpu_to_le16(result);
> >>
> >> - l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_RSP, sizeof(rsp), &rsp);
> >> + l2cap_send_cmd(chan->conn, chan->ident, L2CAP_MOVE_CHAN_RSP,
> >> + sizeof(rsp), &rsp);
> >> }
> >>
> >> static void l2cap_send_move_chan_cfm(struct l2cap_chan *chan, u16 result)
> >> @@ -4260,11 +4261,114 @@ static void __release_logical_link(struct l2cap_chan *chan)
> >> /* Placeholder - release the logical link */
> >> }
> >>
> >> +static void l2cap_logical_fail(struct l2cap_chan *chan)
> >> +{
> >> + /* Logical link setup failed */
> >> + if (chan->state != BT_CONNECTED) {
> >> + /* Create channel failure, disconnect */
> >> + l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
> >
> > lets do this:
> >
> > if (chan->state != BT_CONNECTED) {
> > ...
> > return;
> > }
> >
>
> Ok.
>
> >> + } else if (chan->move_role == L2CAP_MOVE_ROLE_RESPONDER) {
> >> + l2cap_move_revert(chan);
> >> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> >> + chan->move_state = L2CAP_MOVE_STABLE;
> >> + l2cap_send_move_chan_rsp(chan, L2CAP_MR_NOT_SUPP);
> >> + } else if (chan->move_role == L2CAP_MOVE_ROLE_INITIATOR) {
> >> + if (chan->move_state == L2CAP_MOVE_WAIT_LOGICAL_COMP ||
> >> + chan->move_state == L2CAP_MOVE_WAIT_LOGICAL_CFM) {
> >> + /* Remote has only sent pending or
> >> + * success responses, clean up
> >> + */
> >> + l2cap_move_revert(chan);
> >> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> >> + chan->move_state = L2CAP_MOVE_STABLE;
> >> + }
> >> +
> >> + /* Other amp move states imply that the move
> >> + * has already aborted
> >> + */
> >> + l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
> >> + }
> >
> > And turn this into a switch statement.
> >
> >> +
> >> + __release_logical_link(chan);
> >
> > And leave this to the caller.
> >
>
> Ok.
>
> >> +}
> >> +
> >> +static void l2cap_logical_finish_create(struct l2cap_chan *chan,
> >> + struct hci_chan *hchan)
> >> +{
> >> + struct l2cap_conf_rsp rsp;
> >> + u8 code;
> >> +
> >> + chan->hs_hcon = hchan->conn;
> >> + chan->hs_hcon->l2cap_data = chan->conn;
> >> +
> >> + code = l2cap_build_conf_rsp(chan, &rsp,
> >> + L2CAP_CONF_SUCCESS, 0);
> >> + l2cap_send_cmd(chan->conn, chan->ident, L2CAP_CONF_RSP, code,
> >> + &rsp);
> >> + set_bit(CONF_OUTPUT_DONE, &chan->conf_state);
> >> +
> >> + if (test_bit(CONF_INPUT_DONE, &chan->conf_state)) {
> >> + int err = 0;
> >> +
> >> + set_default_fcs(chan);
> >> +
> >> + err = l2cap_ertm_init(chan);
> >> + if (err < 0)
> >> + l2cap_send_disconn_req(chan->conn, chan, -err);
> >> + else
> >> + l2cap_chan_ready(chan);
> >> + }
> >> +}
> >> +
> >> +static void l2cap_logical_finish_move(struct l2cap_chan *chan,
> >> + struct hci_chan *hchan)
> >> +{
> >> + chan->hs_hcon = hchan->conn;
> >> + chan->hs_hcon->l2cap_data = chan->conn;
> >> +
> >> + BT_DBG("move_state %d", chan->move_state);
> >> +
> >> + switch (chan->move_state) {
> >> + case L2CAP_MOVE_WAIT_LOGICAL_COMP:
> >> + /* Move confirm will be sent after a success
> >> + * response is received
> >> + */
> >> + chan->move_state = L2CAP_MOVE_WAIT_RSP_SUCCESS;
> >> + break;
> >> + case L2CAP_MOVE_WAIT_LOGICAL_CFM:
> >> + if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
> >> + chan->move_state = L2CAP_MOVE_WAIT_LOCAL_BUSY;
> >
> > My brain just hurts from these nested if-else. A nested two switch does
> > not make it any better though. So we can leave it as this. Except the
> > statement below is used multiple places and we have a function for it.
> >
>
> This version (v4) of the patch reflects some consolidation in these
> statements already, where I put more code inside
> l2cap_send_move_chan_cfm and l2cap_send_move_chan_rsp. The move_state
> assignments don't fit well in those helper functions.
>
> The next 7 lines of code are not duplicated anywhere else. The first
> block (L2CAP_MOVE_WAIT_CONFIRM_RSP + send confirm) is used in one
> other place. The second block (L2CAP_MOVE_WAIT_CONFIRM + send
> response) is also used in just one other place -- but a different one.
> The surrounding logic based on chan->move_role is not shared.
>
> Do you want me to create 2-line helper functions for each case, or
> were you thinking there was more duplicated code around? Adding new
> functions is a net gain in lines of code and doesn't seem like a big
> win for clarity.
just leave this one as it is then. Not sure we can gain anything. Thanks
for having a second look a this.
> >> + } else if (chan->move_role == L2CAP_MOVE_ROLE_INITIATOR) {
> >> + chan->move_state = L2CAP_MOVE_WAIT_CONFIRM_RSP;
> >> + l2cap_send_move_chan_cfm(chan, L2CAP_MC_CONFIRMED);
> >> + } else if (chan->move_role == L2CAP_MOVE_ROLE_RESPONDER) {
> >> + chan->move_state = L2CAP_MOVE_WAIT_CONFIRM;
> >> + l2cap_send_move_chan_rsp(chan, L2CAP_MR_SUCCESS);
> >> + }
> >> + break;
> >> + default:
> >> + /* Move was not in expected state, free the channel */
> >> + __release_logical_link(chan);
> >> +
> >> + chan->move_state = L2CAP_MOVE_STABLE;
> >> + }
> >> +}
> >> +
> >> +/* Call with chan locked */
> >> static void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
> >> u8 status)
> >> {
> >> - /* Placeholder */
> >> - return;
> >> + BT_DBG("chan %p, hchan %p, status %d", chan, hchan, status);
> >> +
> >> + if (status) {
> >> + l2cap_logical_fail(chan);
> >
> > I rather have a return here.
> >
> > if (status) {
> > l2cap_logical_fail(chan);
> > __release_logical_link(chan);
> > return;
> > }
> >
>
> Ok.
>
> >> + } else if (chan->state != BT_CONNECTED) {
> >> + /* Ignore logical link if channel is on BR/EDR */
> >> + if (chan->local_amp_id)
> >> + l2cap_logical_finish_create(chan, hchan);
> >> + } else {
> >> + l2cap_logical_finish_move(chan, hchan);
> >> + }
> >> }
> >>
> >> static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> >> @@ -4272,6 +4376,7 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> >> u16 cmd_len, void *data)
> >> {
> >> struct l2cap_move_chan_req *req = data;
> >> + struct l2cap_move_chan_rsp rsp;
> >> struct l2cap_chan *chan;
> >> u16 icid = 0;
> >> u16 result = L2CAP_MR_NOT_ALLOWED;
> >> @@ -4348,7 +4453,10 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> >> }
> >>
> >> send_move_response:
> >> - l2cap_send_move_chan_rsp(conn, cmd->ident, icid, result);
> >> + rsp.icid = cpu_to_le16(icid);
> >> + rsp.result = cpu_to_le16(result);
> >> + l2cap_send_cmd(conn, cmd->ident, L2CAP_MOVE_CHAN_RSP,
> >> + sizeof(rsp), &rsp);
> >>
> >> if (chan)
> >> l2cap_chan_unlock(chan);
> >
> > While not part of this patch, I still dislike if (something) unlock
> > style. Please have that fixed as well.
>
> I'll fix it. This is the only "if (chan) / unlock" case left.
Great. We are getting close now.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/7] Bluetooth: mgmt: Add support for switching to LE peripheral mode
From: Marcel Holtmann @ 2012-10-23 21:49 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <20121023202626.GA12042@x220.ger.corp.intel.com>
Hi Johan,
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -786,6 +786,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
> > > if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
> > > hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
> > > &cp);
> > > + else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> > > + hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(cp.le),
> > > + &cp.le);
> >
> > What is this doing and why it is correct? I am failing to see this. We
> > need to enable LE host supported first anyway.
>
> The first if-statement checks if the host feature bits are what we want
> them to be, and if not send the WRITE_LE_HOST_SUPPORTED command. The
> else if part (if the host features are already correct and don't need
> updating) enables advertising if necessary. There's a similar check for
> HCI_LE_PERIPHERAL in the command complete handler for
> HCI_OP_WRITE_LE_HOST_SUPPORTED in case the first if statement was
> triggered. FWIW, I have actually tested this both with hciconfig and
> btmgmt and it works fine.
>
> > > +static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> > > +{
> > > + __u8 *sent, status = *((__u8 *) skb->data);
> > > +
> > > + BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > > +
> > > + sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_ADV_ENABLE);
> > > + if (!sent)
> > > + return;
> > > +
> > > + hci_dev_lock(hdev);
> > > +
> > > + if (!status) {
> > > + if (*sent)
> > > + set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > + else
> > > + clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > + } else {
> > > + if (*sent)
> > > + clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > + else
> > > + set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > > + }
> >
> > Are you sure we want to based LE peripheral enabling based on if we
> > advertise or not. I can see cases where we are a peripheral, but might
> > want to not always advertise.
>
> These set_bit/clear_bit calls are actually all no-op in the case that
> the mgmt interface is used (since the LE_PERIPHERAL flag is already
> modified when receiving the mgmt command). The only reason I put them
> here is so that the reported mgmt settings remain correct and we reject
> connection initiation and scanning if "hciconfig hci0 leadv" or similar
> is used.
>
> The simplistic way I thought we'd initially keep peripheral mode is
> that it always implies advertising (unless a connection exists).
> Otherwise the user would need to switch to LE-off or Central mode.
I am fine with this. However we can not merge this upstream until we
have this figured out and have a good grasp on this. So it would be nice
to have this patch last. Since some of the other patches could be
actually merged right now. They are useful fixes no matter what.
We might want to introduce a enable_peripheral module parameter as safe
guard for the time being. So that the features bit PERIPHERAL is not set
if this is disabled. Just an idea.
>
> > > @@ -1213,48 +1242,71 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> > > goto unlock;
> > > }
> > >
> > > - val = !!cp->val;
> > > - enabled = !!(hdev->host_features[0] & LMP_HOST_LE);
> > > + if (!valid_le_mode(cp->val)) {
> > > + err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > > + MGMT_STATUS_INVALID_PARAMS);
> > > + goto unlock;
> > > + }
> > >
> > > - if (!hdev_is_powered(hdev) || val == enabled) {
> > > - bool changed = false;
> > > + if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > > + err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > > + MGMT_STATUS_BUSY);
> > > + goto unlock;
> > > + }
> > > +
> > > + new_mode = cp->val;
> > > + old_mode = get_le_mode(hdev);
> > > +
> > > + BT_DBG("%s old_mode %u new_mode %u", hdev->name, old_mode, new_mode);
> > > +
> > > + if (new_mode == old_mode) {
> > > + err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > > + goto unlock;
> > > + }
> > > +
> > > + if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
> > > + change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > >
> > > - if (val != test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> > > + if (!hdev_is_powered(hdev)) {
> > > + if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
> > > change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
> > > - changed = true;
> > > - }
> > >
> > > err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > > if (err < 0)
> > > goto unlock;
> > >
> > > - if (changed)
> > > - err = new_settings(hdev, sk);
> > > + err = new_settings(hdev, sk);
> > >
> > > goto unlock;
> > > }
> > >
> > > - if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > > - err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > > - MGMT_STATUS_BUSY);
> > > - goto unlock;
> > > - }
> > > -
> > > cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE, hdev, data, len);
> > > if (!cmd) {
> > > err = -ENOMEM;
> > > goto unlock;
> > > }
> > >
> > > + if ((old_mode != MGMT_LE_OFF && new_mode == MGMT_LE_PERIPHERAL) ||
> > > + old_mode == MGMT_LE_PERIPHERAL) {
> > > + u8 adv_enable = (new_mode == MGMT_LE_PERIPHERAL);
> > > +
> > > + err = hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> > > + sizeof(adv_enable), &adv_enable);
> > > + if (err < 0)
> > > + mgmt_pending_remove(cmd);
> > > +
> > > + goto unlock;
> > > + }
> > > +
> >
> > this gets a bit complicated. We either need more comments or split this
> > out in separate helper functions.
>
> I actually spent over a half a day trying to figure out an easy way to
> simplify this, but the best I got was those two simple helper functions.
> So I think I'll opt for just adding good code comments.
Sounds good to me.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters
From: Marcel Holtmann @ 2012-10-23 21:46 UTC (permalink / raw)
To: Johan Hedberg; +Cc: Anderson Lizardo, linux-bluetooth
In-Reply-To: <20121023204809.GB12042@x220.ger.corp.intel.com>
Hi Johan,
> > Johan: do you have further plans on improving LE peripheral support
> > after these patches? If yes, please keep in mind that we still have
> > ongoing plans to push broadcaster/observer roles upstream, which also
> > requires hability to enable/disable advertising and set advertising
> > data/parameters.
>
> Looking through the various "shall" and "shall not" clauses in the core
> spec it seems to me that only Central, Observer and Broadcaster roles
> are compatible with each other, and Peripheral is mutually exclusive
> with anything else. E.g. section 9.2.4.2 (page 1700):
>
> "While a device is in the Broadcaster, Observer or Central role the
> device shall not support the general discoverable mode."
>
> and section 9.3.4.2 (page 1709):
>
> "While a device is in the Broadcaster, Observer, or the Central role
> the device shall not support the undirected connectable mode."
>
> Both general discoverable and undirected connectable are needed by
> Peripheral role, i.e. essentially excluding the other roles.
>
> Extending the other roles could indeed be a matter of interpreting the
> mgmt_set_le parameter as a bit mask and then returning an error if an
> incompatible combination of roles is attempted. Also, once support for
> new roles is added it should be easy to extend the current state checks
> to make a distinction on exactly what kind of scanning/advertising is
> done instead of just looking at "advertising or not" on a general level.
the way I see this now (which might change), we are doing LE on/off. And
by default this means actually central. For us it is a good assumption
that we are central if LE is enabled. And this means if the LE flag is
set, then we are a central. As it has been so far. In case we want to
switch this to peripheral, then we have to set PERIPHERAL flag. The LE
flag stays on to indicate LE is enabled. !PERIPHERAL flag here means we
are central.
That is why I like the explanation above, if we set ourselves as
peripheral, then we are peripheral and everything else is out of the
question. So either LE is enabled and we act as central or we are acting
as peripheral.
So broadcaster and observer are extra features on top of central. For me
that means we have treat them independent. How much you can be
broadcaster and observer and central at the same time is then again a
different question that we need to answer.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 3/7] Bluetooth: Disallow LE scanning and connecting in peripheral mode
From: Marcel Holtmann @ 2012-10-23 21:37 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1351011241-32515-4-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> When an adapter is in the LE peripheral mode scanning for other devices
> or initiating connections to them is not allowed. This patch makes sure
> that such attempts will result in appropriate error returns.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_conn.c | 3 +++
> net/bluetooth/hci_core.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index fe64621..a75b30e 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -502,6 +502,9 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> {
> struct hci_conn *le;
>
> + if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> + return ERR_PTR(-ENOTSUPP);
> +
> le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> if (!le) {
> le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 487308b..b2adef4 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1575,6 +1575,9 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>
> BT_DBG("%s", hdev->name);
>
> + if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> + return -ENOTSUPP;
> +
> if (work_busy(&hdev->le_scan))
> return -EINPROGRESS;
>
I am fine with this patch, but maybe it would be better to introduce
this flag first. Specific the semantics in the commit message and the
mgmt documentation, then add this patch and only after that actually
start implementing support for it.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 4/7] Bluetooth: Add support for setting LE advertising data
From: Johan Hedberg @ 2012-10-23 21:26 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <CAJdJm_MhB79eQSAHUqOa7Xwf+ujrnQZtG_jqEgMfEFzuoqB9-w@mail.gmail.com>
Hi Lizardo,
On Tue, Oct 23, 2012, Anderson Lizardo wrote:
> On Tue, Oct 23, 2012 at 12:53 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > + if (hdev->adv_tx_power) {
> > + ptr[0] = 2;
> > + ptr[1] = EIR_TX_POWER;
> > + ptr[2] = (u8) hdev->adv_tx_power;
> > +
> > + ad_len += 3;
> > + ptr += 3;
> > + }
>
> 0dBm is a valid TX power. Not sure the if() clause is valid here.
If it's not, we'd need to extend the storage size of hdev->adv_tx_power
(to have some invalid value) or add another variable to hdev to indicate
that we know the tx_power. FWIW, create_eir also uses the same test for
including the inq_tx_power, so either both or neither should be fixed.
For simplicity I'd just keep the 0-test unless 0 is a really common
value.
> Also, I'm worried how we are going to put other advertising data here,
> i.e. Manufacturer Specific data or Service Data. On last BlueZ meeting
> we proposed (and have been implementing) the Set Controller Data mgmt
> command to set them. Is this still an acceptable approach?
Yes, though we only have 31 bytes to play with here which is quite
little. Also, we might want to make this dependent on the LE GAP role.
E.g. we might be more interested in manufacturer specific data for
broadcaster role than for peripheral role (maybe a user space
application *only* wants its data for broadcaster role).
Johan
^ permalink raw reply
* Re: [PATCH 3/7] Bluetooth: Disallow LE scanning and connecting in peripheral mode
From: Johan Hedberg @ 2012-10-23 20:59 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <CAJdJm_PXwxZ1cyCd=2g170AYx7yjx24cHmksJCTbkxtxyOV4-g@mail.gmail.com>
Hi Lizardo,
On Tue, Oct 23, 2012, Anderson Lizardo wrote:
> This will conflict with an Observer role running on the same device.
> Multiple roles are allowed to run concurrently if supported by the
> controller. From Core spec page 1639:
>
> A device may operate in multiple GAP roles concurrently if supported
> by the Controller. The Host should read the supported Link Layer
> States and State combinations from the Controller before any
> procedures or modes are used.
>
> Do you propose SET_LE to become a bitfield to support concurrent
> roles? I.e. Broadcaster/Observer.
See the other email I sent about this. If peripheral mode is enabled I
don't see how we could simultaneously allow another LE GAP mode based on
the restrictions of the core spec. And yes, we could make set_le a bit
field, but peripheral will probably still remain mutually exclusive with
any other LE GAP mode.
Johan
^ permalink raw reply
* Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters
From: Johan Hedberg @ 2012-10-23 20:48 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: Marcel Holtmann, linux-bluetooth
In-Reply-To: <CAJdJm_OLBgRVBFLD4sEp+OK-qkF_-CXXzuZOCAyiaD+TVMOYKQ@mail.gmail.com>
Hi Lizardo,
On Tue, Oct 23, 2012, Anderson Lizardo wrote:
> Johan: do you have further plans on improving LE peripheral support
> after these patches? If yes, please keep in mind that we still have
> ongoing plans to push broadcaster/observer roles upstream, which also
> requires hability to enable/disable advertising and set advertising
> data/parameters.
Looking through the various "shall" and "shall not" clauses in the core
spec it seems to me that only Central, Observer and Broadcaster roles
are compatible with each other, and Peripheral is mutually exclusive
with anything else. E.g. section 9.2.4.2 (page 1700):
"While a device is in the Broadcaster, Observer or Central role the
device shall not support the general discoverable mode."
and section 9.3.4.2 (page 1709):
"While a device is in the Broadcaster, Observer, or the Central role
the device shall not support the undirected connectable mode."
Both general discoverable and undirected connectable are needed by
Peripheral role, i.e. essentially excluding the other roles.
Extending the other roles could indeed be a matter of interpreting the
mgmt_set_le parameter as a bit mask and then returning an error if an
incompatible combination of roles is attempted. Also, once support for
new roles is added it should be easy to extend the current state checks
to make a distinction on exactly what kind of scanning/advertising is
done instead of just looking at "advertising or not" on a general level.
Johan
^ permalink raw reply
* Re: [PATCH 1/7] Bluetooth: mgmt: Add support for switching to LE peripheral mode
From: Johan Hedberg @ 2012-10-23 20:26 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1351018879.1785.44.camel@aeonflux>
Hi Marcel,
On Tue, Oct 23, 2012, Marcel Holtmann wrote:
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -786,6 +786,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
> > if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
> > hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
> > &cp);
> > + else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> > + hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(cp.le),
> > + &cp.le);
>
> What is this doing and why it is correct? I am failing to see this. We
> need to enable LE host supported first anyway.
The first if-statement checks if the host feature bits are what we want
them to be, and if not send the WRITE_LE_HOST_SUPPORTED command. The
else if part (if the host features are already correct and don't need
updating) enables advertising if necessary. There's a similar check for
HCI_LE_PERIPHERAL in the command complete handler for
HCI_OP_WRITE_LE_HOST_SUPPORTED in case the first if statement was
triggered. FWIW, I have actually tested this both with hciconfig and
btmgmt and it works fine.
> > +static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > + __u8 *sent, status = *((__u8 *) skb->data);
> > +
> > + BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > +
> > + sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_ADV_ENABLE);
> > + if (!sent)
> > + return;
> > +
> > + hci_dev_lock(hdev);
> > +
> > + if (!status) {
> > + if (*sent)
> > + set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + else
> > + clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + } else {
> > + if (*sent)
> > + clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + else
> > + set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + }
>
> Are you sure we want to based LE peripheral enabling based on if we
> advertise or not. I can see cases where we are a peripheral, but might
> want to not always advertise.
These set_bit/clear_bit calls are actually all no-op in the case that
the mgmt interface is used (since the LE_PERIPHERAL flag is already
modified when receiving the mgmt command). The only reason I put them
here is so that the reported mgmt settings remain correct and we reject
connection initiation and scanning if "hciconfig hci0 leadv" or similar
is used.
The simplistic way I thought we'd initially keep peripheral mode is
that it always implies advertising (unless a connection exists).
Otherwise the user would need to switch to LE-off or Central mode.
> > @@ -1213,48 +1242,71 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> > goto unlock;
> > }
> >
> > - val = !!cp->val;
> > - enabled = !!(hdev->host_features[0] & LMP_HOST_LE);
> > + if (!valid_le_mode(cp->val)) {
> > + err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > + MGMT_STATUS_INVALID_PARAMS);
> > + goto unlock;
> > + }
> >
> > - if (!hdev_is_powered(hdev) || val == enabled) {
> > - bool changed = false;
> > + if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > + err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > + MGMT_STATUS_BUSY);
> > + goto unlock;
> > + }
> > +
> > + new_mode = cp->val;
> > + old_mode = get_le_mode(hdev);
> > +
> > + BT_DBG("%s old_mode %u new_mode %u", hdev->name, old_mode, new_mode);
> > +
> > + if (new_mode == old_mode) {
> > + err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > + goto unlock;
> > + }
> > +
> > + if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
> > + change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> >
> > - if (val != test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> > + if (!hdev_is_powered(hdev)) {
> > + if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
> > change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
> > - changed = true;
> > - }
> >
> > err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > if (err < 0)
> > goto unlock;
> >
> > - if (changed)
> > - err = new_settings(hdev, sk);
> > + err = new_settings(hdev, sk);
> >
> > goto unlock;
> > }
> >
> > - if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > - err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > - MGMT_STATUS_BUSY);
> > - goto unlock;
> > - }
> > -
> > cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE, hdev, data, len);
> > if (!cmd) {
> > err = -ENOMEM;
> > goto unlock;
> > }
> >
> > + if ((old_mode != MGMT_LE_OFF && new_mode == MGMT_LE_PERIPHERAL) ||
> > + old_mode == MGMT_LE_PERIPHERAL) {
> > + u8 adv_enable = (new_mode == MGMT_LE_PERIPHERAL);
> > +
> > + err = hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> > + sizeof(adv_enable), &adv_enable);
> > + if (err < 0)
> > + mgmt_pending_remove(cmd);
> > +
> > + goto unlock;
> > + }
> > +
>
> this gets a bit complicated. We either need more comments or split this
> out in separate helper functions.
I actually spent over a half a day trying to figure out an easy way to
simplify this, but the best I got was those two simple helper functions.
So I think I'll opt for just adding good code comments.
Johan
^ permalink raw reply
* Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters
From: Anderson Lizardo @ 2012-10-23 19:31 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Johan Hedberg, linux-bluetooth
In-Reply-To: <1351018956.1785.45.camel@aeonflux>
Hi Marcel,
On Tue, Oct 23, 2012 at 3:02 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Johan,
>
>> This patch makes sure that settings which are specific for BR/EDR
>> capable adapters are not allowed for non-BR/EDR (e.g. LE-only) adapters.
>> Instead, a "not supported" error is returned of such a setting is
>> attempted to be set for a non-BR/EDR adapter.
>>
>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>> ---
>> net/bluetooth/mgmt.c | 22 +++++++++++++++++++---
>> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> can we lead with this patch. It seems to be useful no matter what we do
> for LE peripheral or single-mode controllers.
I agree that current upstream will be more consistent if these
settings are made "not supported" for LE (as they indeed are not
implemented yet). I was just confused that the same patch series that
improve LE peripheral support does not include properly support for
Connectable/Discoverable modes on this role (which may restrict the
usefulness?).
Johan: do you have further plans on improving LE peripheral support
after these patches? If yes, please keep in mind that we still have
ongoing plans to push broadcaster/observer roles upstream, which also
requires hability to enable/disable advertising and set advertising
data/parameters.
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCHv4 10/18] Bluetooth: Add logical link confirm
From: Mat Martineau @ 2012-10-23 19:27 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, gustavo, sunnyk, andrei.emeltchenko.news
In-Reply-To: <1351018415.1785.39.camel@aeonflux>
Marcel -
On Tue, 23 Oct 2012, Marcel Holtmann wrote:
> Hi Mat,
>
>> The logical link confirm callback is executed when the AMP controller
>> completes its logical link setup. During a channel move, a newly
>> formed logical link allows a move responder to send a move channel
>> response. A move initiator will send a move channel confirm. A
>> failed logical link will end the channel move and send an appropriate
>> response or confirm command indicating a failure.
>>
>> If the channel is being created on an AMP controller, L2CAP
>> configuration is completed after the logical link is set up.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>> net/bluetooth/l2cap_core.c | 124 ++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 116 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 69d43c9..0edc955 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -3799,6 +3799,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>> goto unlock;
>> }
>>
>> + chan->ident = cmd->ident;
>> l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP, len, rsp);
>> chan->num_conf_rsp++;
>>
>> @@ -4198,17 +4199,17 @@ static int l2cap_create_channel_req(struct l2cap_conn *conn,
>> return 0;
>> }
>>
>> -static void l2cap_send_move_chan_rsp(struct l2cap_conn *conn, u8 ident,
>> - u16 icid, u16 result)
>> +static void l2cap_send_move_chan_rsp(struct l2cap_chan *chan, u16 result)
>> {
>> struct l2cap_move_chan_rsp rsp;
>>
>> - BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
>> + BT_DBG("chan %p, result 0x%4.4x", chan, result);
>>
>> - rsp.icid = cpu_to_le16(icid);
>> + rsp.icid = cpu_to_le16(chan->dcid);
>> rsp.result = cpu_to_le16(result);
>>
>> - l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_RSP, sizeof(rsp), &rsp);
>> + l2cap_send_cmd(chan->conn, chan->ident, L2CAP_MOVE_CHAN_RSP,
>> + sizeof(rsp), &rsp);
>> }
>>
>> static void l2cap_send_move_chan_cfm(struct l2cap_chan *chan, u16 result)
>> @@ -4260,11 +4261,114 @@ static void __release_logical_link(struct l2cap_chan *chan)
>> /* Placeholder - release the logical link */
>> }
>>
>> +static void l2cap_logical_fail(struct l2cap_chan *chan)
>> +{
>> + /* Logical link setup failed */
>> + if (chan->state != BT_CONNECTED) {
>> + /* Create channel failure, disconnect */
>> + l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
>
> lets do this:
>
> if (chan->state != BT_CONNECTED) {
> ...
> return;
> }
>
Ok.
>> + } else if (chan->move_role == L2CAP_MOVE_ROLE_RESPONDER) {
>> + l2cap_move_revert(chan);
>> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
>> + chan->move_state = L2CAP_MOVE_STABLE;
>> + l2cap_send_move_chan_rsp(chan, L2CAP_MR_NOT_SUPP);
>> + } else if (chan->move_role == L2CAP_MOVE_ROLE_INITIATOR) {
>> + if (chan->move_state == L2CAP_MOVE_WAIT_LOGICAL_COMP ||
>> + chan->move_state == L2CAP_MOVE_WAIT_LOGICAL_CFM) {
>> + /* Remote has only sent pending or
>> + * success responses, clean up
>> + */
>> + l2cap_move_revert(chan);
>> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
>> + chan->move_state = L2CAP_MOVE_STABLE;
>> + }
>> +
>> + /* Other amp move states imply that the move
>> + * has already aborted
>> + */
>> + l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
>> + }
>
> And turn this into a switch statement.
>
>> +
>> + __release_logical_link(chan);
>
> And leave this to the caller.
>
Ok.
>> +}
>> +
>> +static void l2cap_logical_finish_create(struct l2cap_chan *chan,
>> + struct hci_chan *hchan)
>> +{
>> + struct l2cap_conf_rsp rsp;
>> + u8 code;
>> +
>> + chan->hs_hcon = hchan->conn;
>> + chan->hs_hcon->l2cap_data = chan->conn;
>> +
>> + code = l2cap_build_conf_rsp(chan, &rsp,
>> + L2CAP_CONF_SUCCESS, 0);
>> + l2cap_send_cmd(chan->conn, chan->ident, L2CAP_CONF_RSP, code,
>> + &rsp);
>> + set_bit(CONF_OUTPUT_DONE, &chan->conf_state);
>> +
>> + if (test_bit(CONF_INPUT_DONE, &chan->conf_state)) {
>> + int err = 0;
>> +
>> + set_default_fcs(chan);
>> +
>> + err = l2cap_ertm_init(chan);
>> + if (err < 0)
>> + l2cap_send_disconn_req(chan->conn, chan, -err);
>> + else
>> + l2cap_chan_ready(chan);
>> + }
>> +}
>> +
>> +static void l2cap_logical_finish_move(struct l2cap_chan *chan,
>> + struct hci_chan *hchan)
>> +{
>> + chan->hs_hcon = hchan->conn;
>> + chan->hs_hcon->l2cap_data = chan->conn;
>> +
>> + BT_DBG("move_state %d", chan->move_state);
>> +
>> + switch (chan->move_state) {
>> + case L2CAP_MOVE_WAIT_LOGICAL_COMP:
>> + /* Move confirm will be sent after a success
>> + * response is received
>> + */
>> + chan->move_state = L2CAP_MOVE_WAIT_RSP_SUCCESS;
>> + break;
>> + case L2CAP_MOVE_WAIT_LOGICAL_CFM:
>> + if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
>> + chan->move_state = L2CAP_MOVE_WAIT_LOCAL_BUSY;
>
> My brain just hurts from these nested if-else. A nested two switch does
> not make it any better though. So we can leave it as this. Except the
> statement below is used multiple places and we have a function for it.
>
This version (v4) of the patch reflects some consolidation in these
statements already, where I put more code inside
l2cap_send_move_chan_cfm and l2cap_send_move_chan_rsp. The move_state
assignments don't fit well in those helper functions.
The next 7 lines of code are not duplicated anywhere else. The first
block (L2CAP_MOVE_WAIT_CONFIRM_RSP + send confirm) is used in one
other place. The second block (L2CAP_MOVE_WAIT_CONFIRM + send
response) is also used in just one other place -- but a different one.
The surrounding logic based on chan->move_role is not shared.
Do you want me to create 2-line helper functions for each case, or
were you thinking there was more duplicated code around? Adding new
functions is a net gain in lines of code and doesn't seem like a big
win for clarity.
>> + } else if (chan->move_role == L2CAP_MOVE_ROLE_INITIATOR) {
>> + chan->move_state = L2CAP_MOVE_WAIT_CONFIRM_RSP;
>> + l2cap_send_move_chan_cfm(chan, L2CAP_MC_CONFIRMED);
>> + } else if (chan->move_role == L2CAP_MOVE_ROLE_RESPONDER) {
>> + chan->move_state = L2CAP_MOVE_WAIT_CONFIRM;
>> + l2cap_send_move_chan_rsp(chan, L2CAP_MR_SUCCESS);
>> + }
>> + break;
>> + default:
>> + /* Move was not in expected state, free the channel */
>> + __release_logical_link(chan);
>> +
>> + chan->move_state = L2CAP_MOVE_STABLE;
>> + }
>> +}
>> +
>> +/* Call with chan locked */
>> static void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
>> u8 status)
>> {
>> - /* Placeholder */
>> - return;
>> + BT_DBG("chan %p, hchan %p, status %d", chan, hchan, status);
>> +
>> + if (status) {
>> + l2cap_logical_fail(chan);
>
> I rather have a return here.
>
> if (status) {
> l2cap_logical_fail(chan);
> __release_logical_link(chan);
> return;
> }
>
Ok.
>> + } else if (chan->state != BT_CONNECTED) {
>> + /* Ignore logical link if channel is on BR/EDR */
>> + if (chan->local_amp_id)
>> + l2cap_logical_finish_create(chan, hchan);
>> + } else {
>> + l2cap_logical_finish_move(chan, hchan);
>> + }
>> }
>>
>> static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
>> @@ -4272,6 +4376,7 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
>> u16 cmd_len, void *data)
>> {
>> struct l2cap_move_chan_req *req = data;
>> + struct l2cap_move_chan_rsp rsp;
>> struct l2cap_chan *chan;
>> u16 icid = 0;
>> u16 result = L2CAP_MR_NOT_ALLOWED;
>> @@ -4348,7 +4453,10 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
>> }
>>
>> send_move_response:
>> - l2cap_send_move_chan_rsp(conn, cmd->ident, icid, result);
>> + rsp.icid = cpu_to_le16(icid);
>> + rsp.result = cpu_to_le16(result);
>> + l2cap_send_cmd(conn, cmd->ident, L2CAP_MOVE_CHAN_RSP,
>> + sizeof(rsp), &rsp);
>>
>> if (chan)
>> l2cap_chan_unlock(chan);
>
> While not part of this patch, I still dislike if (something) unlock
> style. Please have that fixed as well.
I'll fix it. This is the only "if (chan) / unlock" case left.
> Rest looks fine.
Thanks,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH 7/7] Bluetooth: Make use feature test macros
From: Marcel Holtmann @ 2012-10-23 19:06 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1351011241-32515-8-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> For better code readability and avoiding simple bugs of checking the
> wrong byte of the features make use of feature test macros whenever
> possible.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 8 ++++++++
> net/bluetooth/hci_event.c | 26 +++++++++++++-------------
> net/bluetooth/mgmt.c | 8 ++++----
> 3 files changed, 25 insertions(+), 17 deletions(-)
nice cleanup we should do first as well.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 6/7] Bluetooth: Sort feature test macros by bitmask location
From: Marcel Holtmann @ 2012-10-23 19:06 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1351011241-32515-7-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> This patch sorts the feature test marcos according to the location of
> each feature bit in the features bit mask. This helps easy lookup when
> adding new marcos and wanting to check if there already is a macro
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
this could go first as well.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 5/7] Bluetooth: Fix updating host feature bits for LE
From: Marcel Holtmann @ 2012-10-23 19:04 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1351011241-32515-6-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> When LE has been enabled with the simultaneous BR/EDR & LE parameter set
> to true we should also update the host features stored in struct hci_dev
> accordingly.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_event.c | 5 +++++
> 1 file changed, 5 insertions(+)
should go first as well.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters
From: Marcel Holtmann @ 2012-10-23 19:03 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: Johan Hedberg, linux-bluetooth
In-Reply-To: <CAJdJm_PoB=e_pi2gAN4AOn_ETyHz3nDihUy8yu2bthe0+p5cxw@mail.gmail.com>
Hi Anderson,
> > @@ -377,15 +377,15 @@ static u32 get_supported_settings(struct hci_dev *hdev)
> > u32 settings = 0;
> >
> > settings |= MGMT_SETTING_POWERED;
> > - settings |= MGMT_SETTING_CONNECTABLE;
> > - settings |= MGMT_SETTING_FAST_CONNECTABLE;
> > - settings |= MGMT_SETTING_DISCOVERABLE;
> > settings |= MGMT_SETTING_PAIRABLE;
> >
> > if (lmp_ssp_capable(hdev))
> > settings |= MGMT_SETTING_SSP;
> >
> > if (lmp_bredr_capable(hdev)) {
> > + settings |= MGMT_SETTING_CONNECTABLE;
> > + settings |= MGMT_SETTING_FAST_CONNECTABLE;
> > + settings |= MGMT_SETTING_DISCOVERABLE;
> > settings |= MGMT_SETTING_BREDR;
> > settings |= MGMT_SETTING_LINK_SECURITY;
>
> "Discoverable" (at least as a Discovery mode from GAP) is mandatory
> for LE Peripheral role (see page 1698). Same for "Connectable" (see
> page 1706).
>
> Unless you have other plans to implement this for single mode LE
> peripherals running bluez?
at this moment, we plan to keep LE a bit independent from BR/EDR.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/7] Bluetooth: mgmt: Restrict BR/EDR settings to BR/EDR-only adapters
From: Marcel Holtmann @ 2012-10-23 19:02 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1351011241-32515-3-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> This patch makes sure that settings which are specific for BR/EDR
> capable adapters are not allowed for non-BR/EDR (e.g. LE-only) adapters.
> Instead, a "not supported" error is returned of such a setting is
> attempted to be set for a non-BR/EDR adapter.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
can we lead with this patch. It seems to be useful no matter what we do
for LE peripheral or single-mode controllers.
Regards
Marcel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox