[PATCH v2] Fix busylooped in ppp_disconnect for huawei modem

[PATCH v2] Fix busylooped in ppp_disconnect for huawei modem

[Zhenhua Zhang]

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

Huawei modem closes the modem port after PPP disconnect. So the channel
of gatchat is NULL in ppp_disconnect. In such case, we resume the chat
and it causes huawei_disconnect() get called and the gprs context is
removed later.

Before removing this gprs context, we should reply the pending DBus
message to the client.
---
 drivers/atmodem/gprs-context.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index fea80b0..2f5be9b 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -88,12 +88,18 @@ static void ppp_disconnect(GAtPPPDisconnectReason reason, gpointer user_data)
 {
 	struct ofono_gprs_context *gc = user_data;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	GAtIO *io = g_at_chat_get_io(gcd->chat);
 
 	DBG("");
 
 	g_at_ppp_unref(gcd->ppp);
 	gcd->ppp = NULL;
-	g_at_chat_resume(gcd->chat);
+
+	if (g_at_io_get_channel(io) == NULL) {
+		CALLBACK_WITH_FAILURE(gcd->up_cb, NULL, FALSE, NULL,
+					NULL, NULL, NULL, gcd->cb_data);
+		goto done;
+	}
 
 	switch (gcd->state) {
 	case STATE_ENABLING:
@@ -108,8 +114,15 @@ static void ppp_disconnect(GAtPPPDisconnectReason reason, gpointer user_data)
 		break;
 	}
 
+done:
 	gcd->active_context = 0;
 	gcd->state = STATE_IDLE;
+	/*
+	 * If the channel of gcd->chat is NULL, it might cause
+	 * gprs_context_remove get called and the gprs context will be
+	 * removed.
+	 */
+	g_at_chat_resume(gcd->chat);
 }
 
 static gboolean setup_ppp(struct ofono_gprs_context *gc)
@@ -257,7 +270,7 @@ static void at_gprs_context_remove(struct ofono_gprs_context *gc)
 
 	DBG("");
 
-	if (gcd->state != STATE_IDLE) {
+	if (gcd->state != STATE_IDLE && gcd->ppp) {
 		g_at_ppp_unref(gcd->ppp);
 		g_at_chat_resume(gcd->chat);
 	}
-- 
1.7.0.4

Re: [PATCH v2] Fix busylooped in ppp_disconnect for huawei modem

[Denis Kenzior]

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

Hi Zhenhua,

On 07/26/2010 11:58 PM, Zhenhua Zhang wrote:
> Huawei modem closes the modem port after PPP disconnect. So the channel
> of gatchat is NULL in ppp_disconnect. In such case, we resume the chat
> and it causes huawei_disconnect() get called and the gprs context is
> removed later.
> 
> Before removing this gprs context, we should reply the pending DBus
> message to the client.
> ---
>  drivers/atmodem/gprs-context.c |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index fea80b0..2f5be9b 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -88,12 +88,18 @@ static void ppp_disconnect(GAtPPPDisconnectReason reason, gpointer user_data)
>  {
>  	struct ofono_gprs_context *gc = user_data;
>  	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	GAtIO *io = g_at_chat_get_io(gcd->chat);
>  
>  	DBG("");
>  
>  	g_at_ppp_unref(gcd->ppp);
>  	gcd->ppp = NULL;
> -	g_at_chat_resume(gcd->chat);
> +
> +	if (g_at_io_get_channel(io) == NULL) {
> +		CALLBACK_WITH_FAILURE(gcd->up_cb, NULL, FALSE, NULL,
> +					NULL, NULL, NULL, gcd->cb_data);
> +		goto done;
> +	}

I don't believe the above addition is actually necessary, just moving
the g_at_chat_resume to the bottom should be enough.

>  
>  	switch (gcd->state) {
>  	case STATE_ENABLING:
> @@ -108,8 +114,15 @@ static void ppp_disconnect(GAtPPPDisconnectReason reason, gpointer user_data)
>  		break;
>  	}
>  
> +done:
>  	gcd->active_context = 0;
>  	gcd->state = STATE_IDLE;
> +	/*
> +	 * If the channel of gcd->chat is NULL, it might cause
> +	 * gprs_context_remove get called and the gprs context will be
> +	 * removed.
> +	 */
> +	g_at_chat_resume(gcd->chat);
>  }
>  
>  static gboolean setup_ppp(struct ofono_gprs_context *gc)
> @@ -257,7 +270,7 @@ static void at_gprs_context_remove(struct ofono_gprs_context *gc)
>  
>  	DBG("");
>  
> -	if (gcd->state != STATE_IDLE) {
> +	if (gcd->state != STATE_IDLE && gcd->ppp) {
>  		g_at_ppp_unref(gcd->ppp);
>  		g_at_chat_resume(gcd->chat);
>  	}

Regards,
-Denis

Re: [PATCH v2] Fix busylooped in ppp_disconnect for huawei modem

[Kalle Valo]

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

Zhenhua Zhang <zhenhua.zhang@intel.com> writes:

> Huawei modem closes the modem port after PPP disconnect. So the channel
> of gatchat is NULL in ppp_disconnect. In such case, we resume the chat
> and it causes huawei_disconnect() get called and the gprs context is
> removed later.
>
> Before removing this gprs context, we should reply the pending DBus
> message to the client.

I tested the patch with my Huawei E1552 and it worked perfectly. Earlier
it was very easy to trigger the crash. I even tried some of my evil
tests and found no issues.

Tested-by: Kalle Valo <kalle.valo@canonical.com> :-)

-- 
Kalle Valo