[PATCH BlueZ v7 1/9] core: Mutually exclude concurrent connections

[PATCH BlueZ v7 1/9] core: Mutually exclude concurrent connections

[João Paulo Rechi Vita]

Since controllers don't support more than one ongoing connection
procedure at the same time, new connection attempts needs to yield if
there is an ongoing connection procedure already.
---
 src/adapter.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
 src/device.c  |  6 +++---
 src/device.h  |  2 +-
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index e154c69..47f82c0 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -135,6 +135,8 @@ struct btd_adapter {
 	GSList *connect_list;		/* Devices to connect when found */
 	guint discov_id;		/* Discovery timer */
 	gboolean discovering;		/* Discovery active */
+	gboolean connecting;		/* Connect active */
+	guint waiting_to_connect;	/* # of devices waiting to connect */
 	gboolean discov_suspended;	/* Discovery suspended */
 	guint auto_timeout_id;		/* Automatic connections timeout */
 	sdp_list_t *services;		/* Services associated to adapter */
@@ -2242,6 +2244,9 @@ void btd_adapter_start(struct btd_adapter *adapter)
 	call_adapter_powered_callbacks(adapter, TRUE);
 
 	info("Adapter %s has been enabled", adapter->path);
+
+	if (g_slist_length(adapter->connect_list) > 0)
+		mgmt_start_discovery(adapter->dev_id);
 }
 
 static void reply_pending_requests(struct btd_adapter *adapter)
@@ -2565,6 +2570,7 @@ void adapter_set_discovering(struct btd_adapter *adapter,
 						gboolean discovering)
 {
 	const char *path = adapter->path;
+	guint connect_list_len;
 
 	adapter->discovering = discovering;
 
@@ -2579,11 +2585,17 @@ void adapter_set_discovering(struct btd_adapter *adapter,
 	g_slist_free_full(adapter->oor_devices, dev_info_free);
 	adapter->oor_devices = g_slist_copy(adapter->found_devices);
 
-	if (!adapter_has_discov_sessions(adapter) || adapter->discov_suspended)
+	if (adapter->discov_suspended)
+		return;
+
+	connect_list_len = g_slist_length(adapter->connect_list);
+
+	if (!adapter_has_discov_sessions(adapter) && connect_list_len == 0)
 		return;
 
-	DBG("hci%u restarting discovery, disc_sessions %u", adapter->dev_id,
-					g_slist_length(adapter->disc_sessions));
+	DBG("hci%u restarting discovery: disc_sessions %u connect_list_len %u",
+		adapter->dev_id, g_slist_length(adapter->disc_sessions),
+							connect_list_len);
 
 	adapter->discov_id = g_idle_add(discovery_cb, adapter);
 }
@@ -2890,17 +2902,46 @@ static char *read_stored_data(bdaddr_t *local, bdaddr_t *peer,
 	return textfile_get(filename, key);
 }
 
+static gboolean clean_connecting_state(GIOChannel *io, GIOCondition cond,
+							gpointer user_data)
+{
+	struct btd_device *device = user_data;
+	struct btd_adapter *adapter = device_get_adapter(device);
+
+	adapter->connecting = FALSE;
+
+	if (adapter->waiting_to_connect == 0 &&
+				g_slist_length(adapter->connect_list) > 0)
+		mgmt_start_discovery(adapter->dev_id);
+
+	btd_device_unref(device);
+
+	return FALSE;
+}
+
 static gboolean connect_pending_cb(gpointer user_data)
 {
 	struct btd_device *device = user_data;
 	struct btd_adapter *adapter = device_get_adapter(device);
+	GIOChannel *io;
 
 	/* in the future we may want to check here if the controller supports
 	 * scanning and connecting at the same time */
 	if (adapter->discovering)
 		return TRUE;
 
-	device_att_connect(device);
+	if (adapter->connecting)
+		return TRUE;
+
+	adapter->connecting = TRUE;
+	adapter->waiting_to_connect--;
+
+	io = device_att_connect(device);
+	g_io_add_watch(io, G_IO_OUT | G_IO_ERR, clean_connecting_state,
+						btd_device_ref(device));
+	g_io_channel_unref(io);
+
+	btd_device_unref(device);
 
 	return FALSE;
 }
@@ -2993,12 +3034,19 @@ void adapter_update_found_devices(struct btd_adapter *adapter,
 
 	if (bdaddr_type == BDADDR_LE_PUBLIC ||
 					bdaddr_type == BDADDR_LE_RANDOM) {
+		struct btd_device *device;
+
 		l = g_slist_find_custom(adapter->connect_list, bdaddr,
 					(GCompareFunc) device_bdaddr_cmp);
-		if (l) {
-			g_idle_add(connect_pending_cb, l->data);
-			stop_discovery(adapter);
-		}
+		if (!l)
+			goto done;
+
+		device = l->data;
+		adapter_connect_list_remove(adapter, device);
+
+		g_idle_add(connect_pending_cb,  btd_device_ref(device));
+		stop_discovery(adapter);
+		adapter->waiting_to_connect++;
 	}
 
 done:
diff --git a/src/device.c b/src/device.c
index 57fdf00..5d92ff5 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2007,7 +2007,7 @@ static void att_success_cb(gpointer user_data)
 	g_slist_foreach(device->attios, attio_connected, device->attrib);
 }
 
-gboolean device_att_connect(gpointer user_data)
+GIOChannel *device_att_connect(gpointer user_data)
 {
 	struct btd_device *device = user_data;
 	struct btd_adapter *adapter = device->adapter;
@@ -2050,12 +2050,12 @@ gboolean device_att_connect(gpointer user_data)
 		error("ATT bt_io_connect(%s): %s", addr, gerr->message);
 		g_error_free(gerr);
 		g_free(attcb);
-		return FALSE;
+		return NULL;
 	}
 
 	device->att_io = io;
 
-	return FALSE;
+	return g_io_channel_ref(io);
 }
 
 static void att_browse_error_cb(const GError *gerr, gpointer user_data)
diff --git a/src/device.h b/src/device.h
index aee6d13..d9aa6f5 100644
--- a/src/device.h
+++ b/src/device.h
@@ -124,4 +124,4 @@ int device_unblock(struct btd_device *device, gboolean silent,
 void device_set_pnpid(struct btd_device *device, uint8_t vendor_id_src,
 			uint16_t vendor_id, uint16_t product_id,
 			uint16_t product_ver);
-gboolean device_att_connect(gpointer user_data);
+GIOChannel *device_att_connect(gpointer user_data);
-- 
1.7.11.4

[PATCH BlueZ v7 2/9] mgmt: Add LE scanning callback

[João Paulo Rechi Vita]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch adds a new callback to allow the adapter to control LE
scanning. The current approach uses the active scanning with default
windows and intervals defined by the core spec without any filtering.
---
 src/mgmt.c | 34 ++++++++++++++++++++++++++++++++++
 src/mgmt.h |  1 +
 2 files changed, 35 insertions(+)

diff --git a/src/mgmt.c b/src/mgmt.c
index dbd9786..405a683 100644
--- a/src/mgmt.c
+++ b/src/mgmt.c
@@ -2055,6 +2055,40 @@ int mgmt_start_discovery(int index)
 	return 0;
 }
 
+int mgmt_start_scanning(int index)
+{
+	char buf[MGMT_HDR_SIZE + sizeof(struct mgmt_cp_start_discovery)];
+	struct mgmt_hdr *hdr = (void *) buf;
+	struct mgmt_cp_start_discovery *cp = (void *) &buf[sizeof(*hdr)];
+	struct controller_info *info = &controllers[index];
+
+	DBG("index %d", index);
+
+	if (!mgmt_low_energy(info->current_settings)) {
+		error("scanning failed: Low Energy not enabled/supported");
+		return -ENOTSUP;
+	}
+
+	info->discov_type = 0;
+	hci_set_bit(BDADDR_LE_PUBLIC, &info->discov_type);
+	hci_set_bit(BDADDR_LE_RANDOM, &info->discov_type);
+
+	memset(buf, 0, sizeof(buf));
+	hdr->opcode = htobs(MGMT_OP_START_DISCOVERY);
+	hdr->len = htobs(sizeof(*cp));
+	hdr->index = htobs(index);
+
+	cp->type = info->discov_type;
+
+	if (write(mgmt_sock, buf, sizeof(buf)) < 0) {
+		int err = -errno;
+		error("failed to write to MGMT socket: %s", strerror(-err));
+		return err;
+	}
+
+	return 0;
+}
+
 int mgmt_stop_discovery(int index)
 {
 	char buf[MGMT_HDR_SIZE + sizeof(struct mgmt_cp_start_discovery)];
diff --git a/src/mgmt.h b/src/mgmt.h
index 95245d2..eb7434a 100644
--- a/src/mgmt.h
+++ b/src/mgmt.h
@@ -33,6 +33,7 @@ int mgmt_set_dev_class(int index, uint8_t major, uint8_t minor);
 int mgmt_set_fast_connectable(int index, gboolean enable);
 
 int mgmt_start_discovery(int index);
+int mgmt_start_scanning(int index);
 int mgmt_stop_discovery(int index);
 
 int mgmt_read_clock(int index, bdaddr_t *bdaddr, int which, int timeout,
-- 
1.7.11.4

Re: [PATCH BlueZ v7 2/9] mgmt: Add LE scanning callback

[Johan Hedberg]

Hi,

On Tue, Sep 25, 2012, João Paulo Rechi Vita wrote:
> +int mgmt_start_scanning(int index)

Firstly I don't think the name is good since BR/EDR also has the concept
of scanning (page scan & inquiry scan). Secondly, maybe it'd be simpler
to reuse mgmt_start_discovery and simply add a new parameter which
provides the value for info->discov_type?

Johan

Re: [PATCH BlueZ v7 2/9] mgmt: Add LE scanning callback

[Claudio Takahasi]

Hi Johan:

On Thu, Sep 27, 2012 at 4:43 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Tue, Sep 25, 2012, João Paulo Rechi Vita wrote:
>> +int mgmt_start_scanning(int index)
>
> Firstly I don't think the name is good since BR/EDR also has the concept
> of scanning (page scan & inquiry scan). Secondly, maybe it'd be simpler
> to reuse mgmt_start_discovery and simply add a new parameter which
> provides the value for info->discov_type?
>
> Johan

My suggestions are:
1. rename to mgmt_start_le_scanning
2. add a new parameter to mgmt_start_discovery: gboolean le_only
3. add a new parameter to mgmt_start_discovery: informing the adapter
operation mode(BREDR/LE/BREDR_LE)

Option 3 will require to expose controller info/settings to adapter.c.
It is necessary to know the controller features to send the right
discover type. btd_adapter_start function could be extended to receive
the supported features or a parameter specifying the operation mode.
IMO, using this approach we are duplicating information.

Which approach do you prefer?

Regards,
Claudio.

Re: [PATCH BlueZ v7 2/9] mgmt: Add LE scanning callback

[Johan Hedberg]

Hi Claudio,

On Fri, Sep 28, 2012, Claudio Takahasi wrote:
> On Thu, Sep 27, 2012 at 4:43 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > On Tue, Sep 25, 2012, João Paulo Rechi Vita wrote:
> >> +int mgmt_start_scanning(int index)
> >
> > Firstly I don't think the name is good since BR/EDR also has the concept
> > of scanning (page scan & inquiry scan). Secondly, maybe it'd be simpler
> > to reuse mgmt_start_discovery and simply add a new parameter which
> > provides the value for info->discov_type?
> 
> My suggestions are:
> 1. rename to mgmt_start_le_scanning
> 2. add a new parameter to mgmt_start_discovery: gboolean le_only
> 3. add a new parameter to mgmt_start_discovery: informing the adapter
> operation mode(BREDR/LE/BREDR_LE)
> 
> Option 3 will require to expose controller info/settings to adapter.c.
> It is necessary to know the controller features to send the right
> discover type. btd_adapter_start function could be extended to receive
> the supported features or a parameter specifying the operation mode.
> IMO, using this approach we are duplicating information.
> 
> Which approach do you prefer?

I'd go with 1 for now. Booleans in places where it's not utterly clear
from the calling code what it means shouldn't be used (even enums are
better), i.e. if you don't know the implementation details of the
function there'd be no way for an outsider to know what exactly the
"TRUE" in mgmt_start_discovery(TRUE) means.

Johan

[PATCH BlueZ v7 3/9] core: Replace interleaved by LE scanning

[João Paulo Rechi Vita]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patches replaces the interleaved discovery by LE scanning when LE
re-connection is required.
---
 src/adapter.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 47f82c0..49dee85 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2246,7 +2246,7 @@ void btd_adapter_start(struct btd_adapter *adapter)
 	info("Adapter %s has been enabled", adapter->path);
 
 	if (g_slist_length(adapter->connect_list) > 0)
-		mgmt_start_discovery(adapter->dev_id);
+		mgmt_start_scanning(adapter->dev_id);
 }
 
 static void reply_pending_requests(struct btd_adapter *adapter)
@@ -2590,14 +2590,22 @@ void adapter_set_discovering(struct btd_adapter *adapter,
 
 	connect_list_len = g_slist_length(adapter->connect_list);
 
-	if (!adapter_has_discov_sessions(adapter) && connect_list_len == 0)
+	if (adapter_has_discov_sessions(adapter)) {
+		adapter->discov_id = g_idle_add(discovery_cb, adapter);
+
+		DBG("hci%u restarting discovery: disc_sessions %u",
+				adapter->dev_id,
+				g_slist_length(adapter->disc_sessions));
 		return;
+	}
 
-	DBG("hci%u restarting discovery: disc_sessions %u connect_list_len %u",
-		adapter->dev_id, g_slist_length(adapter->disc_sessions),
-							connect_list_len);
+	if (connect_list_len > 0) {
+		mgmt_start_scanning(adapter->dev_id);
 
-	adapter->discov_id = g_idle_add(discovery_cb, adapter);
+		DBG("hci%u restarting scanning connect_list_len %u",
+				adapter->dev_id, connect_list_len);
+		return;
+	}
 }
 
 static void suspend_discovery(struct btd_adapter *adapter)
@@ -2912,7 +2920,7 @@ static gboolean clean_connecting_state(GIOChannel *io, GIOCondition cond,
 
 	if (adapter->waiting_to_connect == 0 &&
 				g_slist_length(adapter->connect_list) > 0)
-		mgmt_start_discovery(adapter->dev_id);
+		mgmt_start_scanning(adapter->dev_id);
 
 	btd_device_unref(device);
 
-- 
1.7.11.4

[PATCH BlueZ v7 4/9] core: Start LE scanning when a device requests

[João Paulo Rechi Vita]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch enables the LE scanning when a device requires connection and
there isn't discovery sessions, triggering the General Connection
Establishment Procedure.
---
 src/adapter.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index 49dee85..201e6a0 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2200,6 +2200,11 @@ void adapter_connect_list_add(struct btd_adapter *adapter,
 						btd_device_ref(device));
 	DBG("%s added to %s's connect_list", device_get_path(device),
 								adapter->name);
+
+	if (adapter->disc_sessions)
+		return;
+
+	mgmt_start_scanning(adapter->dev_id);
 }
 
 void adapter_connect_list_remove(struct btd_adapter *adapter,
-- 
1.7.11.4

[PATCH BlueZ v7 5/9] core: Queue discovery if scanning is active

[João Paulo Rechi Vita]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch manages BR/EDR inquiry and BLE scanning discovery sessions.
A scanning session is added in the discovery session list when there is
a bonded device which requires re-connection.

bluetoothd decides if interleaved or scanning needs to be executed based
on the queued discovery sessions. Interleaved discovery has higher
priority, scanning only is executed when there is only a scanning
session active.
---
 src/adapter.c | 65 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 201e6a0..0a0ac8f 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -132,6 +132,7 @@ struct btd_adapter {
 	GSList *devices;		/* Devices structure pointers */
 	GSList *mode_sessions;		/* Request Mode sessions */
 	GSList *disc_sessions;		/* Discovery sessions */
+	struct session_req *scanning_session;
 	GSList *connect_list;		/* Devices to connect when found */
 	guint discov_id;		/* Discovery timer */
 	gboolean discovering;		/* Discovery active */
@@ -221,17 +222,18 @@ static struct session_req *create_session(struct btd_adapter *adapter,
 					DBusMessage *msg, uint8_t mode,
 					GDBusWatchFunction cb)
 {
-	const char *sender = dbus_message_get_sender(msg);
+	const char *sender;
 	struct session_req *req;
 
 	req = g_new0(struct session_req, 1);
 	req->adapter = adapter;
-	req->msg = dbus_message_ref(msg);
 	req->mode = mode;
 
-	if (cb == NULL)
+	if (cb == NULL || msg == NULL)
 		return session_ref(req);
 
+	sender = dbus_message_get_sender(msg);
+	req->msg = dbus_message_ref(msg);
 	req->owner = g_strdup(sender);
 	req->id = g_dbus_add_disconnect_watch(btd_get_dbus_connection(),
 							sender, cb, req, NULL);
@@ -442,7 +444,9 @@ static struct session_req *find_session(GSList *list, const char *sender)
 	for (; list; list = list->next) {
 		struct session_req *req = list->data;
 
-		if (g_str_equal(req->owner, sender))
+		/* req->owner may be NULL if the session has been added by the
+		 * daemon itself, so we use g_strcmp0 instead of g_str_equal */
+		if (g_strcmp0(req->owner, sender) == 0)
 			return req;
 	}
 
@@ -517,7 +521,7 @@ static void session_remove(struct session_req *req)
 	struct btd_adapter *adapter = req->adapter;
 
 	/* Ignore set_mode session */
-	if (req->owner == NULL)
+	if (req->owner == NULL && adapter->pending_mode)
 		return;
 
 	DBG("%s session %p with %s deactivated",
@@ -997,7 +1001,12 @@ static gboolean discovery_cb(gpointer user_data)
 	struct btd_adapter *adapter = user_data;
 
 	adapter->discov_id = 0;
-	mgmt_start_discovery(adapter->dev_id);
+
+	if (adapter->scanning_session &&
+			(g_slist_length(adapter->disc_sessions) == 1))
+		mgmt_start_scanning(adapter->dev_id);
+	else
+		mgmt_start_discovery(adapter->dev_id);
 
 	return FALSE;
 }
@@ -2190,6 +2199,8 @@ const char *btd_adapter_get_name(struct btd_adapter *adapter)
 void adapter_connect_list_add(struct btd_adapter *adapter,
 					struct btd_device *device)
 {
+	struct session_req *req;
+
 	if (g_slist_find(adapter->connect_list, device)) {
 		DBG("ignoring already added device %s",
 						device_get_path(device));
@@ -2201,10 +2212,21 @@ void adapter_connect_list_add(struct btd_adapter *adapter,
 	DBG("%s added to %s's connect_list", device_get_path(device),
 								adapter->name);
 
-	if (adapter->disc_sessions)
+	if (!adapter->up)
+		return;
+
+	if (adapter->off_requested)
+		return;
+
+	if (adapter->scanning_session)
 		return;
 
-	mgmt_start_scanning(adapter->dev_id);
+	if (adapter->disc_sessions == NULL)
+		adapter->discov_id = g_idle_add(discovery_cb, adapter);
+
+	req = create_session(adapter, NULL, 0, NULL);
+	adapter->disc_sessions = g_slist_append(adapter->disc_sessions, req);
+	adapter->scanning_session = req;
 }
 
 void adapter_connect_list_remove(struct btd_adapter *adapter,
@@ -2224,6 +2246,7 @@ void adapter_connect_list_remove(struct btd_adapter *adapter,
 
 void btd_adapter_start(struct btd_adapter *adapter)
 {
+	struct session_req *req;
 	char address[18];
 	gboolean powered;
 
@@ -2250,8 +2273,15 @@ void btd_adapter_start(struct btd_adapter *adapter)
 
 	info("Adapter %s has been enabled", adapter->path);
 
-	if (g_slist_length(adapter->connect_list) > 0)
-		mgmt_start_scanning(adapter->dev_id);
+	if (g_slist_length(adapter->connect_list) == 0 ||
+					adapter->disc_sessions != NULL)
+		return;
+
+	req = create_session(adapter, NULL, 0, NULL);
+	adapter->disc_sessions = g_slist_append(adapter->disc_sessions, req);
+	adapter->scanning_session = req;
+
+	adapter->discov_id = g_idle_add(discovery_cb, adapter);
 }
 
 static void reply_pending_requests(struct btd_adapter *adapter)
@@ -2595,6 +2625,11 @@ void adapter_set_discovering(struct btd_adapter *adapter,
 
 	connect_list_len = g_slist_length(adapter->connect_list);
 
+	if (connect_list_len == 0 && adapter->scanning_session) {
+		session_unref(adapter->scanning_session);
+		adapter->scanning_session = NULL;
+	}
+
 	if (adapter_has_discov_sessions(adapter)) {
 		adapter->discov_id = g_idle_add(discovery_cb, adapter);
 
@@ -2603,14 +2638,6 @@ void adapter_set_discovering(struct btd_adapter *adapter,
 				g_slist_length(adapter->disc_sessions));
 		return;
 	}
-
-	if (connect_list_len > 0) {
-		mgmt_start_scanning(adapter->dev_id);
-
-		DBG("hci%u restarting scanning connect_list_len %u",
-				adapter->dev_id, connect_list_len);
-		return;
-	}
 }
 
 static void suspend_discovery(struct btd_adapter *adapter)
@@ -2925,7 +2952,7 @@ static gboolean clean_connecting_state(GIOChannel *io, GIOCondition cond,
 
 	if (adapter->waiting_to_connect == 0 &&
 				g_slist_length(adapter->connect_list) > 0)
-		mgmt_start_scanning(adapter->dev_id);
+		adapter->discov_id = g_idle_add(discovery_cb, adapter);
 
 	btd_device_unref(device);
 
-- 
1.7.11.4

Re: [PATCH BlueZ v7 5/9] core: Queue discovery if scanning is active

[Vinicius Costa Gomes]

Hi João, Claudio,

On 14:38 Tue 25 Sep, João Paulo Rechi Vita wrote:
> From: Claudio Takahasi <claudio.takahasi@openbossa.org>
> 
> This patch manages BR/EDR inquiry and BLE scanning discovery sessions.
> A scanning session is added in the discovery session list when there is
> a bonded device which requires re-connection.
> 
> bluetoothd decides if interleaved or scanning needs to be executed based
> on the queued discovery sessions. Interleaved discovery has higher
> priority, scanning only is executed when there is only a scanning
> session active.
> ---
>  src/adapter.c | 65 ++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index 201e6a0..0a0ac8f 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -132,6 +132,7 @@ struct btd_adapter {
>  	GSList *devices;		/* Devices structure pointers */
>  	GSList *mode_sessions;		/* Request Mode sessions */
>  	GSList *disc_sessions;		/* Discovery sessions */
> +	struct session_req *scanning_session;
>  	GSList *connect_list;		/* Devices to connect when found */
>  	guint discov_id;		/* Discovery timer */
>  	gboolean discovering;		/* Discovery active */
> @@ -221,17 +222,18 @@ static struct session_req *create_session(struct btd_adapter *adapter,
>  					DBusMessage *msg, uint8_t mode,
>  					GDBusWatchFunction cb)
>  {
> -	const char *sender = dbus_message_get_sender(msg);
> +	const char *sender;
>  	struct session_req *req;
>  
>  	req = g_new0(struct session_req, 1);
>  	req->adapter = adapter;
> -	req->msg = dbus_message_ref(msg);
>  	req->mode = mode;
>  
> -	if (cb == NULL)
> +	if (cb == NULL || msg == NULL)
>  		return session_ref(req);

Sorry for taking so long to notice this, but there is a problem here.
'req->msg' is needed in 'set_mode_complete()' (around adapter.c:2329) even
when 'cb' is NULL: when setting the "Powered" or "Discoverable" property,
the session created doesn't have a 'cb' but it has a 'msg'.


[snip]


Cheers,
-- 
Vinicius

Re: [PATCH BlueZ v7 5/9] core: Queue discovery if scanning is active

[Joao Paulo Rechi Vita]

On Thu, Sep 27, 2012 at 12:16 AM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> Hi João, Claudio,
>
> On 14:38 Tue 25 Sep, João Paulo Rechi Vita wrote:
>> From: Claudio Takahasi <claudio.takahasi@openbossa.org>
>>
>> This patch manages BR/EDR inquiry and BLE scanning discovery sessions.
>> A scanning session is added in the discovery session list when there is
>> a bonded device which requires re-connection.
>>
>> bluetoothd decides if interleaved or scanning needs to be executed based
>> on the queued discovery sessions. Interleaved discovery has higher
>> priority, scanning only is executed when there is only a scanning
>> session active.
>> ---
>>  src/adapter.c | 65 ++++++++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 46 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 201e6a0..0a0ac8f 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -132,6 +132,7 @@ struct btd_adapter {
>>       GSList *devices;                /* Devices structure pointers */
>>       GSList *mode_sessions;          /* Request Mode sessions */
>>       GSList *disc_sessions;          /* Discovery sessions */
>> +     struct session_req *scanning_session;
>>       GSList *connect_list;           /* Devices to connect when found */
>>       guint discov_id;                /* Discovery timer */
>>       gboolean discovering;           /* Discovery active */
>> @@ -221,17 +222,18 @@ static struct session_req *create_session(struct btd_adapter *adapter,
>>                                       DBusMessage *msg, uint8_t mode,
>>                                       GDBusWatchFunction cb)
>>  {
>> -     const char *sender = dbus_message_get_sender(msg);
>> +     const char *sender;
>>       struct session_req *req;
>>
>>       req = g_new0(struct session_req, 1);
>>       req->adapter = adapter;
>> -     req->msg = dbus_message_ref(msg);
>>       req->mode = mode;
>>
>> -     if (cb == NULL)
>> +     if (cb == NULL || msg == NULL)
>>               return session_ref(req);
>
> Sorry for taking so long to notice this, but there is a problem here.
> 'req->msg' is needed in 'set_mode_complete()' (around adapter.c:2329) even
> when 'cb' is NULL: when setting the "Powered" or "Discoverable" property,
> the session created doesn't have a 'cb' but it has a 'msg'.
>

Good catch, Vinicius. Thanks!

-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

[PATCH BlueZ v7 6/9] core: Disable unnecessary auto connections

[João Paulo Rechi Vita]

From: Paulo Alcantara <paulo.alcantara@openbossa.org>

BlueZ host disconnects the link when encryption fails. ECONNABORTED
error is returned by the kernel when the connection is terminated by the
local host. This scenario commonly happens when authentication fails due
PIN or Key Missing.
---
 src/device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/device.c b/src/device.c
index 5d92ff5..96d8b72 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1989,6 +1989,9 @@ static void att_error_cb(const GError *gerr, gpointer user_data)
 	struct att_callbacks *attcb = user_data;
 	struct btd_device *device = attcb->user_data;
 
+	if (g_error_matches(gerr, BT_IO_ERROR, ECONNABORTED))
+		return;
+
 	if (device->auto_connect == FALSE)
 		return;
 
-- 
1.7.11.4

[PATCH BlueZ v7 7/9] core: Re-connect for ECONNRESET or ECONNABORTED

[João Paulo Rechi Vita]

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch keeps scanning and re-connections active if the disconnection
reason is ECONNRESET(Remote Initiated Disconnection).

Re-connection is a behaviour determined by Profiles or by the upper
layer(user actions). For instance, HoG requires re-connection always
active, no matter if the previous disconnection reason was page timeout
or remote initiated disconnection (ECONNRESET). Some devices disconnects
after some idle time, connectable advertises are sent by the peripheral
when commanded by the user(eg: key pressed). Disconnection can be also
triggered by the local host (ECONNABORTED) using command line tools or
Disconnect method in the Device interface.

The peripheral dictates the re-connection controlling the connectable
advertises, BlueZ(central) needs to keep the scanning always active to
able to detect the advertises and trigger the connection.
---
 src/device.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/device.c b/src/device.c
index 96d8b72..d23ea46 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1889,10 +1889,18 @@ static gboolean attrib_disconnected_cb(GIOChannel *io, GIOCondition cond,
 
 	g_slist_foreach(device->attios, attio_disconnected, NULL);
 
-	if (device->auto_connect == FALSE || err != ETIMEDOUT)
+	if (device->auto_connect == FALSE) {
+		DBG("Automatic connection disabled");
 		goto done;
+	}
 
-	adapter_connect_list_add(device_get_adapter(device), device);
+	/*
+	 * Keep scanning/re-connection active if disconnection reason
+	 * is page timeout, remote user terminated connection or local
+	 * initiated disconnection.
+	 */
+	if (err == ETIMEDOUT || err == ECONNRESET || err == ECONNABORTED)
+		adapter_connect_list_add(device_get_adapter(device), device);
 
 done:
 	attio_cleanup(device);
-- 
1.7.11.4

Re: [PATCH BlueZ v7 7/9] core: Re-connect for ECONNRESET or ECONNABORTED

[Johan Hedberg]

Hi,

On Tue, Sep 25, 2012, João Paulo Rechi Vita wrote:
> +	/*
> +	 * Keep scanning/re-connection active if disconnection reason
> +	 * is page timeout, remote user terminated connection or local
> +	 * initiated disconnection.
> +	 */
> +	if (err == ETIMEDOUT || err == ECONNRESET || err == ECONNABORTED)
> +		adapter_connect_list_add(device_get_adapter(device), device);

Page timeouts have always been reported through EHOSTDOWN so either the
comment or the code is wrong here.

Johan

Re: [PATCH BlueZ v7 7/9] core: Re-connect for ECONNRESET or ECONNABORTED

[Claudio Takahasi]

Hi Johan:

On Thu, Sep 27, 2012 at 4:41 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Tue, Sep 25, 2012, João Paulo Rechi Vita wrote:
>> +     /*
>> +      * Keep scanning/re-connection active if disconnection reason
>> +      * is page timeout, remote user terminated connection or local
>> +      * initiated disconnection.
>> +      */
>> +     if (err == ETIMEDOUT || err == ECONNRESET || err == ECONNABORTED)
>> +             adapter_connect_list_add(device_get_adapter(device), device);
>
> Page timeouts have always been reported through EHOSTDOWN so either the
> comment or the code is wrong here.
>
> Johan

The comment is wrong: s/page/connection

Regards,
Claudio

[PATCH BlueZ v7 8/9] mgmt: Add address type to bonding debug message

[João Paulo Rechi Vita]

---
 src/mgmt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/mgmt.c b/src/mgmt.c
index 405a683..67a6993 100644
--- a/src/mgmt.c
+++ b/src/mgmt.c
@@ -2383,7 +2383,8 @@ int mgmt_create_bonding(int index, bdaddr_t *bdaddr, uint8_t addr_type, uint8_t
 	char addr[18];
 
 	ba2str(bdaddr, addr);
-	DBG("hci%d bdaddr %s io_cap 0x%02x", index, addr, io_cap);
+	DBG("hci%d bdaddr %s type %d io_cap 0x%02x",
+					index, addr, addr_type, io_cap);
 
 	memset(buf, 0, sizeof(buf));
 	hdr->opcode = htobs(MGMT_OP_PAIR_DEVICE);
-- 
1.7.11.4

[PATCH BlueZ v7 9/9] core: Suspend scanning before connect on pairing

[João Paulo Rechi Vita]

If there is a disconnected bonded device there will be a scanning
procedure active due to the General Connection Establishment Procedure.
This scan have to be suspended before trying to connect to the remote
device for pairing.
---
 src/device.c | 107 ++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 73 insertions(+), 34 deletions(-)

diff --git a/src/device.c b/src/device.c
index d23ea46..1cd9b5a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1959,6 +1959,36 @@ done:
 	browse_request_free(req);
 }
 
+static void bonding_request_free(struct bonding_req *bonding)
+{
+	struct btd_device *device;
+
+	if (!bonding)
+		return;
+
+	if (bonding->listener_id)
+		g_dbus_remove_watch(btd_get_dbus_connection(),
+							bonding->listener_id);
+
+	if (bonding->msg)
+		dbus_message_unref(bonding->msg);
+
+	device = bonding->device;
+	g_free(bonding);
+
+	if (!device)
+		return;
+
+	device->bonding = NULL;
+
+	if (!device->agent)
+		return;
+
+	agent_cancel(device->agent);
+	agent_free(device->agent);
+	device->agent = NULL;
+}
+
 static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 {
 	struct att_callbacks *attcb = user_data;
@@ -1988,6 +2018,21 @@ static void att_connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 
 	if (attcb->success)
 		attcb->success(user_data);
+
+	if (device->bonding) {
+		/* this is a LE device during pairing */
+		int err = adapter_create_bonding(device->adapter,
+				&device->bdaddr, device->bdaddr_type,
+				agent_get_io_capability(device->agent));
+		if (err < 0) {
+			DBusMessage *reply = btd_error_failed(
+					device->bonding->msg, strerror(-err));
+			g_dbus_send_message(btd_get_dbus_connection(), reply);
+			bonding_request_cancel(device->bonding);
+			bonding_request_free(device->bonding);
+		}
+	}
+
 done:
 	g_free(attcb);
 }
@@ -2039,16 +2084,30 @@ GIOChannel *device_att_connect(gpointer user_data)
 	attcb->user_data = device;
 
 	if (device_is_bredr(device)) {
-		io = bt_io_connect(att_connect_cb,
-					attcb, NULL, &gerr,
+		io = bt_io_connect(att_connect_cb, attcb, NULL, &gerr,
 					BT_IO_OPT_SOURCE_BDADDR, &sba,
 					BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
 					BT_IO_OPT_PSM, ATT_PSM,
 					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
 					BT_IO_OPT_INVALID);
+	} else if (device->bonding) {
+		/* this is a LE device during pairing, using low sec level */
+		io = bt_io_connect(att_connect_cb, attcb, NULL, &gerr,
+				BT_IO_OPT_SOURCE_BDADDR, &sba,
+				BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
+				BT_IO_OPT_DEST_TYPE, device->bdaddr_type,
+				BT_IO_OPT_CID, ATT_CID,
+				BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+				BT_IO_OPT_INVALID);
+		if (io == NULL) {
+			DBusMessage *reply = btd_error_failed(
+					device->bonding->msg, gerr->message);
+			g_dbus_send_message(btd_get_dbus_connection(), reply);
+			bonding_request_cancel(device->bonding);
+			bonding_request_free(device->bonding);
+		}
 	} else {
-		io = bt_io_connect(att_connect_cb,
-				attcb, NULL, &gerr,
+		io = bt_io_connect(att_connect_cb, attcb, NULL, &gerr,
 				BT_IO_OPT_SOURCE_BDADDR, &sba,
 				BT_IO_OPT_DEST_BDADDR, &device->bdaddr,
 				BT_IO_OPT_DEST_TYPE, device->bdaddr_type,
@@ -2368,36 +2427,6 @@ static DBusMessage *new_authentication_return(DBusMessage *msg, uint8_t status)
 	}
 }
 
-static void bonding_request_free(struct bonding_req *bonding)
-{
-	struct btd_device *device;
-
-	if (!bonding)
-		return;
-
-	if (bonding->listener_id)
-		g_dbus_remove_watch(btd_get_dbus_connection(),
-							bonding->listener_id);
-
-	if (bonding->msg)
-		dbus_message_unref(bonding->msg);
-
-	device = bonding->device;
-	g_free(bonding);
-
-	if (!device)
-		return;
-
-	device->bonding = NULL;
-
-	if (!device->agent)
-		return;
-
-	agent_cancel(device->agent);
-	agent_free(device->agent);
-	device->agent = NULL;
-}
-
 void device_set_paired(struct btd_device *device, gboolean value)
 {
 	if (device->paired == value)
@@ -2534,6 +2563,16 @@ DBusMessage *device_create_bonding(struct btd_device *device,
 	device->bonding = bonding;
 	bonding->device = device;
 
+	if (device_is_le(device)) {
+		adapter_connect_list_add(adapter, device);
+		return NULL;
+	}
+
+	err = adapter_create_bonding(adapter, &device->bdaddr,
+					device->bdaddr_type, capability);
+	if (err < 0)
+		return btd_error_failed(msg, strerror(-err));
+
 	return NULL;
 }
 
-- 
1.7.11.4

Re: [PATCH BlueZ v7 1/9] core: Mutually exclude concurrent connections

[Johan Hedberg]

Hi,

On Tue, Sep 25, 2012, João Paulo Rechi Vita wrote:
> Since controllers don't support more than one ongoing connection
> procedure at the same time, new connection attempts needs to yield if
> there is an ongoing connection procedure already.

Simply based on this description the first question that comes to mind
is wouldn't this be better handled on the kernel side? After all other
processes besides bluetoothd (like obexd) are also capable of initiating
connections and we can't control that within bluetoothd.

Johan

Re: [PATCH BlueZ v7 1/9] core: Mutually exclude concurrent connections

[Joao Paulo Rechi Vita]

Hello Johan,

On Thu, Sep 27, 2012 at 4:42 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Tue, Sep 25, 2012, João Paulo Rechi Vita wrote:
>> Since controllers don't support more than one ongoing connection
>> procedure at the same time, new connection attempts needs to yield if
>> there is an ongoing connection procedure already.
>
> Simply based on this description the first question that comes to mind
> is wouldn't this be better handled on the kernel side? After all other
> processes besides bluetoothd (like obexd) are also capable of initiating
> connections and we can't control that within bluetoothd.
>

This whole series implements the General Connection Establishment
Procedure (GCEP), as described in the core spec. According to this
procedure, when a new connection to a remote device is requested, a
scan is initiated and only when an advertise from this device is seen
the connect command is sent to the controller.

We could implement the whole procedure inside the kernel, but on the
discussions we (me, André, Vinicius and Claudio) had, it seems that
some changes on the socket api (related to the l2cap socket timeout)
will be needed to accommodate that. Also, it is still  an open
question if we can make use of the controller white-list to improve LE
connections management, and in this case it wouldn't make sense to
have the GCEP in the kernel.

And more important, having the GCEP in userspace is a big improvement
over what we have now. Current upstream code doesn't handle LE
re-connections at all, so if a remote device disconnects to save power
(which all HoG devices we have do), we're starting the GCEP to
re-connect to it. We can discuss a way to better support multiple
processes opening LE sockets, but I don't think having this code
upstream will hurt right now.

-- 
João Paulo Rechi Vita
Openbossa Labs - INdT