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.
I do not follow the difference between master and slave roles. Why do we have a difference here in the first place.
> 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.
I am getting the feeling the overall code gets more complicated than simpler. The goal is to make the connection handling simpler. And right now, I do not see this.
Regards
Marcel
--
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
