Return-Path: Date: Tue, 12 Oct 2010 15:50:19 +0300 From: Ville Tervo To: ext Marcel Holtmann Cc: "linux-bluetooth@vger.kernel.org" Subject: Re: [PATCH 2/7] Bluetooth: Add LE connect support Message-ID: <20101012125019.GA642@null> References: <1286390535-27462-1-git-send-email-ville.tervo@nokia.com> <1286390535-27462-3-git-send-email-ville.tervo@nokia.com> <1286449089.6145.91.camel@aeonflux> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1286449089.6145.91.camel@aeonflux> List-ID: Hi Marcel, Thanks for the review. Could you comments on hci_connect? On Thu, Oct 07, 2010 at 12:58:09PM +0200, ext Marcel Holtmann wrote: > Hi Ville, > > > Add logic to create LE connections. > > > > Signed-off-by: Ville Tervo > > --- > > include/net/bluetooth/hci.h | 1 + > > include/net/bluetooth/hci_core.h | 6 ++- > > net/bluetooth/hci_conn.c | 38 ++++++++++++++- > > net/bluetooth/hci_event.c | 100 +++++++++++++++++++++++++++++++++++++- > > 4 files changed, 141 insertions(+), 4 deletions(-) > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > index b86aed5..b326240 100644 > > --- a/include/net/bluetooth/hci.h > > +++ b/include/net/bluetooth/hci.h > > @@ -162,6 +162,7 @@ enum { > > #define SCO_LINK 0x00 > > #define ACL_LINK 0x01 > > #define ESCO_LINK 0x02 > > +#define LE_LINK 0x03 > > this is not a value defined by the specification, while the others are. > And some functions match these to HCI event. So if wanna do it like > this, then using something like 0x80 is better. Done with some explanation why 0x80 is used. > > /* LMP features */ > > #define LMP_3SLOT 0x01 > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index ebec8c9..89f4b10 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -170,6 +170,7 @@ struct hci_conn { > > bdaddr_t dst; > > __u16 handle; > > __u16 state; > > + __u16 le_state; > > I don't see the need for a separate state here. The LE link is different > from an ACL link and also from a SCO link. We will create a new hci_conn > for each type of link. I removed le_state completely. > > __u8 mode; > > __u8 type; > > __u8 out; > > @@ -203,6 +204,7 @@ struct hci_conn { > > struct hci_dev *hdev; > > void *l2cap_data; > > void *sco_data; > > + void *le_data; > > void *priv; > > What is the use of le_data here? No use. Some left over earlier version. > > struct hci_conn *link; > > @@ -272,7 +274,7 @@ static inline void hci_conn_hash_add(struct hci_dev *hdev, struct hci_conn *c) > > { > > struct hci_conn_hash *h = &hdev->conn_hash; > > list_add(&c->list, &h->list); > > - if (c->type == ACL_LINK) > > + if (c->type == ACL_LINK || c->type == LE_LINK) > > h->acl_num++; > > else > > h->sco_num++; > > @@ -282,7 +284,7 @@ static inline void hci_conn_hash_del(struct hci_dev *hdev, struct hci_conn *c) > > { > > struct hci_conn_hash *h = &hdev->conn_hash; > > list_del(&c->list); > > - if (c->type == ACL_LINK) > > + if (c->type == ACL_LINK || c->type == LE_LINK) > > h->acl_num--; > > else > > h->sco_num--; > > I would assume that counting LE connection separately would be way > better. We could have ACL link to one device and LE link to another. Done > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > index 145993f..cb41d64 100644 > > --- a/net/bluetooth/hci_conn.c > > +++ b/net/bluetooth/hci_conn.c > > @@ -45,6 +45,27 @@ > > #include > > #include > > > > +void hci_le_connect(struct hci_conn *conn) > > +{ > > + struct hci_dev *hdev = conn->hdev; > > + struct hci_cp_le_create_conn cp; > > + > > + conn->le_state = BT_CONNECT; > > + conn->out = 1; > > + > > + memset(&cp, 0, sizeof(cp)); > > + cp.scan_interval = cpu_to_le16(0x0004); > > + cp.scan_window = cpu_to_le16(0x0004); > > + bacpy(&cp.peer_addr, &conn->dst); > > + cp.conn_interval_min = cpu_to_le16(0x0008); > > + cp.conn_interval_max = cpu_to_le16(0x0100); > > + cp.supervision_timeout = cpu_to_le16(0x0064); > > + cp.min_ce_len = cpu_to_le16(0x0001); > > + cp.max_ce_len = cpu_to_le16(0xffff); > > + > > + hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp); > > +} > > + > > void hci_acl_connect(struct hci_conn *conn) > > { > > struct hci_dev *hdev = conn->hdev; > > @@ -365,15 +386,30 @@ struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src) > > } > > EXPORT_SYMBOL(hci_get_route); > > > > -/* Create SCO or ACL connection. > > +/* Create SCO, ACL or LE connection. > > * Device _must_ be locked */ > > struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8 sec_level, __u8 auth_type) > > { > > struct hci_conn *acl; > > struct hci_conn *sco; > > + struct hci_conn *le; > > > > BT_DBG("%s dst %s", hdev->name, batostr(dst)); > > > > + if (type == LE_LINK) { > > + le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst); > > + > > + if (!le) > > + le = hci_conn_add(hdev, LE_LINK, dst); > > + > > + if (!le) > > + return NULL; > > + > > + hci_le_connect(le); > > + > > + return le; > > + } > > + > > if (!(acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst))) { > > if (!(acl = hci_conn_add(hdev, ACL_LINK, dst))) > > return NULL; > > No liking it this much. We might have to re-think on how to do things > here. Could you eloborate which part you dislike? I'm leaving this currently like it was. > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > > index d3c68de..0b979ae 100644 > > --- a/net/bluetooth/hci_event.c > > +++ b/net/bluetooth/hci_event.c > > @@ -868,6 +868,44 @@ static void hci_cc_le_set_scan(struct hci_dev *hdev, struct sk_buff *skb) > > hci_req_complete(hdev, status); > > } > > > > +static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status) > > +{ > > + struct hci_cp_le_create_conn *cp; > > + struct hci_conn *conn; > > + > > + BT_DBG("%s status 0x%x", hdev->name, status); > > + > > + cp = hci_sent_cmd_data(hdev, HCI_OP_LE_CREATE_CONN); > > + if (!cp) > > + return; > > + > > + hci_dev_lock(hdev); > > + > > + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->peer_addr); > > + > > + BT_DBG("%s bdaddr %s conn %p", hdev->name, batostr(&cp->peer_addr), > > + conn); > > + > > + if (status) { > > + if (conn && conn->le_state == BT_CONNECT) { > > + conn->le_state = BT_CLOSED; > > + hci_proto_connect_cfm(conn, status); > > + hci_conn_del(conn); > > + } > > + } else { > > + if (!conn) { > > + conn = hci_conn_add(hdev, LE_LINK, &cp->peer_addr); > > + if (conn) { > > + conn->out = 1; > > + conn->link_mode |= HCI_LM_MASTER; > > + } else > > + BT_ERR("No memory for new connection"); > > + } > > + } > > + > > + hci_dev_unlock(hdev); > > +} > > Do we have the master/slave association with LE? Yes. But it cannot be changed like in legacy bluetooth connection. conn->out and conn->link_mode |= HCI_LM_MASTER are redutant information. I removed conn->link_mode setting. > > static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) > > { > > __u8 status = *((__u8 *) skb->data); > > @@ -1069,7 +1107,10 @@ static inline void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff > > > > conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle)); > > if (conn) { > > - conn->state = BT_CLOSED; > > + if (conn->type == LE_LINK) > > + conn->le_state = BT_CLOSED; > > + else > > + conn->state = BT_CLOSED; > > If we would just use conn->state then this should not be needed. > > > hci_proto_disconn_cfm(conn, ev->reason); > > hci_conn_del(conn); > > @@ -1430,6 +1471,10 @@ static inline void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb) > > hci_cs_exit_sniff_mode(hdev, ev->status); > > break; > > > > + case HCI_OP_LE_CREATE_CONN: > > + hci_cs_le_create_conn(hdev, ev->status); > > + break; > > + > > default: > > BT_DBG("%s opcode 0x%x", hdev->name, opcode); > > break; > > @@ -1875,6 +1920,55 @@ static inline void hci_remote_host_features_evt(struct hci_dev *hdev, struct sk_ > > hci_dev_unlock(hdev); > > } > > > > +static inline void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) > > +{ > > + struct hci_ev_le_conn_complete *ev = (void *) skb->data; > > + struct hci_conn *conn; > > + > > + BT_DBG("%s status %d", hdev->name, ev->status); > > + > > + hci_dev_lock(hdev); > > + > > + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr); > > + > > + if (!conn) > > + goto unlock; > > The empty line between conn assignment and check should be removed. Done > > > + > > + if (ev->status) { > > + hci_proto_connect_cfm(conn, ev->status); > > + conn->le_state = BT_CLOSED; > > + hci_conn_del(conn); > > + goto unlock; > > + } > > + > > + conn->handle = __le16_to_cpu(ev->handle); > > + conn->le_state = BT_CONNECTED; > > + > > + hci_conn_hold_device(conn); > > + hci_conn_add_sysfs(conn); > > + > > + hci_proto_connect_cfm(conn, ev->status); > > +unlock: > > + hci_dev_unlock(hdev); > > And here you should have an empty line between the label and the last > statement before the label. Done > > +} > > + > > +static inline void hci_le_meta_evt(struct hci_dev *hdev, struct sk_buff *skb) > > +{ > > + struct hci_ev_le_meta *le_ev = (void *) skb->data; > > + __u8 event = le_ev->subevent; > > Why? > > > + > > + skb_pull(skb, sizeof(*le_ev)); > > + > > + switch (event) { > > Using le_ev->subevent would be just fine here. Done > > + case HCI_EV_LE_CONN_COMPLETE: > > + hci_le_conn_complete_evt(hdev, skb); > > + break; > > + > > + default: > > + break; > > + } > > +} > > + > > void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb) > > { > > struct hci_event_hdr *hdr = (void *) skb->data; > > @@ -2011,6 +2105,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb) > > hci_remote_host_features_evt(hdev, skb); > > break; > > > > + case HCI_EV_LE_META: > > + hci_le_meta_evt(hdev, skb); > > + break; > > + > > default: > > BT_DBG("%s event 0x%x", hdev->name, event); > > break; > -- Ville