[PATCHv2 0/2] Robustness fixes for mbm

[PATCHv2 0/2] Robustness fixes for mbm

[Pekka.Pessi]

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

Hi all,

Here are some robustness fixes for MBM modem.

In 1/2 I've corrected my older patches which tried to detect the presence of
the SIM card on the particular error response returned to AT+CPIN?. It did
not work as expected.

The patch 2/2 addresses problems initializing the modem too early. The
initial command tends to fail when a MBM modem is inserted via USB and oFono
tries to access the modem while it is still powering up and initializing
itself.

--Pekka

[PATCHv2 1/2] mbm: fix initial polling for sim

[Pekka.Pessi]

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

From: Pekka Pessi <Pekka.Pessi@nokia.com>

There seems to be no specific error codes returned when SIM is missing.
Poll at least 5 times upon an error and give up after that.
---
 plugins/mbm.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/plugins/mbm.c b/plugins/mbm.c
index eb7b1a4..8541aaf 100644
--- a/plugins/mbm.c
+++ b/plugins/mbm.c
@@ -108,10 +108,8 @@ static void simpin_check(gboolean ok, GAtResult *result, gpointer user_data)
 
 	DBG("");
 
-	/* Modem returns +CME ERROR: 10 if SIM is not ready. */
-	if (!ok && result->final_or_pdu &&
-			!strcmp(result->final_or_pdu, "+CME ERROR: 10") &&
-			data->cpin_poll_count++ < 5) {
+	/* Modem returns an error if SIM is not ready. */
+	if (!ok && data->cpin_poll_count++ < 5) {
 		data->cpin_poll_source =
 			g_timeout_add_seconds(1, init_simpin_check, modem);
 		return;
@@ -119,7 +117,7 @@ static void simpin_check(gboolean ok, GAtResult *result, gpointer user_data)
 
 	data->cpin_poll_count = 0;
 
-	/* Modem returns ERROR if there is no SIM in slot. */
+	/* There is probably no SIM if SIM is not ready after 5 seconds. */
 	data->have_sim = ok;
 
 	ofono_modem_set_powered(modem, TRUE);
-- 
1.7.0.4

[PATCHv2 2/2] mbm: retry modem init

[Pekka.Pessi]

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

From: Pekka Pessi <Pekka.Pessi@nokia.com>

If the first initialization command results are catastrophic. Retry it
10 times and refuse to enable modem if it still fails.
---
 plugins/mbm.c |   75 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/plugins/mbm.c b/plugins/mbm.c
index 8541aaf..753a4f1 100644
--- a/plugins/mbm.c
+++ b/plugins/mbm.c
@@ -55,8 +55,8 @@ static const char *none_prefix[] = { NULL };
 struct mbm_data {
 	GAtChat *modem_port;
 	GAtChat *data_port;
-	guint cpin_poll_source;
-	guint cpin_poll_count;
+	guint poll_source;
+	guint poll_count;
 	gboolean have_sim;
 };
 
@@ -86,8 +86,8 @@ static void mbm_remove(struct ofono_modem *modem)
 	g_at_chat_unref(data->data_port);
 	g_at_chat_unref(data->modem_port);
 
-	if (data->cpin_poll_source > 0)
-		g_source_remove(data->cpin_poll_source);
+	if (data->poll_source > 0)
+		g_source_remove(data->poll_source);
 
 	g_free(data);
 }
@@ -109,13 +109,13 @@ static void simpin_check(gboolean ok, GAtResult *result, gpointer user_data)
 	DBG("");
 
 	/* Modem returns an error if SIM is not ready. */
-	if (!ok && data->cpin_poll_count++ < 5) {
-		data->cpin_poll_source =
+	if (!ok && data->poll_count++ < 5) {
+		data->poll_source =
 			g_timeout_add_seconds(1, init_simpin_check, modem);
 		return;
 	}
 
-	data->cpin_poll_count = 0;
+	data->poll_count = 0;
 
 	/* There is probably no SIM if SIM is not ready after 5 seconds. */
 	data->have_sim = ok;
@@ -128,7 +128,7 @@ static gboolean init_simpin_check(gpointer user_data)
 	struct ofono_modem *modem = user_data;
 	struct mbm_data *data = ofono_modem_get_data(modem);
 
-	data->cpin_poll_source = 0;
+	data->poll_source = 0;
 
 	g_at_chat_send(data->modem_port, "AT+CPIN?", cpin_prefix,
 			simpin_check, modem, NULL);
@@ -220,6 +220,55 @@ static void emrdy_query(gboolean ok, GAtResult *result, gpointer user_data)
 					cfun_query, modem, NULL);
 };
 
+static gboolean init_check(gpointer user_data);
+
+static void init_result(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct mbm_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	if (!ok && data->poll_count++ < 10) {
+		data->poll_source = g_timeout_add_seconds(1, init_check, modem);
+		return;
+	}
+
+	data->poll_count = 0;
+
+	if (!ok) {
+		g_at_chat_unref(data->modem_port);
+		data->modem_port = NULL;
+
+		g_at_chat_unref(data->data_port);
+		data->data_port = NULL;
+
+		ofono_modem_set_powered(modem, FALSE);
+		return;
+	}
+
+	g_at_chat_register(data->modem_port, "*EMRDY:", emrdy_notifier,
+					FALSE, modem, NULL);
+
+	g_at_chat_send(data->modem_port, "AT*E2CFUN=1", none_prefix,
+					NULL, NULL, NULL);
+	g_at_chat_send(data->modem_port, "AT*EMRDY?", none_prefix,
+				emrdy_query, modem, NULL);
+}
+
+static gboolean init_check(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct mbm_data *data = ofono_modem_get_data(modem);
+
+	data->poll_source = 0;
+
+	g_at_chat_send(data->modem_port, "AT&F E0 V1 X4 &C1 +CMEE=1", NULL,
+					init_result, modem, NULL);
+
+	return FALSE;
+}
+
 static GAtChat *create_port(const char *device)
 {
 	GAtSyntax *syntax;
@@ -277,15 +326,7 @@ static int mbm_enable(struct ofono_modem *modem)
 	if (getenv("OFONO_AT_DEBUG"))
 		g_at_chat_set_debug(data->data_port, mbm_debug, "Data:");
 
-	g_at_chat_register(data->modem_port, "*EMRDY:", emrdy_notifier,
-					FALSE, modem, NULL);
-
-	g_at_chat_send(data->modem_port, "AT&F E0 V1 X4 &C1 +CMEE=1", NULL,
-					NULL, NULL, NULL);
-	g_at_chat_send(data->modem_port, "AT*E2CFUN=1", none_prefix,
-					NULL, NULL, NULL);
-	g_at_chat_send(data->modem_port, "AT*EMRDY?", none_prefix,
-				emrdy_query, modem, NULL);
+	init_check(modem);
 
 	return -EINPROGRESS;
 }
-- 
1.7.0.4

Re: [PATCHv2 2/2] mbm: retry modem init

[Marcel Holtmann]

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

Hi Pekka,

> If the first initialization command results are catastrophic. Retry it
> 10 times and refuse to enable modem if it still fails.
> ---
>  plugins/mbm.c |   75 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/plugins/mbm.c b/plugins/mbm.c
> index 8541aaf..753a4f1 100644
> --- a/plugins/mbm.c
> +++ b/plugins/mbm.c
> @@ -55,8 +55,8 @@ static const char *none_prefix[] = { NULL };
>  struct mbm_data {
>  	GAtChat *modem_port;
>  	GAtChat *data_port;
> -	guint cpin_poll_source;
> -	guint cpin_poll_count;
> +	guint poll_source;
> +	guint poll_count;
>  	gboolean have_sim;
>  };
>  
> @@ -86,8 +86,8 @@ static void mbm_remove(struct ofono_modem *modem)
>  	g_at_chat_unref(data->data_port);
>  	g_at_chat_unref(data->modem_port);
>  
> -	if (data->cpin_poll_source > 0)
> -		g_source_remove(data->cpin_poll_source);
> +	if (data->poll_source > 0)
> +		g_source_remove(data->poll_source);
>  
>  	g_free(data);
>  }
> @@ -109,13 +109,13 @@ static void simpin_check(gboolean ok, GAtResult *result, gpointer user_data)
>  	DBG("");
>  
>  	/* Modem returns an error if SIM is not ready. */
> -	if (!ok && data->cpin_poll_count++ < 5) {
> -		data->cpin_poll_source =
> +	if (!ok && data->poll_count++ < 5) {
> +		data->poll_source =
>  			g_timeout_add_seconds(1, init_simpin_check, modem);
>  		return;
>  	}
>  
> -	data->cpin_poll_count = 0;
> +	data->poll_count = 0;
>  
>  	/* There is probably no SIM if SIM is not ready after 5 seconds. */
>  	data->have_sim = ok;
> @@ -128,7 +128,7 @@ static gboolean init_simpin_check(gpointer user_data)
>  	struct ofono_modem *modem = user_data;
>  	struct mbm_data *data = ofono_modem_get_data(modem);
>  
> -	data->cpin_poll_source = 0;
> +	data->poll_source = 0;
>  
>  	g_at_chat_send(data->modem_port, "AT+CPIN?", cpin_prefix,
>  			simpin_check, modem, NULL);
> @@ -220,6 +220,55 @@ static void emrdy_query(gboolean ok, GAtResult *result, gpointer user_data)
>  					cfun_query, modem, NULL);
>  };
>  
> +static gboolean init_check(gpointer user_data);
> +
> +static void init_result(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct mbm_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	if (!ok && data->poll_count++ < 10) {
> +		data->poll_source = g_timeout_add_seconds(1, init_check, modem);
> +		return;
> +	}
> +
> +	data->poll_count = 0;
> +
> +	if (!ok) {
> +		g_at_chat_unref(data->modem_port);
> +		data->modem_port = NULL;
> +
> +		g_at_chat_unref(data->data_port);
> +		data->data_port = NULL;
> +
> +		ofono_modem_set_powered(modem, FALSE);
> +		return;
> +	}
> +
> +	g_at_chat_register(data->modem_port, "*EMRDY:", emrdy_notifier,
> +					FALSE, modem, NULL);
> +
> +	g_at_chat_send(data->modem_port, "AT*E2CFUN=1", none_prefix,
> +					NULL, NULL, NULL);
> +	g_at_chat_send(data->modem_port, "AT*EMRDY?", none_prefix,
> +				emrdy_query, modem, NULL);
> +}
> +
> +static gboolean init_check(gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct mbm_data *data = ofono_modem_get_data(modem);
> +
> +	data->poll_source = 0;
> +
> +	g_at_chat_send(data->modem_port, "AT&F E0 V1 X4 &C1 +CMEE=1", NULL,
> +					init_result, modem, NULL);
> +
> +	return FALSE;
> +}
> +
>  static GAtChat *create_port(const char *device)
>  {
>  	GAtSyntax *syntax;
> @@ -277,15 +326,7 @@ static int mbm_enable(struct ofono_modem *modem)
>  	if (getenv("OFONO_AT_DEBUG"))
>  		g_at_chat_set_debug(data->data_port, mbm_debug, "Data:");
>  
> -	g_at_chat_register(data->modem_port, "*EMRDY:", emrdy_notifier,
> -					FALSE, modem, NULL);
> -
> -	g_at_chat_send(data->modem_port, "AT&F E0 V1 X4 &C1 +CMEE=1", NULL,
> -					NULL, NULL, NULL);
> -	g_at_chat_send(data->modem_port, "AT*E2CFUN=1", none_prefix,
> -					NULL, NULL, NULL);
> -	g_at_chat_send(data->modem_port, "AT*EMRDY?", none_prefix,
> -				emrdy_query, modem, NULL);
> +	init_check(modem);

don't you think we should leave the EMRDY notifier in the mbm_enable
callback. I don't think it is a good idea to register more than one of
these.

Can the AT&F line really fail? Or can we just assume that one will just
work fine and not try to send it again.

Regards

Marcel

Re: [PATCHv2 2/2] mbm: retry modem init

[Pekka Pessi]

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

Hi Marcel,
> don't you think we should leave the EMRDY notifier in the mbm_enable
> callback. I don't think it is a good idea to register more than one of
> these.

I think it is registered only once? Ooops...

> Can the AT&F line really fail? Or can we just assume that one will just
> work fine and not try to send it again.

Oh yes, it will. With connmand I get this kind of AT debug trace about
4 times of 5:

ofonod[3729]: Modem: > AT&F E0 V1 X4 &C1 +CMEE=1\r
ofonod[3729]: Modem: < AT&F E0 V1 X4 &C1 +CMEE=1\r\r\nNO CARRIER\r\n
ofonod[3729]: Modem: > AT*E2CFUN=1\r
ofonod[3729]: Modem: < AT*E2CFUN=1\r\r\nERROR\r\n
ofonod[3729]: Modem: > AT*EMRDY?\r
ofonod[3729]: Modem: < AT*EMRDY?\r\r\nERROR\r\n
ofonod[3729]: plugins/mbm.c:emrdy_query() 0
ofonod[3729]: Modem: > AT+CFUN?\r
ofonod[3729]: Modem: < AT+CFUN?\r
ofonod[3729]: Modem: < \r\n+CFUN: 4\r\n\r\nOK\r\n
ofonod[3729]: plugins/mbm.c:cfun_query() 1
ofonod[3729]: Modem: > AT+CFUN=1\r
ofonod[3729]: Modem: < AT+CFUN=1\r
ofonod[3729]: Modem: < \r\nOK\r\n
ofonod[3729]: plugins/mbm.c:cfun_enable()
ofonod[3729]: Modem: > AT+CPIN?\r
ofonod[3729]: Modem: < AT+CPIN?\r
ofonod[3729]: Modem: < \r\nERROR\r\n

-- 
Pekka.Pessi mail@nokia.com

Re: [PATCHv2 1/2] mbm: fix initial polling for sim

[Marcel Holtmann]

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

Hi Pekka,

> There seems to be no specific error codes returned when SIM is missing.
> Poll at least 5 times upon an error and give up after that.
> ---
>  plugins/mbm.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)

looks good to me. Patch has been applied. Thanks.

Regards

Marcel