[PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status

[PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status

[Tonny Tzeng]

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

This patch is to skip unsolicited CGREG while checking the GPRS attach
status, especially for Huawei modem which sends lac and ci strings in
unquoted format, and casues at_util_parse_reg() return lac value as
status.

---
 drivers/atmodem/atutil.c |   12 ++++++++----
 gatchat/gatresult.c      |    5 +++++
 gatchat/gatresult.h      |    1 +
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
index b6f0d92..c190aa2 100644
--- a/drivers/atmodem/atutil.c
+++ b/drivers/atmodem/atutil.c
@@ -243,13 +243,11 @@ gboolean at_util_parse_reg(GAtResult *result,
const char *prefix,
 		if (g_at_result_iter_next_number(&iter, &s) == FALSE)
 			continue;
 
-		/* Some firmware will report bogus lac/ci when unregistered */
-		if (s != 1 && s != 5)
-			goto out;
-
 		switch (vendor) {
 		case OFONO_VENDOR_HUAWEI:
 		case OFONO_VENDOR_NOVATEL:
+			if (g_at_result_iter_eol(&iter))
+				continue;
 			r = g_at_result_iter_next_unquoted_string(&iter, &str);
 
 			if (r == TRUE)
@@ -257,6 +255,8 @@ gboolean at_util_parse_reg(GAtResult *result, const
char *prefix,
 			else
 				goto out;
 
+			if (g_at_result_iter_eol(&iter))
+				continue;
 			r = g_at_result_iter_next_unquoted_string(&iter, &str);
 
 			if (r == TRUE)
@@ -266,11 +266,15 @@ gboolean at_util_parse_reg(GAtResult *result,
const char *prefix,
 
 			break;
 		default:
+			if (g_at_result_iter_eol(&iter))
+				continue;
 			if (g_at_result_iter_next_string(&iter, &str) == TRUE)
 				l = strtol(str, NULL, 16);
 			else
 				goto out;
 
+			if (g_at_result_iter_eol(&iter))
+				continue;
 			if (g_at_result_iter_next_string(&iter, &str) == TRUE)
 				c = strtol(str, NULL, 16);
 			else
diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c
index 8a6dfae..c919305 100644
--- a/gatchat/gatresult.c
+++ b/gatchat/gatresult.c
@@ -39,6 +39,11 @@ void g_at_result_iter_init(GAtResultIter *iter,
GAtResult *result)
 	iter->line_pos = 0;
 }
 
+gboolean g_at_result_iter_eol(GAtResultIter *iter)
+{
+	return iter->line_pos >= strlen(iter->l->data);
+}
+
 gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix)
 {
 	char *line;
diff --git a/gatchat/gatresult.h b/gatchat/gatresult.h
index a74741f..33c0b2c 100644
--- a/gatchat/gatresult.h
+++ b/gatchat/gatresult.h
@@ -46,6 +46,7 @@ struct _GAtResultIter {
 typedef struct _GAtResultIter GAtResultIter;
 
 void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result);
+gboolean g_at_result_iter_eol(GAtResultIter *iter);
 
 gboolean g_at_result_iter_next(GAtResultIter *iter, const char
*prefix);
 gboolean g_at_result_iter_open_list(GAtResultIter *iter);
-- 
1.7.2.2

Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status

[Denis Kenzior]

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

On 12/03/2010 09:20 AM, Tonny Tzeng wrote:
> This patch is to skip unsolicited CGREG while checking the GPRS attach
> status, especially for Huawei modem which sends lac and ci strings in
> unquoted format, and casues at_util_parse_reg() return lac value as
> status.
> 
> ---
>  drivers/atmodem/atutil.c |   12 ++++++++----
>  gatchat/gatresult.c      |    5 +++++
>  gatchat/gatresult.h      |    1 +

We like to see patches split on a top level directory boundary.  Please
see the 'Submitting patches' section in the HACKING document.

>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
> index b6f0d92..c190aa2 100644
> --- a/drivers/atmodem/atutil.c
> +++ b/drivers/atmodem/atutil.c
> @@ -243,13 +243,11 @@ gboolean at_util_parse_reg(GAtResult *result,
> const char *prefix,
>  		if (g_at_result_iter_next_number(&iter, &s) == FALSE)
>  			continue;
>  
> -		/* Some firmware will report bogus lac/ci when unregistered */
> -		if (s != 1 && s != 5)
> -			goto out;
> -

Skipping this check is still a bad idea.

>  		switch (vendor) {
>  		case OFONO_VENDOR_HUAWEI:
>  		case OFONO_VENDOR_NOVATEL:
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Coding style item M1.

>  			r = g_at_result_iter_next_unquoted_string(&iter, &str);
>  
>  			if (r == TRUE)
> @@ -257,6 +255,8 @@ gboolean at_util_parse_reg(GAtResult *result, const
> char *prefix,
>  			else
>  				goto out;
>  
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Coding style item M1.

>  			r = g_at_result_iter_next_unquoted_string(&iter, &str);
>  
>  			if (r == TRUE)
> @@ -266,11 +266,15 @@ gboolean at_util_parse_reg(GAtResult *result,
> const char *prefix,
>  
>  			break;
>  		default:
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Coding style item M1.  And continuing is not the right thing to do here

>  			if (g_at_result_iter_next_string(&iter, &str) == TRUE)
>  				l = strtol(str, NULL, 16);
>  			else
>  				goto out;
>  
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Ditto

>  			if (g_at_result_iter_next_string(&iter, &str) == TRUE)
>  				c = strtol(str, NULL, 16);
>  			else
> diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c
> index 8a6dfae..c919305 100644
> --- a/gatchat/gatresult.c
> +++ b/gatchat/gatresult.c
> @@ -39,6 +39,11 @@ void g_at_result_iter_init(GAtResultIter *iter,
> GAtResult *result)
>  	iter->line_pos = 0;
>  }
>  
> +gboolean g_at_result_iter_eol(GAtResultIter *iter)
> +{
> +	return iter->line_pos >= strlen(iter->l->data);
> +}
> +

And I pretty much don't see the need for this.  It doesn't really help
you here.

>  gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix)
>  {
>  	char *line;
> diff --git a/gatchat/gatresult.h b/gatchat/gatresult.h
> index a74741f..33c0b2c 100644
> --- a/gatchat/gatresult.h
> +++ b/gatchat/gatresult.h
> @@ -46,6 +46,7 @@ struct _GAtResultIter {
>  typedef struct _GAtResultIter GAtResultIter;
>  
>  void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result);
> +gboolean g_at_result_iter_eol(GAtResultIter *iter);
>  
>  gboolean g_at_result_iter_next(GAtResultIter *iter, const char
> *prefix);
>  gboolean g_at_result_iter_open_list(GAtResultIter *iter);

Anyhow, can you please try the attached patch and see if it solves your
problems.

Regards,
-Denis

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-atutil-Fix-parsing-of-un-quoted-CREG-CGREG.patch --]
[-- Type: text/x-patch, Size: 1462 bytes --]

>From f5473c398a2e29a98c74ad4c98a427564e25d9d0 Mon Sep 17 00:00:00 2001
From: Denis Kenzior <denkenz@gmail.com>
Date: Mon, 6 Dec 2010 21:36:54 -0600
Subject: [PATCH] atutil: Fix parsing of un-quoted CREG / CGREG

On broken hardware like the Huawei, it is possible to receive both an
unsolicited and a solicited version of the CREG / CGREG within within
the same response set.  Skipping of the unsolicited version was not
handled correctly.  This attempts to fix this issue.
---
 drivers/atmodem/atutil.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
index b6f0d92..427b098 100644
--- a/drivers/atmodem/atutil.c
+++ b/drivers/atmodem/atutil.c
@@ -240,8 +240,23 @@ gboolean at_util_parse_reg(GAtResult *result, const char *prefix,
 		g_at_result_iter_next_number(&iter, &m);
 
 		/* Sometimes we get an unsolicited CREG/CGREG here, skip it */
-		if (g_at_result_iter_next_number(&iter, &s) == FALSE)
-			continue;
+		switch (vendor) {
+		case OFONO_VENDOR_HUAWEI:
+		case OFONO_VENDOR_NOVATEL:
+			r = g_at_result_iter_next_unquoted_string(&iter, &str);
+
+			if (r == FALSE || strlen(str) != 1)
+				continue;
+
+			s = strtol(str, NULL, 10);
+
+			break;
+		default:
+			if (g_at_result_iter_next_number(&iter, &s) == FALSE)
+				continue;
+
+			break;
+		}
 
 		/* Some firmware will report bogus lac/ci when unregistered */
 		if (s != 1 && s != 5)
-- 
1.7.0.4

Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status

[Tonny Tzeng]

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

>
> > diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c
> > index 8a6dfae..c919305 100644
> > --- a/gatchat/gatresult.c
> > +++ b/gatchat/gatresult.c
> > @@ -39,6 +39,11 @@ void g_at_result_iter_init(GAtResultIter *iter,
> > GAtResult *result)
> >       iter->line_pos = 0;
> >  }
> >
> > +gboolean g_at_result_iter_eol(GAtResultIter *iter)
> > +{
> > +     return iter->line_pos >= strlen(iter->l->data);
> > +}
> > +
>
> And I pretty much don't see the need for this.  It doesn't really help
> you here.


No, this function can help to ignore unsolicited response properly.
I think the difference between solicited and unsolicited CGREG is the number
of parameters after +CGREG:, so it would be nice if we have a utility
function to know we are at the end of AT return string.

This is what we want to parse the state info from the solicited response:
+CGREG: <n>, <state>, <lac>, <ci>
but in a race condition, we received this unsolicited response from Huawei
modem:
+CGREG: <state>, <lac>, <ci>

In your proposed patch, it bypass the unsolicited response only if the 2nd
number after +CGREG: is not single digit, however if the lac is single
digit, we will still get the wrong state number.

So I still believe we should check whether we are at the end of the return
string before parsing the unquoted lac and ci.  If we are at the end of the
string, then this is a unsolicited code and we should ignore it.  How do you
think?


> >  gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix)
> >  {
> >       char *line;
> > diff --git a/gatchat/gatresult.h b/gatchat/gatresult.h
> > index a74741f..33c0b2c 100644
> > --- a/gatchat/gatresult.h
> > +++ b/gatchat/gatresult.h
> > @@ -46,6 +46,7 @@ struct _GAtResultIter {
> >  typedef struct _GAtResultIter GAtResultIter;
> >
> >  void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result);
> > +gboolean g_at_result_iter_eol(GAtResultIter *iter);
> >
> >  gboolean g_at_result_iter_next(GAtResultIter *iter, const char
> > *prefix);
> >  gboolean g_at_result_iter_open_list(GAtResultIter *iter);
>
> Anyhow, can you please try the attached patch and see if it solves your
> problems.
>
> Regards,
> -Denis
>

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2906 bytes --]

Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status

[Denis Kenzior]

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

Hi Tonny,

> In your proposed patch, it bypass the unsolicited response only if the
> 2nd number after +CGREG: is not single digit, however if the lac is
> single digit, we will still get the wrong state number.

The lac is given as a 2 byte number and should always be printed as four
hexadecimal letters.  If this is not the case, then you should have a
serious talk with your vendor.

> 
> So I still believe we should check whether we are at the end of the
> return string before parsing the unquoted lac and ci.  If we are at the
> end of the string, then this is a unsolicited code and we should ignore
> it.  How do you think?
>  

I'm against this, if you really want to guard against this, then use the
approach similar to comprehension_tlv_iter_copy in src/simutil.[ch].
You can use that to 'peek' ahead and figure out whether the response
contains 3 or 4 elements.

But again, you would not be having this issue in the first place if your
vendor followed standards ;)

Regards,
-Denis

Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status

[Tonny Tzeng]

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

Hi Denis,

On Sat, Dec 11, 2010 at 10:52 PM, Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Tonny,
>
> > In your proposed patch, it bypass the unsolicited response only if the
> > 2nd number after +CGREG: is not single digit, however if the lac is
> > single digit, we will still get the wrong state number.
>
> The lac is given as a 2 byte number and should always be printed as four
> hexadecimal letters.  If this is not the case, then you should have a
> serious talk with your vendor.
>

Well, if this (i.e. lac are 4 hex-digits) is the case, then I totally agree
with you that it could ignore unsolicited response properly in
at_util_parse_reg() for my case.  Thank you so much for the coach :)

Best Regards,
Tonny

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1054 bytes --]