[PATCH] Fix huawei_disconnect function issue

[PATCH] Fix huawei_disconnect function issue

[martin.xu]

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

From: Martin Xu <martin.xu@intel.com>

huawei_disconnect is used to recovery the io and gprs context when
io error happends, see commit 39382730d7758b093ca6271f4e9dea875fa04b3a
However, io error not only happends at PPP disconnect, in theory it
can happends at any situation. I also observed that it happens when modem
go into offline mode at my Huawei EM770W modem. in this case, gprs should
not be reopened.
---
 plugins/huawei.c |   50 ++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 6f05677..5bf02fd 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -470,10 +470,7 @@ static void huawei_disconnect(gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct huawei_data *data = ofono_modem_get_data(modem);
 
-	DBG("");
-
-	if (data->gc)
-		ofono_gprs_context_remove(data->gc);
+	DBG("data->gc %p", data->gc);
 
 	g_at_chat_unref(data->modem);
 	data->modem = NULL;
@@ -485,6 +482,12 @@ static void huawei_disconnect(gpointer user_data)
 	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;
+
+	ofono_gprs_context_remove(data->gc);
+
 	if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
 		ofono_info("Reopened GPRS context channel");
@@ -579,15 +582,29 @@ static int huawei_disable(struct ofono_modem *modem)
 	return -EINPROGRESS;
 }
 
+struct huawei_cb_data {
+	struct cb_data *cbd;
+	ofono_bool_t online;
+	struct ofono_modem *modem;
+};
+
 static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
-	struct cb_data *cbd = user_data;
-	ofono_modem_online_cb_t cb = cbd->cb;
+	struct huawei_cb_data *online_cbd = user_data;
+	ofono_modem_online_cb_t cb = online_cbd->cbd->cb;
+	struct huawei_data *data = ofono_modem_get_data(online_cbd->modem);
+
+	if (ok) {
+		/* offline mode gprs_context and gprs will be flushed */
+		if (online_cbd->online == FALSE) {
+			data->gc = NULL;
+			data->gprs = NULL;
+		}
 
-	if (ok)
-		CALLBACK_WITH_SUCCESS(cb, cbd->data);
+		CALLBACK_WITH_SUCCESS(cb, online_cbd->cbd->data);
+	}
 	else
-		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		CALLBACK_WITH_FAILURE(cb, online_cbd->cbd->data);
 }
 
 static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
@@ -595,21 +612,26 @@ static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
 {
 	struct huawei_data *data = ofono_modem_get_data(modem);
 	GAtChat *chat = data->pcui;
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct huawei_cb_data *online_cbd;
 	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=5";
 
 	DBG("modem %p %s", modem, online ? "online" : "offline");
 
-	if (cbd == NULL)
+	online_cbd = g_try_new0(struct huawei_cb_data, 1);
+	online_cbd->cbd = cb_data_new(cb, user_data);
+	if (online_cbd->cbd == NULL)
 		goto error;
+	online_cbd->online = online;
+	online_cbd->modem = modem;
 
-	if (g_at_chat_send(chat, command, NULL, set_online_cb, cbd, g_free))
+	if (g_at_chat_send(chat, command, NULL, set_online_cb,
+						online_cbd, g_free))
 		return;
 
 error:
-	g_free(cbd);
+	g_free(online_cbd);
 
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static void huawei_pre_sim(struct ofono_modem *modem)
-- 
1.6.1.3

Re: [PATCH] Fix huawei_disconnect function issue

[Denis Kenzior]

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

Hi Martin,

On 03/08/2011 11:27 PM, martin.xu(a)intel.com wrote:
> From: Martin Xu <martin.xu@intel.com>
> 
> huawei_disconnect is used to recovery the io and gprs context when
> io error happends, see commit 39382730d7758b093ca6271f4e9dea875fa04b3a
> However, io error not only happends at PPP disconnect, in theory it
> can happends at any situation. I also observed that it happens when modem
> go into offline mode at my Huawei EM770W modem. in this case, gprs should
> not be reopened.
> ---
>  plugins/huawei.c |   50 ++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 36 insertions(+), 14 deletions(-)
> 

I applied this patch but reworked it afterwards.  Can you test it and
make sure I didn't screw something up?

Regards,
-Denis

[PATCH] Fix huawei_disconnect function issue

[martin.xu]

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

From: Martin Xu <martin.xu@intel.com>

huawei_disconnect is used to recovery the io and gprs context when
io error happends, see commit 39382730d7758b093ca6271f4e9dea875fa04b3a
However, io error not only happends at PPP disconnect, in theory it
can happends at any situation. I also observed that it happens when modem
go into offline mode at my Huawei EM770W modem. in this case, gprs should
not be reopened.
---
 plugins/huawei.c |   49 +++++++++++++++++++++++++++++++++++--------------
 1 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 6f05677..939509f 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -78,6 +78,7 @@ struct huawei_data {
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
 	gboolean voice;
+	gboolean online;
 	gboolean ndis;
 	guint sim_poll_timeout;
 	guint sim_poll_count;
@@ -97,6 +98,7 @@ static int huawei_probe(struct ofono_modem *modem)
 	if (data == NULL)
 		return -ENOMEM;
 
+	data->online = FALSE;
 	ofono_modem_set_data(modem, data);
 
 	return 0;
@@ -470,10 +472,7 @@ static void huawei_disconnect(gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct huawei_data *data = ofono_modem_get_data(modem);
 
-	DBG("");
-
-	if (data->gc)
-		ofono_gprs_context_remove(data->gc);
+	DBG("data->online %d", data->online);
 
 	g_at_chat_unref(data->modem);
 	data->modem = NULL;
@@ -485,6 +484,13 @@ static void huawei_disconnect(gpointer user_data)
 	g_at_chat_set_disconnect_function(data->modem,
 						huawei_disconnect, modem);
 
+	/* reopen GPRS context channel only at online state */
+	if (data->online == FALSE)
+		return;
+
+	if (data->gc)
+		ofono_gprs_context_remove(data->gc);
+
 	if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
 		ofono_info("Reopened GPRS context channel");
@@ -579,15 +585,25 @@ static int huawei_disable(struct ofono_modem *modem)
 	return -EINPROGRESS;
 }
 
+struct huawei_cb_data {
+	struct cb_data *cbd;
+	ofono_bool_t online;
+	struct ofono_modem *modem;
+};
+
 static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
-	struct cb_data *cbd = user_data;
-	ofono_modem_online_cb_t cb = cbd->cb;
+	struct huawei_cb_data *online_cbd = user_data;
+	ofono_modem_online_cb_t cb = online_cbd->cbd->cb;
+	struct huawei_data *data = ofono_modem_get_data(online_cbd->modem);
 
-	if (ok)
-		CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	if (ok) {
+		data->online = online_cbd->online;
+
+		CALLBACK_WITH_SUCCESS(cb, online_cbd->cbd->data);
+	}
 	else
-		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		CALLBACK_WITH_FAILURE(cb, online_cbd->cbd->data);
 }
 
 static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
@@ -595,21 +611,26 @@ static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
 {
 	struct huawei_data *data = ofono_modem_get_data(modem);
 	GAtChat *chat = data->pcui;
-	struct cb_data *cbd = cb_data_new(cb, user_data);
+	struct huawei_cb_data *online_cbd;
 	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=5";
 
 	DBG("modem %p %s", modem, online ? "online" : "offline");
 
-	if (cbd == NULL)
+	online_cbd = g_try_new0(struct huawei_cb_data, 1);
+	online_cbd->cbd = cb_data_new(cb, user_data);
+	if (online_cbd->cbd == NULL)
 		goto error;
+	online_cbd->online = online;
+	online_cbd->modem = modem;
 
-	if (g_at_chat_send(chat, command, NULL, set_online_cb, cbd, g_free))
+	if (g_at_chat_send(chat, command, NULL, set_online_cb,
+						online_cbd, g_free))
 		return;
 
 error:
-	g_free(cbd);
+	g_free(online_cbd);
 
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static void huawei_pre_sim(struct ofono_modem *modem)
-- 
1.6.1.3

Re: [PATCH] Fix huawei_disconnect function issue

[Marcel Holtmann]

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

Hi Martin,

> huawei_disconnect is used to recovery the io and gprs context when
> io error happends, see commit 39382730d7758b093ca6271f4e9dea875fa04b3a
> However, io error not only happends at PPP disconnect, in theory it
> can happends at any situation. I also observed that it happens when modem
> go into offline mode at my Huawei EM770W modem. in this case, gprs should
> not be reopened.

<snip>

>  static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
> @@ -595,21 +611,26 @@ static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
>  {
>  	struct huawei_data *data = ofono_modem_get_data(modem);
>  	GAtChat *chat = data->pcui;
> -	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	struct huawei_cb_data *online_cbd;
>  	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=5";
>  
>  	DBG("modem %p %s", modem, online ? "online" : "offline");
>  
> -	if (cbd == NULL)
> +	online_cbd = g_try_new0(struct huawei_cb_data, 1);
> +	online_cbd->cbd = cb_data_new(cb, user_data);
> +	if (online_cbd->cbd == NULL)
>  		goto error;

you know that you are doing exactly the wrong NULL pointer checks
here ;)

Anyhow, we need to get rid of the if (cbd == NULL) check first. A while
back we changed cb_data_new() to use g_new0() instead of g_try_new0() to
simplify the code. So first that should be all cleaned up.

A quick grep showed these locations for the AT modems:

drivers/mbmmodem/location-reporting.c:  if (cbd == NULL)
plugins/bluetooth.c:    if (cbd == NULL) {
plugins/huawei.c:       if (cbd == NULL)

The ISI modem stuff is a different story and needs to be tackled
separately anyway.

Regards

Marcel

Re: [PATCH] Fix huawei_disconnect function issue

[Lucas De Marchi]

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

Hi Marcel,

On Fri, Mar 4, 2011 at 8:04 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Anyhow, we need to get rid of the if (cbd == NULL) check first. A while
> back we changed cb_data_new() to use g_new0() instead of g_try_new0() to
> simplify the code. So first that should be all cleaned up.
>
> A quick grep showed these locations for the AT modems:
>
> drivers/mbmmodem/location-reporting.c:  if (cbd == NULL)
> plugins/bluetooth.c:    if (cbd == NULL) {
> plugins/huawei.c:       if (cbd == NULL)


Since git-blame was pointing to me for th mbmmodem and the fix was
really simple, I fixed also the huawei case. bluetooth.c does not
apply here since it uses its own struct cb_data and calls
g_try_new0().


regards,

Lucas De Marchi