Hi Marcel,
On Thu, Oct 3, 2013 at 11:15 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.
>
> 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.
Ok, I'll drop this patch in v2.
I'll recheck if we really need to differentiate between master and
slave new connection for auto connection. If positive, I can add this
patch to auto connection patchset.
Thanks,
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
