[PATCH] huawei: cleanup sim polling when the modem is removed.

[PATCH] huawei: cleanup sim polling when the modem is removed.

[Bertrand Aygon]

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

---
 plugins/huawei.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 1380369..eaf8b07 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -107,6 +107,9 @@ static void huawei_remove(struct ofono_modem *modem)
 	/* Cleanup after hot-unplug */
 	g_at_chat_unref(data->pcui);
 
+	if (data->sysinfo_poll_source)
+		g_source_remove(data->sysinfo_poll_source);
+
 	g_free(data);
 }
 
-- 
1.7.4.1

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] huawei: cleanup sim polling when the modem is removed.

[Marcel Holtmann]

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

Hi Bertrand,

>  plugins/huawei.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index 1380369..eaf8b07 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -107,6 +107,9 @@ static void huawei_remove(struct ofono_modem *modem)
>  	/* Cleanup after hot-unplug */
>  	g_at_chat_unref(data->pcui);
>  
> +	if (data->sysinfo_poll_source)
> +		g_source_remove(data->sysinfo_poll_source);
> +
>  	g_free(data);
>  }

so actually you do not wanna do this in the remove callback. You wanna
do this in the disable callback just before you cancel all pending
commands.

Doing this in the remove callback has the potential problem that you
might poll CPIN while trying to execute CFUN. In theory this does not
matter since GAtChat will not execute commands at the same time, but why
bother keeping the timeout running if we already know that we are going
to shutdown the device.

We call disable before we call remove, but the only thing that I have
not checked is what happens to this if we have not succeeded with enable
yet. So if I remember this right, then disable is always called (even if
enable has not yet called set_powered), but I need Denis to confirm
this.

Regards

Marcel

RE: [PATCH] huawei: cleanup sim polling when the modem is removed.

[Aygon, Bertrand]

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

Hi Marcel,

> >  plugins/huawei.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/plugins/huawei.c b/plugins/huawei.c
> > index 1380369..eaf8b07 100644
> > --- a/plugins/huawei.c
> > +++ b/plugins/huawei.c
> > @@ -107,6 +107,9 @@ static void huawei_remove(struct ofono_modem
> *modem)
> >  	/* Cleanup after hot-unplug */
> >  	g_at_chat_unref(data->pcui);
> >
> > +	if (data->sysinfo_poll_source)
> > +		g_source_remove(data->sysinfo_poll_source);
> > +
> >  	g_free(data);
> >  }
> 
> so actually you do not wanna do this in the remove callback. You wanna
> do this in the disable callback just before you cancel all pending
> commands.

Ok, but it will not fix the crash I have saw and that conclude to this fix. If we unplug the modem during the init, we go directly to remove_modem, and not going through disable_modem.

So maybe we have to put this in both places.

> Doing this in the remove callback has the potential problem that you
> might poll CPIN while trying to execute CFUN. In theory this does not
> matter since GAtChat will not execute commands at the same time, but
> why
> bother keeping the timeout running if we already know that we are going
> to shutdown the device.
> 
> We call disable before we call remove, but the only thing that I have
> not checked is what happens to this if we have not succeeded with
> enable
> yet. So if I remember this right, then disable is always called (even
> if
> enable has not yet called set_powered), but I need Denis to confirm
> this.

Except in the unplug test.

Regards,

Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] huawei: cleanup sim polling when the modem is removed.

[Marcel Holtmann]

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

Hi Bertrand,

> We call disable before we call remove, but the only thing that I have
> not checked is what happens to this if we have not succeeded with enable
> yet. So if I remember this right, then disable is always called (even if
> enable has not yet called set_powered), but I need Denis to confirm
> this.

seems we are not calling disable if set_powered has not been called yet.

ofonod[13972]: plugins/huawei.c:huawei_enable() 0x10cfef0
ofonod[13972]: src/modem.c:get_modem_property() modem 0x10cfef0 property Modem
ofonod[13972]: plugins/huawei.c:open_device() Modem /dev/ttyUSB3
ofonod[13972]: src/modem.c:get_modem_property() modem 0x10cfef0 property Pcui
ofonod[13972]: plugins/huawei.c:open_device() Pcui /dev/ttyUSB5
ofonod[13972]: Modem: > ATE0 +CMEE=1\r
ofonod[13972]: PCUI: > ATE0 +CMEE=1\r
ofonod[13972]: PCUI: < ATE0 +CMEE=1\r\r\nOK\r\n
ofonod[13972]: PCUI: > AT+CFUN=?\r
ofonod[13972]: Modem: < \r\nOK\r\n
ofonod[13972]: PCUI: < \r\n+CFUN: (0-1,4-7),(0-1)\r\n\r\nOK\r\n
ofonod[13972]: PCUI: > AT+CFUN=1\r
ofonod[13972]: PCUI: < \r\nOK\r\n
ofonod[13972]: plugins/huawei.c:cfun_enable() 
ofonod[13972]: PCUI: > AT^SYSINFO\r
ofonod[13972]: PCUI: < \r\n^SYSINFO:2,3,0,3,1,,3\r\n\r\nOK\r\n
ofonod[13972]: plugins/huawei.c:sysinfo_enable_cb() 255 -> 1
ofonod[13972]: Modem: > AT&C0\r
ofonod[13972]: PCUI: > AT&C0\r
ofonod[13972]: PCUI: < \r\nOK\r\n
ofonod[13972]: Modem: < \r\nOK\r\n
ofonod[13972]: PCUI: > AT^CVOICE=?\r
ofonod[13972]: PCUI: < \r\n^CVOICE:(1)\r\n\r\nOK\r\n
ofonod[13972]: PCUI: > AT+CFUN=7\r
ofonod[13972]: plugins/udev.c:udev_event() subsystem net remove
ofonod[13972]: plugins/udev.c:remove_modem() /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1/1-1.1:1.1/net/wwan0
ofonod[13972]: src/modem.c:get_modem_property() modem 0x10cfef0 property Path
ofonod[13972]: src/modem.c:ofono_modem_remove() 0x10cfef0
ofonod[13972]: src/modem.c:modem_unregister() 0x10cfef0
ofonod[13972]: plugins/huawei.c:huawei_remove() 0x10cfef0

I unplugged the dongle during the enable phase. So now I am asking
myself if we might wanna have some sort of abort_enable callback?
Anyway, I check up with Denis on this one.

Regards

Marcel

RE: [PATCH] huawei: cleanup sim polling when the modem is removed.

[Marcel Holtmann]

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

Hi Betrand,

> > We call disable before we call remove, but the only thing that I have
> > not checked is what happens to this if we have not succeeded with
> > enable
> > yet. So if I remember this right, then disable is always called (even
> > if
> > enable has not yet called set_powered), but I need Denis to confirm
> > this.
> 
> Except in the unplug test.

it is just the race condition for the time enable is running. Once the
modem is properly enabled, even the unplug case is fine.

ofonod[14344]: plugins/udev.c:udev_event() subsystem net remove
ofonod[14344]: plugins/udev.c:remove_modem() /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1/1-1.1:1.1/net/wwan0
ofonod[14344]: src/modem.c:get_modem_property() modem 0x7beff0 property Path
ofonod[14344]: src/modem.c:ofono_modem_remove() 0x7beff0
ofonod[14344]: src/modem.c:modem_unregister() 0x7beff0
ofonod[14344]: src/modem.c:modem_change_state() old state: 2, new state: 0
ofonod[14344]: src/modem.c:flush_atoms() 
ofonod[14344]: Example Network Time Remove for modem: 0x7beff0
ofonod[14344]: Example History Remove for modem: 0x7beff0
ofonod[14344]: src/gprs.c:gprs_context_unregister() 0x7be230, 0x7bc8c0
ofonod[14344]: src/gprs.c:gprs_context_remove() atom: 0x7be270
ofonod[14344]: drivers/atmodem/gprs-context.c:at_gprs_context_remove() 
ofonod[14344]: Unregistered handle for channel 1: 0x10000
ofonod[14344]: src/gprs.c:gprs_unregister() 0x7bc8c0
ofonod[14344]: src/gprs.c:gprs_remove() atom: 0x7bd840
ofonod[14344]: plugins/push-notification.c:push_notification_cleanup() 0x7ba6a0
ofonod[14344]: plugins/smart-messaging.c:smart_messaging_cleanup() 0x7ba640
ofonod[14344]: src/sms.c:sms_remove() atom: 0x7bf300
ofonod[14344]: src/radio-settings.c:radio_settings_remove() atom: 0x7bea90
ofonod[14344]: src/phonebook.c:phonebook_remove() atom: 0x7ba400
ofonod[14344]: src/sim.c:sim_remove() atom: 0x7b8b00
ofonod[14344]: src/modem.c:devinfo_remove() atom: 0x7b8d00
ofonod[14344]: plugins/huawei.c:huawei_disable() 0x7beff0
ofonod[14344]: plugins/huawei.c:huawei_remove() 0x7beff0

We do call disable before we call remove in the "clean" unplug case. So
that is not the problem.

However there is a real race condition here. My previous comment still
stands that we do want to disarm the timeout in the disable callback
already in case of a clean shutdown.

However this opens another set of questions. Since now we have the
problem that we will cleanup the control TTY, but we will not cleanup
the modem TTY. So we are leaking memory and a file descriptor here.

The file descriptor is kinda nasty since a running process is still
having a file descriptor for an already removed kernel device. So
depending on how clean the kernel driver handles this, it can be no
problem or a big problem. Nevertheless the running ofonod process will
have certain problems with this over time.

Luckily that part is a small race condition that is even extremely hard
to reproduce, but it does need to be fixed.

Regards

Marcel

RE: [PATCH] huawei: cleanup sim polling when the modem is removed.

[Aygon, Bertrand]

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

Hi Marcel,

> > > We call disable before we call remove, but the only thing that I
> have
> > > not checked is what happens to this if we have not succeeded with
> > > enable
> > > yet. So if I remember this right, then disable is always called
> (even
> > > if
> > > enable has not yet called set_powered), but I need Denis to confirm
> > > this.
> >
> > Except in the unplug test.
> 
> it is just the race condition for the time enable is running. Once the
> modem is properly enabled, even the unplug case is fine.
> 
> ofonod[14344]: plugins/udev.c:udev_event() subsystem net remove
> ofonod[14344]: plugins/udev.c:remove_modem()
> /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1/1-1.1:1.1/net/wwan0
> ofonod[14344]: src/modem.c:get_modem_property() modem 0x7beff0 property
> Path
> ofonod[14344]: src/modem.c:ofono_modem_remove() 0x7beff0
> ofonod[14344]: src/modem.c:modem_unregister() 0x7beff0
> ofonod[14344]: src/modem.c:modem_change_state() old state: 2, new
> state: 0
> ofonod[14344]: src/modem.c:flush_atoms()
> ofonod[14344]: Example Network Time Remove for modem: 0x7beff0
> ofonod[14344]: Example History Remove for modem: 0x7beff0
> ofonod[14344]: src/gprs.c:gprs_context_unregister() 0x7be230, 0x7bc8c0
> ofonod[14344]: src/gprs.c:gprs_context_remove() atom: 0x7be270
> ofonod[14344]: drivers/atmodem/gprs-context.c:at_gprs_context_remove()
> ofonod[14344]: Unregistered handle for channel 1: 0x10000
> ofonod[14344]: src/gprs.c:gprs_unregister() 0x7bc8c0
> ofonod[14344]: src/gprs.c:gprs_remove() atom: 0x7bd840
> ofonod[14344]: plugins/push-notification.c:push_notification_cleanup()
> 0x7ba6a0
> ofonod[14344]: plugins/smart-messaging.c:smart_messaging_cleanup()
> 0x7ba640
> ofonod[14344]: src/sms.c:sms_remove() atom: 0x7bf300
> ofonod[14344]: src/radio-settings.c:radio_settings_remove() atom:
> 0x7bea90
> ofonod[14344]: src/phonebook.c:phonebook_remove() atom: 0x7ba400
> ofonod[14344]: src/sim.c:sim_remove() atom: 0x7b8b00
> ofonod[14344]: src/modem.c:devinfo_remove() atom: 0x7b8d00
> ofonod[14344]: plugins/huawei.c:huawei_disable() 0x7beff0
> ofonod[14344]: plugins/huawei.c:huawei_remove() 0x7beff0
> 
> We do call disable before we call remove in the "clean" unplug case. So
> that is not the problem.

Ok, I get it.

Thanks,

Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: [PATCH] huawei: cleanup sim polling when the modem is removed.

[Marcel Holtmann]

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

Hi Bertrand,

> > > > We call disable before we call remove, but the only thing that I
> > have
> > > > not checked is what happens to this if we have not succeeded with
> > > > enable
> > > > yet. So if I remember this right, then disable is always called
> > (even
> > > > if
> > > > enable has not yet called set_powered), but I need Denis to confirm
> > > > this.
> > >
> > > Except in the unplug test.
> > 
> > it is just the race condition for the time enable is running. Once the
> > modem is properly enabled, even the unplug case is fine.
> > 
> > ofonod[14344]: plugins/udev.c:udev_event() subsystem net remove
> > ofonod[14344]: plugins/udev.c:remove_modem()
> > /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1/1-1.1:1.1/net/wwan0
> > ofonod[14344]: src/modem.c:get_modem_property() modem 0x7beff0 property
> > Path
> > ofonod[14344]: src/modem.c:ofono_modem_remove() 0x7beff0
> > ofonod[14344]: src/modem.c:modem_unregister() 0x7beff0
> > ofonod[14344]: src/modem.c:modem_change_state() old state: 2, new
> > state: 0
> > ofonod[14344]: src/modem.c:flush_atoms()
> > ofonod[14344]: Example Network Time Remove for modem: 0x7beff0
> > ofonod[14344]: Example History Remove for modem: 0x7beff0
> > ofonod[14344]: src/gprs.c:gprs_context_unregister() 0x7be230, 0x7bc8c0
> > ofonod[14344]: src/gprs.c:gprs_context_remove() atom: 0x7be270
> > ofonod[14344]: drivers/atmodem/gprs-context.c:at_gprs_context_remove()
> > ofonod[14344]: Unregistered handle for channel 1: 0x10000
> > ofonod[14344]: src/gprs.c:gprs_unregister() 0x7bc8c0
> > ofonod[14344]: src/gprs.c:gprs_remove() atom: 0x7bd840
> > ofonod[14344]: plugins/push-notification.c:push_notification_cleanup()
> > 0x7ba6a0
> > ofonod[14344]: plugins/smart-messaging.c:smart_messaging_cleanup()
> > 0x7ba640
> > ofonod[14344]: src/sms.c:sms_remove() atom: 0x7bf300
> > ofonod[14344]: src/radio-settings.c:radio_settings_remove() atom:
> > 0x7bea90
> > ofonod[14344]: src/phonebook.c:phonebook_remove() atom: 0x7ba400
> > ofonod[14344]: src/sim.c:sim_remove() atom: 0x7b8b00
> > ofonod[14344]: src/modem.c:devinfo_remove() atom: 0x7b8d00
> > ofonod[14344]: plugins/huawei.c:huawei_disable() 0x7beff0
> > ofonod[14344]: plugins/huawei.c:huawei_remove() 0x7beff0
> > 
> > We do call disable before we call remove in the "clean" unplug case. So
> > that is not the problem.
> 
> Ok, I get it.

I will check with Denis if we are going to do something more
sophisticated or leave this up to the modem plugins to figure it out.
However I went back through the code and came up with this conclusion.

We do not need to check anything in the disable callback, since if
enable succeeds or fails, we clean up the poll sources. And if enable
never succeeds, then disable is never called. So that should be just
fine. No leaks here.

The only thing that I have not checked is if we can actually call the
disable modem D-Bus function while enable is in progress and what
happens in that case. I just do not have had time to confirm this case
and what is the order of callbacks and events in that case. Or if we
just block that D-Bus.

So right now, when we have to clean something up, the only rare case is
that when enabling was in progress and we unplug the dongle during that.
This means that your original patches were correct and fixed this case.
I did some simplification in the SIM state polling free function and
added comments to all of them and applied them.

While doing that, I realized also another race condition in the Huawei
modem plugin, where the online enabling polling might race against some
calling disable. I fixed that one as well.

Regards

Marcel

RE: [PATCH] huawei: cleanup sim polling when the modem is removed.

[Aygon, Bertrand]

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

Hi Marcel,

> > > > > We call disable before we call remove, but the only thing that
> I
> > > have
> > > > > not checked is what happens to this if we have not succeeded
> with
> > > > > enable
> > > > > yet. So if I remember this right, then disable is always called
> > > (even
> > > > > if
> > > > > enable has not yet called set_powered), but I need Denis to
> confirm
> > > > > this.
> > > >
> > > > Except in the unplug test.
> > >
> > > it is just the race condition for the time enable is running. Once
> the
> > > modem is properly enabled, even the unplug case is fine.
> > >
> > > ofonod[14344]: plugins/udev.c:udev_event() subsystem net remove
> > > ofonod[14344]: plugins/udev.c:remove_modem()
> > > /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.1/1-1.1:1.1/net/wwan0
> > > ofonod[14344]: src/modem.c:get_modem_property() modem 0x7beff0
> property
> > > Path
> > > ofonod[14344]: src/modem.c:ofono_modem_remove() 0x7beff0
> > > ofonod[14344]: src/modem.c:modem_unregister() 0x7beff0
> > > ofonod[14344]: src/modem.c:modem_change_state() old state: 2, new
> > > state: 0
> > > ofonod[14344]: src/modem.c:flush_atoms()
> > > ofonod[14344]: Example Network Time Remove for modem: 0x7beff0
> > > ofonod[14344]: Example History Remove for modem: 0x7beff0
> > > ofonod[14344]: src/gprs.c:gprs_context_unregister() 0x7be230,
> 0x7bc8c0
> > > ofonod[14344]: src/gprs.c:gprs_context_remove() atom: 0x7be270
> > > ofonod[14344]: drivers/atmodem/gprs-
> context.c:at_gprs_context_remove()
> > > ofonod[14344]: Unregistered handle for channel 1: 0x10000
> > > ofonod[14344]: src/gprs.c:gprs_unregister() 0x7bc8c0
> > > ofonod[14344]: src/gprs.c:gprs_remove() atom: 0x7bd840
> > > ofonod[14344]: plugins/push-
> notification.c:push_notification_cleanup()
> > > 0x7ba6a0
> > > ofonod[14344]: plugins/smart-messaging.c:smart_messaging_cleanup()
> > > 0x7ba640
> > > ofonod[14344]: src/sms.c:sms_remove() atom: 0x7bf300
> > > ofonod[14344]: src/radio-settings.c:radio_settings_remove() atom:
> > > 0x7bea90
> > > ofonod[14344]: src/phonebook.c:phonebook_remove() atom: 0x7ba400
> > > ofonod[14344]: src/sim.c:sim_remove() atom: 0x7b8b00
> > > ofonod[14344]: src/modem.c:devinfo_remove() atom: 0x7b8d00
> > > ofonod[14344]: plugins/huawei.c:huawei_disable() 0x7beff0
> > > ofonod[14344]: plugins/huawei.c:huawei_remove() 0x7beff0
> > >
> > > We do call disable before we call remove in the "clean" unplug
> case. So
> > > that is not the problem.
> >
> > Ok, I get it.
> 
> I will check with Denis if we are going to do something more
> sophisticated or leave this up to the modem plugins to figure it out.
> However I went back through the code and came up with this conclusion.
> 
> We do not need to check anything in the disable callback, since if
> enable succeeds or fails, we clean up the poll sources. And if enable
> never succeeds, then disable is never called. So that should be just
> fine. No leaks here.
> 
> The only thing that I have not checked is if we can actually call the
> disable modem D-Bus function while enable is in progress and what
> happens in that case. I just do not have had time to confirm this case
> and what is the order of callbacks and events in that case. Or if we
> just block that D-Bus.
> 
> So right now, when we have to clean something up, the only rare case is
> that when enabling was in progress and we unplug the dongle during
> that.
> This means that your original patches were correct and fixed this case.
> I did some simplification in the SIM state polling free function and
> added comments to all of them and applied them.
> 
> While doing that, I realized also another race condition in the Huawei
> modem plugin, where the online enabling polling might race against some
> calling disable. I fixed that one as well.

Ok this is perfect.

Thanks for the 'complete' fix.

Regards,

Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.