[PATCH] qmi: track discovery tasks so clean up is possible

[PATCH] qmi: track discovery tasks so clean up is possible

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 9348 bytes --]

There are various device & service discovery tasks that are initiated
based on a qmi_device object.  qmi_device object does not currently
keep track of these tasks.  Unfortunately the qmi_device object can
go away at any time, and these tasks can become orphaned.

The result of this can lead to crashes.  E.g. a discovery task timeout fires
after the qmi_device object has been destroyed.  Since the object is no
longer valid, any accesses to it will likely result in a SEGFAULT.

This patch attempts to track all discovery tasks on the qmi_device
object itself, so that they can be cleaned up properly.  This patch does
not handle the qmi_device_shutdown functionality.
---
 drivers/qmimodem/qmi.c | 149 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 124 insertions(+), 25 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 39fbb19..9b80455 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -43,6 +43,11 @@
 typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data);
 
+struct discovery {
+	void *discover_data;
+	qmi_destroy_func_t destroy;
+};
+
 struct qmi_device {
 	int ref_count;
 	int fd;
@@ -53,6 +58,7 @@ struct qmi_device {
 	GQueue *req_queue;
 	GQueue *control_queue;
 	GQueue *service_queue;
+	GQueue *discovery_queue;
 	uint8_t next_control_tid;
 	uint16_t next_service_tid;
 	qmi_debug_func_t debug_func;
@@ -213,6 +219,24 @@ static gint __request_compare(gconstpointer a, gconstpointer b)
 	return req->tid - tid;
 }
 
+static void __discovery_free(gpointer data, gpointer user_data)
+{
+	struct discovery *d = data;
+	qmi_destroy_func_t destroy = d->destroy;
+
+	destroy(d->discover_data);
+}
+
+static gint __discovery_compare(gconstpointer a, gconstpointer b)
+{
+	const struct discovery *d = a;
+
+	if (d->discover_data == b)
+		return 0;
+
+	return 1;
+}
+
 static void __notify_free(gpointer data, gpointer user_data)
 {
 	struct qmi_notify *notify = data;
@@ -844,6 +868,37 @@ static void read_watch_destroy(gpointer user_data)
 	device->read_watch = 0;
 }
 
+static void __qmi_device_discovery_started(struct qmi_device *device,
+						void *discover_data,
+						qmi_destroy_func_t destroy)
+{
+	struct discovery *d;
+
+	d = g_new0(struct discovery, 1);
+	d->discover_data = discover_data;
+	d->destroy = destroy;
+
+	g_queue_push_tail(device->discovery_queue, d);
+}
+
+static void __qmi_device_discovery_complete(struct qmi_device *device,
+						void *discover_data)
+{
+	GList *list;
+	struct discovery *d;
+
+	list = g_queue_find_custom(device->req_queue,
+				discover_data, __discovery_compare);
+	if (!list)
+		return;
+
+	d = list->data;
+	g_queue_delete_link(device->discovery_queue, list);
+
+	d->destroy(d->discover_data);
+	__discovery_free(d, NULL);
+}
+
 static void service_destroy(gpointer data)
 {
 	struct qmi_service *service = data;
@@ -897,6 +952,7 @@ struct qmi_device *qmi_device_new(int fd)
 	device->req_queue = g_queue_new();
 	device->control_queue = g_queue_new();
 	device->service_queue = g_queue_new();
+	device->discovery_queue = g_queue_new();
 
 	device->service_list = g_hash_table_new_full(g_direct_hash,
 					g_direct_equal, NULL, service_destroy);
@@ -933,6 +989,9 @@ void qmi_device_unref(struct qmi_device *device)
 	g_queue_foreach(device->req_queue, __request_free, NULL);
 	g_queue_free(device->req_queue);
 
+	g_queue_foreach(device->discovery_queue, __discovery_free, NULL);
+	g_queue_free(device->discovery_queue);
+
 	if (device->write_watch > 0)
 		g_source_remove(device->write_watch);
 
@@ -1000,6 +1059,21 @@ struct discover_data {
 	guint timeout;
 };
 
+static void discover_data_free(gpointer user_data)
+{
+	struct discover_data *data = user_data;
+
+	if (data->timeout) {
+		g_source_remove(data->timeout);
+		data->timeout = 0;
+	}
+
+	if (data->destroy)
+		data->destroy(data->user_data);
+
+	g_free(data);
+}
+
 static void discover_callback(uint16_t message, uint16_t length,
 					const void *buffer, void *user_data)
 {
@@ -1013,8 +1087,6 @@ static void discover_callback(uint16_t message, uint16_t length,
 	uint8_t count;
 	unsigned int i;
 
-	g_source_remove(data->timeout);
-
 	count = 0;
 	list = NULL;
 
@@ -1085,10 +1157,7 @@ done:
 	if (data->func)
 		data->func(count, list, data->user_data);
 
-	if (data->destroy)
-		data->destroy(data->user_data);
-
-	g_free(data);
+	__qmi_device_discovery_complete(data->device, data);
 }
 
 static gboolean discover_reply(gpointer user_data)
@@ -1102,10 +1171,7 @@ static gboolean discover_reply(gpointer user_data)
 		data->func(device->version_count,
 				device->version_list, data->user_data);
 
-	if (data->destroy)
-		data->destroy(data->user_data);
-
-	g_free(data);
+	__qmi_device_discovery_complete(data->device, data);
 
 	return FALSE;
 }
@@ -1132,7 +1198,9 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 	data->destroy = destroy;
 
 	if (device->version_list) {
-		g_timeout_add_seconds(0, discover_reply, data);
+		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
+		__qmi_device_discovery_started(device, data,
+							discover_data_free);
 		return true;
 	}
 
@@ -1153,6 +1221,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 	__request_submit(device, req, hdr->transaction);
 
 	data->timeout = g_timeout_add_seconds(5, discover_reply, data);
+	__qmi_device_discovery_started(device, data, discover_data_free);
 
 	return true;
 }
@@ -1714,16 +1783,29 @@ struct service_create_data {
 	guint timeout;
 };
 
-static gboolean service_create_reply(gpointer user_data)
+static void service_create_data_free(gpointer user_data)
 {
 	struct service_create_data *data = user_data;
 
-	data->func(NULL, data->user_data);
+	if (data->timeout) {
+		g_source_remove(data->timeout);
+		data->timeout = 0;
+	}
 
 	if (data->destroy)
 		data->destroy(data->user_data);
 
 	g_free(data);
+}
+
+static gboolean service_create_reply(gpointer user_data)
+{
+	struct service_create_data *data = user_data;
+
+	data->timeout = 0;
+	data->func(NULL, data->user_data);
+
+	__qmi_device_discovery_complete(data->device, data);
 
 	return FALSE;
 }
@@ -1739,8 +1821,6 @@ static void service_create_callback(uint16_t message, uint16_t length,
 	uint16_t len;
 	unsigned int hash_id;
 
-	g_source_remove(data->timeout);
-
 	result_code = tlv_get(buffer, length, 0x02, &len);
 	if (!result_code)
 		goto done;
@@ -1782,13 +1862,9 @@ static void service_create_callback(uint16_t message, uint16_t length,
 
 done:
 	data->func(service, data->user_data);
-
 	qmi_service_unref(service);
 
-	if (data->destroy)
-		data->destroy(data->user_data);
-
-	g_free(data);
+	__qmi_device_discovery_complete(data->device, data);
 }
 
 static void service_create_discover(uint8_t count,
@@ -1819,7 +1895,10 @@ static void service_create_discover(uint8_t count,
 		if (data->timeout > 0)
 			g_source_remove(data->timeout);
 
-		g_timeout_add_seconds(0, service_create_reply, data);
+		data->timeout = g_timeout_add_seconds(0,
+						service_create_reply, data);
+		__qmi_device_discovery_started(device, data,
+						service_create_data_free);
 		return;
 	}
 
@@ -1864,6 +1943,8 @@ static bool service_create(struct qmi_device *device, bool shared,
 
 done:
 	data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
+	__qmi_device_discovery_started(device, data,
+						service_create_data_free);
 
 	return true;
 }
@@ -1883,16 +1964,21 @@ bool qmi_service_create(struct qmi_device *device,
 
 struct service_create_shared_data {
 	struct qmi_service *service;
+	struct qmi_device *device;
 	qmi_create_func_t func;
 	void *user_data;
 	qmi_destroy_func_t destroy;
+	guint timeout;
 };
 
-static gboolean service_create_shared_reply(gpointer user_data)
+static void service_create_shared_data_free(gpointer user_data)
 {
 	struct service_create_shared_data *data = user_data;
 
-	data->func(data->service, data->user_data);
+	if (data->timeout) {
+		g_source_remove(data->timeout);
+		data->timeout = 0;
+	}
 
 	qmi_service_unref(data->service);
 
@@ -1900,6 +1986,16 @@ static gboolean service_create_shared_reply(gpointer user_data)
 		data->destroy(data->user_data);
 
 	g_free(data);
+}
+
+static gboolean service_create_shared_reply(gpointer user_data)
+{
+	struct service_create_shared_data *data = user_data;
+
+	data->timeout = 0;
+	data->func(data->service, data->user_data);
+
+	__qmi_device_discovery_complete(data->device, data);
 
 	return FALSE;
 }
@@ -1927,12 +2023,15 @@ bool qmi_service_create_shared(struct qmi_device *device,
 			return false;
 
 		data->service = qmi_service_ref(service);
-
+		data->device = device;
 		data->func = func;
 		data->user_data = user_data;
 		data->destroy = destroy;
 
-		g_timeout_add(0, service_create_shared_reply, data);
+		data->timeout = g_timeout_add(0,
+					service_create_shared_reply, data);
+		__qmi_device_discovery_started(device, data,
+					service_create_shared_data_free);
 
 		return 0;
 	}
-- 
2.10.2

Re: [PATCH] qmi: track discovery tasks so clean up is possible

[Lukasz Nowak]

[-- Attachment #1: Type: text/plain, Size: 10574 bytes --]

Hi Denis,

I realised last night that I was not freeing the user_data memory.

I would propose to modify your code to cover both discovery and shutdown. Unify struct discover_data and struct shutdown_data into struct callback_data with a union for the func. And then add the shutdown callback to the same queue.

It should also be possible to merge your struct discovery into the new struct callback_data, so that we don't have to allocate two structures for each callback.

Will I modify your patch with that?

Lukasz


On 28/03/17 20:25, Denis Kenzior wrote:
> There are various device & service discovery tasks that are initiated
> based on a qmi_device object.  qmi_device object does not currently
> keep track of these tasks.  Unfortunately the qmi_device object can
> go away at any time, and these tasks can become orphaned.
> 
> The result of this can lead to crashes.  E.g. a discovery task timeout fires
> after the qmi_device object has been destroyed.  Since the object is no
> longer valid, any accesses to it will likely result in a SEGFAULT.
> 
> This patch attempts to track all discovery tasks on the qmi_device
> object itself, so that they can be cleaned up properly.  This patch does
> not handle the qmi_device_shutdown functionality.
> ---
>  drivers/qmimodem/qmi.c | 149 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 124 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index 39fbb19..9b80455 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -43,6 +43,11 @@
>  typedef void (*qmi_message_func_t)(uint16_t message, uint16_t length,
>  					const void *buffer, void *user_data);
>  
> +struct discovery {
> +	void *discover_data;
> +	qmi_destroy_func_t destroy;
> +};
> +
>  struct qmi_device {
>  	int ref_count;
>  	int fd;
> @@ -53,6 +58,7 @@ struct qmi_device {
>  	GQueue *req_queue;
>  	GQueue *control_queue;
>  	GQueue *service_queue;
> +	GQueue *discovery_queue;
>  	uint8_t next_control_tid;
>  	uint16_t next_service_tid;
>  	qmi_debug_func_t debug_func;
> @@ -213,6 +219,24 @@ static gint __request_compare(gconstpointer a, gconstpointer b)
>  	return req->tid - tid;
>  }
>  
> +static void __discovery_free(gpointer data, gpointer user_data)
> +{
> +	struct discovery *d = data;
> +	qmi_destroy_func_t destroy = d->destroy;
> +
> +	destroy(d->discover_data);
> +}
> +
> +static gint __discovery_compare(gconstpointer a, gconstpointer b)
> +{
> +	const struct discovery *d = a;
> +
> +	if (d->discover_data == b)
> +		return 0;
> +
> +	return 1;
> +}
> +
>  static void __notify_free(gpointer data, gpointer user_data)
>  {
>  	struct qmi_notify *notify = data;
> @@ -844,6 +868,37 @@ static void read_watch_destroy(gpointer user_data)
>  	device->read_watch = 0;
>  }
>  
> +static void __qmi_device_discovery_started(struct qmi_device *device,
> +						void *discover_data,
> +						qmi_destroy_func_t destroy)
> +{
> +	struct discovery *d;
> +
> +	d = g_new0(struct discovery, 1);
> +	d->discover_data = discover_data;
> +	d->destroy = destroy;
> +
> +	g_queue_push_tail(device->discovery_queue, d);
> +}
> +
> +static void __qmi_device_discovery_complete(struct qmi_device *device,
> +						void *discover_data)
> +{
> +	GList *list;
> +	struct discovery *d;
> +
> +	list = g_queue_find_custom(device->req_queue,
> +				discover_data, __discovery_compare);
> +	if (!list)
> +		return;
> +
> +	d = list->data;
> +	g_queue_delete_link(device->discovery_queue, list);
> +
> +	d->destroy(d->discover_data);
> +	__discovery_free(d, NULL);
> +}
> +
>  static void service_destroy(gpointer data)
>  {
>  	struct qmi_service *service = data;
> @@ -897,6 +952,7 @@ struct qmi_device *qmi_device_new(int fd)
>  	device->req_queue = g_queue_new();
>  	device->control_queue = g_queue_new();
>  	device->service_queue = g_queue_new();
> +	device->discovery_queue = g_queue_new();
>  
>  	device->service_list = g_hash_table_new_full(g_direct_hash,
>  					g_direct_equal, NULL, service_destroy);
> @@ -933,6 +989,9 @@ void qmi_device_unref(struct qmi_device *device)
>  	g_queue_foreach(device->req_queue, __request_free, NULL);
>  	g_queue_free(device->req_queue);
>  
> +	g_queue_foreach(device->discovery_queue, __discovery_free, NULL);
> +	g_queue_free(device->discovery_queue);
> +
>  	if (device->write_watch > 0)
>  		g_source_remove(device->write_watch);
>  
> @@ -1000,6 +1059,21 @@ struct discover_data {
>  	guint timeout;
>  };
>  
> +static void discover_data_free(gpointer user_data)
> +{
> +	struct discover_data *data = user_data;
> +
> +	if (data->timeout) {
> +		g_source_remove(data->timeout);
> +		data->timeout = 0;
> +	}
> +
> +	if (data->destroy)
> +		data->destroy(data->user_data);
> +
> +	g_free(data);
> +}
> +
>  static void discover_callback(uint16_t message, uint16_t length,
>  					const void *buffer, void *user_data)
>  {
> @@ -1013,8 +1087,6 @@ static void discover_callback(uint16_t message, uint16_t length,
>  	uint8_t count;
>  	unsigned int i;
>  
> -	g_source_remove(data->timeout);
> -
>  	count = 0;
>  	list = NULL;
>  
> @@ -1085,10 +1157,7 @@ done:
>  	if (data->func)
>  		data->func(count, list, data->user_data);
>  
> -	if (data->destroy)
> -		data->destroy(data->user_data);
> -
> -	g_free(data);
> +	__qmi_device_discovery_complete(data->device, data);
>  }
>  
>  static gboolean discover_reply(gpointer user_data)
> @@ -1102,10 +1171,7 @@ static gboolean discover_reply(gpointer user_data)
>  		data->func(device->version_count,
>  				device->version_list, data->user_data);
>  
> -	if (data->destroy)
> -		data->destroy(data->user_data);
> -
> -	g_free(data);
> +	__qmi_device_discovery_complete(data->device, data);
>  
>  	return FALSE;
>  }
> @@ -1132,7 +1198,9 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
>  	data->destroy = destroy;
>  
>  	if (device->version_list) {
> -		g_timeout_add_seconds(0, discover_reply, data);
> +		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
> +		__qmi_device_discovery_started(device, data,
> +							discover_data_free);
>  		return true;
>  	}
>  
> @@ -1153,6 +1221,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
>  	__request_submit(device, req, hdr->transaction);
>  
>  	data->timeout = g_timeout_add_seconds(5, discover_reply, data);
> +	__qmi_device_discovery_started(device, data, discover_data_free);
>  
>  	return true;
>  }
> @@ -1714,16 +1783,29 @@ struct service_create_data {
>  	guint timeout;
>  };
>  
> -static gboolean service_create_reply(gpointer user_data)
> +static void service_create_data_free(gpointer user_data)
>  {
>  	struct service_create_data *data = user_data;
>  
> -	data->func(NULL, data->user_data);
> +	if (data->timeout) {
> +		g_source_remove(data->timeout);
> +		data->timeout = 0;
> +	}
>  
>  	if (data->destroy)
>  		data->destroy(data->user_data);
>  
>  	g_free(data);
> +}
> +
> +static gboolean service_create_reply(gpointer user_data)
> +{
> +	struct service_create_data *data = user_data;
> +
> +	data->timeout = 0;
> +	data->func(NULL, data->user_data);
> +
> +	__qmi_device_discovery_complete(data->device, data);
>  
>  	return FALSE;
>  }
> @@ -1739,8 +1821,6 @@ static void service_create_callback(uint16_t message, uint16_t length,
>  	uint16_t len;
>  	unsigned int hash_id;
>  
> -	g_source_remove(data->timeout);
> -
>  	result_code = tlv_get(buffer, length, 0x02, &len);
>  	if (!result_code)
>  		goto done;
> @@ -1782,13 +1862,9 @@ static void service_create_callback(uint16_t message, uint16_t length,
>  
>  done:
>  	data->func(service, data->user_data);
> -
>  	qmi_service_unref(service);
>  
> -	if (data->destroy)
> -		data->destroy(data->user_data);
> -
> -	g_free(data);
> +	__qmi_device_discovery_complete(data->device, data);
>  }
>  
>  static void service_create_discover(uint8_t count,
> @@ -1819,7 +1895,10 @@ static void service_create_discover(uint8_t count,
>  		if (data->timeout > 0)
>  			g_source_remove(data->timeout);
>  
> -		g_timeout_add_seconds(0, service_create_reply, data);
> +		data->timeout = g_timeout_add_seconds(0,
> +						service_create_reply, data);
> +		__qmi_device_discovery_started(device, data,
> +						service_create_data_free);
>  		return;
>  	}
>  
> @@ -1864,6 +1943,8 @@ static bool service_create(struct qmi_device *device, bool shared,
>  
>  done:
>  	data->timeout = g_timeout_add_seconds(8, service_create_reply, data);
> +	__qmi_device_discovery_started(device, data,
> +						service_create_data_free);
>  
>  	return true;
>  }
> @@ -1883,16 +1964,21 @@ bool qmi_service_create(struct qmi_device *device,
>  
>  struct service_create_shared_data {
>  	struct qmi_service *service;
> +	struct qmi_device *device;
>  	qmi_create_func_t func;
>  	void *user_data;
>  	qmi_destroy_func_t destroy;
> +	guint timeout;
>  };
>  
> -static gboolean service_create_shared_reply(gpointer user_data)
> +static void service_create_shared_data_free(gpointer user_data)
>  {
>  	struct service_create_shared_data *data = user_data;
>  
> -	data->func(data->service, data->user_data);
> +	if (data->timeout) {
> +		g_source_remove(data->timeout);
> +		data->timeout = 0;
> +	}
>  
>  	qmi_service_unref(data->service);
>  
> @@ -1900,6 +1986,16 @@ static gboolean service_create_shared_reply(gpointer user_data)
>  		data->destroy(data->user_data);
>  
>  	g_free(data);
> +}
> +
> +static gboolean service_create_shared_reply(gpointer user_data)
> +{
> +	struct service_create_shared_data *data = user_data;
> +
> +	data->timeout = 0;
> +	data->func(data->service, data->user_data);
> +
> +	__qmi_device_discovery_complete(data->device, data);
>  
>  	return FALSE;
>  }
> @@ -1927,12 +2023,15 @@ bool qmi_service_create_shared(struct qmi_device *device,
>  			return false;
>  
>  		data->service = qmi_service_ref(service);
> -
> +		data->device = device;
>  		data->func = func;
>  		data->user_data = user_data;
>  		data->destroy = destroy;
>  
> -		g_timeout_add(0, service_create_shared_reply, data);
> +		data->timeout = g_timeout_add(0,
> +					service_create_shared_reply, data);
> +		__qmi_device_discovery_started(device, data,
> +					service_create_shared_data_free);
>  
>  		return 0;
>  	}
> 

Re: [PATCH] qmi: track discovery tasks so clean up is possible

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

Hi Lukasz,

Just FYI, we don't like for you to top-post on this mailing list

On 03/29/2017 06:52 AM, Lukasz Nowak wrote:
> Hi Denis,
>
> I realised last night that I was not freeing the user_data memory.
>
> I would propose to modify your code to cover both discovery and shutdown. Unify struct discover_data and struct shutdown_data into struct callback_data with a union for the func. And then add the shutdown callback to the same queue.

The shutdown part is a bit weird.  Right now there's only a single 
caller that uses qmi_device_shutdown, so I'm not quite sure what was 
intended by the multi-client callback handling.

My current thinking is that we should simply modify shutdown to follow a 
'first through the gate' semantic.  E.g. the first client that calls 
shutdown will get a callback, while the rest will get an error, e.g. 
-EALREADY.

That way the shutdown timeout / callback / destroy func can be stored 
directly on struct qmi_device.

>
> It should also be possible to merge your struct discovery into the new struct callback_data, so that we don't have to allocate two structures for each callback.
>

Do you mean to merge discover_data, service_create_data, 
service_create_shared_data and discovery into the same structure?

> Will I modify your patch with that?
>

Regardless, feel free to modify.  It might be easier to see the code 
than discuss it :)

I haven't tested my patch and it is meant mostly as an RFC.  Feel free 
to take it the rest of the way.

Regards,
-Denis

Re: [PATCH] qmi: track discovery tasks so clean up is possible

[Lukasz Nowak]

[-- Attachment #1: Type: text/plain, Size: 2296 bytes --]

Hi Denis,

On 29/03/17 16:11, Denis Kenzior wrote:
> Hi Lukasz,
> 
> Just FYI, we don't like for you to top-post on this mailing list
> 
> On 03/29/2017 06:52 AM, Lukasz Nowak wrote:
>> Hi Denis,
>>
>> I realised last night that I was not freeing the user_data memory.
>>
>> I would propose to modify your code to cover both discovery and shutdown. Unify struct discover_data and struct shutdown_data into struct callback_data with a union for the func. And then add the shutdown callback to the same queue.
> 
> The shutdown part is a bit weird.  Right now there's only a single caller that uses qmi_device_shutdown, so I'm not quite sure what was intended by the multi-client callback handling.
> 
> My current thinking is that we should simply modify shutdown to follow a 'first through the gate' semantic.  E.g. the first client that calls shutdown will get a callback, while the rest will get an error, e.g. -EALREADY.
> 
> That way the shutdown timeout / callback / destroy func can be stored directly on struct qmi_device.

I don't think this is the purpose of the shutdown timeout. It is used to wait for all the services to shut down before issuing a user callback.
When a device is being disabled by software, the code will issue several qmi_service_unref() for all the services, and then qmi_device_shutdown()
Then shutdown_timeout() will be re-scheduled until all the services close down (device->release_users is decremented in service_release_callback()). Only then shutdown_reply() will trigger gobi.c:shutdown_cb() 

> 
>>
>> It should also be possible to merge your struct discovery into the new struct callback_data, so that we don't have to allocate two structures for each callback.
>>
> 
> Do you mean to merge discover_data, service_create_data, service_create_shared_data and discovery into the same structure?

I have coded a single structure, with a union for custom fields for each callback type. And added shutdown callback to the device's callback_queue.

> 
>> Will I modify your patch with that?
>>
> 
> Regardless, feel free to modify.  It might be easier to see the code than discuss it :)
> 
> I haven't tested my patch and it is meant mostly as an RFC.  Feel free to take it the rest of the way.
> 
> Regards,
> -Denis