* [PATCH v3 01/18] Bluetooth: hidp: remove redundant error message
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-06 2:41 ` Gustavo Padovan
2013-04-05 12:57 ` [PATCH v3 02/18] Bluetooth: hidp: verify l2cap sockets David Herrmann
` (18 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
We print this error twice in the first error-path so remove it. One error
message is enough.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/sock.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
index 82a829d..20ebd4f 100644
--- a/net/bluetooth/hidp/sock.c
+++ b/net/bluetooth/hidp/sock.c
@@ -296,7 +296,6 @@ int __init hidp_init_sockets(void)
return 0;
error:
- BT_ERR("Can't register HIDP socket");
proto_unregister(&hidp_proto);
return err;
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 02/18] Bluetooth: hidp: verify l2cap sockets
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
2013-04-05 12:57 ` [PATCH v3 01/18] Bluetooth: hidp: remove redundant error message David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-06 2:44 ` Gustavo Padovan
2013-04-05 12:57 ` [PATCH v3 03/18] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
` (17 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
We need to verify that the given sockets actually are l2cap sockets. If
they aren't, we are not supposed to access bt_sk(sock) and we shouldn't
start the session if the offsets turn out to be valid local BT addresses.
That is, if someone passes a TCP socket to HIDCONNADD, then we access some
random offset in the TCP socket (which isn't even guaranteed to be valid).
Fix this by checking that the socket is an l2cap socket.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/hidp/core.c | 2 ++
net/bluetooth/l2cap_sock.c | 6 ++++++
3 files changed, 9 insertions(+)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 7588ef4..db6694f 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -787,6 +787,7 @@ extern bool disable_ertm;
int l2cap_init_sockets(void);
void l2cap_cleanup_sockets(void);
+bool l2cap_is_socket(struct socket *sock);
void __l2cap_connect_rsp_defer(struct l2cap_chan *chan);
int __l2cap_wait_ack(struct sock *sk);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index a7352ff..c547b55 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -969,6 +969,8 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
BT_DBG("");
+ if (!l2cap_is_socket(ctrl_sock) || !l2cap_is_socket(intr_sock))
+ return -EINVAL;
if (bacmp(&bt_sk(ctrl_sock->sk)->src, &bt_sk(intr_sock->sk)->src) ||
bacmp(&bt_sk(ctrl_sock->sk)->dst, &bt_sk(intr_sock->sk)->dst))
return -ENOTUNIQ;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1bcfb84..461e8bc 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -43,6 +43,12 @@ static void l2cap_sock_init(struct sock *sk, struct sock *parent);
static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
int proto, gfp_t prio);
+bool l2cap_is_socket(struct socket *sock)
+{
+ return sock && sock->ops == &l2cap_sock_ops;
+}
+EXPORT_SYMBOL(l2cap_is_socket);
+
static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
{
struct sock *sk = sock->sk;
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 02/18] Bluetooth: hidp: verify l2cap sockets
2013-04-05 12:57 ` [PATCH v3 02/18] Bluetooth: hidp: verify l2cap sockets David Herrmann
@ 2013-04-06 2:44 ` Gustavo Padovan
0 siblings, 0 replies; 44+ messages in thread
From: Gustavo Padovan @ 2013-04-06 2:44 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, Marcel Holtmann
Hi David,
* David Herrmann <dh.herrmann@gmail.com> [2013-04-05 14:57:34 +0200]:
> We need to verify that the given sockets actually are l2cap sockets. If
> they aren't, we are not supposed to access bt_sk(sock) and we shouldn't
> start the session if the offsets turn out to be valid local BT addresses.
>
> That is, if someone passes a TCP socket to HIDCONNADD, then we access some
> random offset in the TCP socket (which isn't even guaranteed to be valid).
>
> Fix this by checking that the socket is an l2cap socket.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> include/net/bluetooth/l2cap.h | 1 +
> net/bluetooth/hidp/core.c | 2 ++
> net/bluetooth/l2cap_sock.c | 6 ++++++
> 3 files changed, 9 insertions(+)
Patch has been applied to bluetooth-next. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 03/18] Bluetooth: rename hci_conn_put to hci_conn_drop
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
2013-04-05 12:57 ` [PATCH v3 01/18] Bluetooth: hidp: remove redundant error message David Herrmann
2013-04-05 12:57 ` [PATCH v3 02/18] Bluetooth: hidp: verify l2cap sockets David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-06 2:48 ` Gustavo Padovan
2013-04-05 12:57 ` [PATCH v3 04/18] Bluetooth: remove unneeded hci_conn_hold/put_device() David Herrmann
` (16 subsequent siblings)
19 siblings, 1 reply; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
We use _get() and _put() for device ref-counting in the kernel. However,
hci_conn_put() is _not_ used for ref-counting, hence, rename it to
hci_conn_drop() so we can later fix ref-counting and introduce
hci_conn_put().
hci_conn_hold() and hci_conn_put() are currently used to manage how long a
connection should be held alive. When the last user drops the connection,
we spawn a delayed work that performs the disconnect. Obviously, this has
nothing to do with ref-counting for the _object_ but rather for the
keep-alive of the connection.
But we really _need_ proper ref-counting for the _object_ to allow
connection-users like rfcomm-tty, HIDP or others.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_conn.c | 6 +++---
net/bluetooth/hci_event.c | 36 ++++++++++++++++++------------------
net/bluetooth/l2cap_core.c | 6 +++---
net/bluetooth/mgmt.c | 6 +++---
net/bluetooth/sco.c | 6 +++---
net/bluetooth/smp.c | 2 +-
7 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 014a2ea..2fde836 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -605,7 +605,7 @@ static inline void hci_conn_hold(struct hci_conn *conn)
cancel_delayed_work(&conn->disc_work);
}
-static inline void hci_conn_put(struct hci_conn *conn)
+static inline void hci_conn_drop(struct hci_conn *conn)
{
BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 4925a02..319e7a4 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -433,7 +433,7 @@ int hci_conn_del(struct hci_conn *conn)
struct hci_conn *acl = conn->link;
if (acl) {
acl->link = NULL;
- hci_conn_put(acl);
+ hci_conn_drop(acl);
}
}
@@ -565,7 +565,7 @@ static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
if (!sco) {
sco = hci_conn_add(hdev, type, dst);
if (!sco) {
- hci_conn_put(acl);
+ hci_conn_drop(acl);
return ERR_PTR(-ENOMEM);
}
}
@@ -980,7 +980,7 @@ void hci_chan_del(struct hci_chan *chan)
synchronize_rcu();
- hci_conn_put(conn);
+ hci_conn_drop(conn);
skb_queue_purge(&chan->data_q);
kfree(chan);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 81b4448..11df1c4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1475,7 +1475,7 @@ static void hci_cs_auth_requested(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -1502,7 +1502,7 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -1664,7 +1664,7 @@ static void hci_cs_read_remote_features(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -1691,7 +1691,7 @@ static void hci_cs_read_remote_ext_features(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -2154,7 +2154,7 @@ static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
} else {
conn->state = BT_CONNECT2;
hci_proto_connect_cfm(conn, 0);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
} else {
/* Connection rejected */
@@ -2261,14 +2261,14 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
} else {
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
} else {
hci_auth_cfm(conn, ev->status);
hci_conn_hold(conn);
conn->disc_timeout = HCI_DISCONN_TIMEOUT;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) {
@@ -2352,7 +2352,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (ev->status && conn->state == BT_CONNECTED) {
hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
goto unlock;
}
@@ -2361,7 +2361,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
} else
hci_encrypt_cfm(conn, ev->status, ev->encrypt);
}
@@ -2436,7 +2436,7 @@ static void hci_remote_features_evt(struct hci_dev *hdev,
if (!hci_outgoing_auth_needed(hdev, conn)) {
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
unlock:
@@ -2996,7 +2996,7 @@ static void hci_pin_code_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (conn->state == BT_CONNECTED) {
hci_conn_hold(conn);
conn->disc_timeout = HCI_PAIRING_TIMEOUT;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
if (!test_bit(HCI_PAIRABLE, &hdev->dev_flags))
@@ -3099,7 +3099,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (ev->key_type != HCI_LK_CHANGED_COMBINATION)
conn->key_type = ev->key_type;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
if (test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
@@ -3268,7 +3268,7 @@ static void hci_remote_ext_features_evt(struct hci_dev *hdev,
if (!hci_outgoing_auth_needed(hdev, conn)) {
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
unlock:
@@ -3413,7 +3413,7 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
if (ev->status && conn->state == BT_CONNECTED) {
hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
goto unlock;
}
@@ -3422,13 +3422,13 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
} else {
hci_auth_cfm(conn, ev->status);
hci_conn_hold(conn);
conn->disc_timeout = HCI_DISCONN_TIMEOUT;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
unlock:
@@ -3689,7 +3689,7 @@ static void hci_simple_pair_complete_evt(struct hci_dev *hdev,
mgmt_auth_failed(hdev, &conn->dst, conn->type, conn->dst_type,
ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
unlock:
hci_dev_unlock(hdev);
@@ -3777,7 +3777,7 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev,
hci_conn_hold(hcon);
hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
hci_conn_hold_device(hcon);
hci_conn_add_sysfs(hcon);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 22e6583..175dd32 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -571,7 +571,7 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
chan->conn = NULL;
if (chan->chan_type != L2CAP_CHAN_CONN_FIX_A2MP)
- hci_conn_put(conn->hcon);
+ hci_conn_drop(conn->hcon);
if (mgr && mgr->bredr_chan == chan)
mgr->bredr_chan = NULL;
@@ -1702,7 +1702,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
conn = l2cap_conn_add(hcon, 0);
if (!conn) {
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
err = -ENOMEM;
goto done;
}
@@ -1712,7 +1712,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
if (!list_empty(&conn->chan_l)) {
err = -EBUSY;
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
}
if (err)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f559b96..cca9187 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1857,7 +1857,7 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
conn->security_cfm_cb = NULL;
conn->disconn_cfm_cb = NULL;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
mgmt_pending_remove(cmd);
}
@@ -1943,7 +1943,7 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (conn->connect_cfm_cb) {
- hci_conn_put(conn);
+ hci_conn_drop(conn);
err = cmd_complete(sk, hdev->id, MGMT_OP_PAIR_DEVICE,
MGMT_STATUS_BUSY, &rp, sizeof(rp));
goto unlock;
@@ -1952,7 +1952,7 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
cmd = mgmt_pending_add(sk, MGMT_OP_PAIR_DEVICE, hdev, data, len);
if (!cmd) {
err = -ENOMEM;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
goto unlock;
}
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 57f250c..727dc36 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -185,7 +185,7 @@ static int sco_connect(struct sock *sk)
conn = sco_conn_add(hcon);
if (!conn) {
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
err = -ENOMEM;
goto done;
}
@@ -355,7 +355,7 @@ static void __sco_sock_close(struct sock *sk)
if (sco_pi(sk)->conn->hcon) {
sk->sk_state = BT_DISCONN;
sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
- hci_conn_put(sco_pi(sk)->conn->hcon);
+ hci_conn_drop(sco_pi(sk)->conn->hcon);
sco_pi(sk)->conn->hcon = NULL;
} else
sco_chan_del(sk, ECONNRESET);
@@ -883,7 +883,7 @@ static void sco_chan_del(struct sock *sk, int err)
sco_conn_unlock(conn);
if (conn->hcon)
- hci_conn_put(conn->hcon);
+ hci_conn_drop(conn->hcon);
}
sk->sk_state = BT_CLOSED;
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 5abefb1..b2296d3 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -522,7 +522,7 @@ void smp_chan_destroy(struct l2cap_conn *conn)
kfree(smp);
conn->smp_chan = NULL;
conn->hcon->smp_conn = NULL;
- hci_conn_put(conn->hcon);
+ hci_conn_drop(conn->hcon);
}
int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 03/18] Bluetooth: rename hci_conn_put to hci_conn_drop
2013-04-05 12:57 ` [PATCH v3 03/18] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
@ 2013-04-06 2:48 ` Gustavo Padovan
2013-04-06 18:31 ` David Herrmann
0 siblings, 1 reply; 44+ messages in thread
From: Gustavo Padovan @ 2013-04-06 2:48 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, Marcel Holtmann
Hi David,
* David Herrmann <dh.herrmann@gmail.com> [2013-04-05 14:57:35 +0200]:
> We use _get() and _put() for device ref-counting in the kernel. However,
> hci_conn_put() is _not_ used for ref-counting, hence, rename it to
> hci_conn_drop() so we can later fix ref-counting and introduce
> hci_conn_put().
>
> hci_conn_hold() and hci_conn_put() are currently used to manage how long a
> connection should be held alive. When the last user drops the connection,
> we spawn a delayed work that performs the disconnect. Obviously, this has
> nothing to do with ref-counting for the _object_ but rather for the
> keep-alive of the connection.
>
> But we really _need_ proper ref-counting for the _object_ to allow
> connection-users like rfcomm-tty, HIDP or others.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> net/bluetooth/hci_conn.c | 6 +++---
> net/bluetooth/hci_event.c | 36 ++++++++++++++++++------------------
> net/bluetooth/l2cap_core.c | 6 +++---
> net/bluetooth/mgmt.c | 6 +++---
> net/bluetooth/sco.c | 6 +++---
> net/bluetooth/smp.c | 2 +-
> 7 files changed, 32 insertions(+), 32 deletions(-)
This patch doesn't apply anymore. Maybe you didn't rebase it on the latest
bluetooth-next before send it. Please do this and resend. You can already add
Marcel's acks to your patches when you resend it.
Gustavo
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 03/18] Bluetooth: rename hci_conn_put to hci_conn_drop
2013-04-06 2:48 ` Gustavo Padovan
@ 2013-04-06 18:31 ` David Herrmann
0 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:31 UTC (permalink / raw)
To: Gustavo Padovan, David Herrmann, linux-bluetooth@vger.kernel.org,
Marcel Holtmann
Hi Gustavo
On Sat, Apr 6, 2013 at 4:48 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi David,
>
> * David Herrmann <dh.herrmann@gmail.com> [2013-04-05 14:57:35 +0200]:
>
>> We use _get() and _put() for device ref-counting in the kernel. However,
>> hci_conn_put() is _not_ used for ref-counting, hence, rename it to
>> hci_conn_drop() so we can later fix ref-counting and introduce
>> hci_conn_put().
>>
>> hci_conn_hold() and hci_conn_put() are currently used to manage how long a
>> connection should be held alive. When the last user drops the connection,
>> we spawn a delayed work that performs the disconnect. Obviously, this has
>> nothing to do with ref-counting for the _object_ but rather for the
>> keep-alive of the connection.
>>
>> But we really _need_ proper ref-counting for the _object_ to allow
>> connection-users like rfcomm-tty, HIDP or others.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> include/net/bluetooth/hci_core.h | 2 +-
>> net/bluetooth/hci_conn.c | 6 +++---
>> net/bluetooth/hci_event.c | 36 ++++++++++++++++++------------------
>> net/bluetooth/l2cap_core.c | 6 +++---
>> net/bluetooth/mgmt.c | 6 +++---
>> net/bluetooth/sco.c | 6 +++---
>> net/bluetooth/smp.c | 2 +-
>> 7 files changed, 32 insertions(+), 32 deletions(-)
>
> This patch doesn't apply anymore. Maybe you didn't rebase it on the latest
> bluetooth-next before send it. Please do this and resend. You can already add
> Marcel's acks to your patches when you resend it.
Sorry, I rebased in v1, but it seems there were some changes in
between. I rebased again and added Marcel's Acked-By.
Thanks!
David
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 04/18] Bluetooth: remove unneeded hci_conn_hold/put_device()
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (2 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 03/18] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 05/18] Bluetooth: introduce hci_conn ref-counting David Herrmann
` (15 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
hci_conn_hold/put_device() is used to control when hci_conn->dev is no
longer needed and can be deleted from the system. Lets first look how they
are currently used throughout the code (excluding HIDP!).
All code that uses hci_conn_hold_device() looks like this:
...
hci_conn_hold_device();
hci_conn_add_sysfs();
...
On the other side, hci_conn_put_device() is exclusively used in
hci_conn_del().
So, considering that hci_conn_del() must not be called twice (which would
fail horribly), we know that hci_conn_put_device() is only called _once_
(which is in hci_conn_del()).
On the other hand, hci_conn_add_sysfs() must not be called twice, either
(it would call device_add twice, which breaks the device, see
drivers/base/core.c). So we know that hci_conn_hold_device() is also
called only once (it's only called directly before hci_conn_add_sysfs()).
So hold and put are known to be called only once. That means we can safely
remove them and directly call hci_conn_del_sysfs() in hci_conn_del().
But there is one issue left: HIDP also uses hci_conn_hold/put_device().
However, this case can be ignored and simply removed as it is totally
broken. The issue is, the only thing HIDP delays with
hci_conn_hold_device() is the removal of the hci_conn->dev from sysfs.
But, the hci_conn device has no mechanism to get notified when its own
parent (hci_dev) gets removed from sysfs. hci_dev_hold/put() does _not_
control when it is removed but only when the device object is created
and destroyed.
And hci_dev calls hci_conn_flush_*() when it removes itself from sysfs,
which itself causes hci_conn_del() to be called, but it does _not_ cause
hci_conn_del_sysfs() to be called, which is wrong.
Hence, we fix it to call hci_conn_del_sysfs() in hci_conn_del(). This
guarantees that a hci_conn object is removed from sysfs _before_ its
parent hci_dev is removed.
The changes to HIDP look scary, wrong and broken. However, if you look at
the HIDP session management, you will notice they're already broken in the
exact _same_ way (ever tried "unplugging" HIDP devices? Breaks _all_ the
time).
So this patch only makes HIDP look _scary_ and _obviously broken_. It does
not break HIDP itself, it already is!
See later patches in this series which fix HIDP to use proper
session-management.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/net/bluetooth/hci_core.h | 4 ----
net/bluetooth/hci_conn.c | 17 +----------------
net/bluetooth/hci_event.c | 4 ----
net/bluetooth/hidp/core.c | 20 +++-----------------
4 files changed, 4 insertions(+), 41 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2fde836..cd5b126 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -338,7 +338,6 @@ struct hci_conn {
struct timer_list auto_accept_timer;
struct device dev;
- atomic_t devref;
struct hci_dev *hdev;
void *l2cap_data;
@@ -594,9 +593,6 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
-void hci_conn_hold_device(struct hci_conn *conn);
-void hci_conn_put_device(struct hci_conn *conn);
-
static inline void hci_conn_hold(struct hci_conn *conn)
{
BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 319e7a4..134ab9f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -398,8 +398,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
if (hdev->notify)
hdev->notify(hdev, HCI_NOTIFY_CONN_ADD);
- atomic_set(&conn->devref, 0);
-
hci_conn_init_sysfs(conn);
return conn;
@@ -448,7 +446,7 @@ int hci_conn_del(struct hci_conn *conn)
skb_queue_purge(&conn->data_q);
- hci_conn_put_device(conn);
+ hci_conn_del_sysfs(conn);
hci_dev_put(hdev);
@@ -835,19 +833,6 @@ void hci_conn_check_pending(struct hci_dev *hdev)
hci_dev_unlock(hdev);
}
-void hci_conn_hold_device(struct hci_conn *conn)
-{
- atomic_inc(&conn->devref);
-}
-EXPORT_SYMBOL(hci_conn_hold_device);
-
-void hci_conn_put_device(struct hci_conn *conn)
-{
- if (atomic_dec_and_test(&conn->devref))
- hci_conn_del_sysfs(conn);
-}
-EXPORT_SYMBOL(hci_conn_put_device);
-
int hci_get_conn_list(void __user *arg)
{
struct hci_conn *c;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 11df1c4..d089f5e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2000,7 +2000,6 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
} else
conn->state = BT_CONNECTED;
- hci_conn_hold_device(conn);
hci_conn_add_sysfs(conn);
if (test_bit(HCI_AUTH, &hdev->flags))
@@ -3302,7 +3301,6 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
conn->handle = __le16_to_cpu(ev->handle);
conn->state = BT_CONNECTED;
- hci_conn_hold_device(conn);
hci_conn_add_sysfs(conn);
break;
@@ -3779,7 +3777,6 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev,
hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
hci_conn_drop(hcon);
- hci_conn_hold_device(hcon);
hci_conn_add_sysfs(hcon);
amp_physical_cfm(bredr_hcon, hcon);
@@ -3913,7 +3910,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
conn->handle = __le16_to_cpu(ev->handle);
conn->state = BT_CONNECTED;
- hci_conn_hold_device(conn);
hci_conn_add_sysfs(conn);
hci_proto_connect_cfm(conn, ev->status);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index c547b55..f8144fe 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -73,18 +73,6 @@ static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
return NULL;
}
-static void __hidp_link_session(struct hidp_session *session)
-{
- list_add(&session->list, &hidp_session_list);
-}
-
-static void __hidp_unlink_session(struct hidp_session *session)
-{
- hci_conn_put_device(session->conn);
-
- list_del(&session->list);
-}
-
static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
{
memset(ci, 0, sizeof(*ci));
@@ -756,7 +744,7 @@ static int hidp_session(void *arg)
fput(session->ctrl_sock->file);
- __hidp_unlink_session(session);
+ list_del(&session->list);
up_write(&hidp_session_sem);
@@ -779,8 +767,6 @@ static struct hci_conn *hidp_get_connection(struct hidp_session *session)
hci_dev_lock(hdev);
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
- if (conn)
- hci_conn_hold_device(conn);
hci_dev_unlock(hdev);
hci_dev_put(hdev);
@@ -1022,7 +1008,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
session->idle_to = req->idle_to;
- __hidp_link_session(session);
+ list_add(&session->list, &hidp_session_list);
if (req->rd_size > 0) {
err = hidp_setup_hid(session, req);
@@ -1102,7 +1088,7 @@ unlink:
session->rd_data = NULL;
purge:
- __hidp_unlink_session(session);
+ list_del(&session->list);
skb_queue_purge(&session->ctrl_transmit);
skb_queue_purge(&session->intr_transmit);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 05/18] Bluetooth: introduce hci_conn ref-counting
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (3 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 04/18] Bluetooth: remove unneeded hci_conn_hold/put_device() David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 06/18] Bluetooth: hidp: remove unused session->state field David Herrmann
` (14 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
We currently do not allow using hci_conn from outside of HCI-core.
However, several other users could make great use of it. This includes
HIDP, rfcomm and all other sub-protocols that rely on an active
connection.
Hence, we now introduce hci_conn ref-counting. We currently never call
get_device(). put_device() is exclusively used in hci_conn_del_sysfs().
Hence, we currently never have a greater device-refcnt than 1.
Therefore, it is safe to move the put_device() call from
hci_conn_del_sysfs() to hci_conn_del() (it's the only caller). In fact,
this even fixes a "use-after-free" bug as we access hci_conn after calling
hci_conn_del_sysfs() in hci_conn_del().
>From now on we can add references to hci_conn objects in other layers
(like l2cap_sock, HIDP, rfcomm, ...) and grab a reference via
hci_conn_get(). This does _not_ guarantee, that the connection is still
alive. But, this isn't what we want. We can simply lock the hci_conn
device and use "device_is_registered(hci_conn->dev)" to test that.
However, this is hardly necessary as outside users should never rely on
the HCI connection to be alive, anyway. Instead, they should solely rely
on the device-object to be available.
But if sub-devices want the hci_conn object as sysfs parent, they need to
be notified when the connection drops. This will be introduced in later
patches with l2cap_users.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/net/bluetooth/hci_core.h | 31 +++++++++++++++++++++++++++++++
net/bluetooth/hci_conn.c | 3 +--
net/bluetooth/hci_sysfs.c | 1 -
3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cd5b126..fa8fe72 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -593,6 +593,37 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
+/*
+ * hci_conn_get() and hci_conn_put() are used to control the life-time of an
+ * "hci_conn" object. They do not guarantee that the hci_conn object is running,
+ * working or anything else. They just guarantee that the object is available
+ * and can be dereferenced. So you can use its locks, local variables and any
+ * other constant data.
+ * Before accessing runtime data, you _must_ lock the object and then check that
+ * it is still running. As soon as you release the locks, the connection might
+ * get dropped, though.
+ *
+ * On the other hand, hci_conn_hold() and hci_conn_drop() are used to control
+ * how long the underlying connection is held. So every channel that runs on the
+ * hci_conn object calls this to prevent the connection from disappearing. As
+ * long as you hold a device, you must also guarantee that you have a valid
+ * reference to the device via hci_conn_get() (or the initial reference from
+ * hci_conn_add()).
+ * The hold()/drop() ref-count is known to drop below 0 sometimes, which doesn't
+ * break because nobody cares for that. But this means, we cannot use
+ * _get()/_drop() in it, but require the caller to have a valid ref (FIXME).
+ */
+
+static inline void hci_conn_get(struct hci_conn *conn)
+{
+ get_device(&conn->dev);
+}
+
+static inline void hci_conn_put(struct hci_conn *conn)
+{
+ put_device(&conn->dev);
+}
+
static inline void hci_conn_hold(struct hci_conn *conn)
{
BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 134ab9f..75b75a8 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -450,8 +450,7 @@ int hci_conn_del(struct hci_conn *conn)
hci_dev_put(hdev);
- if (conn->handle == 0)
- kfree(conn);
+ hci_conn_put(conn);
return 0;
}
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 55cceee..0384b76 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -145,7 +145,6 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
}
device_del(&conn->dev);
- put_device(&conn->dev);
hci_dev_put(hdev);
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 06/18] Bluetooth: hidp: remove unused session->state field
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (4 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 05/18] Bluetooth: introduce hci_conn ref-counting David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 07/18] Bluetooth: hidp: test "terminate" before sleeping David Herrmann
` (13 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
This field is always BT_CONNECTED. Remove it and set it to BT_CONNECTED in
hidp_copy_session() unconditionally.
Also note that this field is totally bogus. Userspace can query an
hidp-session for its state. However, whenever user-space queries us, this
field should be BT_CONNECTED. If it wasn't BT_CONNECTED, then we would be
currently cleaning up the session and the session itself would exit in the
next few milliseconds. Hence, there is no reason to let user-space know
that the session will exit now if they cannot make _any_ use of that.
Thus, remove the field and let user-space think that a session is always
BT_CONNECTED as long as they can query it.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 5 ++---
net/bluetooth/hidp/hidp.h | 1 -
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index f8144fe..413b900 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -79,7 +79,7 @@ static void __hidp_copy_session(struct hidp_session *session, struct hidp_connin
bacpy(&ci->bdaddr, &session->bdaddr);
ci->flags = session->flags;
- ci->state = session->state;
+ ci->state = BT_CONNECTED;
ci->vendor = 0x0000;
ci->product = 0x0000;
@@ -966,7 +966,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
down_write(&hidp_session_sem);
s = __hidp_get_session(&bt_sk(ctrl_sock->sk)->dst);
- if (s && s->state == BT_CONNECTED) {
+ if (s) {
up_write(&hidp_session_sem);
return -EEXIST;
}
@@ -988,7 +988,6 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
session->ctrl_sock = ctrl_sock;
session->intr_sock = intr_sock;
- session->state = BT_CONNECTED;
session->conn = hidp_get_connection(session);
if (!session->conn) {
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index af1bcc8..57a6191 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -135,7 +135,6 @@ struct hidp_session {
bdaddr_t bdaddr;
- unsigned long state;
unsigned long flags;
unsigned long idle_to;
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 07/18] Bluetooth: hidp: test "terminate" before sleeping
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (5 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 06/18] Bluetooth: hidp: remove unused session->state field David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 08/18] Bluetooth: allow constant arguments for bacmp()/bacpy() David Herrmann
` (12 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
The "terminate" flag is guaranteed to be set before the session terminates
and the handlers are woken up. Hence, we need to add it to the
sleep-condition.
Note that testing the flags is not enough as nothing prevents us from
setting the flags again after the session-handler terminated.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 413b900..2a179ad 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -327,11 +327,13 @@ static int hidp_get_raw_report(struct hid_device *hid,
/* Wait for the return of the report. The returned report
gets put in session->report_return. */
- while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
+ while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
+ !atomic_read(&session->terminate)) {
int res;
res = wait_event_interruptible_timeout(session->report_queue,
- !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
+ !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)
+ || atomic_read(&session->terminate),
5*HZ);
if (res == 0) {
/* timeout */
@@ -396,11 +398,13 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
goto err;
/* Wait for the ACK from the device. */
- while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)) {
+ while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
+ !atomic_read(&session->terminate)) {
int res;
res = wait_event_interruptible_timeout(session->report_queue,
- !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags),
+ !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
+ || atomic_read(&session->terminate),
10*HZ);
if (res == 0) {
/* timeout */
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 08/18] Bluetooth: allow constant arguments for bacmp()/bacpy()
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (6 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 07/18] Bluetooth: hidp: test "terminate" before sleeping David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 09/18] Bluetooth: hidp: move hidp_schedule() to core.c David Herrmann
` (11 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
There is no reason to require the source arguments to be writeable so fix
this to allow constant source addresses.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/net/bluetooth/bluetooth.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 2554b3f..2dbee97 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -170,11 +170,11 @@ typedef struct {
#define BDADDR_LOCAL (&(bdaddr_t) {{0, 0, 0, 0xff, 0xff, 0xff} })
/* Copy, swap, convert BD Address */
-static inline int bacmp(bdaddr_t *ba1, bdaddr_t *ba2)
+static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
{
return memcmp(ba1, ba2, sizeof(bdaddr_t));
}
-static inline void bacpy(bdaddr_t *dst, bdaddr_t *src)
+static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src)
{
memcpy(dst, src, sizeof(bdaddr_t));
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 09/18] Bluetooth: hidp: move hidp_schedule() to core.c
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (7 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 08/18] Bluetooth: allow constant arguments for bacmp()/bacpy() David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 10/18] Bluetooth: l2cap: introduce l2cap_conn ref-counting David Herrmann
` (10 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
There is no reason to keep this helper in the header file. No other file
depends on it so move it into hidp/core.c where it belongs.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 9 +++++++++
net/bluetooth/hidp/hidp.h | 9 ---------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 2a179ad..a16474a 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -59,6 +59,15 @@ static unsigned char hidp_keycode[256] = {
static unsigned char hidp_mkeyspat[] = { 0x01, 0x01, 0x01, 0x01, 0x01, 0x01 };
+static inline void hidp_schedule(struct hidp_session *session)
+{
+ struct sock *ctrl_sk = session->ctrl_sock->sk;
+ struct sock *intr_sk = session->intr_sock->sk;
+
+ wake_up_interruptible(sk_sleep(ctrl_sk));
+ wake_up_interruptible(sk_sleep(intr_sk));
+}
+
static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
{
struct hidp_session *session;
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index 57a6191..c844420 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -174,15 +174,6 @@ struct hidp_session {
int waiting_for_startup;
};
-static inline void hidp_schedule(struct hidp_session *session)
-{
- struct sock *ctrl_sk = session->ctrl_sock->sk;
- struct sock *intr_sk = session->intr_sock->sk;
-
- wake_up_interruptible(sk_sleep(ctrl_sk));
- wake_up_interruptible(sk_sleep(intr_sk));
-}
-
/* HIDP init defines */
extern int __init hidp_init_sockets(void);
extern void __exit hidp_cleanup_sockets(void);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 10/18] Bluetooth: l2cap: introduce l2cap_conn ref-counting
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (8 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 09/18] Bluetooth: hidp: move hidp_schedule() to core.c David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 11/18] Bluetooth: l2cap: add l2cap_user sub-modules David Herrmann
` (9 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
If we want to use l2cap_conn outside of l2cap_core.c, we need refcounting
for these objects. Otherwise, we cannot synchronize l2cap locks with
outside locks and end up with deadlocks.
Hence, introduce ref-counting for l2cap_conn objects. This doesn't affect
l2cap internals at all, as they use a direct synchronization.
We also keep a reference to the parent hci_conn for locking purposes as
l2cap_conn depends on this. This doesn't affect the connection itself but
only the lifetime of the (dead) object.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/net/bluetooth/l2cap.h | 4 ++++
net/bluetooth/l2cap_core.c | 25 ++++++++++++++++++++++++-
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index db6694f..73ac062 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -584,6 +584,7 @@ struct l2cap_conn {
struct list_head chan_l;
struct mutex chan_lock;
+ struct kref ref;
};
#define L2CAP_INFO_CL_MTU_REQ_SENT 0x01
@@ -814,4 +815,7 @@ void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
u8 status);
void __l2cap_physical_cfm(struct l2cap_chan *chan, int result);
+void l2cap_conn_get(struct l2cap_conn *conn);
+void l2cap_conn_put(struct l2cap_conn *conn);
+
#endif /* __L2CAP_H */
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 175dd32..58af8d6 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1486,7 +1486,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
}
hcon->l2cap_data = NULL;
- kfree(conn);
+ conn->hchan = NULL;
+ l2cap_conn_put(conn);
}
static void security_timeout(struct work_struct *work)
@@ -1520,8 +1521,10 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
return NULL;
}
+ kref_init(&conn->ref);
hcon->l2cap_data = conn;
conn->hcon = hcon;
+ hci_conn_get(conn->hcon);
conn->hchan = hchan;
BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);
@@ -1563,6 +1566,26 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
return conn;
}
+static void l2cap_conn_free(struct kref *ref)
+{
+ struct l2cap_conn *conn = container_of(ref, struct l2cap_conn, ref);
+
+ hci_conn_put(conn->hcon);
+ kfree(conn);
+}
+
+void l2cap_conn_get(struct l2cap_conn *conn)
+{
+ kref_get(&conn->ref);
+}
+EXPORT_SYMBOL(l2cap_conn_get);
+
+void l2cap_conn_put(struct l2cap_conn *conn)
+{
+ kref_put(&conn->ref, l2cap_conn_free);
+}
+EXPORT_SYMBOL(l2cap_conn_put);
+
/* ---- Socket interface ---- */
/* Find socket with psm and source / destination bdaddr.
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 11/18] Bluetooth: l2cap: add l2cap_user sub-modules
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (9 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 10/18] Bluetooth: l2cap: introduce l2cap_conn ref-counting David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 12/18] Bluetooth: hidp: add new session-management helpers David Herrmann
` (8 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
Several sub-modules like HIDP, rfcomm, ... need to track l2cap
connections. The l2cap_conn->hcon->dev object is used as parent for sysfs
devices so the sub-modules need to be notified when the hci_conn object is
removed from sysfs.
As submodules normally use the l2cap layer, the l2cap_user objects are
registered there instead of on the underlying hci_conn object. This avoids
any direct dependency on the HCI layer and lets the l2cap core handle any
specifics.
This patch introduces l2cap_user objects which contain a "probe" and
"remove" callback. You can register them on any l2cap_conn object and if
it is active, the "probe" callback will get called. Otherwise, an error is
returned.
The l2cap_conn object will call your "remove" callback directly before it
is removed from user-space. This allows you to remove your submodules
_before_ the parent l2cap_conn and hci_conn object is removed.
At any time you can asynchronously unregister your l2cap_user object if
your submodule vanishes before the l2cap_conn object does.
There is no way around l2cap_user. If we want wire-protocols in the
kernel, we always want the hci_conn object as parent in the sysfs tree. We
cannot use a channel here since we might need multiple channels for a
single protocol.
But the problem is, we _must_ get notified when an l2cap_conn object is
removed. We cannot use reference-counting for object-removal! This is not
how it works. If a hardware is removed, we should immediately remove the
object from sysfs. Any other behavior would be inconsistent with the rest
of the system. Also note that device_del() might sleep, but it doesn't
wait for user-space or block very long. It only _unlinks_ the object from
sysfs and the whole device-tree. Everything else is handled by ref-counts!
This is exactly what the other sub-modules must do: unlink their devices
when the "remove" l2cap_user callback is called. They should not do any
cleanup or synchronous shutdowns.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
include/net/bluetooth/l2cap.h | 10 +++++
net/bluetooth/l2cap_core.c | 86 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 96 insertions(+)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 73ac062..1beba19 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -585,6 +585,13 @@ struct l2cap_conn {
struct list_head chan_l;
struct mutex chan_lock;
struct kref ref;
+ struct list_head users;
+};
+
+struct l2cap_user {
+ struct list_head list;
+ int (*probe) (struct l2cap_conn *conn, struct l2cap_user *user);
+ void (*remove) (struct l2cap_conn *conn, struct l2cap_user *user);
};
#define L2CAP_INFO_CL_MTU_REQ_SENT 0x01
@@ -818,4 +825,7 @@ void __l2cap_physical_cfm(struct l2cap_chan *chan, int result);
void l2cap_conn_get(struct l2cap_conn *conn);
void l2cap_conn_put(struct l2cap_conn *conn);
+int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user);
+void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user);
+
#endif /* __L2CAP_H */
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 58af8d6..f8f7d25 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1446,6 +1446,89 @@ static void l2cap_info_timeout(struct work_struct *work)
l2cap_conn_start(conn);
}
+/*
+ * l2cap_user
+ * External modules can register l2cap_user objects on l2cap_conn. The ->probe
+ * callback is called during registration. The ->remove callback is called
+ * during unregistration.
+ * An l2cap_user object can either be explicitly unregistered or when the
+ * underlying l2cap_conn object is deleted. This guarantees that l2cap->hcon,
+ * l2cap->hchan, .. are valid as long as the remove callback hasn't been called.
+ * External modules must own a reference to the l2cap_conn object if they intend
+ * to call l2cap_unregister_user(). The l2cap_conn object might get destroyed at
+ * any time if they don't.
+ */
+
+int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
+{
+ struct hci_dev *hdev = conn->hcon->hdev;
+ int ret;
+
+ /* We need to check whether l2cap_conn is registered. If it is not, we
+ * must not register the l2cap_user. l2cap_conn_del() is unregisters
+ * l2cap_conn objects, but doesn't provide its own locking. Instead, it
+ * relies on the parent hci_conn object to be locked. This itself relies
+ * on the hci_dev object to be locked. So we must lock the hci device
+ * here, too. */
+
+ hci_dev_lock(hdev);
+
+ if (user->list.next || user->list.prev) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ /* conn->hchan is NULL after l2cap_conn_del() was called */
+ if (!conn->hchan) {
+ ret = -ENODEV;
+ goto out_unlock;
+ }
+
+ ret = user->probe(conn, user);
+ if (ret)
+ goto out_unlock;
+
+ list_add(&user->list, &conn->users);
+ ret = 0;
+
+out_unlock:
+ hci_dev_unlock(hdev);
+ return ret;
+}
+EXPORT_SYMBOL(l2cap_register_user);
+
+void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
+{
+ struct hci_dev *hdev = conn->hcon->hdev;
+
+ hci_dev_lock(hdev);
+
+ if (!user->list.next || !user->list.prev)
+ goto out_unlock;
+
+ list_del(&user->list);
+ user->list.next = NULL;
+ user->list.prev = NULL;
+ user->remove(conn, user);
+
+out_unlock:
+ hci_dev_unlock(hdev);
+}
+EXPORT_SYMBOL(l2cap_unregister_user);
+
+static void l2cap_unregister_all_users(struct l2cap_conn *conn)
+{
+ struct l2cap_user *user;
+
+ while (!list_empty(&conn->users)) {
+ user = list_first_entry(&conn->users, struct l2cap_user, list);
+ list_del(&user->list);
+ user->list.next = NULL;
+ user->list.prev = NULL;
+ user->remove(conn, user);
+ }
+}
+
static void l2cap_conn_del(struct hci_conn *hcon, int err)
{
struct l2cap_conn *conn = hcon->l2cap_data;
@@ -1458,6 +1541,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
kfree_skb(conn->rx_skb);
+ l2cap_unregister_all_users(conn);
+
mutex_lock(&conn->chan_lock);
/* Kill channels */
@@ -1555,6 +1640,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
mutex_init(&conn->chan_lock);
INIT_LIST_HEAD(&conn->chan_l);
+ INIT_LIST_HEAD(&conn->users);
if (hcon->type == LE_LINK)
INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 12/18] Bluetooth: hidp: add new session-management helpers
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (10 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 11/18] Bluetooth: l2cap: add l2cap_user sub-modules David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 13/18] Bluetooth: hidp: remove old session-management David Herrmann
` (7 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
This is a rewrite of the HIDP session management. It implements HIDP as an
l2cap_user sub-module so we get proper notification when the underlying
connection goes away.
The helpers are not yet used but only added in this commit. The old
session management is still used and will be removed in a following patch.
The old session-management was flawed. Hotplugging is horribly broken and
we have no way of getting notified when the underlying connection goes
down. The whole idea of removing the HID/input sub-devices from within the
session itself is broken and suffers from major dead-locks. We never can
guarantee that the session can unregister itself as long as we use
synchronous shutdowns. This can only work with asynchronous shutdowns.
However, in this case we _must_ be able to unregister the session from the
outside as otherwise the l2cap_conn object might be unlinked before we
are.
The new session-management is based on l2cap_user. There is only one
way how to add a session and how to delete a session: "probe" and "remove"
callbacks from l2cap_user.
This guarantees that the session can be registered and unregistered at
_any_ time without any synchronous shutdown.
On the other hand, much work has been put into proper session-refcounting.
We can unregister/unlink the session only if we can guarantee that it will
stay alive. But for asynchronous shutdowns we never know when the last
user goes away so we must use proper ref-counting.
The old ->conn field has been renamed to ->hconn so we can reuse ->conn in
the new session management. No other existing HIDP code is modified.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 555 +++++++++++++++++++++++++++++++++++++++++++++-
net/bluetooth/hidp/hidp.h | 53 +++--
2 files changed, 583 insertions(+), 25 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index a16474a..43131d2 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1,6 +1,7 @@
/*
HIDP implementation for Linux Bluetooth stack (BlueZ).
Copyright (C) 2003-2004 Marcel Holtmann <marcel@holtmann.org>
+ Copyright (C) 2013 David Herrmann <dh.herrmann@gmail.com>
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License version 2 as
@@ -20,6 +21,7 @@
SOFTWARE IS DISCLAIMED.
*/
+#include <linux/kref.h>
#include <linux/module.h>
#include <linux/file.h>
#include <linux/kthread.h>
@@ -59,6 +61,13 @@ static unsigned char hidp_keycode[256] = {
static unsigned char hidp_mkeyspat[] = { 0x01, 0x01, 0x01, 0x01, 0x01, 0x01 };
+static int hidp_session_probe(struct l2cap_conn *conn,
+ struct l2cap_user *user);
+static void hidp_session_remove(struct l2cap_conn *conn,
+ struct l2cap_user *user);
+static int hidp_session_thread(void *arg);
+static void hidp_session_terminate(struct hidp_session *s);
+
static inline void hidp_schedule(struct hidp_session *session)
{
struct sock *ctrl_sk = session->ctrl_sock->sk;
@@ -834,7 +843,7 @@ static int hidp_setup_input(struct hidp_session *session,
input->relbit[0] |= BIT_MASK(REL_WHEEL);
}
- input->dev.parent = &session->conn->dev;
+ input->dev.parent = &session->hconn->dev;
input->event = hidp_input_event;
@@ -938,7 +947,7 @@ static int hidp_setup_hid(struct hidp_session *session,
snprintf(hid->uniq, sizeof(hid->uniq), "%pMR",
&bt_sk(session->ctrl_sock->sk)->dst);
- hid->dev.parent = &session->conn->dev;
+ hid->dev.parent = &session->hconn->dev;
hid->ll_driver = &hidp_hid_driver;
hid->hid_get_raw_report = hidp_get_raw_report;
@@ -960,6 +969,543 @@ fault:
return err;
}
+/* initialize session devices */
+static int hidp_session_dev_init(struct hidp_session *session,
+ struct hidp_connadd_req *req)
+{
+ int ret;
+
+ if (req->rd_size > 0) {
+ ret = hidp_setup_hid(session, req);
+ if (ret && ret != -ENODEV)
+ return ret;
+ }
+
+ if (!session->hid) {
+ ret = hidp_setup_input(session, req);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+/* destroy session devices */
+static void hidp_session_dev_destroy(struct hidp_session *session)
+{
+ if (session->hid)
+ put_device(&session->hid->dev);
+ else if (session->input)
+ input_put_device(session->input);
+
+ kfree(session->rd_data);
+ session->rd_data = NULL;
+}
+
+/* add HID/input devices to their underlying bus systems */
+static int hidp_session_dev_add(struct hidp_session *session)
+{
+ int ret;
+
+ /* Both HID and input systems drop a ref-count when unregistering the
+ * device but they don't take a ref-count when registering them. Work
+ * around this by explicitly taking a refcount during registration
+ * which is dropped automatically by unregistering the devices. */
+
+ if (session->hid) {
+ ret = hid_add_device(session->hid);
+ if (ret)
+ return ret;
+ get_device(&session->hid->dev);
+ } else if (session->input) {
+ ret = input_register_device(session->input);
+ if (ret)
+ return ret;
+ input_get_device(session->input);
+ }
+
+ return 0;
+}
+
+/* remove HID/input devices from their bus systems */
+static void hidp_session_dev_del(struct hidp_session *session)
+{
+ if (session->hid)
+ hid_destroy_device(session->hid);
+ else if (session->input)
+ input_unregister_device(session->input);
+}
+
+/*
+ * Create new session object
+ * Allocate session object, initialize static fields, copy input data into the
+ * object and take a reference to all sub-objects.
+ * This returns 0 on success and puts a pointer to the new session object in
+ * \out. Otherwise, an error code is returned.
+ * The new session object has an initial ref-count of 1.
+ */
+static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr,
+ struct socket *ctrl_sock,
+ struct socket *intr_sock,
+ struct hidp_connadd_req *req,
+ struct l2cap_conn *conn)
+{
+ struct hidp_session *session;
+ int ret;
+ struct bt_sock *ctrl, *intr;
+
+ ctrl = bt_sk(ctrl_sock->sk);
+ intr = bt_sk(intr_sock->sk);
+
+ session = kzalloc(sizeof(*session), GFP_KERNEL);
+ if (!session)
+ return -ENOMEM;
+
+ /* object and runtime management */
+ kref_init(&session->ref);
+ atomic_set(&session->state, HIDP_SESSION_IDLING);
+ init_waitqueue_head(&session->state_queue);
+ session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
+
+ /* connection management */
+ bacpy(&session->bdaddr, bdaddr);
+ session->conn = conn;
+ session->user.probe = hidp_session_probe;
+ session->user.remove = hidp_session_remove;
+ session->ctrl_sock = ctrl_sock;
+ session->intr_sock = intr_sock;
+ skb_queue_head_init(&session->ctrl_transmit);
+ skb_queue_head_init(&session->intr_transmit);
+ session->ctrl_mtu = min_t(uint, l2cap_pi(ctrl)->chan->omtu,
+ l2cap_pi(ctrl)->chan->imtu);
+ session->intr_mtu = min_t(uint, l2cap_pi(intr)->chan->omtu,
+ l2cap_pi(intr)->chan->imtu);
+ session->idle_to = req->idle_to;
+
+ /* device management */
+ setup_timer(&session->timer, hidp_idle_timeout,
+ (unsigned long)session);
+
+ /* session data */
+ mutex_init(&session->report_mutex);
+ init_waitqueue_head(&session->report_queue);
+
+ ret = hidp_session_dev_init(session, req);
+ if (ret)
+ goto err_free;
+
+ l2cap_conn_get(session->conn);
+ get_file(session->intr_sock->file);
+ get_file(session->ctrl_sock->file);
+ *out = session;
+ return 0;
+
+err_free:
+ kfree(session);
+ return ret;
+}
+
+/* increase ref-count of the given session by one */
+static void hidp_session_get(struct hidp_session *session)
+{
+ kref_get(&session->ref);
+}
+
+/* release callback */
+static void session_free(struct kref *ref)
+{
+ struct hidp_session *session = container_of(ref, struct hidp_session,
+ ref);
+
+ hidp_session_dev_destroy(session);
+ skb_queue_purge(&session->ctrl_transmit);
+ skb_queue_purge(&session->intr_transmit);
+ fput(session->intr_sock->file);
+ fput(session->ctrl_sock->file);
+ l2cap_conn_put(session->conn);
+ kfree(session);
+}
+
+/* decrease ref-count of the given session by one */
+static void hidp_session_put(struct hidp_session *session)
+{
+ kref_put(&session->ref, session_free);
+}
+
+/*
+ * Search the list of active sessions for a session with target address
+ * \bdaddr. You must hold at least a read-lock on \hidp_session_sem. As long as
+ * you do not release this lock, the session objects cannot vanish and you can
+ * safely take a reference to the session yourself.
+ */
+static struct hidp_session *__hidp_session_find(const bdaddr_t *bdaddr)
+{
+ struct hidp_session *session;
+
+ list_for_each_entry(session, &hidp_session_list, list) {
+ if (!bacmp(bdaddr, &session->bdaddr))
+ return session;
+ }
+
+ return NULL;
+}
+
+/*
+ * Same as __hidp_session_find() but no locks must be held. This also takes a
+ * reference of the returned session (if non-NULL) so you must drop this
+ * reference if you no longer use the object.
+ */
+static struct hidp_session *hidp_session_find(const bdaddr_t *bdaddr)
+{
+ struct hidp_session *session;
+
+ down_read(&hidp_session_sem);
+
+ session = __hidp_session_find(bdaddr);
+ if (session)
+ hidp_session_get(session);
+
+ up_read(&hidp_session_sem);
+
+ return session;
+}
+
+/*
+ * Start session synchronously
+ * This starts a session thread and waits until initialization
+ * is done or returns an error if it couldn't be started.
+ * If this returns 0 the session thread is up and running. You must call
+ * hipd_session_stop_sync() before deleting any runtime resources.
+ */
+static int hidp_session_start_sync(struct hidp_session *session)
+{
+ unsigned int vendor, product;
+
+ if (session->hid) {
+ vendor = session->hid->vendor;
+ product = session->hid->product;
+ } else if (session->input) {
+ vendor = session->input->id.vendor;
+ product = session->input->id.product;
+ } else {
+ vendor = 0x0000;
+ product = 0x0000;
+ }
+
+ session->task = kthread_run(hidp_session_thread, session,
+ "khidpd_%04x%04x", vendor, product);
+ if (IS_ERR(session->task))
+ return PTR_ERR(session->task);
+
+ while (atomic_read(&session->state) <= HIDP_SESSION_IDLING)
+ wait_event(session->state_queue,
+ atomic_read(&session->state) > HIDP_SESSION_IDLING);
+
+ return 0;
+}
+
+/*
+ * Terminate session thread
+ * Wake up session thread and notify it to stop. This is asynchronous and
+ * returns immediately. Call this whenever a runtime error occurs and you want
+ * the session to stop.
+ * Note: wake_up_process() performs any necessary memory-barriers for us.
+ */
+static void hidp_session_terminate(struct hidp_session *session)
+{
+ atomic_inc(&session->terminate);
+ wake_up_process(session->task);
+}
+
+/*
+ * Probe HIDP session
+ * This is called from the l2cap_conn core when our l2cap_user object is bound
+ * to the hci-connection. We get the session via the \user object and can now
+ * start the session thread, register the HID/input devices and link it into
+ * the global session list.
+ * The global session-list owns its own reference to the session object so you
+ * can drop your own reference after registering the l2cap_user object.
+ */
+static int hidp_session_probe(struct l2cap_conn *conn,
+ struct l2cap_user *user)
+{
+ struct hidp_session *session = container_of(user,
+ struct hidp_session,
+ user);
+ struct hidp_session *s;
+ int ret;
+
+ down_write(&hidp_session_sem);
+
+ /* check that no other session for this device exists */
+ s = __hidp_session_find(&session->bdaddr);
+ if (s) {
+ ret = -EEXIST;
+ goto out_unlock;
+ }
+
+ ret = hidp_session_start_sync(session);
+ if (ret)
+ goto out_unlock;
+
+ ret = hidp_session_dev_add(session);
+ if (ret)
+ goto out_stop;
+
+ hidp_session_get(session);
+ list_add(&session->list, &hidp_session_list);
+ ret = 0;
+ goto out_unlock;
+
+out_stop:
+ hidp_session_terminate(session);
+out_unlock:
+ up_write(&hidp_session_sem);
+ return ret;
+}
+
+/*
+ * Remove HIDP session
+ * Called from the l2cap_conn core when either we explicitly unregistered
+ * the l2cap_user object or if the underlying connection is shut down.
+ * We signal the hidp-session thread to shut down, unregister the HID/input
+ * devices and unlink the session from the global list.
+ * This drops the reference to the session that is owned by the global
+ * session-list.
+ * Note: We _must_ not synchronosly wait for the session-thread to shut down.
+ * This is, because the session-thread might be waiting for an HCI lock that is
+ * held while we are called. Therefore, we only unregister the devices and
+ * notify the session-thread to terminate. The thread itself owns a reference
+ * to the session object so it can safely shut down.
+ */
+static void hidp_session_remove(struct l2cap_conn *conn,
+ struct l2cap_user *user)
+{
+ struct hidp_session *session = container_of(user,
+ struct hidp_session,
+ user);
+
+ down_write(&hidp_session_sem);
+
+ hidp_session_terminate(session);
+ hidp_session_dev_del(session);
+ list_del(&session->list);
+
+ up_write(&hidp_session_sem);
+
+ hidp_session_put(session);
+}
+
+/*
+ * Session Worker
+ * This performs the actual main-loop of the HIDP worker. We first check
+ * whether the underlying connection is still alive, then parse all pending
+ * messages and finally send all outstanding messages.
+ */
+static void hidp_session_run(struct hidp_session *session)
+{
+ struct sock *ctrl_sk = session->ctrl_sock->sk;
+ struct sock *intr_sk = session->intr_sock->sk;
+ struct sk_buff *skb;
+
+ for (;;) {
+ /*
+ * This thread can be woken up two ways:
+ * - You call hidp_session_terminate() which sets the
+ * session->terminate flag and wakes this thread up.
+ * - Via modifying the socket state of ctrl/intr_sock. This
+ * thread is woken up by ->sk_state_changed().
+ *
+ * Note: set_current_state() performs any necessary
+ * memory-barriers for us.
+ */
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ if (atomic_read(&session->terminate))
+ break;
+
+ if (ctrl_sk->sk_state != BT_CONNECTED ||
+ intr_sk->sk_state != BT_CONNECTED)
+ break;
+
+ /* parse incoming intr-skbs */
+ while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
+ skb_orphan(skb);
+ if (!skb_linearize(skb))
+ hidp_recv_intr_frame(session, skb);
+ else
+ kfree_skb(skb);
+ }
+
+ /* send pending intr-skbs */
+ hidp_process_intr_transmit(session);
+
+ /* parse incoming ctrl-skbs */
+ while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
+ skb_orphan(skb);
+ if (!skb_linearize(skb))
+ hidp_recv_ctrl_frame(session, skb);
+ else
+ kfree_skb(skb);
+ }
+
+ /* send pending ctrl-skbs */
+ hidp_process_ctrl_transmit(session);
+
+ schedule();
+ }
+
+ atomic_inc(&session->terminate);
+ set_current_state(TASK_RUNNING);
+}
+
+/*
+ * HIDP session thread
+ * This thread runs the I/O for a single HIDP session. Startup is synchronous
+ * which allows us to take references to ourself here instead of doing that in
+ * the caller.
+ * When we are ready to run we notify the caller and call hidp_session_run().
+ */
+static int hidp_session_thread(void *arg)
+{
+ struct hidp_session *session = arg;
+ wait_queue_t ctrl_wait, intr_wait;
+
+ BT_DBG("session %p", session);
+
+ /* initialize runtime environment */
+ hidp_session_get(session);
+ __module_get(THIS_MODULE);
+ set_user_nice(current, -15);
+ hidp_set_timer(session);
+
+ init_waitqueue_entry(&ctrl_wait, current);
+ init_waitqueue_entry(&intr_wait, current);
+ add_wait_queue(sk_sleep(session->ctrl_sock->sk), &ctrl_wait);
+ add_wait_queue(sk_sleep(session->intr_sock->sk), &intr_wait);
+ /* This memory barrier is paired with wq_has_sleeper(). See
+ * sock_poll_wait() for more information why this is needed. */
+ smp_mb();
+
+ /* notify synchronous startup that we're ready */
+ atomic_inc(&session->state);
+ wake_up(&session->state_queue);
+
+ /* run session */
+ hidp_session_run(session);
+
+ /* cleanup runtime environment */
+ remove_wait_queue(sk_sleep(session->intr_sock->sk), &intr_wait);
+ remove_wait_queue(sk_sleep(session->intr_sock->sk), &ctrl_wait);
+ wake_up_interruptible(&session->report_queue);
+ hidp_del_timer(session);
+
+ /*
+ * If we stopped ourself due to any internal signal, we should try to
+ * unregister our own session here to avoid having it linger until the
+ * parent l2cap_conn dies or user-space cleans it up.
+ * This does not deadlock as we don't do any synchronous shutdown.
+ * Instead, this call has the same semantics as if user-space tried to
+ * delete the session.
+ */
+ l2cap_unregister_user(session->conn, &session->user);
+ hidp_session_put(session);
+
+ module_put_and_exit(0);
+ return 0;
+}
+
+static int hidp_verify_sockets(struct socket *ctrl_sock,
+ struct socket *intr_sock)
+{
+ struct bt_sock *ctrl, *intr;
+ struct hidp_session *session;
+
+ if (!l2cap_is_socket(ctrl_sock) || !l2cap_is_socket(intr_sock))
+ return -EINVAL;
+
+ ctrl = bt_sk(ctrl_sock->sk);
+ intr = bt_sk(intr_sock->sk);
+
+ if (bacmp(&ctrl->src, &intr->src) || bacmp(&ctrl->dst, &intr->dst))
+ return -ENOTUNIQ;
+ if (ctrl->sk.sk_state != BT_CONNECTED ||
+ intr->sk.sk_state != BT_CONNECTED)
+ return -EBADFD;
+
+ /* early session check, we check again during session registration */
+ session = hidp_session_find(&ctrl->dst);
+ if (session) {
+ hidp_session_put(session);
+ return -EEXIST;
+ }
+
+ return 0;
+}
+
+int hidp_connection_add(struct hidp_connadd_req *req,
+ struct socket *ctrl_sock,
+ struct socket *intr_sock)
+{
+ struct hidp_session *session;
+ struct l2cap_conn *conn;
+ struct l2cap_chan *chan = l2cap_pi(ctrl_sock->sk)->chan;
+ int ret;
+
+ ret = hidp_verify_sockets(ctrl_sock, intr_sock);
+ if (ret)
+ return ret;
+
+ conn = NULL;
+ l2cap_chan_lock(chan);
+ if (chan->conn) {
+ l2cap_conn_get(chan->conn);
+ conn = chan->conn;
+ }
+ l2cap_chan_unlock(chan);
+
+ if (!conn)
+ return -EBADFD;
+
+ ret = hidp_session_new(&session, &bt_sk(ctrl_sock->sk)->dst, ctrl_sock,
+ intr_sock, req, conn);
+ if (ret)
+ goto out_conn;
+
+ ret = l2cap_register_user(conn, &session->user);
+ if (ret)
+ goto out_session;
+
+ ret = 0;
+
+out_session:
+ hidp_session_put(session);
+out_conn:
+ l2cap_conn_put(conn);
+ return ret;
+}
+
+int hidp_connection_del(struct hidp_conndel_req *req)
+{
+ struct hidp_session *session;
+
+ session = hidp_session_find(&req->bdaddr);
+ if (!session)
+ return -ENOENT;
+
+ if (req->flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG))
+ hidp_send_ctrl_message(session,
+ HIDP_TRANS_HID_CONTROL |
+ HIDP_CTRL_VIRTUAL_CABLE_UNPLUG,
+ NULL, 0);
+ else
+ l2cap_unregister_user(session->conn, &session->user);
+
+ hidp_session_put(session);
+
+ return 0;
+}
+
int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
{
struct hidp_session *session, *s;
@@ -1002,8 +1548,8 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
session->ctrl_sock = ctrl_sock;
session->intr_sock = intr_sock;
- session->conn = hidp_get_connection(session);
- if (!session->conn) {
+ session->hconn = hidp_get_connection(session);
+ if (!session->hconn) {
err = -ENOTCONN;
goto failed;
}
@@ -1204,6 +1750,7 @@ module_init(hidp_init);
module_exit(hidp_exit);
MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
+MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
MODULE_DESCRIPTION("Bluetooth HIDP ver " VERSION);
MODULE_VERSION(VERSION);
MODULE_LICENSE("GPL");
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index c844420..c4fb980 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -24,7 +24,9 @@
#define __HIDP_H
#include <linux/types.h>
+#include <linux/kref.h>
#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/l2cap.h>
/* HIDP header masks */
#define HIDP_HEADER_TRANS_MASK 0xf0
@@ -119,42 +121,55 @@ struct hidp_connlist_req {
struct hidp_conninfo __user *ci;
};
+int hidp_connection_add(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
+int hidp_connection_del(struct hidp_conndel_req *req);
int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
int hidp_del_connection(struct hidp_conndel_req *req);
int hidp_get_connlist(struct hidp_connlist_req *req);
int hidp_get_conninfo(struct hidp_conninfo *ci);
+enum hidp_session_state {
+ HIDP_SESSION_IDLING,
+ HIDP_SESSION_RUNNING,
+};
+
/* HIDP session defines */
struct hidp_session {
struct list_head list;
+ struct kref ref;
- struct hci_conn *conn;
+ /* runtime management */
+ atomic_t state;
+ wait_queue_head_t state_queue;
+ atomic_t terminate;
+ struct task_struct *task;
+ unsigned long flags;
+ /* connection management */
+ bdaddr_t bdaddr;
+ struct hci_conn *hconn;
+ struct l2cap_conn *conn;
+ struct l2cap_user user;
struct socket *ctrl_sock;
struct socket *intr_sock;
-
- bdaddr_t bdaddr;
-
- unsigned long flags;
- unsigned long idle_to;
-
+ struct sk_buff_head ctrl_transmit;
+ struct sk_buff_head intr_transmit;
uint ctrl_mtu;
uint intr_mtu;
+ unsigned long idle_to;
- atomic_t terminate;
- struct task_struct *task;
-
- unsigned char keys[8];
- unsigned char leds;
-
+ /* device management */
struct input_dev *input;
-
struct hid_device *hid;
-
struct timer_list timer;
- struct sk_buff_head ctrl_transmit;
- struct sk_buff_head intr_transmit;
+ /* Report descriptor */
+ __u8 *rd_data;
+ uint rd_size;
+
+ /* session data */
+ unsigned char keys[8];
+ unsigned char leds;
/* Used in hidp_get_raw_report() */
int waiting_report_type; /* HIDP_DATA_RTYPE_* */
@@ -166,10 +181,6 @@ struct hidp_session {
/* Used in hidp_output_raw_report() */
int output_report_success; /* boolean */
- /* Report descriptor */
- __u8 *rd_data;
- uint rd_size;
-
wait_queue_head_t startup_queue;
int waiting_for_startup;
};
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 13/18] Bluetooth: hidp: remove old session-management
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (11 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 12/18] Bluetooth: hidp: add new session-management helpers David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 14/18] Bluetooth: hidp: handle kernel_sendmsg() errors correctly David Herrmann
` (6 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
We have the full new session-management now available so lets switch over
and remove all the old code. Few semantics changed, so we need to adjust
the sock.c callers a bit. But this mostly simplifies the logic.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 338 ++--------------------------------------------
net/bluetooth/hidp/hidp.h | 6 -
net/bluetooth/hidp/sock.c | 21 +--
3 files changed, 18 insertions(+), 347 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 43131d2..9555407 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -77,21 +77,7 @@ static inline void hidp_schedule(struct hidp_session *session)
wake_up_interruptible(sk_sleep(intr_sk));
}
-static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
-{
- struct hidp_session *session;
-
- BT_DBG("");
-
- list_for_each_entry(session, &hidp_session_list, list) {
- if (!bacmp(bdaddr, &session->bdaddr))
- return session;
- }
-
- return NULL;
-}
-
-static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
+static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
{
memset(ci, 0, sizeof(*ci));
bacpy(&ci->bdaddr, &session->bdaddr);
@@ -453,8 +439,7 @@ static void hidp_idle_timeout(unsigned long arg)
{
struct hidp_session *session = (struct hidp_session *) arg;
- atomic_inc(&session->terminate);
- wake_up_process(session->task);
+ hidp_session_terminate(session);
}
static void hidp_set_timer(struct hidp_session *session)
@@ -522,8 +507,7 @@ static void hidp_process_hid_control(struct hidp_session *session,
skb_queue_purge(&session->ctrl_transmit);
skb_queue_purge(&session->intr_transmit);
- atomic_inc(&session->terminate);
- wake_up_process(current);
+ hidp_session_terminate(session);
}
}
@@ -683,119 +667,6 @@ static void hidp_process_ctrl_transmit(struct hidp_session *session)
}
}
-static int hidp_session(void *arg)
-{
- struct hidp_session *session = arg;
- struct sock *ctrl_sk = session->ctrl_sock->sk;
- struct sock *intr_sk = session->intr_sock->sk;
- struct sk_buff *skb;
- wait_queue_t ctrl_wait, intr_wait;
-
- BT_DBG("session %p", session);
-
- __module_get(THIS_MODULE);
- set_user_nice(current, -15);
-
- init_waitqueue_entry(&ctrl_wait, current);
- init_waitqueue_entry(&intr_wait, current);
- add_wait_queue(sk_sleep(ctrl_sk), &ctrl_wait);
- add_wait_queue(sk_sleep(intr_sk), &intr_wait);
- session->waiting_for_startup = 0;
- wake_up_interruptible(&session->startup_queue);
- set_current_state(TASK_INTERRUPTIBLE);
- while (!atomic_read(&session->terminate)) {
- if (ctrl_sk->sk_state != BT_CONNECTED ||
- intr_sk->sk_state != BT_CONNECTED)
- break;
-
- while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
- skb_orphan(skb);
- if (!skb_linearize(skb))
- hidp_recv_intr_frame(session, skb);
- else
- kfree_skb(skb);
- }
-
- hidp_process_intr_transmit(session);
-
- while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
- skb_orphan(skb);
- if (!skb_linearize(skb))
- hidp_recv_ctrl_frame(session, skb);
- else
- kfree_skb(skb);
- }
-
- hidp_process_ctrl_transmit(session);
-
- schedule();
- set_current_state(TASK_INTERRUPTIBLE);
- }
- set_current_state(TASK_RUNNING);
- remove_wait_queue(sk_sleep(intr_sk), &intr_wait);
- remove_wait_queue(sk_sleep(ctrl_sk), &ctrl_wait);
-
- clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
- clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
- wake_up_interruptible(&session->report_queue);
-
- down_write(&hidp_session_sem);
-
- hidp_del_timer(session);
-
- if (session->input) {
- input_unregister_device(session->input);
- session->input = NULL;
- }
-
- if (session->hid) {
- hid_destroy_device(session->hid);
- session->hid = NULL;
- }
-
- /* Wakeup user-space polling for socket errors */
- session->intr_sock->sk->sk_err = EUNATCH;
- session->ctrl_sock->sk->sk_err = EUNATCH;
-
- hidp_schedule(session);
-
- fput(session->intr_sock->file);
-
- wait_event_timeout(*(sk_sleep(ctrl_sk)),
- (ctrl_sk->sk_state == BT_CLOSED), msecs_to_jiffies(500));
-
- fput(session->ctrl_sock->file);
-
- list_del(&session->list);
-
- up_write(&hidp_session_sem);
-
- kfree(session->rd_data);
- kfree(session);
- module_put_and_exit(0);
- return 0;
-}
-
-static struct hci_conn *hidp_get_connection(struct hidp_session *session)
-{
- bdaddr_t *src = &bt_sk(session->ctrl_sock->sk)->src;
- bdaddr_t *dst = &bt_sk(session->ctrl_sock->sk)->dst;
- struct hci_conn *conn;
- struct hci_dev *hdev;
-
- hdev = hci_get_route(dst, src);
- if (!hdev)
- return NULL;
-
- hci_dev_lock(hdev);
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
- hci_dev_unlock(hdev);
-
- hci_dev_put(hdev);
-
- return conn;
-}
-
static int hidp_setup_input(struct hidp_session *session,
struct hidp_connadd_req *req)
{
@@ -843,7 +714,7 @@ static int hidp_setup_input(struct hidp_session *session,
input->relbit[0] |= BIT_MASK(REL_WHEEL);
}
- input->dev.parent = &session->hconn->dev;
+ input->dev.parent = &session->conn->hcon->dev;
input->event = hidp_input_event;
@@ -947,7 +818,7 @@ static int hidp_setup_hid(struct hidp_session *session,
snprintf(hid->uniq, sizeof(hid->uniq), "%pMR",
&bt_sk(session->ctrl_sock->sk)->dst);
- hid->dev.parent = &session->hconn->dev;
+ hid->dev.parent = &session->conn->hcon->dev;
hid->ll_driver = &hidp_hid_driver;
hid->hid_get_raw_report = hidp_get_raw_report;
@@ -1506,187 +1377,6 @@ int hidp_connection_del(struct hidp_conndel_req *req)
return 0;
}
-int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
-{
- struct hidp_session *session, *s;
- int vendor, product;
- int err;
-
- BT_DBG("");
-
- if (!l2cap_is_socket(ctrl_sock) || !l2cap_is_socket(intr_sock))
- return -EINVAL;
- if (bacmp(&bt_sk(ctrl_sock->sk)->src, &bt_sk(intr_sock->sk)->src) ||
- bacmp(&bt_sk(ctrl_sock->sk)->dst, &bt_sk(intr_sock->sk)->dst))
- return -ENOTUNIQ;
-
- BT_DBG("rd_data %p rd_size %d", req->rd_data, req->rd_size);
-
- down_write(&hidp_session_sem);
-
- s = __hidp_get_session(&bt_sk(ctrl_sock->sk)->dst);
- if (s) {
- up_write(&hidp_session_sem);
- return -EEXIST;
- }
-
- session = kzalloc(sizeof(struct hidp_session), GFP_KERNEL);
- if (!session) {
- up_write(&hidp_session_sem);
- return -ENOMEM;
- }
-
- bacpy(&session->bdaddr, &bt_sk(ctrl_sock->sk)->dst);
-
- session->ctrl_mtu = min_t(uint, l2cap_pi(ctrl_sock->sk)->chan->omtu,
- l2cap_pi(ctrl_sock->sk)->chan->imtu);
- session->intr_mtu = min_t(uint, l2cap_pi(intr_sock->sk)->chan->omtu,
- l2cap_pi(intr_sock->sk)->chan->imtu);
-
- BT_DBG("ctrl mtu %d intr mtu %d", session->ctrl_mtu, session->intr_mtu);
-
- session->ctrl_sock = ctrl_sock;
- session->intr_sock = intr_sock;
-
- session->hconn = hidp_get_connection(session);
- if (!session->hconn) {
- err = -ENOTCONN;
- goto failed;
- }
-
- setup_timer(&session->timer, hidp_idle_timeout, (unsigned long)session);
-
- skb_queue_head_init(&session->ctrl_transmit);
- skb_queue_head_init(&session->intr_transmit);
-
- mutex_init(&session->report_mutex);
- init_waitqueue_head(&session->report_queue);
- init_waitqueue_head(&session->startup_queue);
- session->waiting_for_startup = 1;
- session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
- session->idle_to = req->idle_to;
-
- list_add(&session->list, &hidp_session_list);
-
- if (req->rd_size > 0) {
- err = hidp_setup_hid(session, req);
- if (err && err != -ENODEV)
- goto purge;
- }
-
- if (!session->hid) {
- err = hidp_setup_input(session, req);
- if (err < 0)
- goto purge;
- }
-
- hidp_set_timer(session);
-
- if (session->hid) {
- vendor = session->hid->vendor;
- product = session->hid->product;
- } else if (session->input) {
- vendor = session->input->id.vendor;
- product = session->input->id.product;
- } else {
- vendor = 0x0000;
- product = 0x0000;
- }
-
- session->task = kthread_run(hidp_session, session, "khidpd_%04x%04x",
- vendor, product);
- if (IS_ERR(session->task)) {
- err = PTR_ERR(session->task);
- goto unlink;
- }
-
- while (session->waiting_for_startup) {
- wait_event_interruptible(session->startup_queue,
- !session->waiting_for_startup);
- }
-
- if (session->hid)
- err = hid_add_device(session->hid);
- else
- err = input_register_device(session->input);
-
- if (err < 0) {
- atomic_inc(&session->terminate);
- wake_up_process(session->task);
- up_write(&hidp_session_sem);
- return err;
- }
-
- if (session->input) {
- hidp_send_ctrl_message(session,
- HIDP_TRANS_SET_PROTOCOL | HIDP_PROTO_BOOT, NULL, 0);
- session->flags |= (1 << HIDP_BOOT_PROTOCOL_MODE);
-
- session->leds = 0xff;
- hidp_input_event(session->input, EV_LED, 0, 0);
- }
-
- up_write(&hidp_session_sem);
- return 0;
-
-unlink:
- hidp_del_timer(session);
-
- if (session->input) {
- input_unregister_device(session->input);
- session->input = NULL;
- }
-
- if (session->hid) {
- hid_destroy_device(session->hid);
- session->hid = NULL;
- }
-
- kfree(session->rd_data);
- session->rd_data = NULL;
-
-purge:
- list_del(&session->list);
-
- skb_queue_purge(&session->ctrl_transmit);
- skb_queue_purge(&session->intr_transmit);
-
-failed:
- up_write(&hidp_session_sem);
-
- kfree(session);
- return err;
-}
-
-int hidp_del_connection(struct hidp_conndel_req *req)
-{
- struct hidp_session *session;
- int err = 0;
-
- BT_DBG("");
-
- down_read(&hidp_session_sem);
-
- session = __hidp_get_session(&req->bdaddr);
- if (session) {
- if (req->flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG)) {
- hidp_send_ctrl_message(session,
- HIDP_TRANS_HID_CONTROL | HIDP_CTRL_VIRTUAL_CABLE_UNPLUG, NULL, 0);
- } else {
- /* Flush the transmit queues */
- skb_queue_purge(&session->ctrl_transmit);
- skb_queue_purge(&session->intr_transmit);
-
- atomic_inc(&session->terminate);
- wake_up_process(session->task);
- }
- } else
- err = -ENOENT;
-
- up_read(&hidp_session_sem);
- return err;
-}
-
int hidp_get_connlist(struct hidp_connlist_req *req)
{
struct hidp_session *session;
@@ -1699,7 +1389,7 @@ int hidp_get_connlist(struct hidp_connlist_req *req)
list_for_each_entry(session, &hidp_session_list, list) {
struct hidp_conninfo ci;
- __hidp_copy_session(session, &ci);
+ hidp_copy_session(session, &ci);
if (copy_to_user(req->ci, &ci, sizeof(ci))) {
err = -EFAULT;
@@ -1720,18 +1410,14 @@ int hidp_get_connlist(struct hidp_connlist_req *req)
int hidp_get_conninfo(struct hidp_conninfo *ci)
{
struct hidp_session *session;
- int err = 0;
- down_read(&hidp_session_sem);
-
- session = __hidp_get_session(&ci->bdaddr);
- if (session)
- __hidp_copy_session(session, ci);
- else
- err = -ENOENT;
+ session = hidp_session_find(&ci->bdaddr);
+ if (session) {
+ hidp_copy_session(session, ci);
+ hidp_session_put(session);
+ }
- up_read(&hidp_session_sem);
- return err;
+ return session ? 0 : -ENOENT;
}
static int __init hidp_init(void)
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index c4fb980..6162ce8 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -123,8 +123,6 @@ struct hidp_connlist_req {
int hidp_connection_add(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
int hidp_connection_del(struct hidp_conndel_req *req);
-int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
-int hidp_del_connection(struct hidp_conndel_req *req);
int hidp_get_connlist(struct hidp_connlist_req *req);
int hidp_get_conninfo(struct hidp_conninfo *ci);
@@ -147,7 +145,6 @@ struct hidp_session {
/* connection management */
bdaddr_t bdaddr;
- struct hci_conn *hconn;
struct l2cap_conn *conn;
struct l2cap_user user;
struct socket *ctrl_sock;
@@ -180,9 +177,6 @@ struct hidp_session {
/* Used in hidp_output_raw_report() */
int output_report_success; /* boolean */
-
- wait_queue_head_t startup_queue;
- int waiting_for_startup;
};
/* HIDP init defines */
diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
index 20ebd4f..809a460 100644
--- a/net/bluetooth/hidp/sock.c
+++ b/net/bluetooth/hidp/sock.c
@@ -77,21 +77,12 @@ static int hidp_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
return err;
}
- if (csock->sk->sk_state != BT_CONNECTED ||
- isock->sk->sk_state != BT_CONNECTED) {
- sockfd_put(csock);
- sockfd_put(isock);
- return -EBADFD;
- }
+ err = hidp_connection_add(&ca, csock, isock);
+ if (!err && copy_to_user(argp, &ca, sizeof(ca)))
+ err = -EFAULT;
- err = hidp_add_connection(&ca, csock, isock);
- if (!err) {
- if (copy_to_user(argp, &ca, sizeof(ca)))
- err = -EFAULT;
- } else {
- sockfd_put(csock);
- sockfd_put(isock);
- }
+ sockfd_put(csock);
+ sockfd_put(isock);
return err;
@@ -102,7 +93,7 @@ static int hidp_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
if (copy_from_user(&cd, argp, sizeof(cd)))
return -EFAULT;
- return hidp_del_connection(&cd);
+ return hidp_connection_del(&cd);
case HIDPGETCONNLIST:
if (copy_from_user(&cl, argp, sizeof(cl)))
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 14/18] Bluetooth: hidp: handle kernel_sendmsg() errors correctly
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (12 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 13/18] Bluetooth: hidp: remove old session-management David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 15/18] Bluetooth: hidp: merge hidp_process_{ctrl,intr}_transmit() David Herrmann
` (5 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
We shouldn't push back the skbs if kernel_sendmsg() fails. Instead, we
terminate the connection and drop the skb. Only on EAGAIN we push it back
and return.
l2cap doesn't return EAGAIN, yet, but this guarantees we're safe if it
will at some time in the future.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 9555407..73bcc63 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -636,13 +636,19 @@ static int hidp_send_frame(struct socket *sock, unsigned char *data, int len)
static void hidp_process_intr_transmit(struct hidp_session *session)
{
struct sk_buff *skb;
+ int ret;
BT_DBG("session %p", session);
while ((skb = skb_dequeue(&session->intr_transmit))) {
- if (hidp_send_frame(session->intr_sock, skb->data, skb->len) < 0) {
+ ret = hidp_send_frame(session->intr_sock, skb->data, skb->len);
+ if (ret == -EAGAIN) {
skb_queue_head(&session->intr_transmit, skb);
break;
+ } else if (ret < 0) {
+ hidp_session_terminate(session);
+ kfree_skb(skb);
+ break;
}
hidp_set_timer(session);
@@ -653,13 +659,19 @@ static void hidp_process_intr_transmit(struct hidp_session *session)
static void hidp_process_ctrl_transmit(struct hidp_session *session)
{
struct sk_buff *skb;
+ int ret;
BT_DBG("session %p", session);
while ((skb = skb_dequeue(&session->ctrl_transmit))) {
- if (hidp_send_frame(session->ctrl_sock, skb->data, skb->len) < 0) {
+ ret = hidp_send_frame(session->ctrl_sock, skb->data, skb->len);
+ if (ret == -EAGAIN) {
skb_queue_head(&session->ctrl_transmit, skb);
break;
+ } else if (ret < 0) {
+ hidp_session_terminate(session);
+ kfree_skb(skb);
+ break;
}
hidp_set_timer(session);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 15/18] Bluetooth: hidp: merge hidp_process_{ctrl,intr}_transmit()
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (13 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 14/18] Bluetooth: hidp: handle kernel_sendmsg() errors correctly David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 16/18] Bluetooth: hidp: merge 'send' functions into hidp_send_message() David Herrmann
` (4 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
Both hidp_process_ctrl_transmit() and hidp_process_intr_transmit() are
exactly the same apart from the transmit-queue and socket pointers.
Therefore, pass them as argument and merge both functions into one so we
avoid 25 lines of code-duplication.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 40 +++++++++++-----------------------------
1 file changed, 11 insertions(+), 29 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 73bcc63..a566dc2 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -633,40 +633,20 @@ static int hidp_send_frame(struct socket *sock, unsigned char *data, int len)
return kernel_sendmsg(sock, &msg, &iv, 1, len);
}
-static void hidp_process_intr_transmit(struct hidp_session *session)
+/* dequeue message from @transmit and send via @sock */
+static void hidp_process_transmit(struct hidp_session *session,
+ struct sk_buff_head *transmit,
+ struct socket *sock)
{
struct sk_buff *skb;
int ret;
BT_DBG("session %p", session);
- while ((skb = skb_dequeue(&session->intr_transmit))) {
- ret = hidp_send_frame(session->intr_sock, skb->data, skb->len);
+ while ((skb = skb_dequeue(transmit))) {
+ ret = hidp_send_frame(sock, skb->data, skb->len);
if (ret == -EAGAIN) {
- skb_queue_head(&session->intr_transmit, skb);
- break;
- } else if (ret < 0) {
- hidp_session_terminate(session);
- kfree_skb(skb);
- break;
- }
-
- hidp_set_timer(session);
- kfree_skb(skb);
- }
-}
-
-static void hidp_process_ctrl_transmit(struct hidp_session *session)
-{
- struct sk_buff *skb;
- int ret;
-
- BT_DBG("session %p", session);
-
- while ((skb = skb_dequeue(&session->ctrl_transmit))) {
- ret = hidp_send_frame(session->ctrl_sock, skb->data, skb->len);
- if (ret == -EAGAIN) {
- skb_queue_head(&session->ctrl_transmit, skb);
+ skb_queue_head(transmit, skb);
break;
} else if (ret < 0) {
hidp_session_terminate(session);
@@ -1221,7 +1201,8 @@ static void hidp_session_run(struct hidp_session *session)
}
/* send pending intr-skbs */
- hidp_process_intr_transmit(session);
+ hidp_process_transmit(session, &session->intr_transmit,
+ session->intr_sock);
/* parse incoming ctrl-skbs */
while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
@@ -1233,7 +1214,8 @@ static void hidp_session_run(struct hidp_session *session)
}
/* send pending ctrl-skbs */
- hidp_process_ctrl_transmit(session);
+ hidp_process_transmit(session, &session->ctrl_transmit,
+ session->ctrl_sock);
schedule();
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 16/18] Bluetooth: hidp: merge 'send' functions into hidp_send_message()
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (14 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 15/18] Bluetooth: hidp: merge hidp_process_{ctrl,intr}_transmit() David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 17/18] Bluetooth: hidp: don't send boot-protocol messages as HID-reports David Herrmann
` (3 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
We handle skb buffers all over the place, even though we have
hidp_send_*_message() helpers. This creates a more generic
hidp_send_message() helper and uses it instead of dealing with transmit
queues directly everywhere.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 156 ++++++++++++++++++----------------------------
1 file changed, 60 insertions(+), 96 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index a566dc2..b6c5a82 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -68,15 +68,6 @@ static void hidp_session_remove(struct l2cap_conn *conn,
static int hidp_session_thread(void *arg);
static void hidp_session_terminate(struct hidp_session *s);
-static inline void hidp_schedule(struct hidp_session *session)
-{
- struct sock *ctrl_sk = session->ctrl_sock->sk;
- struct sock *intr_sk = session->intr_sock->sk;
-
- wake_up_interruptible(sk_sleep(ctrl_sk));
- wake_up_interruptible(sk_sleep(intr_sk));
-}
-
static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
{
memset(ci, 0, sizeof(*ci));
@@ -107,11 +98,56 @@ static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo
}
}
+/* assemble skb, queue message on @transmit and wake up the session thread */
+static int hidp_send_message(struct hidp_session *session, struct socket *sock,
+ struct sk_buff_head *transmit, unsigned char hdr,
+ const unsigned char *data, int size)
+{
+ struct sk_buff *skb;
+ struct sock *sk = sock->sk;
+
+ BT_DBG("session %p data %p size %d", session, data, size);
+
+ if (atomic_read(&session->terminate))
+ return -EIO;
+
+ skb = alloc_skb(size + 1, GFP_ATOMIC);
+ if (!skb) {
+ BT_ERR("Can't allocate memory for new frame");
+ return -ENOMEM;
+ }
+
+ *skb_put(skb, 1) = hdr;
+ if (data && size > 0)
+ memcpy(skb_put(skb, size), data, size);
+
+ skb_queue_tail(transmit, skb);
+ wake_up_interruptible(sk_sleep(sk));
+
+ return 0;
+}
+
+static int hidp_send_ctrl_message(struct hidp_session *session,
+ unsigned char hdr, const unsigned char *data,
+ int size)
+{
+ return hidp_send_message(session, session->ctrl_sock,
+ &session->ctrl_transmit, hdr, data, size);
+}
+
+static int hidp_send_intr_message(struct hidp_session *session,
+ unsigned char hdr, const unsigned char *data,
+ int size)
+{
+ return hidp_send_message(session, session->intr_sock,
+ &session->intr_transmit, hdr, data, size);
+}
+
static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
unsigned int type, unsigned int code, int value)
{
unsigned char newleds;
- struct sk_buff *skb;
+ unsigned char hdr, data[2];
BT_DBG("session %p type %d code %d value %d", session, type, code, value);
@@ -129,21 +165,11 @@ static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
session->leds = newleds;
- skb = alloc_skb(3, GFP_ATOMIC);
- if (!skb) {
- BT_ERR("Can't allocate memory for new frame");
- return -ENOMEM;
- }
-
- *skb_put(skb, 1) = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
- *skb_put(skb, 1) = 0x01;
- *skb_put(skb, 1) = newleds;
-
- skb_queue_tail(&session->intr_transmit, skb);
+ hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
+ data[0] = 0x01;
+ data[1] = newleds;
- hidp_schedule(session);
-
- return 0;
+ return hidp_send_intr_message(session, hdr, data, 2);
}
static int hidp_hidinput_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
@@ -216,71 +242,9 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
input_sync(dev);
}
-static int __hidp_send_ctrl_message(struct hidp_session *session,
- unsigned char hdr, unsigned char *data,
- int size)
-{
- struct sk_buff *skb;
-
- BT_DBG("session %p data %p size %d", session, data, size);
-
- if (atomic_read(&session->terminate))
- return -EIO;
-
- skb = alloc_skb(size + 1, GFP_ATOMIC);
- if (!skb) {
- BT_ERR("Can't allocate memory for new frame");
- return -ENOMEM;
- }
-
- *skb_put(skb, 1) = hdr;
- if (data && size > 0)
- memcpy(skb_put(skb, size), data, size);
-
- skb_queue_tail(&session->ctrl_transmit, skb);
-
- return 0;
-}
-
-static int hidp_send_ctrl_message(struct hidp_session *session,
- unsigned char hdr, unsigned char *data, int size)
-{
- int err;
-
- err = __hidp_send_ctrl_message(session, hdr, data, size);
-
- hidp_schedule(session);
-
- return err;
-}
-
-static int hidp_queue_report(struct hidp_session *session,
- unsigned char *data, int size)
-{
- struct sk_buff *skb;
-
- BT_DBG("session %p hid %p data %p size %d", session, session->hid, data, size);
-
- skb = alloc_skb(size + 1, GFP_ATOMIC);
- if (!skb) {
- BT_ERR("Can't allocate memory for new frame");
- return -ENOMEM;
- }
-
- *skb_put(skb, 1) = 0xa2;
- if (size > 0)
- memcpy(skb_put(skb, size), data, size);
-
- skb_queue_tail(&session->intr_transmit, skb);
-
- hidp_schedule(session);
-
- return 0;
-}
-
static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
{
- unsigned char buf[32];
+ unsigned char buf[32], hdr;
int rsize;
rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
@@ -288,8 +252,9 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
return -EIO;
hid_output_report(report, buf);
+ hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
- return hidp_queue_report(session, buf, rsize);
+ return hidp_send_intr_message(session, hdr, buf, rsize);
}
static int hidp_get_raw_report(struct hid_device *hid,
@@ -325,7 +290,7 @@ static int hidp_get_raw_report(struct hid_device *hid,
session->waiting_report_number = numbered_reports ? report_number : -1;
set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
data[0] = report_number;
- ret = hidp_send_ctrl_message(hid->driver_data, report_type, data, 1);
+ ret = hidp_send_ctrl_message(session, report_type, data, 1);
if (ret)
goto err;
@@ -385,7 +350,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
break;
case HID_OUTPUT_REPORT:
- report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
+ report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
break;
default:
return -EINVAL;
@@ -396,8 +361,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
/* Set up our wait, and send the report request to the device. */
set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
- ret = hidp_send_ctrl_message(hid->driver_data, report_type, data,
- count);
+ ret = hidp_send_ctrl_message(session, report_type, data, count);
if (ret)
goto err;
@@ -482,12 +446,12 @@ static void hidp_process_handshake(struct hidp_session *session,
case HIDP_HSHK_ERR_FATAL:
/* Device requests a reboot, as this is the only way this error
* can be recovered. */
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HID_CONTROL | HIDP_CTRL_SOFT_RESET, NULL, 0);
break;
default:
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
break;
}
@@ -535,7 +499,7 @@ static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
break;
default:
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
}
@@ -582,7 +546,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
break;
default:
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_UNSUPPORTED_REQUEST, NULL, 0);
break;
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 17/18] Bluetooth: hidp: don't send boot-protocol messages as HID-reports
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (15 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 16/18] Bluetooth: hidp: merge 'send' functions into hidp_send_message() David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-05 12:57 ` [PATCH v3 18/18] Bluetooth: hidp: fix sending output reports on intr channel David Herrmann
` (2 subsequent siblings)
19 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
If a device is registered as HID device, it is always in Report-Mode.
Therefore, we must not send Boot-Protocol messages on
hidinput_input_event() callbacks. This confuses devices and may cause
disconnects on protocol errors.
We disable the hidinput_input_event() callback for now. We can implement
it properly later, but lets first fix the current code by disabling it.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index b6c5a82..3354071 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -143,13 +143,15 @@ static int hidp_send_intr_message(struct hidp_session *session,
&session->intr_transmit, hdr, data, size);
}
-static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
- unsigned int type, unsigned int code, int value)
+static int hidp_input_event(struct input_dev *dev, unsigned int type,
+ unsigned int code, int value)
{
+ struct hidp_session *session = input_get_drvdata(dev);
unsigned char newleds;
unsigned char hdr, data[2];
- BT_DBG("session %p type %d code %d value %d", session, type, code, value);
+ BT_DBG("session %p type %d code %d value %d",
+ session, type, code, value);
if (type != EV_LED)
return -1;
@@ -172,21 +174,6 @@ static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
return hidp_send_intr_message(session, hdr, data, 2);
}
-static int hidp_hidinput_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
-{
- struct hid_device *hid = input_get_drvdata(dev);
- struct hidp_session *session = hid->driver_data;
-
- return hidp_queue_event(session, dev, type, code, value);
-}
-
-static int hidp_input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
-{
- struct hidp_session *session = input_get_drvdata(dev);
-
- return hidp_queue_event(session, dev, type, code, value);
-}
-
static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
{
struct input_dev *dev = session->input;
@@ -729,7 +716,6 @@ static struct hid_ll_driver hidp_hid_driver = {
.stop = hidp_stop,
.open = hidp_open,
.close = hidp_close,
- .hidinput_input_event = hidp_hidinput_event,
};
/* This function sets up the hid device. It does not add it
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 18/18] Bluetooth: hidp: fix sending output reports on intr channel
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (16 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 17/18] Bluetooth: hidp: don't send boot-protocol messages as HID-reports David Herrmann
@ 2013-04-05 12:57 ` David Herrmann
2013-04-18 2:49 ` Gustavo Padovan
2013-04-05 19:01 ` [PATCH v3 00/18] Rework HIDP Session Management Marcel Holtmann
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
19 siblings, 1 reply; 44+ messages in thread
From: David Herrmann @ 2013-04-05 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann, Gustavo Padovan, David Herrmann
According to the specifications, data output reports must be sent on the
interrupt channel. See also usbhid implementation.
Sending these reports on the control channel breaks newer Wii Remotes.
Note that this will make output reports asynchronous. However, that's how
hid_output_raw_report() is supposed to work with HID_OUTPUT_REPORT as
report type. There are no responses to output reports.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
net/bluetooth/hidp/core.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 3354071..c756b06 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -332,14 +332,11 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
struct hidp_session *session = hid->driver_data;
int ret;
- switch (report_type) {
- case HID_FEATURE_REPORT:
- report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
- break;
- case HID_OUTPUT_REPORT:
+ if (report_type == HID_OUTPUT_REPORT) {
report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
- break;
- default:
+ return hidp_send_intr_message(session, report_type,
+ data, count);
+ } else if (report_type != HID_FEATURE_REPORT) {
return -EINVAL;
}
@@ -348,6 +345,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
/* Set up our wait, and send the report request to the device. */
set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
+ report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
ret = hidp_send_ctrl_message(session, report_type, data, count);
if (ret)
goto err;
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 18/18] Bluetooth: hidp: fix sending output reports on intr channel
2013-04-05 12:57 ` [PATCH v3 18/18] Bluetooth: hidp: fix sending output reports on intr channel David Herrmann
@ 2013-04-18 2:49 ` Gustavo Padovan
0 siblings, 0 replies; 44+ messages in thread
From: Gustavo Padovan @ 2013-04-18 2:49 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, Marcel Holtmann
Hi David,
* David Herrmann <dh.herrmann@gmail.com> [2013-04-05 14:57:50 +0200]:
> According to the specifications, data output reports must be sent on the
> interrupt channel. See also usbhid implementation.
> Sending these reports on the control channel breaks newer Wii Remotes.
>
> Note that this will make output reports asynchronous. However, that's how
> hid_output_raw_report() is supposed to work with HID_OUTPUT_REPORT as
> report type. There are no responses to output reports.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> net/bluetooth/hidp/core.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
All patches in this series are now applied. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 00/18] Rework HIDP Session Management
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (17 preceding siblings ...)
2013-04-05 12:57 ` [PATCH v3 18/18] Bluetooth: hidp: fix sending output reports on intr channel David Herrmann
@ 2013-04-05 19:01 ` Marcel Holtmann
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
19 siblings, 0 replies; 44+ messages in thread
From: Marcel Holtmann @ 2013-04-05 19:01 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, Gustavo Padovan
Hi David,
> This is v3 of the HIDP session management rework. Only thing that changed is
> that I moved hci_conn_user to l2cap_user as requested. The l2cap_user object is
> now bound to l2cap_conn objects instead of hci_conn. This avoids any direct HCI
> dependency in external modules.
>
> Note that this requires l2cap_conn ref-counting. However, this is pretty easy to
> implement and does not affect existing code at all as we use direct
> synchronization.
>
> Also note that I designed it in a way that it is independent of l2cap_sock. So
> if we succeed it making l2cap_core independent of l2cap_sock, we can move HIDP
> or other l2cap_user users over and make them independent of l2cap_sock, too.
this looks good. And using L2CAP as entry point makes this cleaner.
And triple thanks for the really great commit messages. That made it a lot easier for me to review these patches.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH v4 00/16] Rework HIDP Session Management
2013-04-05 12:57 [PATCH v3 00/18] Rework HIDP Session Management David Herrmann
` (18 preceding siblings ...)
2013-04-05 19:01 ` [PATCH v3 00/18] Rework HIDP Session Management Marcel Holtmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 01/16] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
` (15 more replies)
19 siblings, 16 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
Hi
No changes from v3, but rebased onto bluetooth-next/master. Sorry, I forgot
that on the previous series.
I tested it with bluetooth-next and everything works great. Only conflicts
were some unrelated HCI core-fixes.
Regards
David
David Herrmann (16):
Bluetooth: rename hci_conn_put to hci_conn_drop
Bluetooth: remove unneeded hci_conn_hold/put_device()
Bluetooth: introduce hci_conn ref-counting
Bluetooth: hidp: remove unused session->state field
Bluetooth: hidp: test "terminate" before sleeping
Bluetooth: allow constant arguments for bacmp()/bacpy()
Bluetooth: hidp: move hidp_schedule() to core.c
Bluetooth: l2cap: introduce l2cap_conn ref-counting
Bluetooth: l2cap: add l2cap_user sub-modules
Bluetooth: hidp: add new session-management helpers
Bluetooth: hidp: remove old session-management
Bluetooth: hidp: handle kernel_sendmsg() errors correctly
Bluetooth: hidp: merge hidp_process_{ctrl,intr}_transmit()
Bluetooth: hidp: merge 'send' functions into hidp_send_message()
Bluetooth: hidp: don't send boot-protocol messages as HID-reports
Bluetooth: hidp: fix sending output reports on intr channel
include/net/bluetooth/bluetooth.h | 4 +-
include/net/bluetooth/hci_core.h | 35 +-
include/net/bluetooth/l2cap.h | 14 +
net/bluetooth/hci_conn.c | 26 +-
net/bluetooth/hci_event.c | 40 +-
net/bluetooth/hci_sysfs.c | 1 -
net/bluetooth/hidp/core.c | 994 ++++++++++++++++++++++----------------
net/bluetooth/hidp/hidp.h | 67 ++-
net/bluetooth/hidp/sock.c | 21 +-
net/bluetooth/l2cap_core.c | 117 ++++-
net/bluetooth/mgmt.c | 6 +-
net/bluetooth/sco.c | 6 +-
net/bluetooth/smp.c | 2 +-
13 files changed, 810 insertions(+), 523 deletions(-)
--
1.8.2
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH v4 01/16] Bluetooth: rename hci_conn_put to hci_conn_drop
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-11 19:45 ` Gustavo Padovan
2013-04-06 18:28 ` [PATCH v4 02/16] Bluetooth: remove unneeded hci_conn_hold/put_device() David Herrmann
` (14 subsequent siblings)
15 siblings, 1 reply; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
We use _get() and _put() for device ref-counting in the kernel. However,
hci_conn_put() is _not_ used for ref-counting, hence, rename it to
hci_conn_drop() so we can later fix ref-counting and introduce
hci_conn_put().
hci_conn_hold() and hci_conn_put() are currently used to manage how long a
connection should be held alive. When the last user drops the connection,
we spawn a delayed work that performs the disconnect. Obviously, this has
nothing to do with ref-counting for the _object_ but rather for the
keep-alive of the connection.
But we really _need_ proper ref-counting for the _object_ to allow
connection-users like rfcomm-tty, HIDP or others.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_conn.c | 6 +++---
net/bluetooth/hci_event.c | 36 ++++++++++++++++++------------------
net/bluetooth/l2cap_core.c | 6 +++---
net/bluetooth/mgmt.c | 6 +++---
net/bluetooth/sco.c | 6 +++---
net/bluetooth/smp.c | 2 +-
7 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d4e13bf..78ea9c7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -612,7 +612,7 @@ static inline void hci_conn_hold(struct hci_conn *conn)
cancel_delayed_work(&conn->disc_work);
}
-static inline void hci_conn_put(struct hci_conn *conn)
+static inline void hci_conn_drop(struct hci_conn *conn)
{
BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9f9016..30d7dfc 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -433,7 +433,7 @@ int hci_conn_del(struct hci_conn *conn)
struct hci_conn *acl = conn->link;
if (acl) {
acl->link = NULL;
- hci_conn_put(acl);
+ hci_conn_drop(acl);
}
}
@@ -565,7 +565,7 @@ static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
if (!sco) {
sco = hci_conn_add(hdev, type, dst);
if (!sco) {
- hci_conn_put(acl);
+ hci_conn_drop(acl);
return ERR_PTR(-ENOMEM);
}
}
@@ -980,7 +980,7 @@ void hci_chan_del(struct hci_chan *chan)
synchronize_rcu();
- hci_conn_put(conn);
+ hci_conn_drop(conn);
skb_queue_purge(&chan->data_q);
kfree(chan);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0a2b128..2cf28b1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1190,7 +1190,7 @@ static void hci_cs_auth_requested(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -1217,7 +1217,7 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -1379,7 +1379,7 @@ static void hci_cs_read_remote_features(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -1406,7 +1406,7 @@ static void hci_cs_read_remote_ext_features(struct hci_dev *hdev, __u8 status)
if (conn) {
if (conn->state == BT_CONFIG) {
hci_proto_connect_cfm(conn, status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
}
@@ -1860,7 +1860,7 @@ static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
} else {
conn->state = BT_CONNECT2;
hci_proto_connect_cfm(conn, 0);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
} else {
/* Connection rejected */
@@ -1967,14 +1967,14 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
} else {
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
} else {
hci_auth_cfm(conn, ev->status);
hci_conn_hold(conn);
conn->disc_timeout = HCI_DISCONN_TIMEOUT;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
if (test_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags)) {
@@ -2058,7 +2058,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (ev->status && conn->state == BT_CONNECTED) {
hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
goto unlock;
}
@@ -2067,7 +2067,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
} else
hci_encrypt_cfm(conn, ev->status, ev->encrypt);
}
@@ -2142,7 +2142,7 @@ static void hci_remote_features_evt(struct hci_dev *hdev,
if (!hci_outgoing_auth_needed(hdev, conn)) {
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
unlock:
@@ -2682,7 +2682,7 @@ static void hci_pin_code_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (conn->state == BT_CONNECTED) {
hci_conn_hold(conn);
conn->disc_timeout = HCI_PAIRING_TIMEOUT;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
if (!test_bit(HCI_PAIRABLE, &hdev->dev_flags))
@@ -2785,7 +2785,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
if (ev->key_type != HCI_LK_CHANGED_COMBINATION)
conn->key_type = ev->key_type;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
if (test_bit(HCI_LINK_KEYS, &hdev->dev_flags))
@@ -2954,7 +2954,7 @@ static void hci_remote_ext_features_evt(struct hci_dev *hdev,
if (!hci_outgoing_auth_needed(hdev, conn)) {
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
unlock:
@@ -3087,7 +3087,7 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
if (ev->status && conn->state == BT_CONNECTED) {
hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
goto unlock;
}
@@ -3096,13 +3096,13 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
conn->state = BT_CONNECTED;
hci_proto_connect_cfm(conn, ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
} else {
hci_auth_cfm(conn, ev->status);
hci_conn_hold(conn);
conn->disc_timeout = HCI_DISCONN_TIMEOUT;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
}
unlock:
@@ -3363,7 +3363,7 @@ static void hci_simple_pair_complete_evt(struct hci_dev *hdev,
mgmt_auth_failed(hdev, &conn->dst, conn->type, conn->dst_type,
ev->status);
- hci_conn_put(conn);
+ hci_conn_drop(conn);
unlock:
hci_dev_unlock(hdev);
@@ -3451,7 +3451,7 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev,
hci_conn_hold(hcon);
hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
hci_conn_hold_device(hcon);
hci_conn_add_sysfs(hcon);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 7c7e932..7cdb93c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -571,7 +571,7 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
chan->conn = NULL;
if (chan->chan_type != L2CAP_CHAN_CONN_FIX_A2MP)
- hci_conn_put(conn->hcon);
+ hci_conn_drop(conn->hcon);
if (mgr && mgr->bredr_chan == chan)
mgr->bredr_chan = NULL;
@@ -1697,7 +1697,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
conn = l2cap_conn_add(hcon, 0);
if (!conn) {
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
err = -ENOMEM;
goto done;
}
@@ -1707,7 +1707,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
if (!list_empty(&conn->chan_l)) {
err = -EBUSY;
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
}
if (err)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 03e7e73..34ba164 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2131,7 +2131,7 @@ static void pairing_complete(struct pending_cmd *cmd, u8 status)
conn->security_cfm_cb = NULL;
conn->disconn_cfm_cb = NULL;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
mgmt_pending_remove(cmd);
}
@@ -2222,7 +2222,7 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
}
if (conn->connect_cfm_cb) {
- hci_conn_put(conn);
+ hci_conn_drop(conn);
err = cmd_complete(sk, hdev->id, MGMT_OP_PAIR_DEVICE,
MGMT_STATUS_BUSY, &rp, sizeof(rp));
goto unlock;
@@ -2231,7 +2231,7 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
cmd = mgmt_pending_add(sk, MGMT_OP_PAIR_DEVICE, hdev, data, len);
if (!cmd) {
err = -ENOMEM;
- hci_conn_put(conn);
+ hci_conn_drop(conn);
goto unlock;
}
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index d919d11..9909eec 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -185,7 +185,7 @@ static int sco_connect(struct sock *sk)
conn = sco_conn_add(hcon);
if (!conn) {
- hci_conn_put(hcon);
+ hci_conn_drop(hcon);
err = -ENOMEM;
goto done;
}
@@ -353,7 +353,7 @@ static void __sco_sock_close(struct sock *sk)
if (sco_pi(sk)->conn->hcon) {
sk->sk_state = BT_DISCONN;
sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
- hci_conn_put(sco_pi(sk)->conn->hcon);
+ hci_conn_drop(sco_pi(sk)->conn->hcon);
sco_pi(sk)->conn->hcon = NULL;
} else
sco_chan_del(sk, ECONNRESET);
@@ -882,7 +882,7 @@ static void sco_chan_del(struct sock *sk, int err)
sco_conn_unlock(conn);
if (conn->hcon)
- hci_conn_put(conn->hcon);
+ hci_conn_drop(conn->hcon);
}
sk->sk_state = BT_CLOSED;
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 5abefb1..b2296d3 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -522,7 +522,7 @@ void smp_chan_destroy(struct l2cap_conn *conn)
kfree(smp);
conn->smp_chan = NULL;
conn->hcon->smp_conn = NULL;
- hci_conn_put(conn->hcon);
+ hci_conn_drop(conn->hcon);
}
int smp_user_confirm_reply(struct hci_conn *hcon, u16 mgmt_op, __le32 passkey)
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v4 01/16] Bluetooth: rename hci_conn_put to hci_conn_drop
2013-04-06 18:28 ` [PATCH v4 01/16] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
@ 2013-04-11 19:45 ` Gustavo Padovan
0 siblings, 0 replies; 44+ messages in thread
From: Gustavo Padovan @ 2013-04-11 19:45 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth
Hi David,
* David Herrmann <dh.herrmann@gmail.com> [2013-04-06 20:28:37 +0200]:
> We use _get() and _put() for device ref-counting in the kernel. However,
> hci_conn_put() is _not_ used for ref-counting, hence, rename it to
> hci_conn_drop() so we can later fix ref-counting and introduce
> hci_conn_put().
>
> hci_conn_hold() and hci_conn_put() are currently used to manage how long a
> connection should be held alive. When the last user drops the connection,
> we spawn a delayed work that performs the disconnect. Obviously, this has
> nothing to do with ref-counting for the _object_ but rather for the
> keep-alive of the connection.
>
> But we really _need_ proper ref-counting for the _object_ to allow
> connection-users like rfcomm-tty, HIDP or others.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> net/bluetooth/hci_conn.c | 6 +++---
> net/bluetooth/hci_event.c | 36 ++++++++++++++++++------------------
> net/bluetooth/l2cap_core.c | 6 +++---
> net/bluetooth/mgmt.c | 6 +++---
> net/bluetooth/sco.c | 6 +++---
> net/bluetooth/smp.c | 2 +-
> 7 files changed, 32 insertions(+), 32 deletions(-)
Patch has been applied to bluetooth-next. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 02/16] Bluetooth: remove unneeded hci_conn_hold/put_device()
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
2013-04-06 18:28 ` [PATCH v4 01/16] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-17 5:39 ` Gustavo Padovan
2013-04-06 18:28 ` [PATCH v4 03/16] Bluetooth: introduce hci_conn ref-counting David Herrmann
` (13 subsequent siblings)
15 siblings, 1 reply; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
hci_conn_hold/put_device() is used to control when hci_conn->dev is no
longer needed and can be deleted from the system. Lets first look how they
are currently used throughout the code (excluding HIDP!).
All code that uses hci_conn_hold_device() looks like this:
...
hci_conn_hold_device();
hci_conn_add_sysfs();
...
On the other side, hci_conn_put_device() is exclusively used in
hci_conn_del().
So, considering that hci_conn_del() must not be called twice (which would
fail horribly), we know that hci_conn_put_device() is only called _once_
(which is in hci_conn_del()).
On the other hand, hci_conn_add_sysfs() must not be called twice, either
(it would call device_add twice, which breaks the device, see
drivers/base/core.c). So we know that hci_conn_hold_device() is also
called only once (it's only called directly before hci_conn_add_sysfs()).
So hold and put are known to be called only once. That means we can safely
remove them and directly call hci_conn_del_sysfs() in hci_conn_del().
But there is one issue left: HIDP also uses hci_conn_hold/put_device().
However, this case can be ignored and simply removed as it is totally
broken. The issue is, the only thing HIDP delays with
hci_conn_hold_device() is the removal of the hci_conn->dev from sysfs.
But, the hci_conn device has no mechanism to get notified when its own
parent (hci_dev) gets removed from sysfs. hci_dev_hold/put() does _not_
control when it is removed but only when the device object is created
and destroyed.
And hci_dev calls hci_conn_flush_*() when it removes itself from sysfs,
which itself causes hci_conn_del() to be called, but it does _not_ cause
hci_conn_del_sysfs() to be called, which is wrong.
Hence, we fix it to call hci_conn_del_sysfs() in hci_conn_del(). This
guarantees that a hci_conn object is removed from sysfs _before_ its
parent hci_dev is removed.
The changes to HIDP look scary, wrong and broken. However, if you look at
the HIDP session management, you will notice they're already broken in the
exact _same_ way (ever tried "unplugging" HIDP devices? Breaks _all_ the
time).
So this patch only makes HIDP look _scary_ and _obviously broken_. It does
not break HIDP itself, it already is!
See later patches in this series which fix HIDP to use proper
session-management.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci_core.h | 4 ----
net/bluetooth/hci_conn.c | 17 +----------------
net/bluetooth/hci_event.c | 4 ----
net/bluetooth/hidp/core.c | 20 +++-----------------
4 files changed, 4 insertions(+), 41 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 78ea9c7..5590cc4 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -345,7 +345,6 @@ struct hci_conn {
struct timer_list auto_accept_timer;
struct device dev;
- atomic_t devref;
struct hci_dev *hdev;
void *l2cap_data;
@@ -601,9 +600,6 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
-void hci_conn_hold_device(struct hci_conn *conn);
-void hci_conn_put_device(struct hci_conn *conn);
-
static inline void hci_conn_hold(struct hci_conn *conn)
{
BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 30d7dfc..b6d2831 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -398,8 +398,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
if (hdev->notify)
hdev->notify(hdev, HCI_NOTIFY_CONN_ADD);
- atomic_set(&conn->devref, 0);
-
hci_conn_init_sysfs(conn);
return conn;
@@ -448,7 +446,7 @@ int hci_conn_del(struct hci_conn *conn)
skb_queue_purge(&conn->data_q);
- hci_conn_put_device(conn);
+ hci_conn_del_sysfs(conn);
hci_dev_put(hdev);
@@ -835,19 +833,6 @@ void hci_conn_check_pending(struct hci_dev *hdev)
hci_dev_unlock(hdev);
}
-void hci_conn_hold_device(struct hci_conn *conn)
-{
- atomic_inc(&conn->devref);
-}
-EXPORT_SYMBOL(hci_conn_hold_device);
-
-void hci_conn_put_device(struct hci_conn *conn)
-{
- if (atomic_dec_and_test(&conn->devref))
- hci_conn_del_sysfs(conn);
-}
-EXPORT_SYMBOL(hci_conn_put_device);
-
int hci_get_conn_list(void __user *arg)
{
struct hci_conn *c;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 2cf28b1..b5279a4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1706,7 +1706,6 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
} else
conn->state = BT_CONNECTED;
- hci_conn_hold_device(conn);
hci_conn_add_sysfs(conn);
if (test_bit(HCI_AUTH, &hdev->flags))
@@ -2988,7 +2987,6 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
conn->handle = __le16_to_cpu(ev->handle);
conn->state = BT_CONNECTED;
- hci_conn_hold_device(conn);
hci_conn_add_sysfs(conn);
break;
@@ -3453,7 +3451,6 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev,
hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
hci_conn_drop(hcon);
- hci_conn_hold_device(hcon);
hci_conn_add_sysfs(hcon);
amp_physical_cfm(bredr_hcon, hcon);
@@ -3587,7 +3584,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
conn->handle = __le16_to_cpu(ev->handle);
conn->state = BT_CONNECTED;
- hci_conn_hold_device(conn);
hci_conn_add_sysfs(conn);
hci_proto_connect_cfm(conn, ev->status);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 4ab82cb..9734136 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -73,18 +73,6 @@ static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
return NULL;
}
-static void __hidp_link_session(struct hidp_session *session)
-{
- list_add(&session->list, &hidp_session_list);
-}
-
-static void __hidp_unlink_session(struct hidp_session *session)
-{
- hci_conn_put_device(session->conn);
-
- list_del(&session->list);
-}
-
static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
{
memset(ci, 0, sizeof(*ci));
@@ -760,7 +748,7 @@ static int hidp_session(void *arg)
fput(session->ctrl_sock->file);
- __hidp_unlink_session(session);
+ list_del(&session->list);
up_write(&hidp_session_sem);
@@ -783,8 +771,6 @@ static struct hci_conn *hidp_get_connection(struct hidp_session *session)
hci_dev_lock(hdev);
conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
- if (conn)
- hci_conn_hold_device(conn);
hci_dev_unlock(hdev);
hci_dev_put(hdev);
@@ -1026,7 +1012,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
session->idle_to = req->idle_to;
- __hidp_link_session(session);
+ list_add(&session->list, &hidp_session_list);
if (req->rd_size > 0) {
err = hidp_setup_hid(session, req);
@@ -1106,7 +1092,7 @@ unlink:
session->rd_data = NULL;
purge:
- __hidp_unlink_session(session);
+ list_del(&session->list);
skb_queue_purge(&session->ctrl_transmit);
skb_queue_purge(&session->intr_transmit);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v4 02/16] Bluetooth: remove unneeded hci_conn_hold/put_device()
2013-04-06 18:28 ` [PATCH v4 02/16] Bluetooth: remove unneeded hci_conn_hold/put_device() David Herrmann
@ 2013-04-17 5:39 ` Gustavo Padovan
0 siblings, 0 replies; 44+ messages in thread
From: Gustavo Padovan @ 2013-04-17 5:39 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth
Hi David,
* David Herrmann <dh.herrmann@gmail.com> [2013-04-06 20:28:38 +0200]:
> hci_conn_hold/put_device() is used to control when hci_conn->dev is no
> longer needed and can be deleted from the system. Lets first look how they
> are currently used throughout the code (excluding HIDP!).
>
> All code that uses hci_conn_hold_device() looks like this:
> ...
> hci_conn_hold_device();
> hci_conn_add_sysfs();
> ...
> On the other side, hci_conn_put_device() is exclusively used in
> hci_conn_del().
>
> So, considering that hci_conn_del() must not be called twice (which would
> fail horribly), we know that hci_conn_put_device() is only called _once_
> (which is in hci_conn_del()).
> On the other hand, hci_conn_add_sysfs() must not be called twice, either
> (it would call device_add twice, which breaks the device, see
> drivers/base/core.c). So we know that hci_conn_hold_device() is also
> called only once (it's only called directly before hci_conn_add_sysfs()).
>
> So hold and put are known to be called only once. That means we can safely
> remove them and directly call hci_conn_del_sysfs() in hci_conn_del().
>
> But there is one issue left: HIDP also uses hci_conn_hold/put_device().
> However, this case can be ignored and simply removed as it is totally
> broken. The issue is, the only thing HIDP delays with
> hci_conn_hold_device() is the removal of the hci_conn->dev from sysfs.
> But, the hci_conn device has no mechanism to get notified when its own
> parent (hci_dev) gets removed from sysfs. hci_dev_hold/put() does _not_
> control when it is removed but only when the device object is created
> and destroyed.
> And hci_dev calls hci_conn_flush_*() when it removes itself from sysfs,
> which itself causes hci_conn_del() to be called, but it does _not_ cause
> hci_conn_del_sysfs() to be called, which is wrong.
>
> Hence, we fix it to call hci_conn_del_sysfs() in hci_conn_del(). This
> guarantees that a hci_conn object is removed from sysfs _before_ its
> parent hci_dev is removed.
>
> The changes to HIDP look scary, wrong and broken. However, if you look at
> the HIDP session management, you will notice they're already broken in the
> exact _same_ way (ever tried "unplugging" HIDP devices? Breaks _all_ the
> time).
> So this patch only makes HIDP look _scary_ and _obviously broken_. It does
> not break HIDP itself, it already is!
>
> See later patches in this series which fix HIDP to use proper
> session-management.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> ---
> include/net/bluetooth/hci_core.h | 4 ----
> net/bluetooth/hci_conn.c | 17 +----------------
> net/bluetooth/hci_event.c | 4 ----
> net/bluetooth/hidp/core.c | 20 +++-----------------
> 4 files changed, 4 insertions(+), 41 deletions(-)
Patch has been applied to bluetooth-next. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 03/16] Bluetooth: introduce hci_conn ref-counting
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
2013-04-06 18:28 ` [PATCH v4 01/16] Bluetooth: rename hci_conn_put to hci_conn_drop David Herrmann
2013-04-06 18:28 ` [PATCH v4 02/16] Bluetooth: remove unneeded hci_conn_hold/put_device() David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 04/16] Bluetooth: hidp: remove unused session->state field David Herrmann
` (12 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
We currently do not allow using hci_conn from outside of HCI-core.
However, several other users could make great use of it. This includes
HIDP, rfcomm and all other sub-protocols that rely on an active
connection.
Hence, we now introduce hci_conn ref-counting. We currently never call
get_device(). put_device() is exclusively used in hci_conn_del_sysfs().
Hence, we currently never have a greater device-refcnt than 1.
Therefore, it is safe to move the put_device() call from
hci_conn_del_sysfs() to hci_conn_del() (it's the only caller). In fact,
this even fixes a "use-after-free" bug as we access hci_conn after calling
hci_conn_del_sysfs() in hci_conn_del().
>From now on we can add references to hci_conn objects in other layers
(like l2cap_sock, HIDP, rfcomm, ...) and grab a reference via
hci_conn_get(). This does _not_ guarantee, that the connection is still
alive. But, this isn't what we want. We can simply lock the hci_conn
device and use "device_is_registered(hci_conn->dev)" to test that.
However, this is hardly necessary as outside users should never rely on
the HCI connection to be alive, anyway. Instead, they should solely rely
on the device-object to be available.
But if sub-devices want the hci_conn object as sysfs parent, they need to
be notified when the connection drops. This will be introduced in later
patches with l2cap_users.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/hci_core.h | 31 +++++++++++++++++++++++++++++++
net/bluetooth/hci_conn.c | 3 +--
net/bluetooth/hci_sysfs.c | 1 -
3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5590cc4..d324b11 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -600,6 +600,37 @@ int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
+/*
+ * hci_conn_get() and hci_conn_put() are used to control the life-time of an
+ * "hci_conn" object. They do not guarantee that the hci_conn object is running,
+ * working or anything else. They just guarantee that the object is available
+ * and can be dereferenced. So you can use its locks, local variables and any
+ * other constant data.
+ * Before accessing runtime data, you _must_ lock the object and then check that
+ * it is still running. As soon as you release the locks, the connection might
+ * get dropped, though.
+ *
+ * On the other hand, hci_conn_hold() and hci_conn_drop() are used to control
+ * how long the underlying connection is held. So every channel that runs on the
+ * hci_conn object calls this to prevent the connection from disappearing. As
+ * long as you hold a device, you must also guarantee that you have a valid
+ * reference to the device via hci_conn_get() (or the initial reference from
+ * hci_conn_add()).
+ * The hold()/drop() ref-count is known to drop below 0 sometimes, which doesn't
+ * break because nobody cares for that. But this means, we cannot use
+ * _get()/_drop() in it, but require the caller to have a valid ref (FIXME).
+ */
+
+static inline void hci_conn_get(struct hci_conn *conn)
+{
+ get_device(&conn->dev);
+}
+
+static inline void hci_conn_put(struct hci_conn *conn)
+{
+ put_device(&conn->dev);
+}
+
static inline void hci_conn_hold(struct hci_conn *conn)
{
BT_DBG("hcon %p orig refcnt %d", conn, atomic_read(&conn->refcnt));
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b6d2831..1f5f737 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -450,8 +450,7 @@ int hci_conn_del(struct hci_conn *conn)
hci_dev_put(hdev);
- if (conn->handle == 0)
- kfree(conn);
+ hci_conn_put(conn);
return 0;
}
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index ff38561..6fe15c8 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -146,7 +146,6 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
}
device_del(&conn->dev);
- put_device(&conn->dev);
hci_dev_put(hdev);
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 04/16] Bluetooth: hidp: remove unused session->state field
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (2 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 03/16] Bluetooth: introduce hci_conn ref-counting David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 05/16] Bluetooth: hidp: test "terminate" before sleeping David Herrmann
` (11 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
This field is always BT_CONNECTED. Remove it and set it to BT_CONNECTED in
hidp_copy_session() unconditionally.
Also note that this field is totally bogus. Userspace can query an
hidp-session for its state. However, whenever user-space queries us, this
field should be BT_CONNECTED. If it wasn't BT_CONNECTED, then we would be
currently cleaning up the session and the session itself would exit in the
next few milliseconds. Hence, there is no reason to let user-space know
that the session will exit now if they cannot make _any_ use of that.
Thus, remove the field and let user-space think that a session is always
BT_CONNECTED as long as they can query it.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 5 ++---
net/bluetooth/hidp/hidp.h | 1 -
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 9734136..22e9ab1 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -79,7 +79,7 @@ static void __hidp_copy_session(struct hidp_session *session, struct hidp_connin
bacpy(&ci->bdaddr, &session->bdaddr);
ci->flags = session->flags;
- ci->state = session->state;
+ ci->state = BT_CONNECTED;
ci->vendor = 0x0000;
ci->product = 0x0000;
@@ -970,7 +970,7 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
down_write(&hidp_session_sem);
s = __hidp_get_session(&bt_sk(ctrl_sock->sk)->dst);
- if (s && s->state == BT_CONNECTED) {
+ if (s) {
up_write(&hidp_session_sem);
return -EEXIST;
}
@@ -992,7 +992,6 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
session->ctrl_sock = ctrl_sock;
session->intr_sock = intr_sock;
- session->state = BT_CONNECTED;
session->conn = hidp_get_connection(session);
if (!session->conn) {
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index af1bcc8..57a6191 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -135,7 +135,6 @@ struct hidp_session {
bdaddr_t bdaddr;
- unsigned long state;
unsigned long flags;
unsigned long idle_to;
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 05/16] Bluetooth: hidp: test "terminate" before sleeping
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (3 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 04/16] Bluetooth: hidp: remove unused session->state field David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 06/16] Bluetooth: allow constant arguments for bacmp()/bacpy() David Herrmann
` (10 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
The "terminate" flag is guaranteed to be set before the session terminates
and the handlers are woken up. Hence, we need to add it to the
sleep-condition.
Note that testing the flags is not enough as nothing prevents us from
setting the flags again after the session-handler terminated.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 22e9ab1..e01a924 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -330,11 +330,13 @@ static int hidp_get_raw_report(struct hid_device *hid,
/* Wait for the return of the report. The returned report
gets put in session->report_return. */
- while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
+ while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
+ !atomic_read(&session->terminate)) {
int res;
res = wait_event_interruptible_timeout(session->report_queue,
- !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
+ !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)
+ || atomic_read(&session->terminate),
5*HZ);
if (res == 0) {
/* timeout */
@@ -399,11 +401,13 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
goto err;
/* Wait for the ACK from the device. */
- while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)) {
+ while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
+ !atomic_read(&session->terminate)) {
int res;
res = wait_event_interruptible_timeout(session->report_queue,
- !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags),
+ !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
+ || atomic_read(&session->terminate),
10*HZ);
if (res == 0) {
/* timeout */
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 06/16] Bluetooth: allow constant arguments for bacmp()/bacpy()
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (4 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 05/16] Bluetooth: hidp: test "terminate" before sleeping David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 07/16] Bluetooth: hidp: move hidp_schedule() to core.c David Herrmann
` (9 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
There is no reason to require the source arguments to be writeable so fix
this to allow constant source addresses.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/bluetooth.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 591fee7..6912ef9 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -193,11 +193,11 @@ static inline bool bdaddr_type_is_le(__u8 type)
#define BDADDR_LOCAL (&(bdaddr_t) {{0, 0, 0, 0xff, 0xff, 0xff} })
/* Copy, swap, convert BD Address */
-static inline int bacmp(bdaddr_t *ba1, bdaddr_t *ba2)
+static inline int bacmp(const bdaddr_t *ba1, const bdaddr_t *ba2)
{
return memcmp(ba1, ba2, sizeof(bdaddr_t));
}
-static inline void bacpy(bdaddr_t *dst, bdaddr_t *src)
+static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src)
{
memcpy(dst, src, sizeof(bdaddr_t));
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 07/16] Bluetooth: hidp: move hidp_schedule() to core.c
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (5 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 06/16] Bluetooth: allow constant arguments for bacmp()/bacpy() David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 08/16] Bluetooth: l2cap: introduce l2cap_conn ref-counting David Herrmann
` (8 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
There is no reason to keep this helper in the header file. No other file
depends on it so move it into hidp/core.c where it belongs.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 9 +++++++++
net/bluetooth/hidp/hidp.h | 9 ---------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index e01a924..cef1021 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -59,6 +59,15 @@ static unsigned char hidp_keycode[256] = {
static unsigned char hidp_mkeyspat[] = { 0x01, 0x01, 0x01, 0x01, 0x01, 0x01 };
+static inline void hidp_schedule(struct hidp_session *session)
+{
+ struct sock *ctrl_sk = session->ctrl_sock->sk;
+ struct sock *intr_sk = session->intr_sock->sk;
+
+ wake_up_interruptible(sk_sleep(ctrl_sk));
+ wake_up_interruptible(sk_sleep(intr_sk));
+}
+
static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
{
struct hidp_session *session;
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index 57a6191..c844420 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -174,15 +174,6 @@ struct hidp_session {
int waiting_for_startup;
};
-static inline void hidp_schedule(struct hidp_session *session)
-{
- struct sock *ctrl_sk = session->ctrl_sock->sk;
- struct sock *intr_sk = session->intr_sock->sk;
-
- wake_up_interruptible(sk_sleep(ctrl_sk));
- wake_up_interruptible(sk_sleep(intr_sk));
-}
-
/* HIDP init defines */
extern int __init hidp_init_sockets(void);
extern void __exit hidp_cleanup_sockets(void);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 08/16] Bluetooth: l2cap: introduce l2cap_conn ref-counting
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (6 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 07/16] Bluetooth: hidp: move hidp_schedule() to core.c David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 09/16] Bluetooth: l2cap: add l2cap_user sub-modules David Herrmann
` (7 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
If we want to use l2cap_conn outside of l2cap_core.c, we need refcounting
for these objects. Otherwise, we cannot synchronize l2cap locks with
outside locks and end up with deadlocks.
Hence, introduce ref-counting for l2cap_conn objects. This doesn't affect
l2cap internals at all, as they use a direct synchronization.
We also keep a reference to the parent hci_conn for locking purposes as
l2cap_conn depends on this. This doesn't affect the connection itself but
only the lifetime of the (dead) object.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/l2cap.h | 4 ++++
net/bluetooth/l2cap_core.c | 25 ++++++++++++++++++++++++-
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 278830e..7b4cc5b 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -583,6 +583,7 @@ struct l2cap_conn {
struct list_head chan_l;
struct mutex chan_lock;
+ struct kref ref;
};
#define L2CAP_INFO_CL_MTU_REQ_SENT 0x01
@@ -813,4 +814,7 @@ void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
u8 status);
void __l2cap_physical_cfm(struct l2cap_chan *chan, int result);
+void l2cap_conn_get(struct l2cap_conn *conn);
+void l2cap_conn_put(struct l2cap_conn *conn);
+
#endif /* __L2CAP_H */
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 7cdb93c..5f273a3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1486,7 +1486,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
}
hcon->l2cap_data = NULL;
- kfree(conn);
+ conn->hchan = NULL;
+ l2cap_conn_put(conn);
}
static void security_timeout(struct work_struct *work)
@@ -1520,8 +1521,10 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
return NULL;
}
+ kref_init(&conn->ref);
hcon->l2cap_data = conn;
conn->hcon = hcon;
+ hci_conn_get(conn->hcon);
conn->hchan = hchan;
BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);
@@ -1558,6 +1561,26 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
return conn;
}
+static void l2cap_conn_free(struct kref *ref)
+{
+ struct l2cap_conn *conn = container_of(ref, struct l2cap_conn, ref);
+
+ hci_conn_put(conn->hcon);
+ kfree(conn);
+}
+
+void l2cap_conn_get(struct l2cap_conn *conn)
+{
+ kref_get(&conn->ref);
+}
+EXPORT_SYMBOL(l2cap_conn_get);
+
+void l2cap_conn_put(struct l2cap_conn *conn)
+{
+ kref_put(&conn->ref, l2cap_conn_free);
+}
+EXPORT_SYMBOL(l2cap_conn_put);
+
/* ---- Socket interface ---- */
/* Find socket with psm and source / destination bdaddr.
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 09/16] Bluetooth: l2cap: add l2cap_user sub-modules
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (7 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 08/16] Bluetooth: l2cap: introduce l2cap_conn ref-counting David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 10/16] Bluetooth: hidp: add new session-management helpers David Herrmann
` (6 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
Several sub-modules like HIDP, rfcomm, ... need to track l2cap
connections. The l2cap_conn->hcon->dev object is used as parent for sysfs
devices so the sub-modules need to be notified when the hci_conn object is
removed from sysfs.
As submodules normally use the l2cap layer, the l2cap_user objects are
registered there instead of on the underlying hci_conn object. This avoids
any direct dependency on the HCI layer and lets the l2cap core handle any
specifics.
This patch introduces l2cap_user objects which contain a "probe" and
"remove" callback. You can register them on any l2cap_conn object and if
it is active, the "probe" callback will get called. Otherwise, an error is
returned.
The l2cap_conn object will call your "remove" callback directly before it
is removed from user-space. This allows you to remove your submodules
_before_ the parent l2cap_conn and hci_conn object is removed.
At any time you can asynchronously unregister your l2cap_user object if
your submodule vanishes before the l2cap_conn object does.
There is no way around l2cap_user. If we want wire-protocols in the
kernel, we always want the hci_conn object as parent in the sysfs tree. We
cannot use a channel here since we might need multiple channels for a
single protocol.
But the problem is, we _must_ get notified when an l2cap_conn object is
removed. We cannot use reference-counting for object-removal! This is not
how it works. If a hardware is removed, we should immediately remove the
object from sysfs. Any other behavior would be inconsistent with the rest
of the system. Also note that device_del() might sleep, but it doesn't
wait for user-space or block very long. It only _unlinks_ the object from
sysfs and the whole device-tree. Everything else is handled by ref-counts!
This is exactly what the other sub-modules must do: unlink their devices
when the "remove" l2cap_user callback is called. They should not do any
cleanup or synchronous shutdowns.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
include/net/bluetooth/l2cap.h | 10 +++++
net/bluetooth/l2cap_core.c | 86 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 96 insertions(+)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 7b4cc5b..fb94cf1 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -584,6 +584,13 @@ struct l2cap_conn {
struct list_head chan_l;
struct mutex chan_lock;
struct kref ref;
+ struct list_head users;
+};
+
+struct l2cap_user {
+ struct list_head list;
+ int (*probe) (struct l2cap_conn *conn, struct l2cap_user *user);
+ void (*remove) (struct l2cap_conn *conn, struct l2cap_user *user);
};
#define L2CAP_INFO_CL_MTU_REQ_SENT 0x01
@@ -817,4 +824,7 @@ void __l2cap_physical_cfm(struct l2cap_chan *chan, int result);
void l2cap_conn_get(struct l2cap_conn *conn);
void l2cap_conn_put(struct l2cap_conn *conn);
+int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user);
+void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user);
+
#endif /* __L2CAP_H */
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5f273a3..e7c06d1 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1446,6 +1446,89 @@ static void l2cap_info_timeout(struct work_struct *work)
l2cap_conn_start(conn);
}
+/*
+ * l2cap_user
+ * External modules can register l2cap_user objects on l2cap_conn. The ->probe
+ * callback is called during registration. The ->remove callback is called
+ * during unregistration.
+ * An l2cap_user object can either be explicitly unregistered or when the
+ * underlying l2cap_conn object is deleted. This guarantees that l2cap->hcon,
+ * l2cap->hchan, .. are valid as long as the remove callback hasn't been called.
+ * External modules must own a reference to the l2cap_conn object if they intend
+ * to call l2cap_unregister_user(). The l2cap_conn object might get destroyed at
+ * any time if they don't.
+ */
+
+int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
+{
+ struct hci_dev *hdev = conn->hcon->hdev;
+ int ret;
+
+ /* We need to check whether l2cap_conn is registered. If it is not, we
+ * must not register the l2cap_user. l2cap_conn_del() is unregisters
+ * l2cap_conn objects, but doesn't provide its own locking. Instead, it
+ * relies on the parent hci_conn object to be locked. This itself relies
+ * on the hci_dev object to be locked. So we must lock the hci device
+ * here, too. */
+
+ hci_dev_lock(hdev);
+
+ if (user->list.next || user->list.prev) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ /* conn->hchan is NULL after l2cap_conn_del() was called */
+ if (!conn->hchan) {
+ ret = -ENODEV;
+ goto out_unlock;
+ }
+
+ ret = user->probe(conn, user);
+ if (ret)
+ goto out_unlock;
+
+ list_add(&user->list, &conn->users);
+ ret = 0;
+
+out_unlock:
+ hci_dev_unlock(hdev);
+ return ret;
+}
+EXPORT_SYMBOL(l2cap_register_user);
+
+void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
+{
+ struct hci_dev *hdev = conn->hcon->hdev;
+
+ hci_dev_lock(hdev);
+
+ if (!user->list.next || !user->list.prev)
+ goto out_unlock;
+
+ list_del(&user->list);
+ user->list.next = NULL;
+ user->list.prev = NULL;
+ user->remove(conn, user);
+
+out_unlock:
+ hci_dev_unlock(hdev);
+}
+EXPORT_SYMBOL(l2cap_unregister_user);
+
+static void l2cap_unregister_all_users(struct l2cap_conn *conn)
+{
+ struct l2cap_user *user;
+
+ while (!list_empty(&conn->users)) {
+ user = list_first_entry(&conn->users, struct l2cap_user, list);
+ list_del(&user->list);
+ user->list.next = NULL;
+ user->list.prev = NULL;
+ user->remove(conn, user);
+ }
+}
+
static void l2cap_conn_del(struct hci_conn *hcon, int err)
{
struct l2cap_conn *conn = hcon->l2cap_data;
@@ -1458,6 +1541,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
kfree_skb(conn->rx_skb);
+ l2cap_unregister_all_users(conn);
+
mutex_lock(&conn->chan_lock);
/* Kill channels */
@@ -1550,6 +1635,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
mutex_init(&conn->chan_lock);
INIT_LIST_HEAD(&conn->chan_l);
+ INIT_LIST_HEAD(&conn->users);
if (hcon->type == LE_LINK)
INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 10/16] Bluetooth: hidp: add new session-management helpers
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (8 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 09/16] Bluetooth: l2cap: add l2cap_user sub-modules David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 11/16] Bluetooth: hidp: remove old session-management David Herrmann
` (5 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
This is a rewrite of the HIDP session management. It implements HIDP as an
l2cap_user sub-module so we get proper notification when the underlying
connection goes away.
The helpers are not yet used but only added in this commit. The old
session management is still used and will be removed in a following patch.
The old session-management was flawed. Hotplugging is horribly broken and
we have no way of getting notified when the underlying connection goes
down. The whole idea of removing the HID/input sub-devices from within the
session itself is broken and suffers from major dead-locks. We never can
guarantee that the session can unregister itself as long as we use
synchronous shutdowns. This can only work with asynchronous shutdowns.
However, in this case we _must_ be able to unregister the session from the
outside as otherwise the l2cap_conn object might be unlinked before we
are.
The new session-management is based on l2cap_user. There is only one
way how to add a session and how to delete a session: "probe" and "remove"
callbacks from l2cap_user.
This guarantees that the session can be registered and unregistered at
_any_ time without any synchronous shutdown.
On the other hand, much work has been put into proper session-refcounting.
We can unregister/unlink the session only if we can guarantee that it will
stay alive. But for asynchronous shutdowns we never know when the last
user goes away so we must use proper ref-counting.
The old ->conn field has been renamed to ->hconn so we can reuse ->conn in
the new session management. No other existing HIDP code is modified.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 555 +++++++++++++++++++++++++++++++++++++++++++++-
net/bluetooth/hidp/hidp.h | 53 +++--
2 files changed, 583 insertions(+), 25 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index cef1021..8d30a33 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -1,6 +1,7 @@
/*
HIDP implementation for Linux Bluetooth stack (BlueZ).
Copyright (C) 2003-2004 Marcel Holtmann <marcel@holtmann.org>
+ Copyright (C) 2013 David Herrmann <dh.herrmann@gmail.com>
This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License version 2 as
@@ -20,6 +21,7 @@
SOFTWARE IS DISCLAIMED.
*/
+#include <linux/kref.h>
#include <linux/module.h>
#include <linux/file.h>
#include <linux/kthread.h>
@@ -59,6 +61,13 @@ static unsigned char hidp_keycode[256] = {
static unsigned char hidp_mkeyspat[] = { 0x01, 0x01, 0x01, 0x01, 0x01, 0x01 };
+static int hidp_session_probe(struct l2cap_conn *conn,
+ struct l2cap_user *user);
+static void hidp_session_remove(struct l2cap_conn *conn,
+ struct l2cap_user *user);
+static int hidp_session_thread(void *arg);
+static void hidp_session_terminate(struct hidp_session *s);
+
static inline void hidp_schedule(struct hidp_session *session)
{
struct sock *ctrl_sk = session->ctrl_sock->sk;
@@ -838,7 +847,7 @@ static int hidp_setup_input(struct hidp_session *session,
input->relbit[0] |= BIT_MASK(REL_WHEEL);
}
- input->dev.parent = &session->conn->dev;
+ input->dev.parent = &session->hconn->dev;
input->event = hidp_input_event;
@@ -942,7 +951,7 @@ static int hidp_setup_hid(struct hidp_session *session,
snprintf(hid->uniq, sizeof(hid->uniq), "%pMR",
&bt_sk(session->ctrl_sock->sk)->dst);
- hid->dev.parent = &session->conn->dev;
+ hid->dev.parent = &session->hconn->dev;
hid->ll_driver = &hidp_hid_driver;
hid->hid_get_raw_report = hidp_get_raw_report;
@@ -964,6 +973,543 @@ fault:
return err;
}
+/* initialize session devices */
+static int hidp_session_dev_init(struct hidp_session *session,
+ struct hidp_connadd_req *req)
+{
+ int ret;
+
+ if (req->rd_size > 0) {
+ ret = hidp_setup_hid(session, req);
+ if (ret && ret != -ENODEV)
+ return ret;
+ }
+
+ if (!session->hid) {
+ ret = hidp_setup_input(session, req);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+/* destroy session devices */
+static void hidp_session_dev_destroy(struct hidp_session *session)
+{
+ if (session->hid)
+ put_device(&session->hid->dev);
+ else if (session->input)
+ input_put_device(session->input);
+
+ kfree(session->rd_data);
+ session->rd_data = NULL;
+}
+
+/* add HID/input devices to their underlying bus systems */
+static int hidp_session_dev_add(struct hidp_session *session)
+{
+ int ret;
+
+ /* Both HID and input systems drop a ref-count when unregistering the
+ * device but they don't take a ref-count when registering them. Work
+ * around this by explicitly taking a refcount during registration
+ * which is dropped automatically by unregistering the devices. */
+
+ if (session->hid) {
+ ret = hid_add_device(session->hid);
+ if (ret)
+ return ret;
+ get_device(&session->hid->dev);
+ } else if (session->input) {
+ ret = input_register_device(session->input);
+ if (ret)
+ return ret;
+ input_get_device(session->input);
+ }
+
+ return 0;
+}
+
+/* remove HID/input devices from their bus systems */
+static void hidp_session_dev_del(struct hidp_session *session)
+{
+ if (session->hid)
+ hid_destroy_device(session->hid);
+ else if (session->input)
+ input_unregister_device(session->input);
+}
+
+/*
+ * Create new session object
+ * Allocate session object, initialize static fields, copy input data into the
+ * object and take a reference to all sub-objects.
+ * This returns 0 on success and puts a pointer to the new session object in
+ * \out. Otherwise, an error code is returned.
+ * The new session object has an initial ref-count of 1.
+ */
+static int hidp_session_new(struct hidp_session **out, const bdaddr_t *bdaddr,
+ struct socket *ctrl_sock,
+ struct socket *intr_sock,
+ struct hidp_connadd_req *req,
+ struct l2cap_conn *conn)
+{
+ struct hidp_session *session;
+ int ret;
+ struct bt_sock *ctrl, *intr;
+
+ ctrl = bt_sk(ctrl_sock->sk);
+ intr = bt_sk(intr_sock->sk);
+
+ session = kzalloc(sizeof(*session), GFP_KERNEL);
+ if (!session)
+ return -ENOMEM;
+
+ /* object and runtime management */
+ kref_init(&session->ref);
+ atomic_set(&session->state, HIDP_SESSION_IDLING);
+ init_waitqueue_head(&session->state_queue);
+ session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
+
+ /* connection management */
+ bacpy(&session->bdaddr, bdaddr);
+ session->conn = conn;
+ session->user.probe = hidp_session_probe;
+ session->user.remove = hidp_session_remove;
+ session->ctrl_sock = ctrl_sock;
+ session->intr_sock = intr_sock;
+ skb_queue_head_init(&session->ctrl_transmit);
+ skb_queue_head_init(&session->intr_transmit);
+ session->ctrl_mtu = min_t(uint, l2cap_pi(ctrl)->chan->omtu,
+ l2cap_pi(ctrl)->chan->imtu);
+ session->intr_mtu = min_t(uint, l2cap_pi(intr)->chan->omtu,
+ l2cap_pi(intr)->chan->imtu);
+ session->idle_to = req->idle_to;
+
+ /* device management */
+ setup_timer(&session->timer, hidp_idle_timeout,
+ (unsigned long)session);
+
+ /* session data */
+ mutex_init(&session->report_mutex);
+ init_waitqueue_head(&session->report_queue);
+
+ ret = hidp_session_dev_init(session, req);
+ if (ret)
+ goto err_free;
+
+ l2cap_conn_get(session->conn);
+ get_file(session->intr_sock->file);
+ get_file(session->ctrl_sock->file);
+ *out = session;
+ return 0;
+
+err_free:
+ kfree(session);
+ return ret;
+}
+
+/* increase ref-count of the given session by one */
+static void hidp_session_get(struct hidp_session *session)
+{
+ kref_get(&session->ref);
+}
+
+/* release callback */
+static void session_free(struct kref *ref)
+{
+ struct hidp_session *session = container_of(ref, struct hidp_session,
+ ref);
+
+ hidp_session_dev_destroy(session);
+ skb_queue_purge(&session->ctrl_transmit);
+ skb_queue_purge(&session->intr_transmit);
+ fput(session->intr_sock->file);
+ fput(session->ctrl_sock->file);
+ l2cap_conn_put(session->conn);
+ kfree(session);
+}
+
+/* decrease ref-count of the given session by one */
+static void hidp_session_put(struct hidp_session *session)
+{
+ kref_put(&session->ref, session_free);
+}
+
+/*
+ * Search the list of active sessions for a session with target address
+ * \bdaddr. You must hold at least a read-lock on \hidp_session_sem. As long as
+ * you do not release this lock, the session objects cannot vanish and you can
+ * safely take a reference to the session yourself.
+ */
+static struct hidp_session *__hidp_session_find(const bdaddr_t *bdaddr)
+{
+ struct hidp_session *session;
+
+ list_for_each_entry(session, &hidp_session_list, list) {
+ if (!bacmp(bdaddr, &session->bdaddr))
+ return session;
+ }
+
+ return NULL;
+}
+
+/*
+ * Same as __hidp_session_find() but no locks must be held. This also takes a
+ * reference of the returned session (if non-NULL) so you must drop this
+ * reference if you no longer use the object.
+ */
+static struct hidp_session *hidp_session_find(const bdaddr_t *bdaddr)
+{
+ struct hidp_session *session;
+
+ down_read(&hidp_session_sem);
+
+ session = __hidp_session_find(bdaddr);
+ if (session)
+ hidp_session_get(session);
+
+ up_read(&hidp_session_sem);
+
+ return session;
+}
+
+/*
+ * Start session synchronously
+ * This starts a session thread and waits until initialization
+ * is done or returns an error if it couldn't be started.
+ * If this returns 0 the session thread is up and running. You must call
+ * hipd_session_stop_sync() before deleting any runtime resources.
+ */
+static int hidp_session_start_sync(struct hidp_session *session)
+{
+ unsigned int vendor, product;
+
+ if (session->hid) {
+ vendor = session->hid->vendor;
+ product = session->hid->product;
+ } else if (session->input) {
+ vendor = session->input->id.vendor;
+ product = session->input->id.product;
+ } else {
+ vendor = 0x0000;
+ product = 0x0000;
+ }
+
+ session->task = kthread_run(hidp_session_thread, session,
+ "khidpd_%04x%04x", vendor, product);
+ if (IS_ERR(session->task))
+ return PTR_ERR(session->task);
+
+ while (atomic_read(&session->state) <= HIDP_SESSION_IDLING)
+ wait_event(session->state_queue,
+ atomic_read(&session->state) > HIDP_SESSION_IDLING);
+
+ return 0;
+}
+
+/*
+ * Terminate session thread
+ * Wake up session thread and notify it to stop. This is asynchronous and
+ * returns immediately. Call this whenever a runtime error occurs and you want
+ * the session to stop.
+ * Note: wake_up_process() performs any necessary memory-barriers for us.
+ */
+static void hidp_session_terminate(struct hidp_session *session)
+{
+ atomic_inc(&session->terminate);
+ wake_up_process(session->task);
+}
+
+/*
+ * Probe HIDP session
+ * This is called from the l2cap_conn core when our l2cap_user object is bound
+ * to the hci-connection. We get the session via the \user object and can now
+ * start the session thread, register the HID/input devices and link it into
+ * the global session list.
+ * The global session-list owns its own reference to the session object so you
+ * can drop your own reference after registering the l2cap_user object.
+ */
+static int hidp_session_probe(struct l2cap_conn *conn,
+ struct l2cap_user *user)
+{
+ struct hidp_session *session = container_of(user,
+ struct hidp_session,
+ user);
+ struct hidp_session *s;
+ int ret;
+
+ down_write(&hidp_session_sem);
+
+ /* check that no other session for this device exists */
+ s = __hidp_session_find(&session->bdaddr);
+ if (s) {
+ ret = -EEXIST;
+ goto out_unlock;
+ }
+
+ ret = hidp_session_start_sync(session);
+ if (ret)
+ goto out_unlock;
+
+ ret = hidp_session_dev_add(session);
+ if (ret)
+ goto out_stop;
+
+ hidp_session_get(session);
+ list_add(&session->list, &hidp_session_list);
+ ret = 0;
+ goto out_unlock;
+
+out_stop:
+ hidp_session_terminate(session);
+out_unlock:
+ up_write(&hidp_session_sem);
+ return ret;
+}
+
+/*
+ * Remove HIDP session
+ * Called from the l2cap_conn core when either we explicitly unregistered
+ * the l2cap_user object or if the underlying connection is shut down.
+ * We signal the hidp-session thread to shut down, unregister the HID/input
+ * devices and unlink the session from the global list.
+ * This drops the reference to the session that is owned by the global
+ * session-list.
+ * Note: We _must_ not synchronosly wait for the session-thread to shut down.
+ * This is, because the session-thread might be waiting for an HCI lock that is
+ * held while we are called. Therefore, we only unregister the devices and
+ * notify the session-thread to terminate. The thread itself owns a reference
+ * to the session object so it can safely shut down.
+ */
+static void hidp_session_remove(struct l2cap_conn *conn,
+ struct l2cap_user *user)
+{
+ struct hidp_session *session = container_of(user,
+ struct hidp_session,
+ user);
+
+ down_write(&hidp_session_sem);
+
+ hidp_session_terminate(session);
+ hidp_session_dev_del(session);
+ list_del(&session->list);
+
+ up_write(&hidp_session_sem);
+
+ hidp_session_put(session);
+}
+
+/*
+ * Session Worker
+ * This performs the actual main-loop of the HIDP worker. We first check
+ * whether the underlying connection is still alive, then parse all pending
+ * messages and finally send all outstanding messages.
+ */
+static void hidp_session_run(struct hidp_session *session)
+{
+ struct sock *ctrl_sk = session->ctrl_sock->sk;
+ struct sock *intr_sk = session->intr_sock->sk;
+ struct sk_buff *skb;
+
+ for (;;) {
+ /*
+ * This thread can be woken up two ways:
+ * - You call hidp_session_terminate() which sets the
+ * session->terminate flag and wakes this thread up.
+ * - Via modifying the socket state of ctrl/intr_sock. This
+ * thread is woken up by ->sk_state_changed().
+ *
+ * Note: set_current_state() performs any necessary
+ * memory-barriers for us.
+ */
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ if (atomic_read(&session->terminate))
+ break;
+
+ if (ctrl_sk->sk_state != BT_CONNECTED ||
+ intr_sk->sk_state != BT_CONNECTED)
+ break;
+
+ /* parse incoming intr-skbs */
+ while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
+ skb_orphan(skb);
+ if (!skb_linearize(skb))
+ hidp_recv_intr_frame(session, skb);
+ else
+ kfree_skb(skb);
+ }
+
+ /* send pending intr-skbs */
+ hidp_process_intr_transmit(session);
+
+ /* parse incoming ctrl-skbs */
+ while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
+ skb_orphan(skb);
+ if (!skb_linearize(skb))
+ hidp_recv_ctrl_frame(session, skb);
+ else
+ kfree_skb(skb);
+ }
+
+ /* send pending ctrl-skbs */
+ hidp_process_ctrl_transmit(session);
+
+ schedule();
+ }
+
+ atomic_inc(&session->terminate);
+ set_current_state(TASK_RUNNING);
+}
+
+/*
+ * HIDP session thread
+ * This thread runs the I/O for a single HIDP session. Startup is synchronous
+ * which allows us to take references to ourself here instead of doing that in
+ * the caller.
+ * When we are ready to run we notify the caller and call hidp_session_run().
+ */
+static int hidp_session_thread(void *arg)
+{
+ struct hidp_session *session = arg;
+ wait_queue_t ctrl_wait, intr_wait;
+
+ BT_DBG("session %p", session);
+
+ /* initialize runtime environment */
+ hidp_session_get(session);
+ __module_get(THIS_MODULE);
+ set_user_nice(current, -15);
+ hidp_set_timer(session);
+
+ init_waitqueue_entry(&ctrl_wait, current);
+ init_waitqueue_entry(&intr_wait, current);
+ add_wait_queue(sk_sleep(session->ctrl_sock->sk), &ctrl_wait);
+ add_wait_queue(sk_sleep(session->intr_sock->sk), &intr_wait);
+ /* This memory barrier is paired with wq_has_sleeper(). See
+ * sock_poll_wait() for more information why this is needed. */
+ smp_mb();
+
+ /* notify synchronous startup that we're ready */
+ atomic_inc(&session->state);
+ wake_up(&session->state_queue);
+
+ /* run session */
+ hidp_session_run(session);
+
+ /* cleanup runtime environment */
+ remove_wait_queue(sk_sleep(session->intr_sock->sk), &intr_wait);
+ remove_wait_queue(sk_sleep(session->intr_sock->sk), &ctrl_wait);
+ wake_up_interruptible(&session->report_queue);
+ hidp_del_timer(session);
+
+ /*
+ * If we stopped ourself due to any internal signal, we should try to
+ * unregister our own session here to avoid having it linger until the
+ * parent l2cap_conn dies or user-space cleans it up.
+ * This does not deadlock as we don't do any synchronous shutdown.
+ * Instead, this call has the same semantics as if user-space tried to
+ * delete the session.
+ */
+ l2cap_unregister_user(session->conn, &session->user);
+ hidp_session_put(session);
+
+ module_put_and_exit(0);
+ return 0;
+}
+
+static int hidp_verify_sockets(struct socket *ctrl_sock,
+ struct socket *intr_sock)
+{
+ struct bt_sock *ctrl, *intr;
+ struct hidp_session *session;
+
+ if (!l2cap_is_socket(ctrl_sock) || !l2cap_is_socket(intr_sock))
+ return -EINVAL;
+
+ ctrl = bt_sk(ctrl_sock->sk);
+ intr = bt_sk(intr_sock->sk);
+
+ if (bacmp(&ctrl->src, &intr->src) || bacmp(&ctrl->dst, &intr->dst))
+ return -ENOTUNIQ;
+ if (ctrl->sk.sk_state != BT_CONNECTED ||
+ intr->sk.sk_state != BT_CONNECTED)
+ return -EBADFD;
+
+ /* early session check, we check again during session registration */
+ session = hidp_session_find(&ctrl->dst);
+ if (session) {
+ hidp_session_put(session);
+ return -EEXIST;
+ }
+
+ return 0;
+}
+
+int hidp_connection_add(struct hidp_connadd_req *req,
+ struct socket *ctrl_sock,
+ struct socket *intr_sock)
+{
+ struct hidp_session *session;
+ struct l2cap_conn *conn;
+ struct l2cap_chan *chan = l2cap_pi(ctrl_sock->sk)->chan;
+ int ret;
+
+ ret = hidp_verify_sockets(ctrl_sock, intr_sock);
+ if (ret)
+ return ret;
+
+ conn = NULL;
+ l2cap_chan_lock(chan);
+ if (chan->conn) {
+ l2cap_conn_get(chan->conn);
+ conn = chan->conn;
+ }
+ l2cap_chan_unlock(chan);
+
+ if (!conn)
+ return -EBADFD;
+
+ ret = hidp_session_new(&session, &bt_sk(ctrl_sock->sk)->dst, ctrl_sock,
+ intr_sock, req, conn);
+ if (ret)
+ goto out_conn;
+
+ ret = l2cap_register_user(conn, &session->user);
+ if (ret)
+ goto out_session;
+
+ ret = 0;
+
+out_session:
+ hidp_session_put(session);
+out_conn:
+ l2cap_conn_put(conn);
+ return ret;
+}
+
+int hidp_connection_del(struct hidp_conndel_req *req)
+{
+ struct hidp_session *session;
+
+ session = hidp_session_find(&req->bdaddr);
+ if (!session)
+ return -ENOENT;
+
+ if (req->flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG))
+ hidp_send_ctrl_message(session,
+ HIDP_TRANS_HID_CONTROL |
+ HIDP_CTRL_VIRTUAL_CABLE_UNPLUG,
+ NULL, 0);
+ else
+ l2cap_unregister_user(session->conn, &session->user);
+
+ hidp_session_put(session);
+
+ return 0;
+}
+
int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
{
struct hidp_session *session, *s;
@@ -1006,8 +1552,8 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
session->ctrl_sock = ctrl_sock;
session->intr_sock = intr_sock;
- session->conn = hidp_get_connection(session);
- if (!session->conn) {
+ session->hconn = hidp_get_connection(session);
+ if (!session->hconn) {
err = -ENOTCONN;
goto failed;
}
@@ -1208,6 +1754,7 @@ module_init(hidp_init);
module_exit(hidp_exit);
MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
+MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
MODULE_DESCRIPTION("Bluetooth HIDP ver " VERSION);
MODULE_VERSION(VERSION);
MODULE_LICENSE("GPL");
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index c844420..c4fb980 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -24,7 +24,9 @@
#define __HIDP_H
#include <linux/types.h>
+#include <linux/kref.h>
#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/l2cap.h>
/* HIDP header masks */
#define HIDP_HEADER_TRANS_MASK 0xf0
@@ -119,42 +121,55 @@ struct hidp_connlist_req {
struct hidp_conninfo __user *ci;
};
+int hidp_connection_add(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
+int hidp_connection_del(struct hidp_conndel_req *req);
int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
int hidp_del_connection(struct hidp_conndel_req *req);
int hidp_get_connlist(struct hidp_connlist_req *req);
int hidp_get_conninfo(struct hidp_conninfo *ci);
+enum hidp_session_state {
+ HIDP_SESSION_IDLING,
+ HIDP_SESSION_RUNNING,
+};
+
/* HIDP session defines */
struct hidp_session {
struct list_head list;
+ struct kref ref;
- struct hci_conn *conn;
+ /* runtime management */
+ atomic_t state;
+ wait_queue_head_t state_queue;
+ atomic_t terminate;
+ struct task_struct *task;
+ unsigned long flags;
+ /* connection management */
+ bdaddr_t bdaddr;
+ struct hci_conn *hconn;
+ struct l2cap_conn *conn;
+ struct l2cap_user user;
struct socket *ctrl_sock;
struct socket *intr_sock;
-
- bdaddr_t bdaddr;
-
- unsigned long flags;
- unsigned long idle_to;
-
+ struct sk_buff_head ctrl_transmit;
+ struct sk_buff_head intr_transmit;
uint ctrl_mtu;
uint intr_mtu;
+ unsigned long idle_to;
- atomic_t terminate;
- struct task_struct *task;
-
- unsigned char keys[8];
- unsigned char leds;
-
+ /* device management */
struct input_dev *input;
-
struct hid_device *hid;
-
struct timer_list timer;
- struct sk_buff_head ctrl_transmit;
- struct sk_buff_head intr_transmit;
+ /* Report descriptor */
+ __u8 *rd_data;
+ uint rd_size;
+
+ /* session data */
+ unsigned char keys[8];
+ unsigned char leds;
/* Used in hidp_get_raw_report() */
int waiting_report_type; /* HIDP_DATA_RTYPE_* */
@@ -166,10 +181,6 @@ struct hidp_session {
/* Used in hidp_output_raw_report() */
int output_report_success; /* boolean */
- /* Report descriptor */
- __u8 *rd_data;
- uint rd_size;
-
wait_queue_head_t startup_queue;
int waiting_for_startup;
};
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 11/16] Bluetooth: hidp: remove old session-management
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (9 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 10/16] Bluetooth: hidp: add new session-management helpers David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 12/16] Bluetooth: hidp: handle kernel_sendmsg() errors correctly David Herrmann
` (4 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
We have the full new session-management now available so lets switch over
and remove all the old code. Few semantics changed, so we need to adjust
the sock.c callers a bit. But this mostly simplifies the logic.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 339 ++--------------------------------------------
net/bluetooth/hidp/hidp.h | 6 -
net/bluetooth/hidp/sock.c | 21 +--
3 files changed, 18 insertions(+), 348 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 8d30a33..481bbb8 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -77,21 +77,7 @@ static inline void hidp_schedule(struct hidp_session *session)
wake_up_interruptible(sk_sleep(intr_sk));
}
-static struct hidp_session *__hidp_get_session(bdaddr_t *bdaddr)
-{
- struct hidp_session *session;
-
- BT_DBG("");
-
- list_for_each_entry(session, &hidp_session_list, list) {
- if (!bacmp(bdaddr, &session->bdaddr))
- return session;
- }
-
- return NULL;
-}
-
-static void __hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
+static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
{
memset(ci, 0, sizeof(*ci));
bacpy(&ci->bdaddr, &session->bdaddr);
@@ -456,8 +442,7 @@ static void hidp_idle_timeout(unsigned long arg)
{
struct hidp_session *session = (struct hidp_session *) arg;
- atomic_inc(&session->terminate);
- wake_up_process(session->task);
+ hidp_session_terminate(session);
}
static void hidp_set_timer(struct hidp_session *session)
@@ -525,8 +510,7 @@ static void hidp_process_hid_control(struct hidp_session *session,
skb_queue_purge(&session->ctrl_transmit);
skb_queue_purge(&session->intr_transmit);
- atomic_inc(&session->terminate);
- wake_up_process(current);
+ hidp_session_terminate(session);
}
}
@@ -686,120 +670,6 @@ static void hidp_process_ctrl_transmit(struct hidp_session *session)
}
}
-static int hidp_session(void *arg)
-{
- struct hidp_session *session = arg;
- struct sock *ctrl_sk = session->ctrl_sock->sk;
- struct sock *intr_sk = session->intr_sock->sk;
- struct sk_buff *skb;
- wait_queue_t ctrl_wait, intr_wait;
-
- BT_DBG("session %p", session);
-
- __module_get(THIS_MODULE);
- set_user_nice(current, -15);
-
- init_waitqueue_entry(&ctrl_wait, current);
- init_waitqueue_entry(&intr_wait, current);
- add_wait_queue(sk_sleep(ctrl_sk), &ctrl_wait);
- add_wait_queue(sk_sleep(intr_sk), &intr_wait);
- session->waiting_for_startup = 0;
- wake_up_interruptible(&session->startup_queue);
- set_current_state(TASK_INTERRUPTIBLE);
- while (!atomic_read(&session->terminate)) {
- if (ctrl_sk->sk_state != BT_CONNECTED ||
- intr_sk->sk_state != BT_CONNECTED)
- break;
-
- while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
- skb_orphan(skb);
- if (!skb_linearize(skb))
- hidp_recv_intr_frame(session, skb);
- else
- kfree_skb(skb);
- }
-
- hidp_process_intr_transmit(session);
-
- while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
- skb_orphan(skb);
- if (!skb_linearize(skb))
- hidp_recv_ctrl_frame(session, skb);
- else
- kfree_skb(skb);
- }
-
- hidp_process_ctrl_transmit(session);
-
- schedule();
- set_current_state(TASK_INTERRUPTIBLE);
- }
- set_current_state(TASK_RUNNING);
- atomic_inc(&session->terminate);
- remove_wait_queue(sk_sleep(intr_sk), &intr_wait);
- remove_wait_queue(sk_sleep(ctrl_sk), &ctrl_wait);
-
- clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
- clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
- wake_up_interruptible(&session->report_queue);
-
- down_write(&hidp_session_sem);
-
- hidp_del_timer(session);
-
- if (session->input) {
- input_unregister_device(session->input);
- session->input = NULL;
- }
-
- if (session->hid) {
- hid_destroy_device(session->hid);
- session->hid = NULL;
- }
-
- /* Wakeup user-space polling for socket errors */
- session->intr_sock->sk->sk_err = EUNATCH;
- session->ctrl_sock->sk->sk_err = EUNATCH;
-
- hidp_schedule(session);
-
- fput(session->intr_sock->file);
-
- wait_event_timeout(*(sk_sleep(ctrl_sk)),
- (ctrl_sk->sk_state == BT_CLOSED), msecs_to_jiffies(500));
-
- fput(session->ctrl_sock->file);
-
- list_del(&session->list);
-
- up_write(&hidp_session_sem);
-
- kfree(session->rd_data);
- kfree(session);
- module_put_and_exit(0);
- return 0;
-}
-
-static struct hci_conn *hidp_get_connection(struct hidp_session *session)
-{
- bdaddr_t *src = &bt_sk(session->ctrl_sock->sk)->src;
- bdaddr_t *dst = &bt_sk(session->ctrl_sock->sk)->dst;
- struct hci_conn *conn;
- struct hci_dev *hdev;
-
- hdev = hci_get_route(dst, src);
- if (!hdev)
- return NULL;
-
- hci_dev_lock(hdev);
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
- hci_dev_unlock(hdev);
-
- hci_dev_put(hdev);
-
- return conn;
-}
-
static int hidp_setup_input(struct hidp_session *session,
struct hidp_connadd_req *req)
{
@@ -847,7 +717,7 @@ static int hidp_setup_input(struct hidp_session *session,
input->relbit[0] |= BIT_MASK(REL_WHEEL);
}
- input->dev.parent = &session->hconn->dev;
+ input->dev.parent = &session->conn->hcon->dev;
input->event = hidp_input_event;
@@ -951,7 +821,7 @@ static int hidp_setup_hid(struct hidp_session *session,
snprintf(hid->uniq, sizeof(hid->uniq), "%pMR",
&bt_sk(session->ctrl_sock->sk)->dst);
- hid->dev.parent = &session->hconn->dev;
+ hid->dev.parent = &session->conn->hcon->dev;
hid->ll_driver = &hidp_hid_driver;
hid->hid_get_raw_report = hidp_get_raw_report;
@@ -1510,187 +1380,6 @@ int hidp_connection_del(struct hidp_conndel_req *req)
return 0;
}
-int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock)
-{
- struct hidp_session *session, *s;
- int vendor, product;
- int err;
-
- BT_DBG("");
-
- if (!l2cap_is_socket(ctrl_sock) || !l2cap_is_socket(intr_sock))
- return -EINVAL;
- if (bacmp(&bt_sk(ctrl_sock->sk)->src, &bt_sk(intr_sock->sk)->src) ||
- bacmp(&bt_sk(ctrl_sock->sk)->dst, &bt_sk(intr_sock->sk)->dst))
- return -ENOTUNIQ;
-
- BT_DBG("rd_data %p rd_size %d", req->rd_data, req->rd_size);
-
- down_write(&hidp_session_sem);
-
- s = __hidp_get_session(&bt_sk(ctrl_sock->sk)->dst);
- if (s) {
- up_write(&hidp_session_sem);
- return -EEXIST;
- }
-
- session = kzalloc(sizeof(struct hidp_session), GFP_KERNEL);
- if (!session) {
- up_write(&hidp_session_sem);
- return -ENOMEM;
- }
-
- bacpy(&session->bdaddr, &bt_sk(ctrl_sock->sk)->dst);
-
- session->ctrl_mtu = min_t(uint, l2cap_pi(ctrl_sock->sk)->chan->omtu,
- l2cap_pi(ctrl_sock->sk)->chan->imtu);
- session->intr_mtu = min_t(uint, l2cap_pi(intr_sock->sk)->chan->omtu,
- l2cap_pi(intr_sock->sk)->chan->imtu);
-
- BT_DBG("ctrl mtu %d intr mtu %d", session->ctrl_mtu, session->intr_mtu);
-
- session->ctrl_sock = ctrl_sock;
- session->intr_sock = intr_sock;
-
- session->hconn = hidp_get_connection(session);
- if (!session->hconn) {
- err = -ENOTCONN;
- goto failed;
- }
-
- setup_timer(&session->timer, hidp_idle_timeout, (unsigned long)session);
-
- skb_queue_head_init(&session->ctrl_transmit);
- skb_queue_head_init(&session->intr_transmit);
-
- mutex_init(&session->report_mutex);
- init_waitqueue_head(&session->report_queue);
- init_waitqueue_head(&session->startup_queue);
- session->waiting_for_startup = 1;
- session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
- session->idle_to = req->idle_to;
-
- list_add(&session->list, &hidp_session_list);
-
- if (req->rd_size > 0) {
- err = hidp_setup_hid(session, req);
- if (err && err != -ENODEV)
- goto purge;
- }
-
- if (!session->hid) {
- err = hidp_setup_input(session, req);
- if (err < 0)
- goto purge;
- }
-
- hidp_set_timer(session);
-
- if (session->hid) {
- vendor = session->hid->vendor;
- product = session->hid->product;
- } else if (session->input) {
- vendor = session->input->id.vendor;
- product = session->input->id.product;
- } else {
- vendor = 0x0000;
- product = 0x0000;
- }
-
- session->task = kthread_run(hidp_session, session, "khidpd_%04x%04x",
- vendor, product);
- if (IS_ERR(session->task)) {
- err = PTR_ERR(session->task);
- goto unlink;
- }
-
- while (session->waiting_for_startup) {
- wait_event_interruptible(session->startup_queue,
- !session->waiting_for_startup);
- }
-
- if (session->hid)
- err = hid_add_device(session->hid);
- else
- err = input_register_device(session->input);
-
- if (err < 0) {
- atomic_inc(&session->terminate);
- wake_up_process(session->task);
- up_write(&hidp_session_sem);
- return err;
- }
-
- if (session->input) {
- hidp_send_ctrl_message(session,
- HIDP_TRANS_SET_PROTOCOL | HIDP_PROTO_BOOT, NULL, 0);
- session->flags |= (1 << HIDP_BOOT_PROTOCOL_MODE);
-
- session->leds = 0xff;
- hidp_input_event(session->input, EV_LED, 0, 0);
- }
-
- up_write(&hidp_session_sem);
- return 0;
-
-unlink:
- hidp_del_timer(session);
-
- if (session->input) {
- input_unregister_device(session->input);
- session->input = NULL;
- }
-
- if (session->hid) {
- hid_destroy_device(session->hid);
- session->hid = NULL;
- }
-
- kfree(session->rd_data);
- session->rd_data = NULL;
-
-purge:
- list_del(&session->list);
-
- skb_queue_purge(&session->ctrl_transmit);
- skb_queue_purge(&session->intr_transmit);
-
-failed:
- up_write(&hidp_session_sem);
-
- kfree(session);
- return err;
-}
-
-int hidp_del_connection(struct hidp_conndel_req *req)
-{
- struct hidp_session *session;
- int err = 0;
-
- BT_DBG("");
-
- down_read(&hidp_session_sem);
-
- session = __hidp_get_session(&req->bdaddr);
- if (session) {
- if (req->flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG)) {
- hidp_send_ctrl_message(session,
- HIDP_TRANS_HID_CONTROL | HIDP_CTRL_VIRTUAL_CABLE_UNPLUG, NULL, 0);
- } else {
- /* Flush the transmit queues */
- skb_queue_purge(&session->ctrl_transmit);
- skb_queue_purge(&session->intr_transmit);
-
- atomic_inc(&session->terminate);
- wake_up_process(session->task);
- }
- } else
- err = -ENOENT;
-
- up_read(&hidp_session_sem);
- return err;
-}
-
int hidp_get_connlist(struct hidp_connlist_req *req)
{
struct hidp_session *session;
@@ -1703,7 +1392,7 @@ int hidp_get_connlist(struct hidp_connlist_req *req)
list_for_each_entry(session, &hidp_session_list, list) {
struct hidp_conninfo ci;
- __hidp_copy_session(session, &ci);
+ hidp_copy_session(session, &ci);
if (copy_to_user(req->ci, &ci, sizeof(ci))) {
err = -EFAULT;
@@ -1724,18 +1413,14 @@ int hidp_get_connlist(struct hidp_connlist_req *req)
int hidp_get_conninfo(struct hidp_conninfo *ci)
{
struct hidp_session *session;
- int err = 0;
- down_read(&hidp_session_sem);
-
- session = __hidp_get_session(&ci->bdaddr);
- if (session)
- __hidp_copy_session(session, ci);
- else
- err = -ENOENT;
+ session = hidp_session_find(&ci->bdaddr);
+ if (session) {
+ hidp_copy_session(session, ci);
+ hidp_session_put(session);
+ }
- up_read(&hidp_session_sem);
- return err;
+ return session ? 0 : -ENOENT;
}
static int __init hidp_init(void)
diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
index c4fb980..6162ce8 100644
--- a/net/bluetooth/hidp/hidp.h
+++ b/net/bluetooth/hidp/hidp.h
@@ -123,8 +123,6 @@ struct hidp_connlist_req {
int hidp_connection_add(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
int hidp_connection_del(struct hidp_conndel_req *req);
-int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock, struct socket *intr_sock);
-int hidp_del_connection(struct hidp_conndel_req *req);
int hidp_get_connlist(struct hidp_connlist_req *req);
int hidp_get_conninfo(struct hidp_conninfo *ci);
@@ -147,7 +145,6 @@ struct hidp_session {
/* connection management */
bdaddr_t bdaddr;
- struct hci_conn *hconn;
struct l2cap_conn *conn;
struct l2cap_user user;
struct socket *ctrl_sock;
@@ -180,9 +177,6 @@ struct hidp_session {
/* Used in hidp_output_raw_report() */
int output_report_success; /* boolean */
-
- wait_queue_head_t startup_queue;
- int waiting_for_startup;
};
/* HIDP init defines */
diff --git a/net/bluetooth/hidp/sock.c b/net/bluetooth/hidp/sock.c
index e6bf36a..2f4cbb0 100644
--- a/net/bluetooth/hidp/sock.c
+++ b/net/bluetooth/hidp/sock.c
@@ -77,21 +77,12 @@ static int hidp_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
return err;
}
- if (csock->sk->sk_state != BT_CONNECTED ||
- isock->sk->sk_state != BT_CONNECTED) {
- sockfd_put(csock);
- sockfd_put(isock);
- return -EBADFD;
- }
+ err = hidp_connection_add(&ca, csock, isock);
+ if (!err && copy_to_user(argp, &ca, sizeof(ca)))
+ err = -EFAULT;
- err = hidp_add_connection(&ca, csock, isock);
- if (!err) {
- if (copy_to_user(argp, &ca, sizeof(ca)))
- err = -EFAULT;
- } else {
- sockfd_put(csock);
- sockfd_put(isock);
- }
+ sockfd_put(csock);
+ sockfd_put(isock);
return err;
@@ -102,7 +93,7 @@ static int hidp_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
if (copy_from_user(&cd, argp, sizeof(cd)))
return -EFAULT;
- return hidp_del_connection(&cd);
+ return hidp_connection_del(&cd);
case HIDPGETCONNLIST:
if (copy_from_user(&cl, argp, sizeof(cl)))
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 12/16] Bluetooth: hidp: handle kernel_sendmsg() errors correctly
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (10 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 11/16] Bluetooth: hidp: remove old session-management David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 13/16] Bluetooth: hidp: merge hidp_process_{ctrl,intr}_transmit() David Herrmann
` (3 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
We shouldn't push back the skbs if kernel_sendmsg() fails. Instead, we
terminate the connection and drop the skb. Only on EAGAIN we push it back
and return.
l2cap doesn't return EAGAIN, yet, but this guarantees we're safe if it
will at some time in the future.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 481bbb8..3f6ef06 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -639,13 +639,19 @@ static int hidp_send_frame(struct socket *sock, unsigned char *data, int len)
static void hidp_process_intr_transmit(struct hidp_session *session)
{
struct sk_buff *skb;
+ int ret;
BT_DBG("session %p", session);
while ((skb = skb_dequeue(&session->intr_transmit))) {
- if (hidp_send_frame(session->intr_sock, skb->data, skb->len) < 0) {
+ ret = hidp_send_frame(session->intr_sock, skb->data, skb->len);
+ if (ret == -EAGAIN) {
skb_queue_head(&session->intr_transmit, skb);
break;
+ } else if (ret < 0) {
+ hidp_session_terminate(session);
+ kfree_skb(skb);
+ break;
}
hidp_set_timer(session);
@@ -656,13 +662,19 @@ static void hidp_process_intr_transmit(struct hidp_session *session)
static void hidp_process_ctrl_transmit(struct hidp_session *session)
{
struct sk_buff *skb;
+ int ret;
BT_DBG("session %p", session);
while ((skb = skb_dequeue(&session->ctrl_transmit))) {
- if (hidp_send_frame(session->ctrl_sock, skb->data, skb->len) < 0) {
+ ret = hidp_send_frame(session->ctrl_sock, skb->data, skb->len);
+ if (ret == -EAGAIN) {
skb_queue_head(&session->ctrl_transmit, skb);
break;
+ } else if (ret < 0) {
+ hidp_session_terminate(session);
+ kfree_skb(skb);
+ break;
}
hidp_set_timer(session);
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 13/16] Bluetooth: hidp: merge hidp_process_{ctrl,intr}_transmit()
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (11 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 12/16] Bluetooth: hidp: handle kernel_sendmsg() errors correctly David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 14/16] Bluetooth: hidp: merge 'send' functions into hidp_send_message() David Herrmann
` (2 subsequent siblings)
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
Both hidp_process_ctrl_transmit() and hidp_process_intr_transmit() are
exactly the same apart from the transmit-queue and socket pointers.
Therefore, pass them as argument and merge both functions into one so we
avoid 25 lines of code-duplication.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 40 +++++++++++-----------------------------
1 file changed, 11 insertions(+), 29 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 3f6ef06..8f81379 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -636,40 +636,20 @@ static int hidp_send_frame(struct socket *sock, unsigned char *data, int len)
return kernel_sendmsg(sock, &msg, &iv, 1, len);
}
-static void hidp_process_intr_transmit(struct hidp_session *session)
+/* dequeue message from @transmit and send via @sock */
+static void hidp_process_transmit(struct hidp_session *session,
+ struct sk_buff_head *transmit,
+ struct socket *sock)
{
struct sk_buff *skb;
int ret;
BT_DBG("session %p", session);
- while ((skb = skb_dequeue(&session->intr_transmit))) {
- ret = hidp_send_frame(session->intr_sock, skb->data, skb->len);
+ while ((skb = skb_dequeue(transmit))) {
+ ret = hidp_send_frame(sock, skb->data, skb->len);
if (ret == -EAGAIN) {
- skb_queue_head(&session->intr_transmit, skb);
- break;
- } else if (ret < 0) {
- hidp_session_terminate(session);
- kfree_skb(skb);
- break;
- }
-
- hidp_set_timer(session);
- kfree_skb(skb);
- }
-}
-
-static void hidp_process_ctrl_transmit(struct hidp_session *session)
-{
- struct sk_buff *skb;
- int ret;
-
- BT_DBG("session %p", session);
-
- while ((skb = skb_dequeue(&session->ctrl_transmit))) {
- ret = hidp_send_frame(session->ctrl_sock, skb->data, skb->len);
- if (ret == -EAGAIN) {
- skb_queue_head(&session->ctrl_transmit, skb);
+ skb_queue_head(transmit, skb);
break;
} else if (ret < 0) {
hidp_session_terminate(session);
@@ -1224,7 +1204,8 @@ static void hidp_session_run(struct hidp_session *session)
}
/* send pending intr-skbs */
- hidp_process_intr_transmit(session);
+ hidp_process_transmit(session, &session->intr_transmit,
+ session->intr_sock);
/* parse incoming ctrl-skbs */
while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
@@ -1236,7 +1217,8 @@ static void hidp_session_run(struct hidp_session *session)
}
/* send pending ctrl-skbs */
- hidp_process_ctrl_transmit(session);
+ hidp_process_transmit(session, &session->ctrl_transmit,
+ session->ctrl_sock);
schedule();
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 14/16] Bluetooth: hidp: merge 'send' functions into hidp_send_message()
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (12 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 13/16] Bluetooth: hidp: merge hidp_process_{ctrl,intr}_transmit() David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 15/16] Bluetooth: hidp: don't send boot-protocol messages as HID-reports David Herrmann
2013-04-06 18:28 ` [PATCH v4 16/16] Bluetooth: hidp: fix sending output reports on intr channel David Herrmann
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
We handle skb buffers all over the place, even though we have
hidp_send_*_message() helpers. This creates a more generic
hidp_send_message() helper and uses it instead of dealing with transmit
queues directly everywhere.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 156 ++++++++++++++++++----------------------------
1 file changed, 60 insertions(+), 96 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 8f81379..5fcc038 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -68,15 +68,6 @@ static void hidp_session_remove(struct l2cap_conn *conn,
static int hidp_session_thread(void *arg);
static void hidp_session_terminate(struct hidp_session *s);
-static inline void hidp_schedule(struct hidp_session *session)
-{
- struct sock *ctrl_sk = session->ctrl_sock->sk;
- struct sock *intr_sk = session->intr_sock->sk;
-
- wake_up_interruptible(sk_sleep(ctrl_sk));
- wake_up_interruptible(sk_sleep(intr_sk));
-}
-
static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo *ci)
{
memset(ci, 0, sizeof(*ci));
@@ -107,11 +98,56 @@ static void hidp_copy_session(struct hidp_session *session, struct hidp_conninfo
}
}
+/* assemble skb, queue message on @transmit and wake up the session thread */
+static int hidp_send_message(struct hidp_session *session, struct socket *sock,
+ struct sk_buff_head *transmit, unsigned char hdr,
+ const unsigned char *data, int size)
+{
+ struct sk_buff *skb;
+ struct sock *sk = sock->sk;
+
+ BT_DBG("session %p data %p size %d", session, data, size);
+
+ if (atomic_read(&session->terminate))
+ return -EIO;
+
+ skb = alloc_skb(size + 1, GFP_ATOMIC);
+ if (!skb) {
+ BT_ERR("Can't allocate memory for new frame");
+ return -ENOMEM;
+ }
+
+ *skb_put(skb, 1) = hdr;
+ if (data && size > 0)
+ memcpy(skb_put(skb, size), data, size);
+
+ skb_queue_tail(transmit, skb);
+ wake_up_interruptible(sk_sleep(sk));
+
+ return 0;
+}
+
+static int hidp_send_ctrl_message(struct hidp_session *session,
+ unsigned char hdr, const unsigned char *data,
+ int size)
+{
+ return hidp_send_message(session, session->ctrl_sock,
+ &session->ctrl_transmit, hdr, data, size);
+}
+
+static int hidp_send_intr_message(struct hidp_session *session,
+ unsigned char hdr, const unsigned char *data,
+ int size)
+{
+ return hidp_send_message(session, session->intr_sock,
+ &session->intr_transmit, hdr, data, size);
+}
+
static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
unsigned int type, unsigned int code, int value)
{
unsigned char newleds;
- struct sk_buff *skb;
+ unsigned char hdr, data[2];
BT_DBG("session %p type %d code %d value %d", session, type, code, value);
@@ -129,21 +165,11 @@ static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
session->leds = newleds;
- skb = alloc_skb(3, GFP_ATOMIC);
- if (!skb) {
- BT_ERR("Can't allocate memory for new frame");
- return -ENOMEM;
- }
-
- *skb_put(skb, 1) = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
- *skb_put(skb, 1) = 0x01;
- *skb_put(skb, 1) = newleds;
-
- skb_queue_tail(&session->intr_transmit, skb);
+ hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
+ data[0] = 0x01;
+ data[1] = newleds;
- hidp_schedule(session);
-
- return 0;
+ return hidp_send_intr_message(session, hdr, data, 2);
}
static int hidp_hidinput_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
@@ -216,71 +242,9 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
input_sync(dev);
}
-static int __hidp_send_ctrl_message(struct hidp_session *session,
- unsigned char hdr, unsigned char *data,
- int size)
-{
- struct sk_buff *skb;
-
- BT_DBG("session %p data %p size %d", session, data, size);
-
- if (atomic_read(&session->terminate))
- return -EIO;
-
- skb = alloc_skb(size + 1, GFP_ATOMIC);
- if (!skb) {
- BT_ERR("Can't allocate memory for new frame");
- return -ENOMEM;
- }
-
- *skb_put(skb, 1) = hdr;
- if (data && size > 0)
- memcpy(skb_put(skb, size), data, size);
-
- skb_queue_tail(&session->ctrl_transmit, skb);
-
- return 0;
-}
-
-static int hidp_send_ctrl_message(struct hidp_session *session,
- unsigned char hdr, unsigned char *data, int size)
-{
- int err;
-
- err = __hidp_send_ctrl_message(session, hdr, data, size);
-
- hidp_schedule(session);
-
- return err;
-}
-
-static int hidp_queue_report(struct hidp_session *session,
- unsigned char *data, int size)
-{
- struct sk_buff *skb;
-
- BT_DBG("session %p hid %p data %p size %d", session, session->hid, data, size);
-
- skb = alloc_skb(size + 1, GFP_ATOMIC);
- if (!skb) {
- BT_ERR("Can't allocate memory for new frame");
- return -ENOMEM;
- }
-
- *skb_put(skb, 1) = 0xa2;
- if (size > 0)
- memcpy(skb_put(skb, size), data, size);
-
- skb_queue_tail(&session->intr_transmit, skb);
-
- hidp_schedule(session);
-
- return 0;
-}
-
static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
{
- unsigned char buf[32];
+ unsigned char buf[32], hdr;
int rsize;
rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
@@ -288,8 +252,9 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
return -EIO;
hid_output_report(report, buf);
+ hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
- return hidp_queue_report(session, buf, rsize);
+ return hidp_send_intr_message(session, hdr, buf, rsize);
}
static int hidp_get_raw_report(struct hid_device *hid,
@@ -328,7 +293,7 @@ static int hidp_get_raw_report(struct hid_device *hid,
session->waiting_report_number = numbered_reports ? report_number : -1;
set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
data[0] = report_number;
- ret = hidp_send_ctrl_message(hid->driver_data, report_type, data, 1);
+ ret = hidp_send_ctrl_message(session, report_type, data, 1);
if (ret)
goto err;
@@ -388,7 +353,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
break;
case HID_OUTPUT_REPORT:
- report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
+ report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
break;
default:
return -EINVAL;
@@ -399,8 +364,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
/* Set up our wait, and send the report request to the device. */
set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
- ret = hidp_send_ctrl_message(hid->driver_data, report_type, data,
- count);
+ ret = hidp_send_ctrl_message(session, report_type, data, count);
if (ret)
goto err;
@@ -485,12 +449,12 @@ static void hidp_process_handshake(struct hidp_session *session,
case HIDP_HSHK_ERR_FATAL:
/* Device requests a reboot, as this is the only way this error
* can be recovered. */
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HID_CONTROL | HIDP_CTRL_SOFT_RESET, NULL, 0);
break;
default:
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
break;
}
@@ -538,7 +502,7 @@ static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
break;
default:
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
}
@@ -585,7 +549,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
break;
default:
- __hidp_send_ctrl_message(session,
+ hidp_send_ctrl_message(session,
HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_UNSUPPORTED_REQUEST, NULL, 0);
break;
}
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 15/16] Bluetooth: hidp: don't send boot-protocol messages as HID-reports
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (13 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 14/16] Bluetooth: hidp: merge 'send' functions into hidp_send_message() David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
2013-04-06 18:28 ` [PATCH v4 16/16] Bluetooth: hidp: fix sending output reports on intr channel David Herrmann
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
If a device is registered as HID device, it is always in Report-Mode.
Therefore, we must not send Boot-Protocol messages on
hidinput_input_event() callbacks. This confuses devices and may cause
disconnects on protocol errors.
We disable the hidinput_input_event() callback for now. We can implement
it properly later, but lets first fix the current code by disabling it.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 5fcc038..13a0a05 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -143,13 +143,15 @@ static int hidp_send_intr_message(struct hidp_session *session,
&session->intr_transmit, hdr, data, size);
}
-static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
- unsigned int type, unsigned int code, int value)
+static int hidp_input_event(struct input_dev *dev, unsigned int type,
+ unsigned int code, int value)
{
+ struct hidp_session *session = input_get_drvdata(dev);
unsigned char newleds;
unsigned char hdr, data[2];
- BT_DBG("session %p type %d code %d value %d", session, type, code, value);
+ BT_DBG("session %p type %d code %d value %d",
+ session, type, code, value);
if (type != EV_LED)
return -1;
@@ -172,21 +174,6 @@ static int hidp_queue_event(struct hidp_session *session, struct input_dev *dev,
return hidp_send_intr_message(session, hdr, data, 2);
}
-static int hidp_hidinput_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
-{
- struct hid_device *hid = input_get_drvdata(dev);
- struct hidp_session *session = hid->driver_data;
-
- return hidp_queue_event(session, dev, type, code, value);
-}
-
-static int hidp_input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
-{
- struct hidp_session *session = input_get_drvdata(dev);
-
- return hidp_queue_event(session, dev, type, code, value);
-}
-
static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
{
struct input_dev *dev = session->input;
@@ -732,7 +719,6 @@ static struct hid_ll_driver hidp_hid_driver = {
.stop = hidp_stop,
.open = hidp_open,
.close = hidp_close,
- .hidinput_input_event = hidp_hidinput_event,
};
/* This function sets up the hid device. It does not add it
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v4 16/16] Bluetooth: hidp: fix sending output reports on intr channel
2013-04-06 18:28 ` [PATCH v4 00/16] " David Herrmann
` (14 preceding siblings ...)
2013-04-06 18:28 ` [PATCH v4 15/16] Bluetooth: hidp: don't send boot-protocol messages as HID-reports David Herrmann
@ 2013-04-06 18:28 ` David Herrmann
15 siblings, 0 replies; 44+ messages in thread
From: David Herrmann @ 2013-04-06 18:28 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan, David Herrmann
According to the specifications, data output reports must be sent on the
interrupt channel. See also usbhid implementation.
Sending these reports on the control channel breaks newer Wii Remotes.
Note that this will make output reports asynchronous. However, that's how
hid_output_raw_report() is supposed to work with HID_OUTPUT_REPORT as
report type. There are no responses to output reports.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/hidp/core.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 13a0a05..940f5ac 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -335,14 +335,11 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
struct hidp_session *session = hid->driver_data;
int ret;
- switch (report_type) {
- case HID_FEATURE_REPORT:
- report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
- break;
- case HID_OUTPUT_REPORT:
+ if (report_type == HID_OUTPUT_REPORT) {
report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
- break;
- default:
+ return hidp_send_intr_message(session, report_type,
+ data, count);
+ } else if (report_type != HID_FEATURE_REPORT) {
return -EINVAL;
}
@@ -351,6 +348,7 @@ static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, s
/* Set up our wait, and send the report request to the device. */
set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
+ report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
ret = hidp_send_ctrl_message(session, report_type, data, count);
if (ret)
goto err;
--
1.8.2
^ permalink raw reply related [flat|nested] 44+ messages in thread