[PATCH 1/1] he910: set modem unpowered after IO disconnect

[PATCH 1/1] he910: set modem unpowered after IO disconnect

[Jonas Bonn]

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

This patch address an issue with SIM hotplug.  If the SIM card is removed
while there is an active connection then the io_disconnect function
of gatchat.c pulls the rug out from under us by cleaning up the modem
chat that we hold a reference to.

This patch addresses this by hooking into the notification from
io_disconnect and setting the modem to 'unpowered' state in order to force
a reinitialization when the SIM is reinserted.

The modem is not actually powered off as that would turn off SIM detection
as well.

With this patch, SIM hotplug works reliably.
---
 plugins/he910.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/plugins/he910.c b/plugins/he910.c
index 38dbfac..999889f 100644
--- a/plugins/he910.c
+++ b/plugins/he910.c
@@ -76,6 +76,22 @@ static void he910_debug(const char *str, void *user_data)
 	ofono_info("%s%s", prefix, str);
 }
 
+static void he910_disconnected_cb(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct he910_data *data = ofono_modem_get_data(modem);
+
+	DBG("HE910 disconnected");
+
+	ofono_modem_set_powered(modem, FALSE);
+
+	g_at_chat_unref(data->chat);
+	data->chat = NULL;
+
+	g_at_chat_unref(data->modem);
+	data->modem = NULL;
+}
+
 static GAtChat *open_device(struct ofono_modem *modem,
 				const char *key, char *debug)
 {
@@ -110,6 +126,8 @@ static GAtChat *open_device(struct ofono_modem *modem,
 	if (chat == NULL)
 		return NULL;
 
+	g_at_chat_set_disconnect_function(chat, he910_disconnected_cb, modem);
+
 	if (getenv("OFONO_AT_DEBUG"))
 		g_at_chat_set_debug(chat, he910_debug, debug);
 
-- 
1.9.1

Re: [PATCH 1/1] he910: set modem unpowered after IO disconnect

[Denis Kenzior]

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

Hi Jonas,

On 05/02/2014 09:15 AM, Jonas Bonn wrote:
> This patch address an issue with SIM hotplug.  If the SIM card is removed
> while there is an active connection then the io_disconnect function
> of gatchat.c pulls the rug out from under us by cleaning up the modem
> chat that we hold a reference to.

So what actually happens when the SIM is pulled?  This seems to imply
that the modem port at least sends a HUP and is no longer usable.  Is
the control port also HUPed?

> 
> This patch addresses this by hooking into the notification from
> io_disconnect and setting the modem to 'unpowered' state in order to force
> a reinitialization when the SIM is reinserted.
> 
> The modem is not actually powered off as that would turn off SIM detection
> as well.

Are you able to at least turn off radio circuits, e.g. via CFUN=4?

> 
> With this patch, SIM hotplug works reliably.

When the modem is turned off, the application really has no idea why it
was turned off (whether due to an administrative action or due to SIM
being removed).   So the proposed patch is not a good implementation of
'SIM hotplug'.  One should implement this via
ofono_sim_inserted_notify().  Any reason why this can't be done?

Regards,
-Denis

Re: [PATCH 1/1] he910: set modem unpowered after IO disconnect

[Jonas Bonn]

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

Hi Denis,

On 05/06/2014 03:38 PM, Denis Kenzior wrote:
> Hi Jonas,
> 
> On 05/02/2014 09:15 AM, Jonas Bonn wrote:
>> This patch address an issue with SIM hotplug.  If the SIM card is removed
>> while there is an active connection then the io_disconnect function
>> of gatchat.c pulls the rug out from under us by cleaning up the modem
>> chat that we hold a reference to.
> 
> So what actually happens when the SIM is pulled?  This seems to imply
> that the modem port at least sends a HUP and is no longer usable.  Is
> the control port also HUPed?

The control port is still usable so it doesn't appear that it HUP's.

> 
>>
>> This patch addresses this by hooking into the notification from
>> io_disconnect and setting the modem to 'unpowered' state in order to force
>> a reinitialization when the SIM is reinserted.
>>
>> The modem is not actually powered off as that would turn off SIM detection
>> as well.
> 
> Are you able to at least turn off radio circuits, e.g. via CFUN=4?

As the control port is usable, I presume so.  Setting CFUN=4 kills power
to the SIM, though, so we wouldn't be able to detect SIM reinsertion if
we did this (though I don't think that's what you're asking for).

> 
>>
>> With this patch, SIM hotplug works reliably.
> 
> When the modem is turned off, the application really has no idea why it
> was turned off (whether due to an administrative action or due to SIM
> being removed).   So the proposed patch is not a good implementation of
> 'SIM hotplug'.  One should implement this via
> ofono_sim_inserted_notify().  Any reason why this can't be done?

I'm not sure that's sufficient...

When the modem port HUP's, io_disconnect is called which results in
modem->chat being cleaned up; not unref'd, either, but forcibly free'd.
 Somehow we need to be able to recreate this data structure, but since
it's created in the modem's init function it won't be unless we "power
down" the modem (or something along those lines).

Suggestions for a better way forward welcome...

/Jonas


> 
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
> 

Re: [PATCH 1/1] he910: set modem unpowered after IO disconnect

[Denis Kenzior]

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

Hi Jonas,

<snip>

>
> The control port is still usable so it doesn't appear that it HUP's.

Hah, so why would they HUP the modem port instead of sending a NO CARRIER.

> As the control port is usable, I presume so.  Setting CFUN=4 kills power
> to the SIM, though, so we wouldn't be able to detect SIM reinsertion if
> we did this (though I don't think that's what you're asking for).

Yes, I know.  But you're turning the device off.  Hence why I asked.

By the way, the newer HE910 firmware does not turn off the SIM card when
CFUN=4 is issued.

>> When the modem is turned off, the application really has no idea why it
>> was turned off (whether due to an administrative action or due to SIM
>> being removed).   So the proposed patch is not a good implementation of
>> 'SIM hotplug'.  One should implement this via
>> ofono_sim_inserted_notify().  Any reason why this can't be done?
> 
> I'm not sure that's sufficient...
> 
> When the modem port HUP's, io_disconnect is called which results in
> modem->chat being cleaned up; not unref'd, either, but forcibly free'd.

Well it is not forcibly freed.  Just the internal state is.  This is why
the disconnect callback is provided.

>  Somehow we need to be able to recreate this data structure, but since
> it's created in the modem's init function it won't be unless we "power
> down" the modem (or something along those lines).

You would need to re-create the affected atoms.  In this case this would
be the gprs-context atom.

> 
> Suggestions for a better way forward welcome...
> 

Is SIM hotswap actually supported by this hardware?  The only HE910
samples I have are miniPCI Express cards.  For those it is unlikely that
a hotswap function is even possible.  Are you getting proper QSS
notifications on the control port when the SIM is removed / inserted?

Regards,
-Denis

[PATCH v2 1/1] he910: reset modem after IO disconnect

[Jonas Bonn]

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

This patch primarily addresses an issue with SIM hotplug.  If the SIM
card is removed while there is an active data connection then the
io_disconnect function of gatchat.c pulls the rug out from under us
by cleaning up the modem chat that we hold a reference to.

This patch addresses this by hooking into the notification from
io_disconnect, cleaning up the chat structures in question, and
resetting the modem.

This patch also addresses a secondary issue whereby killing ofono during
an active connection causes the AUX port to report HUP when ofono is
restarted.

With this patch, SIM hotplug works reliably, as does restarting ofono
during an active connection.
---

Hi Denis,

How about this approach instead?  Using ofono_modem_reset has pretty
much the same effect as my previous patch and _feels" correct...

HUP on the modem port when the SIM is unplugged and HUP on the aux port
when ofono is restarted are easily reproducible for me.

/Jonas

 plugins/he910.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/plugins/he910.c b/plugins/he910.c
index 38dbfac..94fc1c4 100644
--- a/plugins/he910.c
+++ b/plugins/he910.c
@@ -76,6 +76,22 @@ static void he910_debug(const char *str, void *user_data)
 	ofono_info("%s%s", prefix, str);
 }
 
+static void he910_disconnected_cb(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct he910_data *data = ofono_modem_get_data(modem);
+
+	DBG("HE910 disconnected");
+
+	g_at_chat_unref(data->chat);
+	data->chat = NULL;
+
+	g_at_chat_unref(data->modem);
+	data->modem = NULL;
+
+	ofono_modem_reset(modem);
+}
+
 static GAtChat *open_device(struct ofono_modem *modem,
 				const char *key, char *debug)
 {
@@ -110,6 +126,8 @@ static GAtChat *open_device(struct ofono_modem *modem,
 	if (chat == NULL)
 		return NULL;
 
+	g_at_chat_set_disconnect_function(chat, he910_disconnected_cb, modem);
+
 	if (getenv("OFONO_AT_DEBUG"))
 		g_at_chat_set_debug(chat, he910_debug, debug);
 
-- 
1.9.1

Re: [PATCH v2 1/1] he910: reset modem after IO disconnect

[Denis Kenzior]

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

Hi Jonas,

<snip>
>
> This patch also addresses a secondary issue whereby killing ofono during
> an active connection causes the AUX port to report HUP when ofono is
> restarted.
> 

I'm not quite sure how this patch would fix the above.  The order of
events is the same between kill/restart and a modem reset.  I guess the
only difference is that you don't send a CFUN=4 during a modem reset.

> How about this approach instead?  Using ofono_modem_reset has pretty
> much the same effect as my previous patch and _feels" correct...

ofono_modem_reset is intended for firmware crashes and getting back to
previous state quickly.  This is a creative hack, but has some
side-effects you might not intend (e.g. the modem might go online
without being told explicitly)

I don't really mind the patch, but would explore the 'proper' avenues
(e.g. ofono_sim_inserted_notify) first.

Regards,
-Denis

Re: [PATCH v2 1/1] he910: reset modem after IO disconnect

[Jonas Bonn]

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

Hi Denis,

(Merging your two responses into one here...)

On 05/07/2014 06:03 PM, Denis Kenzior wrote:> Hi Jonas,
>
> <snip>
>
>>
>> The control port is still usable so it doesn't appear that it HUP's.
>
> Hah, so why would they HUP the modem port instead of sending a NO CARRIER.

That I can't tell you... but nothing with this modem surprises me
anymore! :)

>
> By the way, the newer HE910 firmware does not turn off the SIM card when
> CFUN=4 is issued.

Interesting...  I'll need to try to get some updated documentation from
Telit then.


>
>>  Somehow we need to be able to recreate this data structure, but since
>> it's created in the modem's init function it won't be unless we "power
>> down" the modem (or something along those lines).
>
> You would need to re-create the affected atoms.  In this case this would
> be the gprs-context atom.

The gprs-context is created in post_online... we require functioning
communication to the modem to get that far.

When the modem or aux port HUP's, io_disconnect cleans up the at_chat
and chat structures associated with it.  This means that the
command_queue disappears and no more commands can be sent to the modem
via that chat (which we still reference).  We can hook into
user_disconenct to find out that the chat has been cleaned up and drop
our references, but we need to get the modem back on its feet again by
reopening the modem and aux ports and redoing the modem initialization.

>
>>
>> Suggestions for a better way forward welcome...
>>
>
> Is SIM hotswap actually supported by this hardware?  The only HE910
> samples I have are miniPCI Express cards.  For those it is unlikely that
> a hotswap function is even possible.  Are you getting proper QSS
> notifications on the control port when the SIM is removed / inserted?

SIM hotswap is definitely supported by this hardware.

QSS notifications work perfectly and ofono_simi_inserted_notify is
invoked properly via the switch_sim_status function.  The modem I have
is BGA-type soldered onto board and SIM holder pops out through unit's
case to allow for easy SIM swap.

And SIM hotswap works fine with the patches I have sent... even if
thoses patches maybe aren't 100% correct.  But I'm not really sure what
100% correct would be for this case since we're essentially trying to
work around a modem quirk (bug?) in that it HUP's occasionally.  That's
where I need your help! :)

On 05/07/2014 06:28 PM, Denis Kenzior wrote:
> Hi Jonas,
> 
> <snip>
>>
>> This patch also addresses a secondary issue whereby killing ofono during
>> an active connection causes the AUX port to report HUP when ofono is
>> restarted.
>>
> 
> I'm not quite sure how this patch would fix the above.  The order of
> events is the same between kill/restart and a modem reset.  I guess the
> only difference is that you don't send a CFUN=4 during a modem reset.
> 

I think it might be related to the fact that the modem (according to
documentation) requires that 10 seconds elapse between transitions from
CFUN 1 to 4 and vice versa.  Restarting ofono presumably is quicker than
this and results in a CFUN  1 - > 4 -> 1 transition that is too fast and
the AUX port HUP's as a result.  Simply retrying the initialization (as
the modem_reset patch does) gets us around this.

(I haven't really checked, but maybe the AUX port doesn't HUP if we wait
a bit before restarting ofono).

>> How about this approach instead?  Using ofono_modem_reset has pretty
>> much the same effect as my previous patch and _feels" correct...
> 
> ofono_modem_reset is intended for firmware crashes and getting back to
> previous state quickly.  This is a creative hack, but has some
> side-effects you might not intend (e.g. the modem might go online
> without being told explicitly)
> 
> I don't really mind the patch, but would explore the 'proper' avenues
> (e.g. ofono_sim_inserted_notify) first.

Like I said above, this is in place and works in the unpatched code.
The issue at hand is that the modem HUP's: the modem port when the SIM
card is pulled and the aux port when ofono is restarted.

Maybe this issue goes away with a firmware update... will need to look
into that, too.

/Jonas

Re: [PATCH v2 1/1] he910: reset modem after IO disconnect

[Denis Kenzior]

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

Hi Jonas,

>>
>>>  Somehow we need to be able to recreate this data structure, but since
>>> it's created in the modem's init function it won't be unless we "power
>>> down" the modem (or something along those lines).
>>
>> You would need to re-create the affected atoms.  In this case this would
>> be the gprs-context atom.
> 
> The gprs-context is created in post_online... we require functioning
> communication to the modem to get that far.

So here's the thing, when you pull the SIM and call
ofono_sim_inserted_notify(modem, FALSE) all post_sim and post_online
atoms get destroyed.  So everything would be re-created anyway once you
reach post_sim/post_online state again.  So in theory, the only thing
you need to take care of is re-open the modem chat port and re-set the
slave.

> 
> When the modem or aux port HUP's, io_disconnect cleans up the at_chat
> and chat structures associated with it.  This means that the
> command_queue disappears and no more commands can be sent to the modem
> via that chat (which we still reference).  We can hook into
> user_disconenct to find out that the chat has been cleaned up and drop
> our references, but we need to get the modem back on its feet again by
> reopening the modem and aux ports and redoing the modem initialization.
> 

So I'm confused, you said that the aux port is not HUPed.  If that is
the case, then the AUX queue (which is used by every atom except
gprs-context) is not cleared.  Essentially you need to issue an
ofono_sim_inserted_notify(modem, FALSE) and re-create the modem chat.
After that things should just work.

> SIM hotswap is definitely supported by this hardware.

Okay, cool.  That is good to know.

Regards,
-Denis