[PATCH] qmi: Optimize structure allocations

[PATCH] qmi: Optimize structure allocations

[Denis Kenzior]

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

struct discovery was allocated for every discovery procedure that was
kicked off, which itself allocated a structure.  This patch uses a
class/subclass concept to only allocate a single structure per discovery
procedure.
---
 drivers/qmimodem/qmi.c | 64 ++++++++++++++++----------------------------------
 1 file changed, 20 insertions(+), 44 deletions(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index a0d79e1..cda7f32 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -44,7 +44,6 @@ 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;
 };
 
@@ -224,17 +223,7 @@ 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;
+	destroy(d);
 }
 
 static void __notify_free(gpointer data, gpointer user_data)
@@ -883,32 +872,17 @@ static void read_watch_destroy(gpointer user_data)
 }
 
 static void __qmi_device_discovery_started(struct qmi_device *device,
-						void *discover_data,
-						qmi_destroy_func_t destroy)
+						struct discovery *d)
 {
-	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)
+						struct discovery *d)
 {
-	GList *list;
-	struct discovery *d;
-
-	list = g_queue_find_custom(device->discovery_queue,
-				discover_data, __discovery_compare);
-	if (!list)
+	if (g_queue_remove(device->discovery_queue, d) != TRUE)
 		return;
 
-	d = list->data;
-	g_queue_delete_link(device->discovery_queue, list);
-
 	__discovery_free(d, NULL);
 }
 
@@ -1065,6 +1039,7 @@ static const void *tlv_get(const void *data, uint16_t size,
 }
 
 struct discover_data {
+	struct discovery super;
 	struct qmi_device *device;
 	qmi_discover_func_t func;
 	void *user_data;
@@ -1170,7 +1145,7 @@ done:
 	if (data->func)
 		data->func(count, list, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__qmi_device_discovery_complete(data->device, &data->super);
 }
 
 static gboolean discover_reply(gpointer user_data)
@@ -1184,7 +1159,7 @@ static gboolean discover_reply(gpointer user_data)
 		data->func(device->version_count,
 				device->version_list, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__qmi_device_discovery_complete(data->device, &data->super);
 
 	return FALSE;
 }
@@ -1205,6 +1180,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 	if (!data)
 		return false;
 
+	data->super.destroy = discover_data_free;
 	data->device = device;
 	data->func = func;
 	data->user_data = user_data;
@@ -1212,8 +1188,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
 
 	if (device->version_list) {
 		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
-		__qmi_device_discovery_started(device, data,
-							discover_data_free);
+		__qmi_device_discovery_started(device, &data->super);
 		return true;
 	}
 
@@ -1234,7 +1209,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);
+	__qmi_device_discovery_started(device, &data->super);
 
 	return true;
 }
@@ -1785,6 +1760,7 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
 }
 
 struct service_create_data {
+	struct discovery super;
 	struct qmi_device *device;
 	bool shared;
 	uint8_t type;
@@ -1818,7 +1794,7 @@ static gboolean service_create_reply(gpointer user_data)
 	data->timeout = 0;
 	data->func(NULL, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__qmi_device_discovery_complete(data->device, &data->super);
 
 	return FALSE;
 }
@@ -1877,7 +1853,7 @@ done:
 	data->func(service, data->user_data);
 	qmi_service_unref(service);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__qmi_device_discovery_complete(data->device, &data->super);
 }
 
 static void service_create_discover(uint8_t count,
@@ -1910,8 +1886,7 @@ static void service_create_discover(uint8_t count,
 
 		data->timeout = g_timeout_add_seconds(0,
 						service_create_reply, data);
-		__qmi_device_discovery_started(device, data,
-						service_create_data_free);
+		__qmi_device_discovery_started(device, &data->super);
 		return;
 	}
 
@@ -1934,6 +1909,7 @@ static bool service_create(struct qmi_device *device, bool shared,
 	if (!data)
 		return false;
 
+	data->super.destroy = service_create_data_free;
 	data->device = device;
 	data->shared = shared;
 	data->type = type;
@@ -1956,8 +1932,7 @@ 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);
+	__qmi_device_discovery_started(device, &data->super);
 
 	return true;
 }
@@ -1976,6 +1951,7 @@ bool qmi_service_create(struct qmi_device *device,
 }
 
 struct service_create_shared_data {
+	struct discovery super;
 	struct qmi_service *service;
 	struct qmi_device *device;
 	qmi_create_func_t func;
@@ -2008,7 +1984,7 @@ static gboolean service_create_shared_reply(gpointer user_data)
 	data->timeout = 0;
 	data->func(data->service, data->user_data);
 
-	__qmi_device_discovery_complete(data->device, data);
+	__qmi_device_discovery_complete(data->device, &data->super);
 
 	return FALSE;
 }
@@ -2035,6 +2011,7 @@ bool qmi_service_create_shared(struct qmi_device *device,
 		if (!data)
 			return false;
 
+		data->super.destroy = service_create_shared_data_free;
 		data->service = qmi_service_ref(service);
 		data->device = device;
 		data->func = func;
@@ -2043,8 +2020,7 @@ bool qmi_service_create_shared(struct qmi_device *device,
 
 		data->timeout = g_timeout_add(0,
 					service_create_shared_reply, data);
-		__qmi_device_discovery_started(device, data,
-					service_create_shared_data_free);
+		__qmi_device_discovery_started(device, &data->super);
 
 		return 0;
 	}
-- 
2.10.2

Re: [PATCH] qmi: Optimize structure allocations

[Jonas Bonn]

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

On 04/03/2017 08:13 PM, Denis Kenzior wrote:
> struct discovery was allocated for every discovery procedure that was
> kicked off, which itself allocated a structure.  This patch uses a
> class/subclass concept to only allocate a single structure per discovery
> procedure.
> ---
>   drivers/qmimodem/qmi.c | 64 ++++++++++++++++----------------------------------
>   1 file changed, 20 insertions(+), 44 deletions(-)

I tested this and see no immediate issues.  Go ahead and merge this.
/Jonas

>
> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index a0d79e1..cda7f32 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -44,7 +44,6 @@ 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;
>   };
>   
> @@ -224,17 +223,7 @@ 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;
> +	destroy(d);
>   }
>   
>   static void __notify_free(gpointer data, gpointer user_data)
> @@ -883,32 +872,17 @@ static void read_watch_destroy(gpointer user_data)
>   }
>   
>   static void __qmi_device_discovery_started(struct qmi_device *device,
> -						void *discover_data,
> -						qmi_destroy_func_t destroy)
> +						struct discovery *d)
>   {
> -	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)
> +						struct discovery *d)
>   {
> -	GList *list;
> -	struct discovery *d;
> -
> -	list = g_queue_find_custom(device->discovery_queue,
> -				discover_data, __discovery_compare);
> -	if (!list)
> +	if (g_queue_remove(device->discovery_queue, d) != TRUE)
>   		return;
>   
> -	d = list->data;
> -	g_queue_delete_link(device->discovery_queue, list);
> -
>   	__discovery_free(d, NULL);
>   }
>   
> @@ -1065,6 +1039,7 @@ static const void *tlv_get(const void *data, uint16_t size,
>   }
>   
>   struct discover_data {
> +	struct discovery super;
>   	struct qmi_device *device;
>   	qmi_discover_func_t func;
>   	void *user_data;
> @@ -1170,7 +1145,7 @@ done:
>   	if (data->func)
>   		data->func(count, list, data->user_data);
>   
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>   }
>   
>   static gboolean discover_reply(gpointer user_data)
> @@ -1184,7 +1159,7 @@ static gboolean discover_reply(gpointer user_data)
>   		data->func(device->version_count,
>   				device->version_list, data->user_data);
>   
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>   
>   	return FALSE;
>   }
> @@ -1205,6 +1180,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
>   	if (!data)
>   		return false;
>   
> +	data->super.destroy = discover_data_free;
>   	data->device = device;
>   	data->func = func;
>   	data->user_data = user_data;
> @@ -1212,8 +1188,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
>   
>   	if (device->version_list) {
>   		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
> -		__qmi_device_discovery_started(device, data,
> -							discover_data_free);
> +		__qmi_device_discovery_started(device, &data->super);
>   		return true;
>   	}
>   
> @@ -1234,7 +1209,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);
> +	__qmi_device_discovery_started(device, &data->super);
>   
>   	return true;
>   }
> @@ -1785,6 +1760,7 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
>   }
>   
>   struct service_create_data {
> +	struct discovery super;
>   	struct qmi_device *device;
>   	bool shared;
>   	uint8_t type;
> @@ -1818,7 +1794,7 @@ static gboolean service_create_reply(gpointer user_data)
>   	data->timeout = 0;
>   	data->func(NULL, data->user_data);
>   
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>   
>   	return FALSE;
>   }
> @@ -1877,7 +1853,7 @@ done:
>   	data->func(service, data->user_data);
>   	qmi_service_unref(service);
>   
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>   }
>   
>   static void service_create_discover(uint8_t count,
> @@ -1910,8 +1886,7 @@ static void service_create_discover(uint8_t count,
>   
>   		data->timeout = g_timeout_add_seconds(0,
>   						service_create_reply, data);
> -		__qmi_device_discovery_started(device, data,
> -						service_create_data_free);
> +		__qmi_device_discovery_started(device, &data->super);
>   		return;
>   	}
>   
> @@ -1934,6 +1909,7 @@ static bool service_create(struct qmi_device *device, bool shared,
>   	if (!data)
>   		return false;
>   
> +	data->super.destroy = service_create_data_free;
>   	data->device = device;
>   	data->shared = shared;
>   	data->type = type;
> @@ -1956,8 +1932,7 @@ 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);
> +	__qmi_device_discovery_started(device, &data->super);
>   
>   	return true;
>   }
> @@ -1976,6 +1951,7 @@ bool qmi_service_create(struct qmi_device *device,
>   }
>   
>   struct service_create_shared_data {
> +	struct discovery super;
>   	struct qmi_service *service;
>   	struct qmi_device *device;
>   	qmi_create_func_t func;
> @@ -2008,7 +1984,7 @@ static gboolean service_create_shared_reply(gpointer user_data)
>   	data->timeout = 0;
>   	data->func(data->service, data->user_data);
>   
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>   
>   	return FALSE;
>   }
> @@ -2035,6 +2011,7 @@ bool qmi_service_create_shared(struct qmi_device *device,
>   		if (!data)
>   			return false;
>   
> +		data->super.destroy = service_create_shared_data_free;
>   		data->service = qmi_service_ref(service);
>   		data->device = device;
>   		data->func = func;
> @@ -2043,8 +2020,7 @@ bool qmi_service_create_shared(struct qmi_device *device,
>   
>   		data->timeout = g_timeout_add(0,
>   					service_create_shared_reply, data);
> -		__qmi_device_discovery_started(device, data,
> -					service_create_shared_data_free);
> +		__qmi_device_discovery_started(device, &data->super);
>   
>   		return 0;
>   	}

Re: [PATCH] qmi: Optimize structure allocations

[Lukasz Nowak]

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

Hi Denis,

On 03/04/17 19:13, Denis Kenzior wrote:
> struct discovery was allocated for every discovery procedure that was
> kicked off, which itself allocated a structure.  This patch uses a
> class/subclass concept to only allocate a single structure per discovery
> procedure.

The primary problem with this is that it still does not cover the shutdown
callback, which is 100% segfaulting for me.
I am pretty sure that shutdown_reply() and gobi.c:shutdown_cb() is currently
guaranteed to arrive after gobi_remove() calls g_free(data).
It is segfaulting for me because I use musl, which stores its own internal
pointers into the freed chunk's memory. shutdown_cb() then dereferences
effectively random memory content.
If glibc preserves memory content for a short while after the free call,
you may not see any ill effects.

Do you want to handle shutdown separately from discovery? i.e. store the
shutdown timeout handle directly in the struct qmi_device, and then cancel
the callback in qmi_device_unref()

I would like to be done with this topic, one way or the other, so please
let me know what you would like to do with it.

Lukasz


> ---
>  drivers/qmimodem/qmi.c | 64 ++++++++++++++++----------------------------------
>  1 file changed, 20 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index a0d79e1..cda7f32 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -44,7 +44,6 @@ 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;
>  };
>  
> @@ -224,17 +223,7 @@ 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;
> +	destroy(d);
>  }
>  
>  static void __notify_free(gpointer data, gpointer user_data)
> @@ -883,32 +872,17 @@ static void read_watch_destroy(gpointer user_data)
>  }
>  
>  static void __qmi_device_discovery_started(struct qmi_device *device,
> -						void *discover_data,
> -						qmi_destroy_func_t destroy)
> +						struct discovery *d)
>  {
> -	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)
> +						struct discovery *d)
>  {
> -	GList *list;
> -	struct discovery *d;
> -
> -	list = g_queue_find_custom(device->discovery_queue,
> -				discover_data, __discovery_compare);
> -	if (!list)
> +	if (g_queue_remove(device->discovery_queue, d) != TRUE)
>  		return;
>  
> -	d = list->data;
> -	g_queue_delete_link(device->discovery_queue, list);
> -
>  	__discovery_free(d, NULL);
>  }
>  
> @@ -1065,6 +1039,7 @@ static const void *tlv_get(const void *data, uint16_t size,
>  }
>  
>  struct discover_data {
> +	struct discovery super;
>  	struct qmi_device *device;
>  	qmi_discover_func_t func;
>  	void *user_data;
> @@ -1170,7 +1145,7 @@ done:
>  	if (data->func)
>  		data->func(count, list, data->user_data);
>  
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>  }
>  
>  static gboolean discover_reply(gpointer user_data)
> @@ -1184,7 +1159,7 @@ static gboolean discover_reply(gpointer user_data)
>  		data->func(device->version_count,
>  				device->version_list, data->user_data);
>  
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>  
>  	return FALSE;
>  }
> @@ -1205,6 +1180,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
>  	if (!data)
>  		return false;
>  
> +	data->super.destroy = discover_data_free;
>  	data->device = device;
>  	data->func = func;
>  	data->user_data = user_data;
> @@ -1212,8 +1188,7 @@ bool qmi_device_discover(struct qmi_device *device, qmi_discover_func_t func,
>  
>  	if (device->version_list) {
>  		data->timeout = g_timeout_add_seconds(0, discover_reply, data);
> -		__qmi_device_discovery_started(device, data,
> -							discover_data_free);
> +		__qmi_device_discovery_started(device, &data->super);
>  		return true;
>  	}
>  
> @@ -1234,7 +1209,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);
> +	__qmi_device_discovery_started(device, &data->super);
>  
>  	return true;
>  }
> @@ -1785,6 +1760,7 @@ bool qmi_result_get_uint64(struct qmi_result *result, uint8_t type,
>  }
>  
>  struct service_create_data {
> +	struct discovery super;
>  	struct qmi_device *device;
>  	bool shared;
>  	uint8_t type;
> @@ -1818,7 +1794,7 @@ static gboolean service_create_reply(gpointer user_data)
>  	data->timeout = 0;
>  	data->func(NULL, data->user_data);
>  
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>  
>  	return FALSE;
>  }
> @@ -1877,7 +1853,7 @@ done:
>  	data->func(service, data->user_data);
>  	qmi_service_unref(service);
>  
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>  }
>  
>  static void service_create_discover(uint8_t count,
> @@ -1910,8 +1886,7 @@ static void service_create_discover(uint8_t count,
>  
>  		data->timeout = g_timeout_add_seconds(0,
>  						service_create_reply, data);
> -		__qmi_device_discovery_started(device, data,
> -						service_create_data_free);
> +		__qmi_device_discovery_started(device, &data->super);
>  		return;
>  	}
>  
> @@ -1934,6 +1909,7 @@ static bool service_create(struct qmi_device *device, bool shared,
>  	if (!data)
>  		return false;
>  
> +	data->super.destroy = service_create_data_free;
>  	data->device = device;
>  	data->shared = shared;
>  	data->type = type;
> @@ -1956,8 +1932,7 @@ 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);
> +	__qmi_device_discovery_started(device, &data->super);
>  
>  	return true;
>  }
> @@ -1976,6 +1951,7 @@ bool qmi_service_create(struct qmi_device *device,
>  }
>  
>  struct service_create_shared_data {
> +	struct discovery super;
>  	struct qmi_service *service;
>  	struct qmi_device *device;
>  	qmi_create_func_t func;
> @@ -2008,7 +1984,7 @@ static gboolean service_create_shared_reply(gpointer user_data)
>  	data->timeout = 0;
>  	data->func(data->service, data->user_data);
>  
> -	__qmi_device_discovery_complete(data->device, data);
> +	__qmi_device_discovery_complete(data->device, &data->super);
>  
>  	return FALSE;
>  }
> @@ -2035,6 +2011,7 @@ bool qmi_service_create_shared(struct qmi_device *device,
>  		if (!data)
>  			return false;
>  
> +		data->super.destroy = service_create_shared_data_free;
>  		data->service = qmi_service_ref(service);
>  		data->device = device;
>  		data->func = func;
> @@ -2043,8 +2020,7 @@ bool qmi_service_create_shared(struct qmi_device *device,
>  
>  		data->timeout = g_timeout_add(0,
>  					service_create_shared_reply, data);
> -		__qmi_device_discovery_started(device, data,
> -					service_create_shared_data_free);
> +		__qmi_device_discovery_started(device, &data->super);
>  
>  		return 0;
>  	}
> 

Re: [PATCH] qmi: Optimize structure allocations

[Denis Kenzior]

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

Hi Lukasz,

On 04/04/2017 02:53 AM, Lukasz Nowak wrote:
> Hi Denis,
>
> On 03/04/17 19:13, Denis Kenzior wrote:
>> struct discovery was allocated for every discovery procedure that was
>> kicked off, which itself allocated a structure.  This patch uses a
>> class/subclass concept to only allocate a single structure per discovery
>> procedure.
>
> The primary problem with this is that it still does not cover the shutdown

Yes I know.  But as I mentioned before, I don't think the shutdown logic 
should be lumped together with the various discovery procedures. 
Besides, the shutdown code was bizarre and not thought through properly.

I have now pushed this patch out as well as another commit that should 
fix the shutdown case.

> callback, which is 100% segfaulting for me.
> I am pretty sure that shutdown_reply() and gobi.c:shutdown_cb() is currently
> guaranteed to arrive after gobi_remove() calls g_free(data).

I actually don't see how.  We should only be calling qmi_device_shutdown 
if the modem is being turned off cleanly (e.g. via Powered=False).  In 
the case of a hot-unplug, we'd be going directly to the .remove method. 
Is your device jumping off the USB bus when .disable is being called by 
any chance?

If so, then you should consider fixing your driver or tell the 
manufacturer to fix their firmware ;)

Regards,
-Denis

Re: [PATCH] qmi: Optimize structure allocations

[Lukasz Nowak]

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

Hi Denis,

On 04/04/17 16:37, Denis Kenzior wrote:
> Hi Lukasz,
> 
> On 04/04/2017 02:53 AM, Lukasz Nowak wrote:
>> Hi Denis,
>>
>> On 03/04/17 19:13, Denis Kenzior wrote:
>>> struct discovery was allocated for every discovery procedure that was
>>> kicked off, which itself allocated a structure.  This patch uses a
>>> class/subclass concept to only allocate a single structure per discovery
>>> procedure.
>>
>> The primary problem with this is that it still does not cover the shutdown
> 
> Yes I know.  But as I mentioned before, I don't think the shutdown logic should be lumped together with the various discovery procedures. Besides, the shutdown code was bizarre and not thought through properly.
> 
> I have now pushed this patch out as well as another commit that should fix the shutdown case.

Thanks. The patch works fine.

> 
>> callback, which is 100% segfaulting for me.
>> I am pretty sure that shutdown_reply() and gobi.c:shutdown_cb() is currently
>> guaranteed to arrive after gobi_remove() calls g_free(data).
> 
> I actually don't see how.  We should only be calling qmi_device_shutdown if the modem is being turned off cleanly (e.g. via Powered=False).  In the case of a hot-unplug, we'd be going directly to the .remove method. Is your device jumping off the USB bus when .disable is being called by any chance?

gobi_disable() is always called right before gobi_remove(). The path for this is:

src/modem.c:modem_unregister()
src/modem.c:set_powered(FALSE)
driver->disable()

and then:
src/modem.c:devinfo_remove()
driver->remove()

As the disable() comes first, gobi does not know that the modem has been removed, and schedules a qmi shutdown timeout(0). Callback would arrive after remove().

Anyway, this is just for reference, as the latest version cancels the callback correctly.

> 
> If so, then you should consider fixing your driver or tell the manufacturer to fix their firmware ;)
> 
> Regards,
> -Denis