[PATCH 1/3] mbm: fix initial polling for sim

[PATCH 1/3] 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 6f71553..bebf5aa 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

[PATCH 2/3] mbm: retry modem init

[Pekka.Pessi]

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

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

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

diff --git a/plugins/mbm.c b/plugins/mbm.c
index bebf5aa..a06c444 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,53 @@ 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*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,13 +324,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*EMRDY?", none_prefix,
-				emrdy_query, modem, NULL);
+	init_check(modem);
 
 	return -EINPROGRESS;
 }
-- 
1.7.0.4

[PATCH 3/3] mbm: register gprs as STE to utilize STE quirks

[Pekka.Pessi]

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

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

No AT+CGEREP=2,1 on mbm either.
---
 plugins/mbm.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/plugins/mbm.c b/plugins/mbm.c
index a06c444..e77e61c 100644
--- a/plugins/mbm.c
+++ b/plugins/mbm.c
@@ -395,7 +395,8 @@ static void mbm_post_sim(struct ofono_modem *modem)
 	ofono_cbs_create(modem, 0, "atmodem", data->modem_port);
 	ofono_ussd_create(modem, 0, "atmodem", data->modem_port);
 
-	gprs = ofono_gprs_create(modem, 0, "atmodem", data->modem_port);
+	gprs = ofono_gprs_create(modem, OFONO_VENDOR_STE, "atmodem",
+					data->modem_port);
 	gc = ofono_gprs_context_create(modem, 0, "mbm", data->modem_port);
 
 	if (gprs && gc)
-- 
1.7.0.4

Re: [PATCH 1/3] mbm: fix initial polling for sim

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1023 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(-)
> 
> diff --git a/plugins/mbm.c b/plugins/mbm.c
> index 6f71553..bebf5aa 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);

maybe the STE modem is a bit more specific here than the MBM one. Which
MBM modem did you test this with?

Regards

Marcel

Re: [PATCH 2/3] mbm: retry modem init

[Marcel Holtmann]

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

Hi Pekka,

> If the first initialization command results are catastrophic. Retry it 10
> times, refuse to enable modem if it fails.
> ---
>  plugins/mbm.c |   71 +++++++++++++++++++++++++++++++++++++++++++++------------
>  1 files changed, 56 insertions(+), 15 deletions(-)
> 
> diff --git a/plugins/mbm.c b/plugins/mbm.c
> index bebf5aa..a06c444 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,53 @@ 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*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,13 +324,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*EMRDY?", none_prefix,
> -				emrdy_query, modem, NULL);
> +	init_check(modem);

you do know that the F35xx series and before only support *EMRDY
notifications and not the query command. Also the notification only
comes ones after reset of the modem on these old cards.

Regards

Marcel

Re: [PATCH 3/3] mbm: register gprs as STE to utilize STE quirks

[Marcel Holtmann]

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

Hi Pekka,

> No AT+CGEREP=2,1 on mbm either.
> ---
>  plugins/mbm.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/plugins/mbm.c b/plugins/mbm.c
> index a06c444..e77e61c 100644
> --- a/plugins/mbm.c
> +++ b/plugins/mbm.c
> @@ -395,7 +395,8 @@ static void mbm_post_sim(struct ofono_modem *modem)
>  	ofono_cbs_create(modem, 0, "atmodem", data->modem_port);
>  	ofono_ussd_create(modem, 0, "atmodem", data->modem_port);
>  
> -	gprs = ofono_gprs_create(modem, 0, "atmodem", data->modem_port);
> +	gprs = ofono_gprs_create(modem, OFONO_VENDOR_STE, "atmodem",
> +					data->modem_port);
>  	gc = ofono_gprs_context_create(modem, 0, "mbm", data->modem_port);

looks good, but I actually prefer if we turn this around. Make the quirk
in drivers/atmodem/gprs.c an OFONO_VENDOR_MBM quirk with a proper
comment that this applies to STE and MBM modems. And then have the MBM
and STE plugin use that quirk.

Regards

Marcel

Re: [PATCH 1/3] mbm: fix initial polling for sim

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Marcel Holtmann <marcel@holtmann.org> wrote:

> maybe the STE modem is a bit more specific here than the MBM one. Which
> MBM modem did you test this with?

I have been using MBM as template for my sim ready changes, so I don't think
there is any point at looking at STE driver at this point in time.

Regards
Sjur

Re: [PATCH 1/3] mbm: fix initial polling for sim

[Pekka Pessi]

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

Hi Marcel and Sjur,

> > 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 6f71553..bebf5aa 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);
>
> maybe the STE modem is a bit more specific here than the MBM one. Which
> MBM modem did you test this with?

I've been testing with F3507g, branded as Dell 5530.

The existing mbm sim detection logic is based on my earlier tests
without connman. It worked when I manually called test/enable-modem.

When using ofono with connman, the modem gets powered immediately after
it gets configured by kernel. Sometimes mbm seems to be still in the middle of
its initialization when ofono tries to enable it.

--Pekka

-- 
Pekka.Pessi mail at nokia.com

Re: [PATCH 3/3] mbm: register gprs as STE to utilize STE quirks

[Marcel Holtmann]

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

Hi Pekka,

> > No AT+CGEREP=2,1 on mbm either.
> > ---
> >  plugins/mbm.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/plugins/mbm.c b/plugins/mbm.c
> > index a06c444..e77e61c 100644
> > --- a/plugins/mbm.c
> > +++ b/plugins/mbm.c
> > @@ -395,7 +395,8 @@ static void mbm_post_sim(struct ofono_modem *modem)
> >  	ofono_cbs_create(modem, 0, "atmodem", data->modem_port);
> >  	ofono_ussd_create(modem, 0, "atmodem", data->modem_port);
> >  
> > -	gprs = ofono_gprs_create(modem, 0, "atmodem", data->modem_port);
> > +	gprs = ofono_gprs_create(modem, OFONO_VENDOR_STE, "atmodem",
> > +					data->modem_port);
> >  	gc = ofono_gprs_context_create(modem, 0, "mbm", data->modem_port);
> 
> looks good, but I actually prefer if we turn this around. Make the quirk
> in drivers/atmodem/gprs.c an OFONO_VENDOR_MBM quirk with a proper
> comment that this applies to STE and MBM modems. And then have the MBM
> and STE plugin use that quirk.

I was doing some cleanup and fixed this by myself now while I was at it.

Regards

Marcel