[PATCH] atmodem: Add MBM/STE quirk for SIM record update

[PATCH] atmodem: Add MBM/STE quirk for SIM record update

[Miia Leinonen]

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

Fixed the <data> string to be set inside quotes for STE/MBM.

---

Hi,

This fix has been tested with STE modem only due the lack of MBM HW.

Would it be possible that someone checks this with MBM modem? The problem is 
likely to appear also there - therefore OFONO_VENDOR_MBM used. Could fix this 
also to cover all modems, but that might cause some backward compatibility 
issues.

BR,
Miia


 drivers/atmodem/sim.c |   59 ++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index d9c0d8d..7a0100a 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -332,17 +332,34 @@ static void at_sim_update_record(struct ofono_sim *sim, int fileid,
 {
 	struct sim_data *sd = ofono_sim_get_data(sim);
 	struct cb_data *cbd = cb_data_new(cb, data);
-	char *buf = g_try_new(char, 36 + length * 2);
+	char *buf;
 	int len, ret;
 
-	if (buf == NULL)
-		goto error;
+	if (sd->vendor == OFONO_VENDOR_MBM) {
+		buf = g_try_new(char, 36 + 2 + length * 2);
 
-	len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid,
-			record, length);
+		if (buf == NULL)
+			goto error;
 
-	for (; length; length--)
-		len += sprintf(buf + len, "%02hhX", *value++);
+		len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,\"", fileid,
+					record, length);
+
+		for (; length; length--)
+			len += sprintf(buf + len, "%02hhX", *value++);
+
+		sprintf(buf + len, "\"");
+	} else {
+		buf = g_try_new(char, 36 + length * 2);
+
+		if (buf == NULL)
+			goto error;
+
+		len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid,
+				record, length);
+
+		for (; length; length--)
+			len += sprintf(buf + len, "%02hhX", *value++);
+	}
 
 	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
 				at_crsm_update_cb, cbd, g_free);
@@ -364,16 +381,32 @@ static void at_sim_update_cyclic(struct ofono_sim *sim, int fileid,
 {
 	struct sim_data *sd = ofono_sim_get_data(sim);
 	struct cb_data *cbd = cb_data_new(cb, data);
-	char *buf = g_try_new(char, 36 + length * 2);
+	char *buf;
 	int len, ret;
 
-	if (buf == NULL)
-		goto error;
+	if (sd->vendor == OFONO_VENDOR_MBM) {
+		buf = g_try_new(char, 36 + 2 + length * 2);
 
-	len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,", fileid, length);
+		if (buf == NULL)
+			goto error;
 
-	for (; length; length--)
-		len += sprintf(buf + len, "%02hhX", *value++);
+		len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,\"", fileid, length);
+
+		for (; length; length--)
+			len += sprintf(buf + len, "%02hhX", *value++);
+
+		sprintf(buf + len, "\"");
+	} else {
+		buf = g_try_new(char, 36 + length * 2);
+
+		if (buf == NULL)
+			goto error;
+
+		len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,", fileid, length);
+
+		for (; length; length--)
+			len += sprintf(buf + len, "%02hhX", *value++);
+	}
 
 	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
 				at_crsm_update_cb, cbd, g_free);
-- 
1.7.1

Re: [PATCH] atmodem: Add MBM/STE quirk for SIM record update

[Miia Leinonen]

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


Hi,

On Wed, 2011-02-23 at 10:09 +0200, Miia Leinonen wrote:
> Fixed the <data> string to be set inside quotes for STE/MBM.
> 
> ---
> 
> Hi,
> 
> This fix has been tested with STE modem only due the lack of MBM HW.
> 
> Would it be possible that someone checks this with MBM modem? The problem is 
> likely to appear also there - therefore OFONO_VENDOR_MBM used. Could fix this 
> also to cover all modems, but that might cause some backward compatibility 
> issues.
> 
> BR,
> Miia

Is this fix OK or should I use some other approach? No one seem to have
commented anything so far.

BR,
Miia

Re: [PATCH] atmodem: Add MBM/STE quirk for SIM record update

[Denis Kenzior]

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

Hi Miia,

On 02/23/2011 02:09 AM, Miia Leinonen wrote:
> Fixed the <data> string to be set inside quotes for STE/MBM.
> 
> ---
> 
> Hi,
> 
> This fix has been tested with STE modem only due the lack of MBM HW.
> 
> Would it be possible that someone checks this with MBM modem? The problem is 
> likely to appear also there - therefore OFONO_VENDOR_MBM used. Could fix this 
> also to cover all modems, but that might cause some backward compatibility 
> issues.

This does seem to be required on MBM

> 
> BR,
> Miia
> 
> 
>  drivers/atmodem/sim.c |   59 ++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
> index d9c0d8d..7a0100a 100644
> --- a/drivers/atmodem/sim.c
> +++ b/drivers/atmodem/sim.c
> @@ -332,17 +332,34 @@ static void at_sim_update_record(struct ofono_sim *sim, int fileid,
>  {
>  	struct sim_data *sd = ofono_sim_get_data(sim);
>  	struct cb_data *cbd = cb_data_new(cb, data);
> -	char *buf = g_try_new(char, 36 + length * 2);
> +	char *buf;
>  	int len, ret;
>  
> -	if (buf == NULL)
> -		goto error;
> +	if (sd->vendor == OFONO_VENDOR_MBM) {
> +		buf = g_try_new(char, 36 + 2 + length * 2);
>  
> -	len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid,
> -			record, length);
> +		if (buf == NULL)
> +			goto error;
>  
> -	for (; length; length--)
> -		len += sprintf(buf + len, "%02hhX", *value++);
> +		len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,\"", fileid,
> +					record, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +
> +		sprintf(buf + len, "\"");
> +	} else {
> +		buf = g_try_new(char, 36 + length * 2);
> +
> +		if (buf == NULL)
> +			goto error;
> +
> +		len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid,
> +				record, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +	}
>  

Please don't duplicate code like this.  It is much better written
something like this:

int size = 36 + 2 + length * 2;

if (vendor == VENDOR_MBM)
	size += 2; /* Add quotes */

buf = g_try_new(char, size);

len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid, record, length);

if (vendor == VENDOR_MBM)
	len += sprint(buf + len, "\"");

for (; length; length--)
	...

if (vendor == VENDOR_MBM)
	...

etc.

>  	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
>  				at_crsm_update_cb, cbd, g_free);
> @@ -364,16 +381,32 @@ static void at_sim_update_cyclic(struct ofono_sim *sim, int fileid,
>  {
>  	struct sim_data *sd = ofono_sim_get_data(sim);
>  	struct cb_data *cbd = cb_data_new(cb, data);
> -	char *buf = g_try_new(char, 36 + length * 2);
> +	char *buf;
>  	int len, ret;
>  
> -	if (buf == NULL)
> -		goto error;
> +	if (sd->vendor == OFONO_VENDOR_MBM) {
> +		buf = g_try_new(char, 36 + 2 + length * 2);
>  
> -	len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,", fileid, length);
> +		if (buf == NULL)
> +			goto error;
>  
> -	for (; length; length--)
> -		len += sprintf(buf + len, "%02hhX", *value++);
> +		len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,\"", fileid, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +
> +		sprintf(buf + len, "\"");
> +	} else {
> +		buf = g_try_new(char, 36 + length * 2);
> +
> +		if (buf == NULL)
> +			goto error;
> +
> +		len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,", fileid, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +	}
>  
>  	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
>  				at_crsm_update_cb, cbd, g_free);

Regards,
-Denis

[PATCH v2] atmodem: Add MBM vendor quirk for SIM record update

[Miia Leinonen]

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

---

Hi Denis,

Modified the fix according to your comment to remove duplicate code.

BR,
Miia

 drivers/atmodem/sim.c |   28 ++++++++++++++++++++++++++--
 1 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index d9c0d8d..c26e44d 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -332,18 +332,30 @@ static void at_sim_update_record(struct ofono_sim *sim, int fileid,
 {
 	struct sim_data *sd = ofono_sim_get_data(sim);
 	struct cb_data *cbd = cb_data_new(cb, data);
-	char *buf = g_try_new(char, 36 + length * 2);
+	char *buf;
 	int len, ret;
 
+	int size = 36 + length * 2;
+
+	if (sd->vendor == OFONO_VENDOR_MBM)
+		size += 2; /*Add quotes*/
+
+	buf = g_try_new(char, size);
 	if (buf == NULL)
 		goto error;
 
 	len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid,
 			record, length);
 
+	if (sd->vendor == OFONO_VENDOR_MBM)
+		len += sprintf(buf + len, "\"");
+
 	for (; length; length--)
 		len += sprintf(buf + len, "%02hhX", *value++);
 
+	if (sd->vendor == OFONO_VENDOR_MBM)
+		sprintf(buf + len, "\"");
+
 	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
 				at_crsm_update_cb, cbd, g_free);
 
@@ -364,17 +376,29 @@ static void at_sim_update_cyclic(struct ofono_sim *sim, int fileid,
 {
 	struct sim_data *sd = ofono_sim_get_data(sim);
 	struct cb_data *cbd = cb_data_new(cb, data);
-	char *buf = g_try_new(char, 36 + length * 2);
+	char *buf;
 	int len, ret;
 
+	int size = 36 + length * 2;
+
+	if (sd->vendor == OFONO_VENDOR_MBM)
+		size += 2; /* Add quotes */
+
+	buf = g_try_new(char, size);
 	if (buf == NULL)
 		goto error;
 
 	len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,", fileid, length);
 
+	if (sd->vendor == OFONO_VENDOR_MBM)
+		len += sprintf(buf + len, "\"");
+
 	for (; length; length--)
 		len += sprintf(buf + len, "%02hhX", *value++);
 
+	if (sd->vendor == OFONO_VENDOR_MBM)
+		sprintf(buf + len, "\"");
+
 	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
 				at_crsm_update_cb, cbd, g_free);
 
-- 
1.7.1

Re: [PATCH v2] atmodem: Add MBM vendor quirk for SIM record update

[Denis Kenzior]

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

Hi Miia,

On 03/02/2011 06:56 AM, Miia Leinonen wrote:
> ---
> 
> Hi Denis,
> 
> Modified the fix according to your comment to remove duplicate code.
> 
> BR,
> Miia
> 
>  drivers/atmodem/sim.c |   28 ++++++++++++++++++++++++++--
>  1 files changed, 26 insertions(+), 2 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis