2012-05-31 00:20:16

by Vinicius Costa Gomes

Subject: [PATCH 1/5] Bluetooth: Refactor LE connection into its own function

The code that handles LE connection is already quite separated from
the rest of the connection procedure, so we can easily put it into
its own.

Signed-off-by: Vinicius Costa Gomes <[email protected]>
---

Because of the moving around, these patches aren't as easy to follow as one would
expect.


net/bluetooth/hci_conn.c | 44 +++++++++++++++++++++++++-------------------
1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 1458667b..a7bfc27 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -469,6 +469,29 @@ struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src)
}
EXPORT_SYMBOL(hci_get_route);

+static struct hci_conn *add_le_conn(struct hci_dev *hdev, bdaddr_t *dst,
+ u8 dst_type, u8 sec_level, u8 auth_type)
+{
+ struct hci_conn *le;
+
+ le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
+ if (!le) {
+ le = hci_conn_add(hdev, LE_LINK, dst);
+ if (!le)
+ return ERR_PTR(-ENOMEM);
+
+ le->dst_type = bdaddr_to_le(dst_type);
+ hci_le_connect(le);
+ }
+
+ le->pending_sec_level = sec_level;
+ le->auth_type = auth_type;
+
+ hci_conn_hold(le);
+
+ return le;
+}
+
/* Create SCO, ACL or LE connection.
* Device _must_ be locked */
struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
@@ -476,28 +499,11 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
{
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 ERR_PTR(-ENOMEM);
-
- le->dst_type = bdaddr_to_le(dst_type);
- hci_le_connect(le);
- }
-
- le->pending_sec_level = sec_level;
- le->auth_type = auth_type;
-
- hci_conn_hold(le);
-
- return le;
- }
+ if (type == LE_LINK)
+ return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);

acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
if (!acl) {
--
1.7.10.3

2012-05-31 17:27:22

by Vinicius Costa Gomes

Subject: Re: [PATCH 4/5] Bluetooth: Simplify a the connection type handling

Hi,

On 10:22 Thu 31 May, Gustavo Padovan wrote:
> Hi Andrei,
>
> * Andrei Emeltchenko <[email protected]> [2012-05-31 10:39:16 +0300]:
>
> > Hi Vinicius,
> >
> > On Wed, May 30, 2012 at 09:20:19PM -0300, Vinicius Costa Gomes wrote:
> > > Now that we have separate ways of doing connections for each link type,
> > > we can do better than an "if" statement to handle each link type.
> > >
> > > Signed-off-by: Vinicius Costa Gomes <[email protected]>
> > > ---
> > > net/bluetooth/hci_conn.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > > index 320cc5d..8848a1e 100644
> > > --- a/net/bluetooth/hci_conn.c
> > > +++ b/net/bluetooth/hci_conn.c
> > > @@ -563,13 +563,16 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
> > > {
> > > BT_DBG("%s dst %s", hdev->name, batostr(dst));
> > >
> > > - if (type == LE_LINK)
> > > + switch (type) {
> > > + case LE_LINK:
> > > return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
> > > -
> > > - if (type == ACL_LINK)
> > > + case ACL_LINK:
> > > return add_acl_conn(hdev, dst, sec_level, auth_type);
> > > + case SCO_LINK:
> > > + return add_sco_conn(hdev, dst, sec_level, auth_type);
> >
> > The patches looks OK but somehow I do not like much returns in switch.
> > Some parsers will notice missing breaks which are not needed here...
>
> Use return here seems ok to me, I just want that these functions gets a hci_
> prefix.

Fair enough. So I am going with hci_add_{acl,sco,le}.

>
> Gustavo


Cheers,
--
Vinicius

2012-05-31 13:22:05

by Gustavo Padovan

Subject: Re: [PATCH 4/5] Bluetooth: Simplify a the connection type handling

Hi Andrei,

* Andrei Emeltchenko <[email protected]> [2012-05-31 10:39:16 +0300]:

> Hi Vinicius,
>
> On Wed, May 30, 2012 at 09:20:19PM -0300, Vinicius Costa Gomes wrote:
> > Now that we have separate ways of doing connections for each link type,
> > we can do better than an "if" statement to handle each link type.
> >
> > Signed-off-by: Vinicius Costa Gomes <[email protected]>
> > ---
> > net/bluetooth/hci_conn.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 320cc5d..8848a1e 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -563,13 +563,16 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
> > {
> > BT_DBG("%s dst %s", hdev->name, batostr(dst));
> >
> > - if (type == LE_LINK)
> > + switch (type) {
> > + case LE_LINK:
> > return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
> > -
> > - if (type == ACL_LINK)
> > + case ACL_LINK:
> > return add_acl_conn(hdev, dst, sec_level, auth_type);
> > + case SCO_LINK:
> > + return add_sco_conn(hdev, dst, sec_level, auth_type);
>
> The patches looks OK but somehow I do not like much returns in switch.
> Some parsers will notice missing breaks which are not needed here...

Use return here seems ok to me, I just want that these functions gets a hci_
prefix.

Gustavo

2012-05-31 07:43:25

by Andrei Emeltchenko

Subject: Re: [PATCH 4/5] Bluetooth: Simplify a the connection type handling

Hi Vinicius,

On Wed, May 30, 2012 at 09:20:19PM -0300, Vinicius Costa Gomes wrote:
> return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
> return add_acl_conn(hdev, dst, sec_level, auth_type);
> + return add_sco_conn(hdev, dst, sec_level, auth_type);

Those new functions are only ones missing hci prefix in that file? Maybe
names like hci_add_acl or hci_connect_acl would be better?

Best regards
Andrei Emeltchenko

2012-05-31 07:39:16

by Andrei Emeltchenko

Subject: Re: [PATCH 4/5] Bluetooth: Simplify a the connection type handling

Hi Vinicius,

On Wed, May 30, 2012 at 09:20:19PM -0300, Vinicius Costa Gomes wrote:
> Now that we have separate ways of doing connections for each link type,
> we can do better than an "if" statement to handle each link type.
>
> Signed-off-by: Vinicius Costa Gomes <[email protected]>
> ---
> net/bluetooth/hci_conn.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 320cc5d..8848a1e 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -563,13 +563,16 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
> {
> BT_DBG("%s dst %s", hdev->name, batostr(dst));
>
> - if (type == LE_LINK)
> + switch (type) {
> + case LE_LINK:
> return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
> -
> - if (type == ACL_LINK)
> + case ACL_LINK:
> return add_acl_conn(hdev, dst, sec_level, auth_type);
> + case SCO_LINK:
> + return add_sco_conn(hdev, dst, sec_level, auth_type);

The patches looks OK but somehow I do not like much returns in switch.
Some parsers will notice missing breaks which are not needed here...

Best regards
Andrei Emeltchenko

2012-05-31 00:20:20

by Vinicius Costa Gomes

Subject: [PATCH 5/5] Bluetooth: Add type information to the hci_connect() debug statement

Now that we have a "connect" function for each link type, we should be
able to indentify which function is going to be called.

Signed-off-by: Vinicius Costa Gomes <[email protected]>
---
net/bluetooth/hci_conn.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 8848a1e..d875c68 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -561,7 +561,7 @@ static struct hci_conn *add_sco_conn(struct hci_dev *hdev, bdaddr_t *dst,
struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
__u8 dst_type, __u8 sec_level, __u8 auth_type)
{
- BT_DBG("%s dst %s", hdev->name, batostr(dst));
+ BT_DBG("%s dst %s type 0x%x", hdev->name, batostr(dst), type);

switch (type) {
case LE_LINK:
--
1.7.10.3

2012-05-31 00:20:19

by Vinicius Costa Gomes

Subject: [PATCH 4/5] Bluetooth: Simplify a the connection type handling

Now that we have separate ways of doing connections for each link type,
we can do better than an "if" statement to handle each link type.

Signed-off-by: Vinicius Costa Gomes <[email protected]>
---
net/bluetooth/hci_conn.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 320cc5d..8848a1e 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -563,13 +563,16 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
{
BT_DBG("%s dst %s", hdev->name, batostr(dst));

- if (type == LE_LINK)
+ switch (type) {
+ case LE_LINK:
return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
-
- if (type == ACL_LINK)
+ case ACL_LINK:
return add_acl_conn(hdev, dst, sec_level, auth_type);
+ case SCO_LINK:
+ return add_sco_conn(hdev, dst, sec_level, auth_type);
+ }

- return add_sco_conn(hdev, dst, sec_level, auth_type);
+ return ERR_PTR(-EINVAL);
}

/* Check link security requirement */
--
1.7.10.3

2012-05-31 00:20:18

by Vinicius Costa Gomes

Subject: [PATCH 3/5] Bluetooth: Refactor SCO connection into its own function

We can do the same that we did for the other link types, for SCO
connections. The only thing that's worth noting is that as SCO
links need an ACL link, this functions uses the function that adds
an ACL link.

Signed-off-by: Vinicius Costa Gomes <[email protected]>
---
net/bluetooth/hci_conn.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ac12cbd..320cc5d 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -516,29 +516,19 @@ static struct hci_conn *add_acl_conn(struct hci_dev *hdev, bdaddr_t *dst,
return acl;
}

-/* 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 dst_type, __u8 sec_level, __u8 auth_type)
+static struct hci_conn *add_sco_conn(struct hci_dev *hdev, bdaddr_t *dst,
+ u8 sec_level, u8 auth_type)
{
struct hci_conn *acl;
struct hci_conn *sco;

- BT_DBG("%s dst %s", hdev->name, batostr(dst));
-
- if (type == LE_LINK)
- return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
-
acl = add_acl_conn(hdev, dst, sec_level, auth_type);
if (IS_ERR(acl))
return acl;

- if (type == ACL_LINK)
- return acl;
-
- sco = hci_conn_hash_lookup_ba(hdev, type, dst);
+ sco = hci_conn_hash_lookup_ba(hdev, SCO_LINK, dst);
if (!sco) {
- sco = hci_conn_add(hdev, type, dst);
+ sco = hci_conn_add(hdev, SCO_LINK, dst);
if (!sco) {
hci_conn_put(acl);
return ERR_PTR(-ENOMEM);
@@ -567,6 +557,21 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
return sco;
}

+/* Create SCO, ACL or LE connection. */
+struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
+ __u8 dst_type, __u8 sec_level, __u8 auth_type)
+{
+ BT_DBG("%s dst %s", hdev->name, batostr(dst));
+
+ if (type == LE_LINK)
+ return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
+
+ if (type == ACL_LINK)
+ return add_acl_conn(hdev, dst, sec_level, auth_type);
+
+ return add_sco_conn(hdev, dst, sec_level, auth_type);
+}
+
/* Check link security requirement */
int hci_conn_check_link_mode(struct hci_conn *conn)
{
--
1.7.10.3

2012-05-31 00:20:17

by Vinicius Costa Gomes

Subject: [PATCH 2/5] Bluetooth: Refactor ACL connection into its own function

The hci_connect() function was starting to get too complicated to be
quickly understood. We can separated the creation of a new ACL
connection into its own function.

Signed-off-by: Vinicius Costa Gomes <[email protected]>
---
net/bluetooth/hci_conn.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index a7bfc27..ac12cbd 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -492,18 +492,10 @@ static struct hci_conn *add_le_conn(struct hci_dev *hdev, bdaddr_t *dst,
return le;
}

-/* 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 dst_type, __u8 sec_level, __u8 auth_type)
+static struct hci_conn *add_acl_conn(struct hci_dev *hdev, bdaddr_t *dst,
+ u8 sec_level, u8 auth_type)
{
struct hci_conn *acl;
- struct hci_conn *sco;
-
- BT_DBG("%s dst %s", hdev->name, batostr(dst));
-
- if (type == LE_LINK)
- return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);

acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
if (!acl) {
@@ -521,6 +513,26 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
hci_acl_connect(acl);
}

+ return acl;
+}
+
+/* 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 dst_type, __u8 sec_level, __u8 auth_type)
+{
+ struct hci_conn *acl;
+ struct hci_conn *sco;
+
+ BT_DBG("%s dst %s", hdev->name, batostr(dst));
+
+ if (type == LE_LINK)
+ return add_le_conn(hdev, dst, dst_type, sec_level, auth_type);
+
+ acl = add_acl_conn(hdev, dst, sec_level, auth_type);
+ if (IS_ERR(acl))
+ return acl;
+
if (type == ACL_LINK)
return acl;

--
1.7.10.3