Hi Peter
On Tue, Aug 30, 2011 at 3:40 PM, Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> wrote:
> Hi David,
>
> On Fri, 2011-08-19 at 10:41 -0400, David Herrmann wrote:
>> Also I think it would make the code nicer when calling hidp_get_device() in
>> hidp_add_connection and hidp_setup_input/hid just retrieves the parent-device
>> from session->conn.
>
> What about something like this instead?
>
> This:
> 1. moves the hidp_get_device out into hidp_add_connection
> 2. renames hidp_get_device to hidp_find_connection
> 3. checks for null connection value return
> 4. performs the connection list access with device lock held
> 5. bumps the hci connection device hold with device lock held
>
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index e02c6a2..f6db935 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -97,8 +97,6 @@ static void __hidp_link_session(struct hidp_session *session)
> {
> __module_get(THIS_MODULE);
> list_add(&session->list, &hidp_session_list);
> -
> - hci_conn_hold_device(session->conn);
> }
>
> static void __hidp_unlink_session(struct hidp_session *session)
> @@ -763,24 +761,26 @@ static int hidp_session(void *arg)
> return 0;
> }
>
> -static struct device *hidp_get_device(struct hidp_session *session)
> +static struct hci_conn *hidp_find_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 device *device = NULL;
> + struct hci_conn *conn;
> struct hci_dev *hdev;
>
> hdev = hci_get_route(dst, src);
> if (!hdev)
> return NULL;
>
> - session->conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
> - if (session->conn)
> - device = &session->conn->dev;
> + hci_dev_lock_bh(hdev);
> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
> + if (conn)
> + hci_conn_hold_device(conn);
> + hci_dev_unlock_bh(hdev);
>
> hci_dev_put(hdev);
>
> - return device;
> + return conn;
> }
>
> static int hidp_setup_input(struct hidp_session *session,
> @@ -830,7 +830,7 @@ static int hidp_setup_input(struct hidp_session *session,
> input->relbit[0] |= BIT_MASK(REL_WHEEL);
> }
>
> - input->dev.parent = hidp_get_device(session);
> + input->dev.parent = &session->conn->dev;
>
> input->event = hidp_input_event;
>
> @@ -934,7 +934,7 @@ static int hidp_setup_hid(struct hidp_session *session,
> strncpy(hid->phys, batostr(&bt_sk(session->ctrl_sock->sk)->src), 64);
> strncpy(hid->uniq, batostr(&bt_sk(session->ctrl_sock->sk)->dst), 64);
>
> - hid->dev.parent = hidp_get_device(session);
> + hid->dev.parent = &session->conn->dev;
> hid->ll_driver = &hidp_hid_driver;
>
> hid->hid_get_raw_report = hidp_get_raw_report;
> @@ -975,6 +975,10 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> goto failed;
> }
>
> + session->conn = hidp_find_connection(session);
> + if (!session->conn)
> + goto failed;
> +
> bacpy(&session->bdaddr, &bt_sk(ctrl_sock->sk)->dst);
>
> session->ctrl_mtu = min_t(uint, l2cap_pi(ctrl_sock->sk)->chan->omtu,
> @@ -1000,6 +1004,8 @@ 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);
> +
> if (req->rd_size > 0) {
> err = hidp_setup_hid(session, req);
> if (err && err != -ENODEV)
> @@ -1012,8 +1018,6 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> goto purge;
> }
>
> - __hidp_link_session(session);
> -
> hidp_set_timer(session);
>
> if (session->hid) {
> @@ -1062,8 +1066,6 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> unlink:
> hidp_del_timer(session);
>
> - __hidp_unlink_session(session);
> -
> if (session->input) {
> input_unregister_device(session->input);
> session->input = NULL;
> @@ -1076,6 +1078,8 @@ unlink:
> session->rd_data = NULL;
>
> purge:
> + __hidp_unlink_session(session);
> +
> skb_queue_purge(&session->ctrl_transmit);
> skb_queue_purge(&session->intr_transmit);
>
> --
Great! This fixes the race-condition I mentioned earlier,
error-recovery also seems fine here. Thanks!
After Marcel or Gustavo commented on that, can you resend it with a
proper commit-message? Or should I resend it?
I am fine with you doing that, here's my signed-off line anyway:
Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxxxxxxx>
> Regards,
> Peter Hurley
Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
