[PATCH] sim: Ensure to call sim_pin_check

[PATCH] sim: Ensure to call sim_pin_check

[Yang Gu]

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

In current code, sim_pin_check() is called inside sim_efpl_read_cb().
However, there may be a chance it would never be called, thus the modem
won't be initialized correctly.
---
 src/sim.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index a450b30..9bc9906 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1380,8 +1380,6 @@ skip_efpl:
 						"PreferredLanguages",
 						DBUS_TYPE_STRING,
 						&sim->language_prefs);
-
-	sim_pin_check(sim);
 }
 
 static void sim_iccid_read_cb(int ok, int length, int record,
@@ -1454,6 +1452,8 @@ static void sim_initialize(struct ofono_sim *sim)
 	ofono_sim_read(sim, SIM_EFPL_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efpl_read_cb, sim);
+
+	sim_pin_check(sim);
 }
 
 static void sim_op_error(struct ofono_sim *sim)
-- 
1.7.0.4

RE: [PATCH] sim: Ensure to call sim_pin_check

[Jeevaka.Badrappan]

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

Hi Yang Gu, 

> In current code, sim_pin_check() is called inside sim_efpl_read_cb().
> However, there may be a chance it would never be called, thus the
modem won't be initialized correctly.
> ---
>  src/sim.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

> diff --git a/src/sim.c b/src/sim.c
> index a450b30..9bc9906 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -1380,8 +1380,6 @@ skip_efpl:
>  						"PreferredLanguages",
>  						DBUS_TYPE_STRING,
>  						&sim->language_prefs);
> -
> -	sim_pin_check(sim);
>  }
 
>  static void sim_iccid_read_cb(int ok, int length, int record, @@
-1454,6 +1452,8 @@ static void sim_initialize(struct ofono_sim *sim)
>  	ofono_sim_read(sim, SIM_EFPL_FILEID,
>  			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
>  			sim_efpl_read_cb, sim);
> +
> +	sim_pin_check(sim);
>  }
 
>  static void sim_op_error(struct ofono_sim *sim)
> --
> 1.7.0.4

As per the 3GPP 31.102 section 5.1.1.2, CHV1 verification procedure will
be done after reading of EFli and EFpl. Earlier( before "sim: Reorder
SIM initialization" patch) src/sim.c had the sim_pin_check()called from
sim_efphase_read_cb() where it called after issuing EFli and EFpl file
read request. The patch "sim: Reorder SIM initialization" changed it to
call from sim_efpl_read_cb() which is as per the specification.If the
reading of EFli and EFpl fails, then sim_pin_check is not getting called
because we are returning from "if (sim->language_prefs == NULL)". In
your patch, you are moving it to be called after the EFpl read request
which is same as what we had before "sim: Reorder SIM initialization"
patch. I believe the right place to call the sim_pin_check() will be
before the if condition(if (sim->language_prefs == NULL)) in
sim_efpl_read_cb(), like shown below

      if (efpl) {
		g_slist_foreach(efpl, (GFunc)g_free, NULL);
	      g_slist_free(efpl);
 	}
	
	sim_pin_check(sim);

      if (sim->language_prefs == NULL)
            return;

      ofono_dbus_signal_array_property_changed(conn, path,
 
OFONO_SIM_MANAGER_INTERFACE,
                                               "PreferredLanguages",
                                               DBUS_TYPE_STRING,
                                               &sim->language_prefs);

Let me know your views on this.

Thanks and Regards,
jeevaka
_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono


----------------------------------------------------------------
Please note: This e-mail may contain confidential information
intended solely for the addressee. If you have received this
e-mail in error, please do not disclose it to anyone, notify
the sender promptly, and delete the message from your system.
Thank you.



----------------------------------------------------------------
Please note: This e-mail may contain confidential information
intended solely for the addressee. If you have received this
e-mail in error, please do not disclose it to anyone, notify
the sender promptly, and delete the message from your system.
Thank you.

Re: [PATCH] sim: Ensure to call sim_pin_check

[Pekka Pessi]

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

2010/8/25  <Jeevaka.Badrappan@elektrobit.com>:
> I believe the right place to call the sim_pin_check() will be
> before the if condition(if (sim->language_prefs == NULL)) in
> sim_efpl_read_cb(), like shown below
>
>      if (efpl) {
>                g_slist_foreach(efpl, (GFunc)g_free, NULL);
>              g_slist_free(efpl);
>        }
>
>        sim_pin_check(sim);
>
>      if (sim->language_prefs == NULL)
>            return;
>
>      ofono_dbus_signal_array_property_changed(conn, path,
>
> OFONO_SIM_MANAGER_INTERFACE,
>                                               "PreferredLanguages",
>                                               DBUS_TYPE_STRING,
>                                               &sim->language_prefs);
>
> Let me know your views on this.

I think you are right. I've submitted a patch doing this in slightly
different manner...

-- 
Pekka.Pessi mail at nokia.com

RE: [PATCH] sim: Ensure to call sim_pin_check

[Gu, Yang]

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

Hi jeevaka,


>>  static void sim_iccid_read_cb(int ok, int length, int record, @@
>-1454,6 +1452,8 @@ static void sim_initialize(struct ofono_sim *sim)
>>  	ofono_sim_read(sim, SIM_EFPL_FILEID,
>>  			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
>>  			sim_efpl_read_cb, sim);
>> +
>> +	sim_pin_check(sim);
>>  }
>
>>  static void sim_op_error(struct ofono_sim *sim)
>> --
>> 1.7.0.4
>
>As per the 3GPP 31.102 section 5.1.1.2, CHV1 verification procedure will
>be done after reading of EFli and EFpl. Earlier( before "sim: Reorder
>SIM initialization" patch) src/sim.c had the sim_pin_check()called from
>sim_efphase_read_cb() where it called after issuing EFli and EFpl file
>read request. The patch "sim: Reorder SIM initialization" changed it to
>call from sim_efpl_read_cb() which is as per the specification.If the
>reading of EFli and EFpl fails, then sim_pin_check is not getting called
>because we are returning from "if (sim->language_prefs == NULL)". In
>your patch, you are moving it to be called after the EFpl read request
>which is same as what we had before "sim: Reorder SIM initialization"
>patch. I believe the right place to call the sim_pin_check() will be
>before the if condition(if (sim->language_prefs == NULL)) in
>sim_efpl_read_cb(), like shown below
>
>      if (efpl) {
>		g_slist_foreach(efpl, (GFunc)g_free, NULL);
>	      g_slist_free(efpl);
> 	}
>
>	sim_pin_check(sim);
>
>      if (sim->language_prefs == NULL)
>            return;
>
>      ofono_dbus_signal_array_property_changed(conn, path,
>
>OFONO_SIM_MANAGER_INTERFACE,
>                                               "PreferredLanguages",
>                                               DBUS_TYPE_STRING,
>                                               &sim->language_prefs);
>
>Let me know your views on this.

Your solution is better than mine, and might be Denis' intention of modification. Honestly, I ever thought about the change like your proposal (Actually it was the same as ppessi's patch), but I had a concern on current code. Now we read EFli and EFpl in parallel. Is it guaranteed the callback of EFli will be called before the callback of EFpl? IMO, it can't always guarantee this. If I'm correct, your solution is just half way to the final target. Maybe we need to work out a better one:)

Regards,
-Yang

Re: [PATCH] sim: Ensure to call sim_pin_check

[Denis Kenzior]

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

Hi Yang,

> Your solution is better than mine, and might be Denis' intention of modification. Honestly, I ever thought about the change like your proposal (Actually it was the same as ppessi's patch), but I had a concern on current code. Now we read EFli and EFpl in parallel. Is it guaranteed the callback of EFli will be called before the callback of EFpl? IMO, it can't always guarantee this. If I'm correct, your solution is just half way to the final target. Maybe we need to work out a better one:)

Please note that the ofono_sim_read functions do not work in parallel.
All file operations are added to a queue and executed sequentially.
Hence we can rely on a certain order here.

Regards,
-Denis

RE: [PATCH] sim: Ensure to call sim_pin_check

[Gu, Yang]

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

Hi Denis,


>-----Original Message-----
>From: Denis Kenzior [mailto:denkenz(a)gmail.com]
>Sent: Wednesday, August 25, 2010 11:07 PM
>To: ofono(a)ofono.org
>Cc: Gu, Yang
>Subject: Re: [PATCH] sim: Ensure to call sim_pin_check
>
>Hi Yang,
>
>> Your solution is better than mine, and might be Denis' intention of modification.
>Honestly, I ever thought about the change like your proposal (Actually it was the same
>as ppessi's patch), but I had a concern on current code. Now we read EFli and EFpl in
>parallel. Is it guaranteed the callback of EFli will be called before the callback of EFpl?
>IMO, it can't always guarantee this. If I'm correct, your solution is just half way to the
>final target. Maybe we need to work out a better one:)
>
>Please note that the ofono_sim_read functions do not work in parallel.
>All file operations are added to a queue and executed sequentially.
>Hence we can rely on a certain order here.

I see. So Jeevaka and Ppessi's patch is definitely better than mine.


Regards,
-Yang