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

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

[Caiwen Zhang]

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

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>
 
 #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;
+	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;
 
 	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;
 
 	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);
+		}
+
 		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));
-- 
1.7.5

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

[Marcel Holtmann]

[-- 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

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

[Zhang, Caiwen]

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


> > 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.
> 

OK, I will rename it.

> > +	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.
> 

Yes, only loose the reference here. In fact, here the timeout has fired.
I will move this to reopen_callback(), it should be better.

> >  	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?
>

If the device is offline, the GPRS context atom has been released when set
the device to offline. That's another issue has been fixed by Martin (Need
set data->gc to NULL, because it has been release if device is offline).

It will create a new GPRS context when the device is switch to online(in
huawei_post_online()), so don't create it here if it is offline.


> >  	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?
>

There is one case that the GPRS atom is not created. If the device is 
switched to online during the time reopen the device, due to data->modem is
NULL, GPRS atom is not created. Here insure GPRS atom is created.

> >  		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));
> 

Best regards
Caiwen

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

[Denis Kenzior]

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

Hi Caiwen,

>> 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?
>>
> 
> If the device is offline, the GPRS context atom has been released when set
> the device to offline. That's another issue has been fixed by Martin (Need
> set data->gc to NULL, because it has been release if device is offline).
> 
> It will create a new GPRS context when the device is switch to online(in
> huawei_post_online()), so don't create it here if it is offline.
> 
>

I suggest you migrate the huawei modem driver to create the gprs atom in
post_sim state instead of post_online.  It will solve many of the issues
you're trying to solve.

Regards,
-Denis

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

[Zhang, Caiwen]

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

Hi Denis,

> >> 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?
> >>
> >
> > If the device is offline, the GPRS context atom has been released
> when set
> > the device to offline. That's another issue has been fixed by Martin
> (Need
> > set data->gc to NULL, because it has been release if device is
> offline).
> >
> > It will create a new GPRS context when the device is switch to
> online(in
> > huawei_post_online()), so don't create it here if it is offline.
> >
> >
> 
> I suggest you migrate the huawei modem driver to create the gprs atom
> in
> post_sim state instead of post_online.  It will solve many of the
> issues
> you're trying to solve.
> 

Thanks your suggestion.

Yes, It can help that we don't need to care whether gprs atom and gprs context atom 
be repeatedly created. But if do this at post_sim state, it will beak the rule
that only enable the atom when it is available. At post_sim state GPRS is still unavailable.
In addition, all other plug-ins do this at post_online, only do this in Huawei plug-in,
it may impact the consistency of the codes.

Best regards
Caiwen

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

[Denis Kenzior]

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

Hi Caiwen,

On 05/11/2011 12:49 AM, Zhang, Caiwen wrote:
> Hi Denis,
> 
>>>> 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?
>>>>
>>>
>>> If the device is offline, the GPRS context atom has been released
>> when set
>>> the device to offline. That's another issue has been fixed by Martin
>> (Need
>>> set data->gc to NULL, because it has been release if device is
>> offline).
>>>
>>> It will create a new GPRS context when the device is switch to
>> online(in
>>> huawei_post_online()), so don't create it here if it is offline.
>>>
>>>
>>
>> I suggest you migrate the huawei modem driver to create the gprs atom
>> in
>> post_sim state instead of post_online.  It will solve many of the
>> issues
>> you're trying to solve.
>>
> 
> Thanks your suggestion.
> 
> Yes, It can help that we don't need to care whether gprs atom and gprs context atom 
> be repeatedly created. But if do this at post_sim state, it will beak the rule
> that only enable the atom when it is available. At post_sim state GPRS is still unavailable.
> In addition, all other plug-ins do this at post_online, only do this in Huawei plug-in,
> it may impact the consistency of the codes.
> 

I wouldn't really suggest this if I didn't think it was a good idea.
All modem drivers should be migrated to create gprs in the post_sim
state instead of post_online at some point anyway.  And even if the GPRS
connections cannot be established when the modem is Offline, the
relevant settings on ConnectionManager interface can still be modified.

Regards,
-Denis