[PATCH] Fix busylooped in ppp_disconnect for huawei modem

[PATCH] Fix busylooped in ppp_disconnect for huawei modem

[Zhenhua Zhang]

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

Huawei modem closes the modem port after PPP disconnect. So the channel
of gatchat is NULL in ppp_disconnect. In such case, we should not resume
the chat and call disconnect function when removing the context.
Secondly, before removing the gprs context, we should reply the pending
DBus message to the client.
---
 drivers/atmodem/gprs-context.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index fea80b0..e2f291a 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -88,11 +88,22 @@ 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;
+
+	if (g_at_io_get_channel(io) == NULL) {
+		CALLBACK_WITH_FAILURE(gcd->up_cb, NULL, FALSE, NULL,
+					NULL, NULL, NULL, gcd->cb_data);
+		gcd->active_context = 0;
+		gcd->state = STATE_IDLE;
+		g_at_chat_resume(gcd->chat);
+		return;
+	}
+
 	g_at_chat_resume(gcd->chat);
 
 	switch (gcd->state) {
@@ -257,7 +268,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] Fix busylooped in ppp_disconnect for huawei modem

[Zhang, Zhenhua]

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

Hi Kalle,

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 should not
> resume 
> the chat and call disconnect function when removing the context.
> Secondly, before removing the gprs context, we should reply the
> pending 
> DBus message to the client.
> ---
>  drivers/atmodem/gprs-context.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c
> b/drivers/atmodem/gprs-context.c 
> index fea80b0..e2f291a 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -88,11 +88,22 @@ 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;
> +
> +	if (g_at_io_get_channel(io) == NULL) {
> +		CALLBACK_WITH_FAILURE(gcd->up_cb, NULL, FALSE, NULL,
> +					NULL, NULL, NULL, gcd->cb_data);
> +		gcd->active_context = 0;
> +		gcd->state = STATE_IDLE;
> +		g_at_chat_resume(gcd->chat);
> +		return;
> +	}
> +
>  	g_at_chat_resume(gcd->chat);
> 
>  	switch (gcd->state) {
> @@ -257,7 +268,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);
>  	}

Could you try my fix for this busy loop issue? It is not perfect but at least work for me.
The problem is: in ppp_disconnect, after g_at_chat_resume(), the gprs context has
been removed and recreated, so the rest gc and gcd are totally wrong.

Btw, I still meet the kernel warning or even Ubuntu crash after activate/deactivate gprs
context multiple times on E1552. Do you see such problem? the kernel dmesg is
attached for reference.

Regards,
Zhenhua

[-- Attachment #2: E1552_dmesg.txt --]
[-- Type: text/plain, Size: 4863 bytes --]

Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301093] ------------[ cut here ]------------
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301106] WARNING: at /build/buildd/linux-2.6.32/drivers/usb/serial/usb-serial.c:406 serial_write_room+0x75/0x80 [usbserial]()
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301108] Hardware name: 7661BL4
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301110] Modules linked in: nls_utf8 isofs option usbserial usb_storage aes_i586 aes_generic hidp rfcomm ppdev sco bnep l2cap binfmt_misc snd_hda_codec_analog fbcon tileblit font bitblit softcursor vga16fb vgastate joydev snd_hda_intel snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss thinkpad_acpi snd_pcm snd_seq_dummy snd_seq_oss snd_seq_midi snd_rawmidi snd_seq_midi_event arc4 snd_seq pcmcia iwlagn snd_timer iwlcore snd_seq_device i915 drm_kms_helper snd mac80211 drm uvcvideo videodev v4l1_compat btusb bluetooth nvram yenta_socket rsrc_nonstatic sdhci_pci sdhci led_class ricoh_mmc pcmcia_core psmouse serio_raw cfg80211 intel_agp agpgart i2c_algo_bit video output soundcore snd_page_alloc lp parport usbhid hid ohci1394 ieee1394 e1000e
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301187] Pid: 2513, comm: ofonod Tainted: G        W  2.6.32-21-generic #32-Ubuntu
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301189] Call Trace:
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301196]  [<c014c3d2>] warn_slowpath_common+0x72/0xa0
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301200]  [<fdaaa735>] ? serial_write_room+0x75/0x80 [usbserial]
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301204]  [<fdaaa735>] ? serial_write_room+0x75/0x80 [usbserial]
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301208]  [<c014c41a>] warn_slowpath_null+0x1a/0x20
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301211]  [<fdaaa735>] serial_write_room+0x75/0x80 [usbserial]
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301216]  [<c03b831d>] tty_write_room+0x1d/0x20
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301219]  [<c03b6127>] n_tty_poll+0x177/0x190
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301222]  [<c03b21f9>] tty_poll+0x69/0x80
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301226]  [<c058b5e8>] ? _spin_lock_irq+0x18/0x20
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301229]  [<c03b5fb0>] ? n_tty_poll+0x0/0x190
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301232]  [<c021727b>] do_poll+0xdb/0x230
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301236]  [<c0217d56>] do_sys_poll+0x136/0x1f0
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301239]  [<c0217ae0>] ? __pollwait+0x0/0xe0
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301242]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301244]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301247]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301250]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301253]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301255]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301258]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301261]  [<c0217bc0>] ? pollwake+0x0/0x60
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301265]  [<c04b3865>] ? sock_sendmsg+0xe5/0x110
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301269]  [<c0167740>] ? autoremove_wake_function+0x0/0x50
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301272]  [<c04b3865>] ? sock_sendmsg+0xe5/0x110
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301276]  [<c03532bd>] ? copy_from_user+0x3d/0x130
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301279]  [<c04bc88a>] ? verify_iovec+0x5a/0xa0
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301282]  [<c04b41ad>] ? sys_sendmsg+0x15d/0x290
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301285]  [<c058b5e8>] ? _spin_lock_irq+0x18/0x20
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301289]  [<c0163ff2>] ? flush_work+0x52/0xa0
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301292]  [<c03b6d83>] ? n_tty_read+0x2f3/0x720
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301295]  [<c03b6580>] ? n_tty_write+0x0/0x270
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301299]  [<c03b9c4d>] ? tty_ldisc_deref+0xd/0x10
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301302]  [<c03b232d>] ? tty_read+0x8d/0xc0
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301304]  [<c03b6a90>] ? n_tty_read+0x0/0x720
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301308]  [<c04b47cc>] ? sys_socketcall+0xcc/0x280
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301311]  [<c0217f89>] sys_poll+0x59/0xc0
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301314]  [<c01033ec>] syscall_call+0x7/0xb
Jul 27 10:13:17 zzhan17-mobile kernel: [  599.301316] ---[ end trace ddff72d5b0b69616 ]---

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

[Denis Kenzior]

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

Hi Zhenhua,

On 07/26/2010 09:39 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 should not resume
> the chat and call disconnect function when removing the context.

Please reword the last sentence, we should resume the chat, but the
question is of timing...

> Secondly, before removing the gprs context, we should reply the pending
> DBus message to the client.
> ---
>  drivers/atmodem/gprs-context.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index fea80b0..e2f291a 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -88,11 +88,22 @@ 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;
> +
> +	if (g_at_io_get_channel(io) == NULL) {
> +		CALLBACK_WITH_FAILURE(gcd->up_cb, NULL, FALSE, NULL,
> +					NULL, NULL, NULL, gcd->cb_data);
> +		gcd->active_context = 0;
> +		gcd->state = STATE_IDLE;
> +		g_at_chat_resume(gcd->chat);
> +		return;
> +	}
> +

Instead of doing that, can't we simply move g_at_chat_resume to the
bottom of the function and put in a comment this might cause
gprs_context_remove to get called?

>  	g_at_chat_resume(gcd->chat);
>  
>  	switch (gcd->state) {
> @@ -257,7 +268,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) {

This along with the funny g_at_chat_resume logic is what seems to be
causing the infinite loop.

>  		g_at_ppp_unref(gcd->ppp);
>  		g_at_chat_resume(gcd->chat);
>  	}

Thanks,
-Denis

RE: [PATCH] Fix busylooped in ppp_disconnect for huawei modem

[Zhang, Zhenhua]

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

Hi Denis,

Denis Kenzior wrote:
> Hi Zhenhua,
> 
> On 07/26/2010 09:39 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 should not
>> resume 
>> the chat and call disconnect function when removing the context.
> 
> Please reword the last sentence, we should resume the chat, but the
> question is of timing...

It should be something like 'not resume the chat twice'. One is in
ppp_disconnect and the second is in at_gprs_context_remove.

>> Secondly, before removing the gprs context, we should reply the
>> pending 
>> DBus message to the client.
>> ---
>>  drivers/atmodem/gprs-context.c |   13 ++++++++++++-
>>  1 files changed, 12 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/atmodem/gprs-context.c
>> b/drivers/atmodem/gprs-context.c 
>> index fea80b0..e2f291a 100644
>> --- a/drivers/atmodem/gprs-context.c
>> +++ b/drivers/atmodem/gprs-context.c
>> @@ -88,11 +88,22 @@ 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;
>> +
>> +	if (g_at_io_get_channel(io) == NULL) {
>> +		CALLBACK_WITH_FAILURE(gcd->up_cb, NULL, FALSE, NULL,
>> +					NULL, NULL, NULL, gcd->cb_data);
>> +		gcd->active_context = 0;
>> +		gcd->state = STATE_IDLE;
>> +		g_at_chat_resume(gcd->chat);
>> +		return;
>> +	}
>> +
> 
> Instead of doing that, can't we simply move g_at_chat_resume to the
> bottom of the function and put in a comment this might cause
> gprs_context_remove to get called?

It's better than my current way. The updated patch will send out soon. Please 
review it.

>>  	g_at_chat_resume(gcd->chat);
>> 
>>  	switch (gcd->state) {
>> @@ -257,7 +268,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) {
> 
> This along with the funny g_at_chat_resume logic is what seems to be
> causing the infinite loop.
> 
>>  		g_at_ppp_unref(gcd->ppp);
>>  		g_at_chat_resume(gcd->chat);
>>  	}
> 
> Thanks,
> -Denis



Regards,
Zhenhua

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

[Kalle Valo]

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

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

> Hi Kalle,

Hi Zhenhua,

> Could you try my fix for this busy loop issue? It is not perfect but
> at least work for me.

I noticed you sent v2. I'm testing that right now and will send the
results as a reply to that mail.

> Btw, I still meet the kernel warning or even Ubuntu crash after
> activate/deactivate gprs context multiple times on E1552. Do you see
> such problem? the kernel dmesg is attached for reference.

I have had lots of disconnect issues, but they have been always in
ofono. I don't recall seeing a single kernel issue during the last three
months I have been testing my Huawei E1552.

-- 
Kalle Valo