* [PATCH 0/2] Enable use of L2CAP_MODE_EXT_FLOWCTL
@ 2020-03-12 22:24 Luiz Augusto von Dentz
2020-03-12 22:24 ` [PATCH 1/2] Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections Luiz Augusto von Dentz
2020-03-12 22:24 ` [PATCH 2/2] Bluetooth: Add BT_MODE socket option Luiz Augusto von Dentz
0 siblings, 2 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2020-03-12 22:24 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This enables use of L2CAP_MODE_EXT_FLOWCTL with BT_MODE socketopt, in
addition to that add support for DEFER_SETUP to be used with client
sockets so they can be grouped and sent together.
This has been tested by introducing support for L2CAP_MODE_EXT_FLOWCTL
into l2cap-tester which now produces the following trace:
Enable ecred PDUs:
#echo 1 > /sys/module/bluetooth/parameters/enable_ecred
#sudo tools/l2cap-tester -p "L2CAP Ext-Flowctl"
L2CAP Ext-Flowctl Client - Success - init
Read Index List callback
Status: 0x00
New hciemu instance created
Index Added callback
Index: 0x0000
Read Info callback
Status: 0x00
Address: 00:AA:01:00:00:00
Version: 0x09
Manufacturer: 0x003f
Supported settings: 0x0001be1b
Current settings: 0x00000200
Class: 0x000000
Name:
Short name:
L2CAP Ext-Flowctl Client - Success - setup
Powering on controller
Controller powered on
Client set connectable status 0x00
L2CAP Ext-Flowctl Client - Success - setup complete
L2CAP Ext-Flowctl Client - Success - run
Connect in progress
Successfully connected
L2CAP Ext-Flowctl Client - Success - test passed
L2CAP Ext-Flowctl Client - Success - teardown
Index Removed callback
Index: 0x0000
L2CAP Ext-Flowctl Client - Success - teardown complete
L2CAP Ext-Flowctl Client - Success - done
L2CAP Ext-Flowctl Client, Direct Advertising - Success - init
Read Index List callback
Status: 0x00
New hciemu instance created
Index Added callback
Index: 0x0000
Read Info callback
Status: 0x00
Address: 00:AA:01:00:00:00
Version: 0x09
Manufacturer: 0x003f
Supported settings: 0x0001be1b
Current settings: 0x00000200
Class: 0x000000
Name:
Short name:
L2CAP Ext-Flowctl Client, Direct Advertising - Success - setup
Powering on controller
Controller powered on
Client set connectable status 0x00
L2CAP Ext-Flowctl Client, Direct Advertising - Success - setup complete
L2CAP Ext-Flowctl Client, Direct Advertising - Success - run
Connect in progress
Received advertising parameters HCI command
L2CAP Ext-Flowctl Client, Direct Advertising - Success - test passed
L2CAP Ext-Flowctl Client, Direct Advertising - Success - teardown
Index Removed callback
Index: 0x0000
L2CAP Ext-Flowctl Client, Direct Advertising - Success - teardown complete
L2CAP Ext-Flowctl Client, Direct Advertising - Success - done
L2CAP Ext-Flowctl Client SMP - Success - init
Read Index List callback
Status: 0x00
New hciemu instance created
Index Added callback
Index: 0x0000
Read Info callback
Status: 0x00
Address: 00:AA:01:00:00:00
Version: 0x09
Manufacturer: 0x003f
Supported settings: 0x0001be1b
Current settings: 0x00000200
Class: 0x000000
Name:
Short name:
L2CAP Ext-Flowctl Client SMP - Success - setup
Powering on controller
Controller powered on
Client set connectable status 0x00
L2CAP Ext-Flowctl Client SMP - Success - setup complete
L2CAP Ext-Flowctl Client SMP - Success - run
Connect in progress
Successfully connected
L2CAP Ext-Flowctl Client SMP - Success - test passed
L2CAP Ext-Flowctl Client SMP - Success - teardown
Index Removed callback
Index: 0x0000
L2CAP Ext-Flowctl Client SMP - Success - teardown complete
L2CAP Ext-Flowctl Client SMP - Success - done
L2CAP Ext-Flowctl Client - Command Reject - init
Read Index List callback
Status: 0x00
New hciemu instance created
Index Added callback
Index: 0x0000
Read Info callback
Status: 0x00
Address: 00:AA:01:00:00:00
Version: 0x09
Manufacturer: 0x003f
Supported settings: 0x0001be1b
Current settings: 0x00000200
Class: 0x000000
Name:
Short name:
L2CAP Ext-Flowctl Client - Command Reject - setup
Powering on controller
Controller powered on
Client set connectable status 0x00
L2CAP Ext-Flowctl Client - Command Reject - setup complete
L2CAP Ext-Flowctl Client - Command Reject - run
Connect in progress
New connection with handle 0x002a
Connect failed: Connection refused (111)
L2CAP Ext-Flowctl Client - Command Reject - test passed
L2CAP Ext-Flowctl Client - Command Reject - teardown
Index Removed callback
Index: 0x0000
L2CAP Ext-Flowctl Client - Command Reject - teardown complete
L2CAP Ext-Flowctl Client - Command Reject - done
L2CAP Ext-Flowctl Client - Open two sockets - init
Read Index List callback
Status: 0x00
New hciemu instance created
Index Added callback
Index: 0x0000
Read Info callback
Status: 0x00
Address: 00:AA:01:00:00:00
Version: 0x09
Manufacturer: 0x003f
Supported settings: 0x0001be1b
Current settings: 0x00000200
Class: 0x000000
Name:
Short name:
L2CAP Ext-Flowctl Client - Open two sockets - setup
Powering on controller
Controller powered on
L2CAP Ext-Flowctl Client - Open two sockets - setup complete
L2CAP Ext-Flowctl Client - Open two sockets - run
Connect in progress, sk = 19 (deferred)
HCI Command 0x200b length 7
HCI Command 0x200c length 2
Connect in progress, sk = 20
Client set connectable status 0x00
HCI Command 0x200c length 2
HCI Command 0x200d length 25
HCI Command 0x2016 length 2
Successfully connected
Successfully connected
L2CAP Ext-Flowctl Client - Open two sockets - test passed
L2CAP Ext-Flowctl Client - Open two sockets - teardown
Index Removed callback
Index: 0x0000
L2CAP Ext-Flowctl Client - Open two sockets - teardown complete
L2CAP Ext-Flowctl Client - Open two sockets - done
L2CAP Ext-Flowctl Client - Open two sockets close one - init
Read Index List callback
Status: 0x00
New hciemu instance created
Index Added callback
Index: 0x0000
Read Info callback
Status: 0x00
Address: 00:AA:01:00:00:00
Version: 0x09
Manufacturer: 0x003f
Supported settings: 0x0001be1b
Current settings: 0x00000200
Class: 0x000000
Name:
Short name:
L2CAP Ext-Flowctl Client - Open two sockets close one - setup
Powering on controller
Controller powered on
L2CAP Ext-Flowctl Client - Open two sockets close one - setup complete
L2CAP Ext-Flowctl Client - Open two sockets close one - run
Connect in progress, sk = 19 (deferred)
HCI Command 0x200b length 7
HCI Command 0x200c length 2
Connect in progress, sk = 20
Closing first socket! -1
Client set connectable status 0x00
HCI Command 0x200c length 2
HCI Command 0x200d length 25
HCI Command 0x2016 length 2
Successfully connected
L2CAP Ext-Flowctl Client - Open two sockets close one - test passed
L2CAP Ext-Flowctl Client - Open two sockets close one - teardown
Index Removed callback
Index: 0x0000
L2CAP Ext-Flowctl Client - Open two sockets close one - teardown complete
L2CAP Ext-Flowctl Client - Open two sockets close one - done
Test Summary
------------
L2CAP Ext-Flowctl Client - Success Passed 0.068 seconds
L2CAP Ext-Flowctl Client, Direct Advertising - Success Passed 0.023 seconds
L2CAP Ext-Flowctl Client SMP - Success Passed 0.027 seconds
L2CAP Ext-Flowctl Client - Command Reject Passed 0.046 seconds
L2CAP Ext-Flowctl Client - Open two sockets Passed 0.014 seconds
L2CAP Ext-Flowctl Client - Open two sockets close one Passed 0.066 seconds
Total: 6, Passed: 6 (100.0%), Failed: 0, Not Run: 0
Overall execution time: 0.247 seconds
Luiz Augusto von Dentz (2):
Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections
Bluetooth: Add BT_MODE socket option
include/net/bluetooth/bluetooth.h | 2 +
include/net/bluetooth/l2cap.h | 5 ++
net/bluetooth/l2cap_core.c | 130 +++++++++++++++++++++++++++---
net/bluetooth/l2cap_sock.c | 92 ++++++++++++++++-----
4 files changed, 199 insertions(+), 30 deletions(-)
--
2.21.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections
2020-03-12 22:24 [PATCH 0/2] Enable use of L2CAP_MODE_EXT_FLOWCTL Luiz Augusto von Dentz
@ 2020-03-12 22:24 ` Luiz Augusto von Dentz
2020-03-18 11:13 ` Marcel Holtmann
2020-03-12 22:24 ` [PATCH 2/2] Bluetooth: Add BT_MODE socket option Luiz Augusto von Dentz
1 sibling, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2020-03-12 22:24 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This uses the DEFER_SETUP flag to group channels with
L2CAP_CREDIT_BASED_CONNECTION_REQ.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/l2cap.h | 5 ++
net/bluetooth/l2cap_core.c | 130 +++++++++++++++++++++++++++++++---
net/bluetooth/l2cap_sock.c | 13 ++--
3 files changed, 133 insertions(+), 15 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 537aaead259f..dada14d0622c 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -47,6 +47,7 @@
#define L2CAP_DEFAULT_ACC_LAT 0xFFFFFFFF
#define L2CAP_BREDR_MAX_PAYLOAD 1019 /* 3-DH5 packet */
#define L2CAP_LE_MIN_MTU 23
+#define L2CAP_ECRED_CONN_SCID_MAX 5
#define L2CAP_DISC_TIMEOUT msecs_to_jiffies(100)
#define L2CAP_DISC_REJ_TIMEOUT msecs_to_jiffies(5000)
@@ -660,6 +661,7 @@ struct l2cap_ops {
void (*suspend) (struct l2cap_chan *chan);
void (*set_shutdown) (struct l2cap_chan *chan);
long (*get_sndtimeo) (struct l2cap_chan *chan);
+ struct pid *(*get_peer_pid) (struct l2cap_chan *chan);
struct sk_buff *(*alloc_skb) (struct l2cap_chan *chan,
unsigned long hdr_len,
unsigned long len, int nb);
@@ -983,6 +985,9 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan);
int l2cap_ertm_init(struct l2cap_chan *chan);
void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
+typedef void (*l2cap_chan_func_t)(struct l2cap_chan *chan, void *data);
+void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
+ void *data);
void l2cap_chan_del(struct l2cap_chan *chan, int err);
void l2cap_send_conn_req(struct l2cap_chan *chan);
void l2cap_move_start(struct l2cap_chan *chan);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5e6e35ab44dd..20c1d5f9c238 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -678,6 +678,29 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
}
EXPORT_SYMBOL_GPL(l2cap_chan_del);
+static void __l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
+ void *data)
+{
+ struct l2cap_chan *chan;
+
+ list_for_each_entry(chan, &conn->chan_l, list) {
+ func(chan, data);
+ }
+}
+
+void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
+ void *data)
+{
+ if (!conn)
+ return;
+
+ mutex_lock(&conn->chan_lock);
+ __l2cap_chan_list(conn, func, data);
+ mutex_unlock(&conn->chan_lock);
+}
+
+EXPORT_SYMBOL_GPL(l2cap_chan_list);
+
static void l2cap_conn_update_id_addr(struct work_struct *work)
{
struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
@@ -1356,29 +1379,77 @@ static void l2cap_le_connect(struct l2cap_chan *chan)
sizeof(req), &req);
}
-static void l2cap_ecred_connect(struct l2cap_chan *chan)
-{
- struct l2cap_conn *conn = chan->conn;
+struct l2cap_ecred_conn_data {
struct {
struct l2cap_ecred_conn_req req;
- __le16 scid;
+ __le16 scid[5];
} __packed pdu;
+ struct l2cap_chan *chan;
+ struct pid *pid;
+ int count;
+};
+
+static void l2cap_ecred_defer_connect(struct l2cap_chan *chan, void *data)
+{
+ struct l2cap_ecred_conn_data *conn = data;
+ struct pid *pid;
+
+ if (chan == conn->chan)
+ return;
+
+ if (!test_and_clear_bit(FLAG_DEFER_SETUP, &chan->flags))
+ return;
+
+ pid = chan->ops->get_peer_pid(chan);
+
+ /* Only add deferred channels with the same PID/PSM */
+ if (conn->pid != pid || chan->psm != conn->chan->psm || chan->ident ||
+ chan->mode != L2CAP_MODE_EXT_FLOWCTL || chan->state != BT_CONNECT)
+ return;
+
+ if (test_and_set_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
+ return;
+
+ /* Set the same ident so we can match on the rsp */
+ chan->ident = conn->chan->ident;
+
+ /* Include all channels deferred */
+ conn->pdu.scid[conn->count] = cpu_to_le16(chan->scid);
+
+ conn->count++;
+}
+
+static void l2cap_ecred_connect(struct l2cap_chan *chan)
+{
+ struct l2cap_conn *conn = chan->conn;
+ struct l2cap_ecred_conn_data data;
+
+ if (test_bit(FLAG_DEFER_SETUP, &chan->flags))
+ return;
if (test_and_set_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
return;
l2cap_ecred_init(chan, 0);
- pdu.req.psm = chan->psm;
- pdu.req.mtu = cpu_to_le16(chan->imtu);
- pdu.req.mps = cpu_to_le16(chan->mps);
- pdu.req.credits = cpu_to_le16(chan->rx_credits);
- pdu.scid = cpu_to_le16(chan->scid);
+ data.pdu.req.psm = chan->psm;
+ data.pdu.req.mtu = cpu_to_le16(chan->imtu);
+ data.pdu.req.mps = cpu_to_le16(chan->mps);
+ data.pdu.req.credits = cpu_to_le16(chan->rx_credits);
+ data.pdu.scid[0] = cpu_to_le16(chan->scid);
chan->ident = l2cap_get_ident(conn);
+ data.pid = chan->ops->get_peer_pid(chan);
+
+ data.count = 1;
+ data.chan = chan;
+ data.pid = chan->ops->get_peer_pid(chan);
+
+ __l2cap_chan_list(conn, l2cap_ecred_defer_connect, &data);
l2cap_send_cmd(conn, chan->ident, L2CAP_ECRED_CONN_REQ,
- sizeof(pdu), &pdu);
+ sizeof(data.pdu.req) + data.count * sizeof(__le16),
+ &data.pdu);
}
static void l2cap_le_start(struct l2cap_chan *chan)
@@ -7694,6 +7765,29 @@ static bool is_valid_psm(u16 psm, u8 dst_type) {
return ((psm & 0x0101) == 0x0001);
}
+struct l2cap_chan_data {
+ struct l2cap_chan *chan;
+ struct pid *pid;
+ int count;
+};
+
+static void l2cap_chan_by_pid(struct l2cap_chan *chan, void *data)
+{
+ struct l2cap_chan_data *d = data;
+
+ if (chan == d->chan)
+ return;
+
+ /* Only count deferred channels with the same PID/PSM */
+ if (d->pid != chan->ops->get_peer_pid(chan) ||
+ !test_bit(FLAG_DEFER_SETUP, &chan->flags) ||
+ chan->psm != d->chan->psm || chan->ident ||
+ chan->state != BT_CONNECT)
+ return;
+
+ d->count++;
+}
+
int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
bdaddr_t *dst, u8 dst_type)
{
@@ -7813,6 +7907,22 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
goto done;
}
+ if (chan->mode == L2CAP_MODE_EXT_FLOWCTL) {
+ struct l2cap_chan_data data;
+
+ data.chan = chan;
+ data.pid = chan->ops->get_peer_pid(chan);
+ data.count = 0;
+
+ l2cap_chan_list(conn, l2cap_chan_by_pid, &data);
+ /* Check if there isn't too many channels being connected */
+ if (!(data.count < L2CAP_ECRED_CONN_SCID_MAX - 1)) {
+ hci_conn_drop(hcon);
+ err = -EPROTO;
+ goto done;
+ }
+ }
+
mutex_lock(&conn->chan_lock);
l2cap_chan_lock(chan);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 40fb10b591bd..e43a90e05972 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -549,11 +549,6 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
break;
case BT_DEFER_SETUP:
- if (sk->sk_state != BT_BOUND && sk->sk_state != BT_LISTEN) {
- err = -EINVAL;
- break;
- }
-
if (put_user(test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags),
(u32 __user *) optval))
err = -EFAULT;
@@ -1504,6 +1499,13 @@ static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan)
return sk->sk_sndtimeo;
}
+static struct pid *l2cap_sock_get_peer_pid_cb(struct l2cap_chan *chan)
+{
+ struct sock *sk = chan->data;
+
+ return sk->sk_peer_pid;
+}
+
static void l2cap_sock_suspend_cb(struct l2cap_chan *chan)
{
struct sock *sk = chan->data;
@@ -1525,6 +1527,7 @@ static const struct l2cap_ops l2cap_chan_ops = {
.suspend = l2cap_sock_suspend_cb,
.set_shutdown = l2cap_sock_set_shutdown_cb,
.get_sndtimeo = l2cap_sock_get_sndtimeo_cb,
+ .get_peer_pid = l2cap_sock_get_peer_pid_cb,
.alloc_skb = l2cap_sock_alloc_skb_cb,
};
--
2.21.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] Bluetooth: Add BT_MODE socket option
2020-03-12 22:24 [PATCH 0/2] Enable use of L2CAP_MODE_EXT_FLOWCTL Luiz Augusto von Dentz
2020-03-12 22:24 ` [PATCH 1/2] Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections Luiz Augusto von Dentz
@ 2020-03-12 22:24 ` Luiz Augusto von Dentz
2020-03-18 11:19 ` Marcel Holtmann
1 sibling, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2020-03-12 22:24 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This adds BT_MODE socket option which can be used to set L2CAP modes,
including modes only supported over LE which were not supported using
the L2CAP_OPTIONS.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/bluetooth.h | 2 +
net/bluetooth/l2cap_sock.c | 79 +++++++++++++++++++++++++------
2 files changed, 66 insertions(+), 15 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 1576353a2773..c361ec7b06aa 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -139,6 +139,8 @@ struct bt_voice {
#define BT_PHY_LE_CODED_TX 0x00002000
#define BT_PHY_LE_CODED_RX 0x00004000
+#define BT_MODE 15
+
__printf(1, 2)
void bt_info(const char *fmt, ...);
__printf(1, 2)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index e43a90e05972..7a8a199c373d 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -619,6 +619,21 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
err = -EFAULT;
break;
+ case BT_MODE:
+ if (!enable_ecred) {
+ err = -ENOPROTOOPT;
+ break;
+ }
+
+ if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
+ err = -EINVAL;
+ break;
+ }
+
+ if (put_user(chan->mode, (u8 __user *) optval))
+ err = -EFAULT;
+ break;
+
default:
err = -ENOPROTOOPT;
break;
@@ -644,6 +659,29 @@ static bool l2cap_valid_mtu(struct l2cap_chan *chan, u16 mtu)
return true;
}
+static int l2cap_set_mode(struct l2cap_chan *chan, u8 mode)
+{
+ switch (chan->mode) {
+ case L2CAP_MODE_LE_FLOWCTL:
+ case L2CAP_MODE_EXT_FLOWCTL:
+ break;
+ case L2CAP_MODE_BASIC:
+ clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
+ break;
+ case L2CAP_MODE_ERTM:
+ case L2CAP_MODE_STREAMING:
+ if (!disable_ertm)
+ break;
+ /* fall through */
+ default:
+ return -EINVAL;
+ }
+
+ chan->mode = mode;
+
+ return 0;
+}
+
static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
char __user *optval, unsigned int optlen)
{
@@ -693,22 +731,9 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
break;
}
- chan->mode = opts.mode;
- switch (chan->mode) {
- case L2CAP_MODE_LE_FLOWCTL:
- break;
- case L2CAP_MODE_BASIC:
- clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
- break;
- case L2CAP_MODE_ERTM:
- case L2CAP_MODE_STREAMING:
- if (!disable_ertm)
- break;
- /* fall through */
- default:
- err = -EINVAL;
+ err = l2cap_set_mode(chan, opts.mode);
+ if (err)
break;
- }
BT_DBG("mode 0x%2.2x", chan->mode);
@@ -963,6 +988,30 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
break;
+ case BT_MODE:
+ if (!enable_ecred) {
+ err = -ENOPROTOOPT;
+ break;
+ }
+
+ if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
+ err = -EINVAL;
+ break;
+ }
+
+ if (get_user(opt, (u8 __user *) optval)) {
+ err = -EFAULT;
+ break;
+ }
+
+ err = l2cap_set_mode(chan, opt);
+ if (err)
+ break;
+
+ BT_DBG("mode 0x%2.2x", chan->mode);
+
+ break;
+
default:
err = -ENOPROTOOPT;
break;
--
2.21.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections
2020-03-12 22:24 ` [PATCH 1/2] Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections Luiz Augusto von Dentz
@ 2020-03-18 11:13 ` Marcel Holtmann
2020-03-18 16:58 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Marcel Holtmann @ 2020-03-18 11:13 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> This uses the DEFER_SETUP flag to group channels with
> L2CAP_CREDIT_BASED_CONNECTION_REQ.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/l2cap.h | 5 ++
> net/bluetooth/l2cap_core.c | 130 +++++++++++++++++++++++++++++++---
> net/bluetooth/l2cap_sock.c | 13 ++--
> 3 files changed, 133 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 537aaead259f..dada14d0622c 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -47,6 +47,7 @@
> #define L2CAP_DEFAULT_ACC_LAT 0xFFFFFFFF
> #define L2CAP_BREDR_MAX_PAYLOAD 1019 /* 3-DH5 packet */
> #define L2CAP_LE_MIN_MTU 23
> +#define L2CAP_ECRED_CONN_SCID_MAX 5
>
> #define L2CAP_DISC_TIMEOUT msecs_to_jiffies(100)
> #define L2CAP_DISC_REJ_TIMEOUT msecs_to_jiffies(5000)
> @@ -660,6 +661,7 @@ struct l2cap_ops {
> void (*suspend) (struct l2cap_chan *chan);
> void (*set_shutdown) (struct l2cap_chan *chan);
> long (*get_sndtimeo) (struct l2cap_chan *chan);
> + struct pid *(*get_peer_pid) (struct l2cap_chan *chan);
I would move this support into a separate patch. I think that can be applied independently.
> struct sk_buff *(*alloc_skb) (struct l2cap_chan *chan,
> unsigned long hdr_len,
> unsigned long len, int nb);
> @@ -983,6 +985,9 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan);
> int l2cap_ertm_init(struct l2cap_chan *chan);
> void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
> void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
> +typedef void (*l2cap_chan_func_t)(struct l2cap_chan *chan, void *data);
> +void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
> + void *data);
> void l2cap_chan_del(struct l2cap_chan *chan, int err);
> void l2cap_send_conn_req(struct l2cap_chan *chan);
> void l2cap_move_start(struct l2cap_chan *chan);
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 5e6e35ab44dd..20c1d5f9c238 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -678,6 +678,29 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
> }
> EXPORT_SYMBOL_GPL(l2cap_chan_del);
>
> +static void __l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
> + void *data)
> +{
> + struct l2cap_chan *chan;
> +
> + list_for_each_entry(chan, &conn->chan_l, list) {
> + func(chan, data);
> + }
> +}
> +
> +void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
> + void *data)
> +{
> + if (!conn)
> + return;
> +
> + mutex_lock(&conn->chan_lock);
> + __l2cap_chan_list(conn, func, data);
> + mutex_unlock(&conn->chan_lock);
> +}
> +
> +EXPORT_SYMBOL_GPL(l2cap_chan_list);
> +
> static void l2cap_conn_update_id_addr(struct work_struct *work)
> {
> struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> @@ -1356,29 +1379,77 @@ static void l2cap_le_connect(struct l2cap_chan *chan)
> sizeof(req), &req);
> }
>
> -static void l2cap_ecred_connect(struct l2cap_chan *chan)
> -{
> - struct l2cap_conn *conn = chan->conn;
> +struct l2cap_ecred_conn_data {
> struct {
> struct l2cap_ecred_conn_req req;
> - __le16 scid;
> + __le16 scid[5];
> } __packed pdu;
> + struct l2cap_chan *chan;
> + struct pid *pid;
> + int count;
> +};
> +
> +static void l2cap_ecred_defer_connect(struct l2cap_chan *chan, void *data)
> +{
> + struct l2cap_ecred_conn_data *conn = data;
> + struct pid *pid;
> +
> + if (chan == conn->chan)
> + return;
> +
> + if (!test_and_clear_bit(FLAG_DEFER_SETUP, &chan->flags))
> + return;
> +
> + pid = chan->ops->get_peer_pid(chan);
> +
> + /* Only add deferred channels with the same PID/PSM */
> + if (conn->pid != pid || chan->psm != conn->chan->psm || chan->ident ||
> + chan->mode != L2CAP_MODE_EXT_FLOWCTL || chan->state != BT_CONNECT)
> + return;
> +
> + if (test_and_set_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
> + return;
> +
> + /* Set the same ident so we can match on the rsp */
> + chan->ident = conn->chan->ident;
> +
> + /* Include all channels deferred */
> + conn->pdu.scid[conn->count] = cpu_to_le16(chan->scid);
> +
> + conn->count++;
> +}
> +
> +static void l2cap_ecred_connect(struct l2cap_chan *chan)
> +{
> + struct l2cap_conn *conn = chan->conn;
> + struct l2cap_ecred_conn_data data;
> +
> + if (test_bit(FLAG_DEFER_SETUP, &chan->flags))
> + return;
>
> if (test_and_set_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
> return;
>
> l2cap_ecred_init(chan, 0);
>
> - pdu.req.psm = chan->psm;
> - pdu.req.mtu = cpu_to_le16(chan->imtu);
> - pdu.req.mps = cpu_to_le16(chan->mps);
> - pdu.req.credits = cpu_to_le16(chan->rx_credits);
> - pdu.scid = cpu_to_le16(chan->scid);
> + data.pdu.req.psm = chan->psm;
> + data.pdu.req.mtu = cpu_to_le16(chan->imtu);
> + data.pdu.req.mps = cpu_to_le16(chan->mps);
> + data.pdu.req.credits = cpu_to_le16(chan->rx_credits);
> + data.pdu.scid[0] = cpu_to_le16(chan->scid);
>
> chan->ident = l2cap_get_ident(conn);
> + data.pid = chan->ops->get_peer_pid(chan);
> +
> + data.count = 1;
> + data.chan = chan;
> + data.pid = chan->ops->get_peer_pid(chan);
> +
> + __l2cap_chan_list(conn, l2cap_ecred_defer_connect, &data);
>
> l2cap_send_cmd(conn, chan->ident, L2CAP_ECRED_CONN_REQ,
> - sizeof(pdu), &pdu);
> + sizeof(data.pdu.req) + data.count * sizeof(__le16),
> + &data.pdu);
> }
>
> static void l2cap_le_start(struct l2cap_chan *chan)
> @@ -7694,6 +7765,29 @@ static bool is_valid_psm(u16 psm, u8 dst_type) {
> return ((psm & 0x0101) == 0x0001);
> }
>
> +struct l2cap_chan_data {
> + struct l2cap_chan *chan;
> + struct pid *pid;
> + int count;
> +};
> +
> +static void l2cap_chan_by_pid(struct l2cap_chan *chan, void *data)
> +{
> + struct l2cap_chan_data *d = data;
> +
> + if (chan == d->chan)
> + return;
> +
> + /* Only count deferred channels with the same PID/PSM */
> + if (d->pid != chan->ops->get_peer_pid(chan) ||
> + !test_bit(FLAG_DEFER_SETUP, &chan->flags) ||
> + chan->psm != d->chan->psm || chan->ident ||
> + chan->state != BT_CONNECT)
> + return;
> +
> + d->count++;
> +}
> +
> int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
> bdaddr_t *dst, u8 dst_type)
> {
> @@ -7813,6 +7907,22 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
> goto done;
> }
>
> + if (chan->mode == L2CAP_MODE_EXT_FLOWCTL) {
> + struct l2cap_chan_data data;
> +
> + data.chan = chan;
> + data.pid = chan->ops->get_peer_pid(chan);
> + data.count = 0;
> +
> + l2cap_chan_list(conn, l2cap_chan_by_pid, &data);
> + /* Check if there isn't too many channels being connected */
> + if (!(data.count < L2CAP_ECRED_CONN_SCID_MAX - 1)) {
> + hci_conn_drop(hcon);
> + err = -EPROTO;
> + goto done;
> + }
> + }
> +
> mutex_lock(&conn->chan_lock);
> l2cap_chan_lock(chan);
>
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 40fb10b591bd..e43a90e05972 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -549,11 +549,6 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
> break;
>
> case BT_DEFER_SETUP:
> - if (sk->sk_state != BT_BOUND && sk->sk_state != BT_LISTEN) {
> - err = -EINVAL;
> - break;
> - }
> -
I removing this really a good idea. I think it is not really so bad to force at least BT_BOUND so that a local controller has been at least somehow selected. Just doing setsockopt(DEFER_SETUP) and then connect() seems weird. Let us force the application to at least bind the local controller for this specific usage. They can still bind with BDADDR_ANY, but that gives us a bit cleaner state handling.
> if (put_user(test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags),
> (u32 __user *) optval))
> err = -EFAULT;
> @@ -1504,6 +1499,13 @@ static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan)
> return sk->sk_sndtimeo;
> }
>
> +static struct pid *l2cap_sock_get_peer_pid_cb(struct l2cap_chan *chan)
> +{
> + struct sock *sk = chan->data;
> +
> + return sk->sk_peer_pid;
> +}
> +
> static void l2cap_sock_suspend_cb(struct l2cap_chan *chan)
> {
> struct sock *sk = chan->data;
> @@ -1525,6 +1527,7 @@ static const struct l2cap_ops l2cap_chan_ops = {
> .suspend = l2cap_sock_suspend_cb,
> .set_shutdown = l2cap_sock_set_shutdown_cb,
> .get_sndtimeo = l2cap_sock_get_sndtimeo_cb,
> + .get_peer_pid = l2cap_sock_get_peer_pid_cb,
> .alloc_skb = l2cap_sock_alloc_skb_cb,
> };
Regards
Marcel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Add BT_MODE socket option
2020-03-12 22:24 ` [PATCH 2/2] Bluetooth: Add BT_MODE socket option Luiz Augusto von Dentz
@ 2020-03-18 11:19 ` Marcel Holtmann
2020-03-18 17:17 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Marcel Holtmann @ 2020-03-18 11:19 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> This adds BT_MODE socket option which can be used to set L2CAP modes,
> including modes only supported over LE which were not supported using
> the L2CAP_OPTIONS.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/bluetooth.h | 2 +
> net/bluetooth/l2cap_sock.c | 79 +++++++++++++++++++++++++------
> 2 files changed, 66 insertions(+), 15 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 1576353a2773..c361ec7b06aa 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -139,6 +139,8 @@ struct bt_voice {
> #define BT_PHY_LE_CODED_TX 0x00002000
> #define BT_PHY_LE_CODED_RX 0x00004000
>
> +#define BT_MODE 15
> +
I would cleanly define the BT_MODE_xx constants here. They don’t need to match spec numbers from L2CAP actually. I would also leave the not used BR/EDR constants out of it and just add modes we do support.
> __printf(1, 2)
> void bt_info(const char *fmt, ...);
> __printf(1, 2)
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index e43a90e05972..7a8a199c373d 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -619,6 +619,21 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
> err = -EFAULT;
> break;
>
> + case BT_MODE:
> + if (!enable_ecred) {
> + err = -ENOPROTOOPT;
> + break;
> + }
> +
> + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> + err = -EINVAL;
> + break;
> + }
> +
> + if (put_user(chan->mode, (u8 __user *) optval))
> + err = -EFAULT;
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
> @@ -644,6 +659,29 @@ static bool l2cap_valid_mtu(struct l2cap_chan *chan, u16 mtu)
> return true;
> }
>
> +static int l2cap_set_mode(struct l2cap_chan *chan, u8 mode)
> +{
> + switch (chan->mode) {
> + case L2CAP_MODE_LE_FLOWCTL:
> + case L2CAP_MODE_EXT_FLOWCTL:
> + break;
> + case L2CAP_MODE_BASIC:
> + clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
> + break;
> + case L2CAP_MODE_ERTM:
> + case L2CAP_MODE_STREAMING:
> + if (!disable_ertm)
> + break;
> + /* fall through */
> + default:
> + return -EINVAL;
> + }
> +
> + chan->mode = mode;
> +
> + return 0;
> +}
> +
> static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
> char __user *optval, unsigned int optlen)
> {
> @@ -693,22 +731,9 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
> break;
> }
>
> - chan->mode = opts.mode;
> - switch (chan->mode) {
> - case L2CAP_MODE_LE_FLOWCTL:
> - break;
> - case L2CAP_MODE_BASIC:
> - clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
> - break;
> - case L2CAP_MODE_ERTM:
> - case L2CAP_MODE_STREAMING:
> - if (!disable_ertm)
> - break;
> - /* fall through */
> - default:
> - err = -EINVAL;
> + err = l2cap_set_mode(chan, opts.mode);
> + if (err)
> break;
> - }
>
I would not yet try to break this out into a separate helper. I think L2CAP_OPTIONS should maybe just stay as it is. And as noted above, we define our own list of constants.
> BT_DBG("mode 0x%2.2x", chan->mode);
>
> @@ -963,6 +988,30 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
>
> break;
>
> + case BT_MODE:
> + if (!enable_ecred) {
> + err = -ENOPROTOOPT;
> + break;
> + }
> +
> + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> + err = -EINVAL;
> + break;
> + }
Especially here I would also check if the mode is valid on the choose transport. For me this would also mean we should require a bind() before allowing to set the mode. So we have at least some idea what transport is planned to be used.
> +
> + if (get_user(opt, (u8 __user *) optval)) {
> + err = -EFAULT;
> + break;
> + }
> +
> + err = l2cap_set_mode(chan, opt);
> + if (err)
> + break;
> +
> + BT_DBG("mode 0x%2.2x", chan->mode);
> +
> + break;
> +
> default:
> err = -ENOPROTOOPT;
> break;
Regards
Marcel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections
2020-03-18 11:13 ` Marcel Holtmann
@ 2020-03-18 16:58 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2020-03-18 16:58 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Mar 18, 2020 at 4:13 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This uses the DEFER_SETUP flag to group channels with
> > L2CAP_CREDIT_BASED_CONNECTION_REQ.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/l2cap.h | 5 ++
> > net/bluetooth/l2cap_core.c | 130 +++++++++++++++++++++++++++++++---
> > net/bluetooth/l2cap_sock.c | 13 ++--
> > 3 files changed, 133 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 537aaead259f..dada14d0622c 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -47,6 +47,7 @@
> > #define L2CAP_DEFAULT_ACC_LAT 0xFFFFFFFF
> > #define L2CAP_BREDR_MAX_PAYLOAD 1019 /* 3-DH5 packet */
> > #define L2CAP_LE_MIN_MTU 23
> > +#define L2CAP_ECRED_CONN_SCID_MAX 5
> >
> > #define L2CAP_DISC_TIMEOUT msecs_to_jiffies(100)
> > #define L2CAP_DISC_REJ_TIMEOUT msecs_to_jiffies(5000)
> > @@ -660,6 +661,7 @@ struct l2cap_ops {
> > void (*suspend) (struct l2cap_chan *chan);
> > void (*set_shutdown) (struct l2cap_chan *chan);
> > long (*get_sndtimeo) (struct l2cap_chan *chan);
> > + struct pid *(*get_peer_pid) (struct l2cap_chan *chan);
>
> I would move this support into a separate patch. I think that can be applied independently.
Will do.
> > struct sk_buff *(*alloc_skb) (struct l2cap_chan *chan,
> > unsigned long hdr_len,
> > unsigned long len, int nb);
> > @@ -983,6 +985,9 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan);
> > int l2cap_ertm_init(struct l2cap_chan *chan);
> > void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
> > void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
> > +typedef void (*l2cap_chan_func_t)(struct l2cap_chan *chan, void *data);
> > +void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
> > + void *data);
> > void l2cap_chan_del(struct l2cap_chan *chan, int err);
> > void l2cap_send_conn_req(struct l2cap_chan *chan);
> > void l2cap_move_start(struct l2cap_chan *chan);
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 5e6e35ab44dd..20c1d5f9c238 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -678,6 +678,29 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
> > }
> > EXPORT_SYMBOL_GPL(l2cap_chan_del);
> >
> > +static void __l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
> > + void *data)
> > +{
> > + struct l2cap_chan *chan;
> > +
> > + list_for_each_entry(chan, &conn->chan_l, list) {
> > + func(chan, data);
> > + }
> > +}
> > +
> > +void l2cap_chan_list(struct l2cap_conn *conn, l2cap_chan_func_t func,
> > + void *data)
> > +{
> > + if (!conn)
> > + return;
> > +
> > + mutex_lock(&conn->chan_lock);
> > + __l2cap_chan_list(conn, func, data);
> > + mutex_unlock(&conn->chan_lock);
> > +}
> > +
> > +EXPORT_SYMBOL_GPL(l2cap_chan_list);
> > +
> > static void l2cap_conn_update_id_addr(struct work_struct *work)
> > {
> > struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> > @@ -1356,29 +1379,77 @@ static void l2cap_le_connect(struct l2cap_chan *chan)
> > sizeof(req), &req);
> > }
> >
> > -static void l2cap_ecred_connect(struct l2cap_chan *chan)
> > -{
> > - struct l2cap_conn *conn = chan->conn;
> > +struct l2cap_ecred_conn_data {
> > struct {
> > struct l2cap_ecred_conn_req req;
> > - __le16 scid;
> > + __le16 scid[5];
> > } __packed pdu;
> > + struct l2cap_chan *chan;
> > + struct pid *pid;
> > + int count;
> > +};
> > +
> > +static void l2cap_ecred_defer_connect(struct l2cap_chan *chan, void *data)
> > +{
> > + struct l2cap_ecred_conn_data *conn = data;
> > + struct pid *pid;
> > +
> > + if (chan == conn->chan)
> > + return;
> > +
> > + if (!test_and_clear_bit(FLAG_DEFER_SETUP, &chan->flags))
> > + return;
> > +
> > + pid = chan->ops->get_peer_pid(chan);
> > +
> > + /* Only add deferred channels with the same PID/PSM */
> > + if (conn->pid != pid || chan->psm != conn->chan->psm || chan->ident ||
> > + chan->mode != L2CAP_MODE_EXT_FLOWCTL || chan->state != BT_CONNECT)
> > + return;
> > +
> > + if (test_and_set_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
> > + return;
> > +
> > + /* Set the same ident so we can match on the rsp */
> > + chan->ident = conn->chan->ident;
> > +
> > + /* Include all channels deferred */
> > + conn->pdu.scid[conn->count] = cpu_to_le16(chan->scid);
> > +
> > + conn->count++;
> > +}
> > +
> > +static void l2cap_ecred_connect(struct l2cap_chan *chan)
> > +{
> > + struct l2cap_conn *conn = chan->conn;
> > + struct l2cap_ecred_conn_data data;
> > +
> > + if (test_bit(FLAG_DEFER_SETUP, &chan->flags))
> > + return;
> >
> > if (test_and_set_bit(FLAG_ECRED_CONN_REQ_SENT, &chan->flags))
> > return;
> >
> > l2cap_ecred_init(chan, 0);
> >
> > - pdu.req.psm = chan->psm;
> > - pdu.req.mtu = cpu_to_le16(chan->imtu);
> > - pdu.req.mps = cpu_to_le16(chan->mps);
> > - pdu.req.credits = cpu_to_le16(chan->rx_credits);
> > - pdu.scid = cpu_to_le16(chan->scid);
> > + data.pdu.req.psm = chan->psm;
> > + data.pdu.req.mtu = cpu_to_le16(chan->imtu);
> > + data.pdu.req.mps = cpu_to_le16(chan->mps);
> > + data.pdu.req.credits = cpu_to_le16(chan->rx_credits);
> > + data.pdu.scid[0] = cpu_to_le16(chan->scid);
> >
> > chan->ident = l2cap_get_ident(conn);
> > + data.pid = chan->ops->get_peer_pid(chan);
> > +
> > + data.count = 1;
> > + data.chan = chan;
> > + data.pid = chan->ops->get_peer_pid(chan);
> > +
> > + __l2cap_chan_list(conn, l2cap_ecred_defer_connect, &data);
> >
> > l2cap_send_cmd(conn, chan->ident, L2CAP_ECRED_CONN_REQ,
> > - sizeof(pdu), &pdu);
> > + sizeof(data.pdu.req) + data.count * sizeof(__le16),
> > + &data.pdu);
> > }
> >
> > static void l2cap_le_start(struct l2cap_chan *chan)
> > @@ -7694,6 +7765,29 @@ static bool is_valid_psm(u16 psm, u8 dst_type) {
> > return ((psm & 0x0101) == 0x0001);
> > }
> >
> > +struct l2cap_chan_data {
> > + struct l2cap_chan *chan;
> > + struct pid *pid;
> > + int count;
> > +};
> > +
> > +static void l2cap_chan_by_pid(struct l2cap_chan *chan, void *data)
> > +{
> > + struct l2cap_chan_data *d = data;
> > +
> > + if (chan == d->chan)
> > + return;
> > +
> > + /* Only count deferred channels with the same PID/PSM */
> > + if (d->pid != chan->ops->get_peer_pid(chan) ||
> > + !test_bit(FLAG_DEFER_SETUP, &chan->flags) ||
> > + chan->psm != d->chan->psm || chan->ident ||
> > + chan->state != BT_CONNECT)
> > + return;
> > +
> > + d->count++;
> > +}
> > +
> > int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
> > bdaddr_t *dst, u8 dst_type)
> > {
> > @@ -7813,6 +7907,22 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
> > goto done;
> > }
> >
> > + if (chan->mode == L2CAP_MODE_EXT_FLOWCTL) {
> > + struct l2cap_chan_data data;
> > +
> > + data.chan = chan;
> > + data.pid = chan->ops->get_peer_pid(chan);
> > + data.count = 0;
> > +
> > + l2cap_chan_list(conn, l2cap_chan_by_pid, &data);
> > + /* Check if there isn't too many channels being connected */
> > + if (!(data.count < L2CAP_ECRED_CONN_SCID_MAX - 1)) {
> > + hci_conn_drop(hcon);
> > + err = -EPROTO;
> > + goto done;
> > + }
> > + }
> > +
> > mutex_lock(&conn->chan_lock);
> > l2cap_chan_lock(chan);
> >
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 40fb10b591bd..e43a90e05972 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -549,11 +549,6 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
> > break;
> >
> > case BT_DEFER_SETUP:
> > - if (sk->sk_state != BT_BOUND && sk->sk_state != BT_LISTEN) {
> > - err = -EINVAL;
> > - break;
> > - }
> > -
>
> I removing this really a good idea. I think it is not really so bad to force at least BT_BOUND so that a local controller has been at least somehow selected. Just doing setsockopt(DEFER_SETUP) and then connect() seems weird. Let us force the application to at least bind the local controller for this specific usage. They can still bind with BDADDR_ANY, but that gives us a bit cleaner state handling.
Right, for some odd reason I remember this check being specific for
listen only but in fact it should work with l2cap-tester as it does
bind before doing BT_DEFER_SETUP.
>
> > if (put_user(test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags),
> > (u32 __user *) optval))
> > err = -EFAULT;
> > @@ -1504,6 +1499,13 @@ static long l2cap_sock_get_sndtimeo_cb(struct l2cap_chan *chan)
> > return sk->sk_sndtimeo;
> > }
> >
> > +static struct pid *l2cap_sock_get_peer_pid_cb(struct l2cap_chan *chan)
> > +{
> > + struct sock *sk = chan->data;
> > +
> > + return sk->sk_peer_pid;
> > +}
> > +
> > static void l2cap_sock_suspend_cb(struct l2cap_chan *chan)
> > {
> > struct sock *sk = chan->data;
> > @@ -1525,6 +1527,7 @@ static const struct l2cap_ops l2cap_chan_ops = {
> > .suspend = l2cap_sock_suspend_cb,
> > .set_shutdown = l2cap_sock_set_shutdown_cb,
> > .get_sndtimeo = l2cap_sock_get_sndtimeo_cb,
> > + .get_peer_pid = l2cap_sock_get_peer_pid_cb,
> > .alloc_skb = l2cap_sock_alloc_skb_cb,
> > };
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Add BT_MODE socket option
2020-03-18 11:19 ` Marcel Holtmann
@ 2020-03-18 17:17 ` Luiz Augusto von Dentz
2020-03-18 19:27 ` Marcel Holtmann
0 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2020-03-18 17:17 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Mar 18, 2020 at 4:19 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This adds BT_MODE socket option which can be used to set L2CAP modes,
> > including modes only supported over LE which were not supported using
> > the L2CAP_OPTIONS.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/bluetooth.h | 2 +
> > net/bluetooth/l2cap_sock.c | 79 +++++++++++++++++++++++++------
> > 2 files changed, 66 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 1576353a2773..c361ec7b06aa 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -139,6 +139,8 @@ struct bt_voice {
> > #define BT_PHY_LE_CODED_TX 0x00002000
> > #define BT_PHY_LE_CODED_RX 0x00004000
> >
> > +#define BT_MODE 15
> > +
>
> I would cleanly define the BT_MODE_xx constants here. They don’t need to match spec numbers from L2CAP actually. I would also leave the not used BR/EDR constants out of it and just add modes we do support.
Id leave the same values since it makes easier to fallback if BT_MODE
is not supported e.g. BT_IO_MODE would have to declare 2 different
namespaces for the new and the old sockopt.
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index e43a90e05972..7a8a199c373d 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -619,6 +619,21 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
> > err = -EFAULT;
> > break;
> >
> > + case BT_MODE:
> > + if (!enable_ecred) {
> > + err = -ENOPROTOOPT;
> > + break;
> > + }
> > +
> > + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> > + err = -EINVAL;
> > + break;
> > + }
> > +
> > + if (put_user(chan->mode, (u8 __user *) optval))
> > + err = -EFAULT;
> > + break;
> > +
> > default:
> > err = -ENOPROTOOPT;
> > break;
> > @@ -644,6 +659,29 @@ static bool l2cap_valid_mtu(struct l2cap_chan *chan, u16 mtu)
> > return true;
> > }
> >
> > +static int l2cap_set_mode(struct l2cap_chan *chan, u8 mode)
> > +{
> > + switch (chan->mode) {
> > + case L2CAP_MODE_LE_FLOWCTL:
> > + case L2CAP_MODE_EXT_FLOWCTL:
> > + break;
> > + case L2CAP_MODE_BASIC:
> > + clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
> > + break;
> > + case L2CAP_MODE_ERTM:
> > + case L2CAP_MODE_STREAMING:
> > + if (!disable_ertm)
> > + break;
> > + /* fall through */
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + chan->mode = mode;
> > +
> > + return 0;
> > +}
> > +
> > static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
> > char __user *optval, unsigned int optlen)
> > {
> > @@ -693,22 +731,9 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
> > break;
> > }
> >
> > - chan->mode = opts.mode;
> > - switch (chan->mode) {
> > - case L2CAP_MODE_LE_FLOWCTL:
> > - break;
> > - case L2CAP_MODE_BASIC:
> > - clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
> > - break;
> > - case L2CAP_MODE_ERTM:
> > - case L2CAP_MODE_STREAMING:
> > - if (!disable_ertm)
> > - break;
> > - /* fall through */
> > - default:
> > - err = -EINVAL;
> > + err = l2cap_set_mode(chan, opts.mode);
> > + if (err)
> > break;
> > - }
> >
>
> I would not yet try to break this out into a separate helper. I think L2CAP_OPTIONS should maybe just stay as it is. And as noted above, we define our own list of constants.
That would complicate userspace code a little to much since that means
we would have 2 different namespace for BT_IO_MODE, as mentioned about
Id keep the same values for modes as in L2CAP_OPTIONS just adding new
definitions names, otherwise we will need a new option for bt_io to
avoid the namespaces to overlap but I rather not do that as BT_IO_MODE
already maps quite well to BT_MODE.
> > BT_DBG("mode 0x%2.2x", chan->mode);
> >
> > @@ -963,6 +988,30 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
> >
> > break;
> >
> > + case BT_MODE:
> > + if (!enable_ecred) {
> > + err = -ENOPROTOOPT;
> > + break;
> > + }
> > +
> > + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> > + err = -EINVAL;
> > + break;
> > + }
>
> Especially here I would also check if the mode is valid on the choose transport. For me this would also mean we should require a bind() before allowing to set the mode. So we have at least some idea what transport is planned to be used.
I will make it check for BOUND state, though l2cap_set_mode does check
the mode already, or you are saying that if we do use a different
namespace we would have to convert, well I guess this further
reinforces my argument that making the values compatible makes things
a lot simpler.
> > +
> > + if (get_user(opt, (u8 __user *) optval)) {
> > + err = -EFAULT;
> > + break;
> > + }
> > +
> > + err = l2cap_set_mode(chan, opt);
> > + if (err)
> > + break;
> > +
> > + BT_DBG("mode 0x%2.2x", chan->mode);
> > +
> > + break;
> > +
> > default:
> > err = -ENOPROTOOPT;
> > break;
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Add BT_MODE socket option
2020-03-18 17:17 ` Luiz Augusto von Dentz
@ 2020-03-18 19:27 ` Marcel Holtmann
2020-03-18 22:04 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Marcel Holtmann @ 2020-03-18 19:27 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
>>> This adds BT_MODE socket option which can be used to set L2CAP modes,
>>> including modes only supported over LE which were not supported using
>>> the L2CAP_OPTIONS.
>>>
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> include/net/bluetooth/bluetooth.h | 2 +
>>> net/bluetooth/l2cap_sock.c | 79 +++++++++++++++++++++++++------
>>> 2 files changed, 66 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>>> index 1576353a2773..c361ec7b06aa 100644
>>> --- a/include/net/bluetooth/bluetooth.h
>>> +++ b/include/net/bluetooth/bluetooth.h
>>> @@ -139,6 +139,8 @@ struct bt_voice {
>>> #define BT_PHY_LE_CODED_TX 0x00002000
>>> #define BT_PHY_LE_CODED_RX 0x00004000
>>>
>>> +#define BT_MODE 15
>>> +
>>
>> I would cleanly define the BT_MODE_xx constants here. They don’t need to match spec numbers from L2CAP actually. I would also leave the not used BR/EDR constants out of it and just add modes we do support.
>
> Id leave the same values since it makes easier to fallback if BT_MODE
> is not supported e.g. BT_IO_MODE would have to declare 2 different
> namespaces for the new and the old sockopt.
I would actually not do that since we already made up a mode that isn’t in spec. And I don’t want to keep having to make up modes until this tiny namespace explodes. We need to accept that for BlueZ we have to define our own API and can not rely on values defined in the specification. It was fine for Bluetooth 2.1 and earlier, but it isn’t anymore.
>
>>> __printf(1, 2)
>>> void bt_info(const char *fmt, ...);
>>> __printf(1, 2)
>>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
>>> index e43a90e05972..7a8a199c373d 100644
>>> --- a/net/bluetooth/l2cap_sock.c
>>> +++ b/net/bluetooth/l2cap_sock.c
>>> @@ -619,6 +619,21 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
>>> err = -EFAULT;
>>> break;
>>>
>>> + case BT_MODE:
>>> + if (!enable_ecred) {
>>> + err = -ENOPROTOOPT;
>>> + break;
>>> + }
>>> +
>>> + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
>>> + err = -EINVAL;
>>> + break;
>>> + }
>>> +
>>> + if (put_user(chan->mode, (u8 __user *) optval))
>>> + err = -EFAULT;
>>> + break;
>>> +
>>> default:
>>> err = -ENOPROTOOPT;
>>> break;
>>> @@ -644,6 +659,29 @@ static bool l2cap_valid_mtu(struct l2cap_chan *chan, u16 mtu)
>>> return true;
>>> }
>>>
>>> +static int l2cap_set_mode(struct l2cap_chan *chan, u8 mode)
>>> +{
>>> + switch (chan->mode) {
>>> + case L2CAP_MODE_LE_FLOWCTL:
>>> + case L2CAP_MODE_EXT_FLOWCTL:
>>> + break;
>>> + case L2CAP_MODE_BASIC:
>>> + clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
>>> + break;
>>> + case L2CAP_MODE_ERTM:
>>> + case L2CAP_MODE_STREAMING:
>>> + if (!disable_ertm)
>>> + break;
>>> + /* fall through */
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + chan->mode = mode;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
>>> char __user *optval, unsigned int optlen)
>>> {
>>> @@ -693,22 +731,9 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
>>> break;
>>> }
>>>
>>> - chan->mode = opts.mode;
>>> - switch (chan->mode) {
>>> - case L2CAP_MODE_LE_FLOWCTL:
>>> - break;
>>> - case L2CAP_MODE_BASIC:
>>> - clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
>>> - break;
>>> - case L2CAP_MODE_ERTM:
>>> - case L2CAP_MODE_STREAMING:
>>> - if (!disable_ertm)
>>> - break;
>>> - /* fall through */
>>> - default:
>>> - err = -EINVAL;
>>> + err = l2cap_set_mode(chan, opts.mode);
>>> + if (err)
>>> break;
>>> - }
>>>
>>
>> I would not yet try to break this out into a separate helper. I think L2CAP_OPTIONS should maybe just stay as it is. And as noted above, we define our own list of constants.
>
> That would complicate userspace code a little to much since that means
> we would have 2 different namespace for BT_IO_MODE, as mentioned about
> Id keep the same values for modes as in L2CAP_OPTIONS just adding new
> definitions names, otherwise we will need a new option for bt_io to
> avoid the namespaces to overlap but I rather not do that as BT_IO_MODE
> already maps quite well to BT_MODE.
Actually we need to change BT_IO_MODE to be abstract and handle it internally. Most likely BT_IO_MODE should just follow what we do for BT_MODE and internally it should re-map it to L2CAP_OPTIONS if needed.
>
>>> BT_DBG("mode 0x%2.2x", chan->mode);
>>>
>>> @@ -963,6 +988,30 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
>>>
>>> break;
>>>
>>> + case BT_MODE:
>>> + if (!enable_ecred) {
>>> + err = -ENOPROTOOPT;
>>> + break;
>>> + }
>>> +
>>> + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
>>> + err = -EINVAL;
>>> + break;
>>> + }
>>
>> Especially here I would also check if the mode is valid on the choose transport. For me this would also mean we should require a bind() before allowing to set the mode. So we have at least some idea what transport is planned to be used.
>
> I will make it check for BOUND state, though l2cap_set_mode does check
> the mode already, or you are saying that if we do use a different
> namespace we would have to convert, well I guess this further
> reinforces my argument that making the values compatible makes things
> a lot simpler.
I care about the new socket options that they are clean and well defined when it comes to error conditions.
Regards
Marcel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Add BT_MODE socket option
2020-03-18 19:27 ` Marcel Holtmann
@ 2020-03-18 22:04 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2020-03-18 22:04 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Mar 18, 2020 at 12:27 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> >>> This adds BT_MODE socket option which can be used to set L2CAP modes,
> >>> including modes only supported over LE which were not supported using
> >>> the L2CAP_OPTIONS.
> >>>
> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>> ---
> >>> include/net/bluetooth/bluetooth.h | 2 +
> >>> net/bluetooth/l2cap_sock.c | 79 +++++++++++++++++++++++++------
> >>> 2 files changed, 66 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> >>> index 1576353a2773..c361ec7b06aa 100644
> >>> --- a/include/net/bluetooth/bluetooth.h
> >>> +++ b/include/net/bluetooth/bluetooth.h
> >>> @@ -139,6 +139,8 @@ struct bt_voice {
> >>> #define BT_PHY_LE_CODED_TX 0x00002000
> >>> #define BT_PHY_LE_CODED_RX 0x00004000
> >>>
> >>> +#define BT_MODE 15
> >>> +
> >>
> >> I would cleanly define the BT_MODE_xx constants here. They don’t need to match spec numbers from L2CAP actually. I would also leave the not used BR/EDR constants out of it and just add modes we do support.
> >
> > Id leave the same values since it makes easier to fallback if BT_MODE
> > is not supported e.g. BT_IO_MODE would have to declare 2 different
> > namespaces for the new and the old sockopt.
>
> I would actually not do that since we already made up a mode that isn’t in spec. And I don’t want to keep having to make up modes until this tiny namespace explodes. We need to accept that for BlueZ we have to define our own API and can not rely on values defined in the specification. It was fine for Bluetooth 2.1 and earlier, but it isn’t anymore.
>
> >
> >>> __printf(1, 2)
> >>> void bt_info(const char *fmt, ...);
> >>> __printf(1, 2)
> >>> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> >>> index e43a90e05972..7a8a199c373d 100644
> >>> --- a/net/bluetooth/l2cap_sock.c
> >>> +++ b/net/bluetooth/l2cap_sock.c
> >>> @@ -619,6 +619,21 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname,
> >>> err = -EFAULT;
> >>> break;
> >>>
> >>> + case BT_MODE:
> >>> + if (!enable_ecred) {
> >>> + err = -ENOPROTOOPT;
> >>> + break;
> >>> + }
> >>> +
> >>> + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> >>> + err = -EINVAL;
> >>> + break;
> >>> + }
> >>> +
> >>> + if (put_user(chan->mode, (u8 __user *) optval))
> >>> + err = -EFAULT;
> >>> + break;
> >>> +
> >>> default:
> >>> err = -ENOPROTOOPT;
> >>> break;
> >>> @@ -644,6 +659,29 @@ static bool l2cap_valid_mtu(struct l2cap_chan *chan, u16 mtu)
> >>> return true;
> >>> }
> >>>
> >>> +static int l2cap_set_mode(struct l2cap_chan *chan, u8 mode)
> >>> +{
> >>> + switch (chan->mode) {
> >>> + case L2CAP_MODE_LE_FLOWCTL:
> >>> + case L2CAP_MODE_EXT_FLOWCTL:
> >>> + break;
> >>> + case L2CAP_MODE_BASIC:
> >>> + clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
> >>> + break;
> >>> + case L2CAP_MODE_ERTM:
> >>> + case L2CAP_MODE_STREAMING:
> >>> + if (!disable_ertm)
> >>> + break;
> >>> + /* fall through */
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + chan->mode = mode;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
> >>> char __user *optval, unsigned int optlen)
> >>> {
> >>> @@ -693,22 +731,9 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname,
> >>> break;
> >>> }
> >>>
> >>> - chan->mode = opts.mode;
> >>> - switch (chan->mode) {
> >>> - case L2CAP_MODE_LE_FLOWCTL:
> >>> - break;
> >>> - case L2CAP_MODE_BASIC:
> >>> - clear_bit(CONF_STATE2_DEVICE, &chan->conf_state);
> >>> - break;
> >>> - case L2CAP_MODE_ERTM:
> >>> - case L2CAP_MODE_STREAMING:
> >>> - if (!disable_ertm)
> >>> - break;
> >>> - /* fall through */
> >>> - default:
> >>> - err = -EINVAL;
> >>> + err = l2cap_set_mode(chan, opts.mode);
> >>> + if (err)
> >>> break;
> >>> - }
> >>>
> >>
> >> I would not yet try to break this out into a separate helper. I think L2CAP_OPTIONS should maybe just stay as it is. And as noted above, we define our own list of constants.
> >
> > That would complicate userspace code a little to much since that means
> > we would have 2 different namespace for BT_IO_MODE, as mentioned about
> > Id keep the same values for modes as in L2CAP_OPTIONS just adding new
> > definitions names, otherwise we will need a new option for bt_io to
> > avoid the namespaces to overlap but I rather not do that as BT_IO_MODE
> > already maps quite well to BT_MODE.
>
> Actually we need to change BT_IO_MODE to be abstract and handle it internally. Most likely BT_IO_MODE should just follow what we do for BT_MODE and internally it should re-map it to L2CAP_OPTIONS if needed.
>
> >
> >>> BT_DBG("mode 0x%2.2x", chan->mode);
> >>>
> >>> @@ -963,6 +988,30 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname,
> >>>
> >>> break;
> >>>
> >>> + case BT_MODE:
> >>> + if (!enable_ecred) {
> >>> + err = -ENOPROTOOPT;
> >>> + break;
> >>> + }
> >>> +
> >>> + if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> >>> + err = -EINVAL;
> >>> + break;
> >>> + }
> >>
> >> Especially here I would also check if the mode is valid on the choose transport. For me this would also mean we should require a bind() before allowing to set the mode. So we have at least some idea what transport is planned to be used.
> >
> > I will make it check for BOUND state, though l2cap_set_mode does check
> > the mode already, or you are saying that if we do use a different
> > namespace we would have to convert, well I guess this further
> > reinforces my argument that making the values compatible makes things
> > a lot simpler.
>
> I care about the new socket options that they are clean and well defined when it comes to error conditions.
Fair enough, Ive made the changes so BT_MODE has its own set of modes
including BT_MODE_EXT_FLOWCTL, for the most part they are backward
compatible but Id move the LE_FLOWCTL to just map to BT_MODE_FLOWCTL
which is possible since we do require bind before setting BT_MODE we
can check the address type.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-18 22:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-12 22:24 [PATCH 0/2] Enable use of L2CAP_MODE_EXT_FLOWCTL Luiz Augusto von Dentz
2020-03-12 22:24 ` [PATCH 1/2] Bluetooth: L2CAP: Use DEFER_SETUP to group ECRED connections Luiz Augusto von Dentz
2020-03-18 11:13 ` Marcel Holtmann
2020-03-18 16:58 ` Luiz Augusto von Dentz
2020-03-12 22:24 ` [PATCH 2/2] Bluetooth: Add BT_MODE socket option Luiz Augusto von Dentz
2020-03-18 11:19 ` Marcel Holtmann
2020-03-18 17:17 ` Luiz Augusto von Dentz
2020-03-18 19:27 ` Marcel Holtmann
2020-03-18 22:04 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).