Hi Marcel,
On Wed, Oct 2, 2013 at 2:21 AM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Andre,
>
> > This patch does some code refactorig in LE Connection Complete HCI
> > event handler. It basically adds a switch statement to separate new
> > master connection code from new slave connection code.
> >
> > Signed-off-by: Andre Guedes <andre.guedes@xxxxxxxxxxxxx>
> > ---
> > include/net/bluetooth/hci.h | 1 +
> > net/bluetooth/hci_event.c | 55 ++++++++++++++++++++++++++++++++-------------
> > 2 files changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 7ede266..8c98f60 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
> >
> > /* Low energy meta events */
> > #define LE_CONN_ROLE_MASTER 0x00
> > +#define LE_CONN_ROLE_SLAVE 0x01
> >
> > #define HCI_EV_LE_CONN_COMPLETE 0x01
> > struct hci_ev_le_conn_complete {
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 1d1ffa6..0e4a9f4 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >
> > hci_dev_lock(hdev);
> >
> > - conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> > - if (!conn) {
> > + if (ev->status) {
> > + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> > + if (!conn)
> > + goto unlock;
> > +
> > + mgmt_connect_failed(hdev, &conn->dst, conn->type,
> > + conn->dst_type, ev->status);
> > + hci_proto_connect_cfm(conn, ev->status);
> > + conn->state = BT_CLOSED;
> > + hci_conn_del(conn);
> > + goto unlock;
> > + }
> > +
> > + switch (ev->role) {
> > + case LE_CONN_ROLE_MASTER:
> > + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
> > + /* If there is no hci_conn object with the given address, it
> > + * means this new connection was triggered through HCI socket
> > + * interface. For that case, we should create a new hci_conn
> > + * object.
> > + */
>
> this comments belong one level down inside the if block. You already commenting on the negative outcome of the if check.
Ok, I'll fix it.
>
>
> > + if (!conn) {
> > + conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> > + if (!conn) {
> > + BT_ERR("No memory for new connection");
> > + goto unlock;
> > + }
> > +
> > + conn->out = true;
> > + conn->link_mode |= HCI_LM_MASTER;
> > + conn->sec_level = BT_SECURITY_LOW;
> > + conn->dst_type = ev->bdaddr_type;
> > + }
> > + break;
> > +
> > + case LE_CONN_ROLE_SLAVE:
>
> And why are we not checking for an existing connection here? At least a small comment is needed to make that part clear.
Differently from master connection, there is no existing hci_conn for
slave connections. For that reason we don't check for an existing
connection here. I'll add a comment.
>
>
> > conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> > if (!conn) {
> > BT_ERR("No memory for new connection");
> > @@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > }
> >
> > conn->dst_type = ev->bdaddr_type;
> > + conn->sec_level = BT_SECURITY_LOW;
> > + break;
> >
> > - if (ev->role == LE_CONN_ROLE_MASTER) {
> > - conn->out = true;
> > - conn->link_mode |= HCI_LM_MASTER;
> > - }
> > - }
> > -
> > - if (ev->status) {
> > - mgmt_connect_failed(hdev, &conn->dst, conn->type,
> > - conn->dst_type, ev->status);
> > - hci_proto_connect_cfm(conn, ev->status);
> > - conn->state = BT_CLOSED;
> > - hci_conn_del(conn);
> > + default:
> > + BT_ERR("Used reserved Role parameter %d", ev->role);
> > goto unlock;
> > }
> >
> > @@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
> > conn->dst_type, 0, NULL, 0, NULL);
> >
> > - conn->sec_level = BT_SECURITY_LOW;
> > conn->handle = __le16_to_cpu(ev->handle);
> > conn->state = BT_CONNECTED;
>
> All in all, I am not really understanding why this makes it this code simpler. I actually think it turns it into more complicated code. So please explain what we are really gaining here. I just see more hash table lookup and for hci_conn_add calls with more error checks.
When controller sends a LE Connection Complete event to host, we have
three different handling: failure, new master connection and new slave
connection. Additionally, new master connection has a special handling
since the connection could be triggered by HCI socket interface.
Before, all these logic were mixed up, making hard to add specific
code for new master connection for instance.
So this patch explicitly separates the three different handling and
adds extra comments aiming to improve hci_le_conn_complete_evt()
readability. Besides that, keeping these different handling separated,
it'll be easier to add the auto connection hooks.
Regards,
Andre
--
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
