Re: [PATCH] reopen data device once if open data device failed

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Caiwen,

> Sometimes when open the data device, it may fail. If open the data device
> failed, retry once one second later.
> 
> ---
>  plugins/huawei.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index e791718..b888b0f 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -51,6 +51,7 @@
>  #include <ofono/phonebook.h>
>  #include <ofono/message-waiting.h>
>  #include <ofono/log.h>
> +#include <ofono.h>

this include is wrong and should not be needed. the ofono.h is internal
to the oFono core and should not be used in plugins.
 
>  #include <drivers/atmodem/atutil.h>
>  #include <drivers/atmodem/vendor.h>
> @@ -80,6 +81,8 @@ struct huawei_data {
>  	gboolean ndis;
>  	guint sim_poll_timeout;
>  	guint sim_poll_count;
> +	guint reopen_source;

Call this one reopen_timeout or similar. reopen_source is too generic.

> +	ofono_bool_t online;
>  };
>  
>  #define MAX_SIM_POLL_COUNT 5
> @@ -107,6 +110,11 @@ static void huawei_remove(struct ofono_modem *modem)
>  
>  	DBG("%p", modem);
>  
> +	if (data->reopen_source > 0) {
> +		g_source_remove(data->reopen_source);
> +		data->reopen_source = 0;
> +	}
> +
>  	ofono_modem_set_data(modem, NULL);
>  
>  	if (data->modem)
> @@ -465,6 +473,14 @@ static GAtChat *open_device(struct ofono_modem *modem,
>  	return chat;
>  }
>  
> +static void huawei_disconnect(gpointer user_data);
> +
> +static gboolean reopen_callback(gpointer user_data)
> +{
> +	huawei_disconnect(user_data);
> +	return FALSE;
> +}
> +
>  static void huawei_disconnect(gpointer user_data)
>  {
>  	struct ofono_modem *modem = user_data;
> @@ -476,15 +492,26 @@ static void huawei_disconnect(gpointer user_data)
>  	data->modem = NULL;
>  
>  	data->modem = open_device(modem, "Modem", "Modem: ");
> -	if (data->modem == NULL)
> +	/* retry once if failed */
> +	if (data->modem == NULL && data->reopen_source == 0) {
> +		data->reopen_source =
> +			g_timeout_add_seconds(1, reopen_callback, modem);
> +
> +		ofono_debug("open device failed, try to reopen it.");
>  		return;
> +	}
> +
> +	if (data->reopen_source > 0)
> +		data->reopen_source = 0;

Using reopen_source this way is wrong. Now you are just loosing the
reference to that timeout. What are you trying to do here?

I would expect that you keep that timeout source around until either you
do not need it anymore or the timeout actually fired and then reset it.

>  	g_at_chat_set_disconnect_function(data->modem,
>  						huawei_disconnect, modem);
>  
> -	/* gprs_context has been destructed and needs not reopen */
> -	if (data->gc == NULL)
> -		return;
> +	/* gprs_context has been destructed, if offline needs not recreate.
> +	    if device is online, need recreate.
> +	 */
> +	 if (data->gc == NULL && data->online == FALSE)
> +		return;

I think this is wrong as well. We should remove that context no matter
if online of offline, but off course you do not wanna create a new GPRS
context if you are actually offline, right?

>  	ofono_gprs_context_remove(data->gc);
>  
> @@ -492,6 +519,13 @@ static void huawei_disconnect(gpointer user_data)
>  			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
>  		ofono_info("Reopened GPRS context channel");
>  
> +		if (__ofono_modem_find_atom(modem,
> +				OFONO_ATOM_TYPE_GPRS) == NULL) {
> +			data->gprs = ofono_gprs_create(modem,
> +					OFONO_VENDOR_HUAWEI,
> +					"atmodem", data->pcui);
> +		}
> +

I am not getting this change. What is it trying to fix?

>  		data->gc = ofono_gprs_context_create(modem, 0, "atmodem",
>  								data->modem);
>  
> @@ -559,6 +593,11 @@ static int huawei_disable(struct ofono_modem *modem)
>  
>  	DBG("%p", modem);
>  
> +	if (data->reopen_source > 0) {
> +		g_source_remove(data->reopen_source);
> +		data->reopen_source = 0;
> +	}
> +
>  	if (data->sim_poll_timeout > 0) {
>  		g_source_remove(data->sim_poll_timeout);
>  		data->sim_poll_timeout = 0;
> @@ -588,6 +627,12 @@ static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	ofono_modem_online_cb_t cb = cbd->cb;
>  	struct ofono_error error;
>  
> +	if (ok) {
> +		struct huawei_data *data = cbd->user;
> +
> +		data->online = TRUE;
> +	}
> +
>  	decode_at_error(&error, g_at_result_final_response(result));
>  	cb(&error, cbd->data);
>  }
> @@ -603,6 +648,7 @@ static void set_offline_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  
>  		data->gc = NULL;
>  		data->gprs = NULL;
> +		data->online = FALSE;
>  	}
>  
>  	decode_at_error(&error, g_at_result_final_response(result));

Regards

Marcel