Re: [sim-ready-nofify-v5 PATCH 2/3] atmodem/sim: use ofono_sim_ready_notify

2011-02-15 Thread Pekka Pessi 
Hi Denis,

2011/2/10 Denis Kenzior denk...@gmail.com:
 So all this logic seems to work by luck.

AFAIK that is the way you wanted it. Continue initialization after
1) pin query returns None, and
2) notify is called after that.

If notify is called when pin is not None, recheck pin.

  oFono roughly does this:
 pin_query:
 send AT+CPIN?
 if READY then set waiting_for_pin to none, and wait in
 ofono_sim_ready_notify
 if PIN then set waiting_for_pin to the pin, and wait in
 ofono_sim_ready_notify

 ofono_sim_ready_notify:
 if waiting_for_pin is none proceed with initialization
 otherwise go back to pin_query

Sounds about correct.

 Here's a log of this running on IFX:
 ofonod[521]: Aux:  AT+CPIN?\r
 ofonod[521]: Aux:  \r\n+CPIN: SIM PIN\r\n
 ofonod[521]: Aux:  \r\nOK\r\n
 ofonod[521]: drivers/atmodem/sim.c:at_cpin_cb() crsm_pin_cb: SIM PIN
 
 ofonod[521]: Aux:  AT+CPIN=\r
 ofonod[521]: Aux:  \r\nOK\r\n
 ofonod[521]: Aux:  AT+CPIN?\r
 ofonod[521]: Aux:  \r\n+CME ERROR: 14\r\n
 ofonod[521]: Querying PIN authentication state failed
 
 ofonod[521]: Aux:  \r\n+XSIM: 7\r\n
 ofonod[521]: drivers/atmodem/sim.c:at_xsim_notify()
 ofonod[521]: drivers/atmodem/sim.c:ready_unregister_and_notify()
 ofonod[521]: src/sim.c:ofono_sim_ready_notify()
 ofonod[521]: plugins/ifx.c:xsim_notify() state 7
 ofonod[521]: Aux:  AT+CPIN?\r
 ofonod[521]: Aux:  \r\n+CPIN: READY\r\n
 ofonod[521]: Aux:  \r\nOK\r\n
 ofonod[521]: drivers/atmodem/sim.c:at_cpin_cb() crsm_pin_cb: READY
 ofonod[521]: drivers/atmodem/sim.c:at_pin_retries_query()
 ofonod[521]: drivers/atmodem/sim.c:ready_timeout()
 ofonod[521]: src/sim.c:ofono_sim_ready_notify()

 As you can see, we are in fact calling ofono_sim_ready_notify twice in
 short succession.  The only reason we're not stuck for 5 seconds the
 second time around is that the xsim notification function sets ready to
 TRUE.

That is the way it was designed.

Second time around driver knows SIM is ready, so it schedules a call
to ofono_sim_ready_notify() in at_wait_for_ready().

We could avoid extra AT+CPIN? if needed, e.g., at_pin_query() could
return error immediately if ready_source is set.

This is all pretty convoluted.  I don't really like this at all.

It is. How would you like to clarify the code? Better naming for
functions, comments?

 Another case to consider is a cold boot quirked modem, PIN disabled.  We
 are highly likely to receive XSIM way before we even query the PIN.  So
 ready is false, ready_id is  0 and we end up waiting for 5 seconds here
 for no good reason.

The +XSIM etc. is subscribed when SIM atom is probed, so oFono ends up
waiting only if SIM probe is done after ready indication. For MBM,
there is no good way to find out the SIM readyness (apart from reading
something from the SIM). For IFX, I think we could check the state
with an AT+XSIMSTATE? request.

 Did you intend if (ok  sd_ready_id == 0) here?

 Otherwise I don't see how non-quirked modems ever signal sim_ready or
 why you would want to start a timer if you know a notification is coming
 anyway.

The at_wait_for_ready() will schedule immediately a callback (timer
with timeout of 0) if modem has not registered a ready indication
(ready_id is 0).

-- 
Pekka.Pessi mail at nokia.com
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono

Re: [sim-ready-nofify-v5 PATCH 2/3] atmodem/sim: use ofono_sim_ready_notify

2011-02-09 Thread Denis Kenzior 
Hi Pekka,

On 02/08/2011 06:32 AM, pekka.pe...@nokia.com wrote:
 From: Pekka Pessi pekka.pe...@nokia.com
 
 Schedule a call to ofono_sim_ready_notify after pin code query returns
 SIM READY.
 
 Vendor quirks:
 - IFX: register unsolicated +XSIM result code
 - MBM: register unsolicated *EPEV result code
 ---
  drivers/atmodem/sim.c |  166 ++--
  1 files changed, 117 insertions(+), 49 deletions(-)
 
 diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
 index d9c0d8d..666edbe 100644
 --- a/drivers/atmodem/sim.c
 +++ b/drivers/atmodem/sim.c
 @@ -46,10 +46,14 @@
  
  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
  
 +#define READY_TIMEOUT5000
 +
  struct sim_data {
   GAtChat *chat;
   unsigned int vendor;
   guint ready_id;
 + guint ready_source;
 + ofono_bool_t ready;
  };
  
  static const char *crsm_prefix[] = { +CRSM:, NULL };
 @@ -679,10 +683,55 @@ static void at_pin_retries_query(struct ofono_sim *sim,
   CALLBACK_WITH_FAILURE(cb, NULL, data);
  }
  
 +static void ready_unregister_and_notify(struct ofono_sim *sim)
 +{
 + struct sim_data *sd = ofono_sim_get_data(sim);
 +
 + DBG();
 +
 + if (sd-ready_source  0) {
 + g_source_remove(sd-ready_source);
 + sd-ready_source = 0;
 + }
 +
 + ofono_sim_ready_notify(sim);
 +}
 +
 +static gboolean ready_timeout(gpointer user_data)
 +{
 + struct ofono_sim *sim = user_data;
 + struct sim_data *sd = ofono_sim_get_data(sim);
 +
 + DBG();
 +
 + sd-ready_source = 0;
 +
 + ofono_sim_ready_notify(sim);
 +
 + return FALSE;
 +}
 +
 +static void at_wait_for_ready(struct ofono_sim *sim)
 +{
 + struct sim_data *sd = ofono_sim_get_data(sim);
 + guint timeout;
 +
 + if (sd-ready_source  0)
 + return;
 +
 + if (!sd-ready  sd-ready_id  0)
 + timeout = READY_TIMEOUT;
 + else
 + timeout = 0;
 +
 + sd-ready_source = g_timeout_add(timeout, ready_timeout, sim);
 +}
 +
  static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
  {
   struct cb_data *cbd = user_data;
 - struct sim_data *sd = ofono_sim_get_data(cbd-user);
 + struct ofono_sim *sim = cbd-user;
 + struct sim_data *sd = ofono_sim_get_data(sim);
   GAtResultIter iter;
   ofono_sim_passwd_cb_t cb = cbd-cb;
   struct ofono_error error;
 @@ -729,6 +778,11 @@ static void at_cpin_cb(gboolean ok, GAtResult *result, 
 gpointer user_data)
   return;
   }
  
 + if (pin_type == OFONO_SIM_PASSWORD_NONE)
 + at_wait_for_ready(sim);
 + else
 + sd-ready = FALSE;
 +

So all this logic seems to work by luck.  oFono roughly does this:
pin_query:
send AT+CPIN?
if READY then set waiting_for_pin to none, and wait in
ofono_sim_ready_notify
if PIN then set waiting_for_pin to the pin, and wait in
ofono_sim_ready_notify

ofono_sim_ready_notify:
if waiting_for_pin is none proceed with initialization
otherwise go back to pin_query

Here's a log of this running on IFX:
ofonod[521]: Aux:  AT+CPIN?\r
ofonod[521]: Aux:  \r\n+CPIN: SIM PIN\r\n
ofonod[521]: Aux:  \r\nOK\r\n
ofonod[521]: drivers/atmodem/sim.c:at_cpin_cb() crsm_pin_cb: SIM PIN

ofonod[521]: Aux:  AT+CPIN=\r
ofonod[521]: Aux:  \r\nOK\r\n
ofonod[521]: Aux:  AT+CPIN?\r
ofonod[521]: Aux:  \r\n+CME ERROR: 14\r\n
ofonod[521]: Querying PIN authentication state failed

ofonod[521]: Aux:  \r\n+XSIM: 7\r\n
ofonod[521]: drivers/atmodem/sim.c:at_xsim_notify()
ofonod[521]: drivers/atmodem/sim.c:ready_unregister_and_notify()
ofonod[521]: src/sim.c:ofono_sim_ready_notify()
ofonod[521]: plugins/ifx.c:xsim_notify() state 7
ofonod[521]: Aux:  AT+CPIN?\r
ofonod[521]: Aux:  \r\n+CPIN: READY\r\n
ofonod[521]: Aux:  \r\nOK\r\n
ofonod[521]: drivers/atmodem/sim.c:at_cpin_cb() crsm_pin_cb: READY
ofonod[521]: drivers/atmodem/sim.c:at_pin_retries_query()
ofonod[521]: drivers/atmodem/sim.c:ready_timeout()
ofonod[521]: src/sim.c:ofono_sim_ready_notify()

As you can see, we are in fact calling ofono_sim_ready_notify twice in
short succession.  The only reason we're not stuck for 5 seconds the
second time around is that the xsim notification function sets ready to
TRUE.  This is all pretty convoluted.  I don't really like this at all.

Another case to consider is a cold boot quirked modem, PIN disabled.  We
are highly likely to receive XSIM way before we even query the PIN.  So
ready is false, ready_id is  0 and we end up waiting for 5 seconds here
for no good reason.

   DBG(crsm_pin_cb: %s, pin_required);
  
   cb(error, pin_type, cbd-data);
 @@ -753,13 +807,13 @@ static void at_pin_query(struct ofono_sim *sim, 
 ofono_sim_passwd_cb_t cb,
  
  static void at_xsim_notify(GAtResult *result, gpointer user_data)
  {
 - struct cb_data *cbd = user_data;
 - struct sim_data *sd = cbd-user;
 - ofono_sim_lock_unlock_cb_t cb = cbd-cb;
 - struct ofono_error error = { .type =

[sim-ready-nofify-v5 PATCH 2/3] atmodem/sim: use ofono_sim_ready_notify

2011-02-08 Thread Pekka . Pessi 
From: Pekka Pessi pekka.pe...@nokia.com

Schedule a call to ofono_sim_ready_notify after pin code query returns
SIM READY.

Vendor quirks:
- IFX: register unsolicated +XSIM result code
- MBM: register unsolicated *EPEV result code
---
 drivers/atmodem/sim.c |  166 ++--
 1 files changed, 117 insertions(+), 49 deletions(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index d9c0d8d..666edbe 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -46,10 +46,14 @@
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
+#define READY_TIMEOUT  5000
+
 struct sim_data {
GAtChat *chat;
unsigned int vendor;
guint ready_id;
+   guint ready_source;
+   ofono_bool_t ready;
 };
 
 static const char *crsm_prefix[] = { +CRSM:, NULL };
@@ -679,10 +683,55 @@ static void at_pin_retries_query(struct ofono_sim *sim,
CALLBACK_WITH_FAILURE(cb, NULL, data);
 }
 
+static void ready_unregister_and_notify(struct ofono_sim *sim)
+{
+   struct sim_data *sd = ofono_sim_get_data(sim);
+
+   DBG();
+
+   if (sd-ready_source  0) {
+   g_source_remove(sd-ready_source);
+   sd-ready_source = 0;
+   }
+
+   ofono_sim_ready_notify(sim);
+}
+
+static gboolean ready_timeout(gpointer user_data)
+{
+   struct ofono_sim *sim = user_data;
+   struct sim_data *sd = ofono_sim_get_data(sim);
+
+   DBG();
+
+   sd-ready_source = 0;
+
+   ofono_sim_ready_notify(sim);
+
+   return FALSE;
+}
+
+static void at_wait_for_ready(struct ofono_sim *sim)
+{
+   struct sim_data *sd = ofono_sim_get_data(sim);
+   guint timeout;
+
+   if (sd-ready_source  0)
+   return;
+
+   if (!sd-ready  sd-ready_id  0)
+   timeout = READY_TIMEOUT;
+   else
+   timeout = 0;
+
+   sd-ready_source = g_timeout_add(timeout, ready_timeout, sim);
+}
+
 static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
struct cb_data *cbd = user_data;
-   struct sim_data *sd = ofono_sim_get_data(cbd-user);
+   struct ofono_sim *sim = cbd-user;
+   struct sim_data *sd = ofono_sim_get_data(sim);
GAtResultIter iter;
ofono_sim_passwd_cb_t cb = cbd-cb;
struct ofono_error error;
@@ -729,6 +778,11 @@ static void at_cpin_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
return;
}
 
+   if (pin_type == OFONO_SIM_PASSWORD_NONE)
+   at_wait_for_ready(sim);
+   else
+   sd-ready = FALSE;
+
DBG(crsm_pin_cb: %s, pin_required);
 
cb(error, pin_type, cbd-data);
@@ -753,13 +807,13 @@ static void at_pin_query(struct ofono_sim *sim, 
ofono_sim_passwd_cb_t cb,
 
 static void at_xsim_notify(GAtResult *result, gpointer user_data)
 {
-   struct cb_data *cbd = user_data;
-   struct sim_data *sd = cbd-user;
-   ofono_sim_lock_unlock_cb_t cb = cbd-cb;
-   struct ofono_error error = { .type = OFONO_ERROR_TYPE_NO_ERROR };
+   struct ofono_sim *sim = user_data;
+   struct sim_data *sd = ofono_sim_get_data(sim);
GAtResultIter iter;
int state;
 
+   DBG();
+
g_at_result_iter_init(iter, result);
 
if (!g_at_result_iter_next(iter, +XSIM:))
@@ -776,65 +830,40 @@ static void at_xsim_notify(GAtResult *result, gpointer 
user_data)
return;
}
 
-   cb(error, cbd-data);
+   sd-ready = TRUE;
 
-   g_at_chat_unregister(sd-chat, sd-ready_id);
-   sd-ready_id = 0;
+   if (sd-ready_source  0)
+   ready_unregister_and_notify(sim);
 }
 
 static void at_epev_notify(GAtResult *result, gpointer user_data)
 {
-   struct cb_data *cbd = user_data;
-   struct sim_data *sd = cbd-user;
-   ofono_sim_lock_unlock_cb_t cb = cbd-cb;
-   struct ofono_error error = { .type = OFONO_ERROR_TYPE_NO_ERROR };
+   struct ofono_sim *sim = user_data;
+   struct sim_data *sd = ofono_sim_get_data(sim);
 
-   cb(error, cbd-data);
+   DBG();
 
-   g_at_chat_unregister(sd-chat, sd-ready_id);
-   sd-ready_id = 0;
+   sd-ready = TRUE;
+
+   if (sd-ready_source  0)
+   ready_unregister_and_notify(sim);
 }
 
 static void at_pin_send_cb(gboolean ok, GAtResult *result,
gpointer user_data)
 {
struct cb_data *cbd = user_data;
-   struct sim_data *sd = cbd-user;
+   struct ofono_sim *sim = cbd-user;
+   struct sim_data *sd = ofono_sim_get_data(sim);
ofono_sim_lock_unlock_cb_t cb = cbd-cb;
struct ofono_error error;
 
-   decode_at_error(error, g_at_result_final_response(result));
+   if (ok  sd-ready_id)
+   at_wait_for_ready(sim);
 
-   if (!ok)
-   goto done;
-
-   switch (sd-vendor) {
-   case OFONO_VENDOR_IFX:
-   /*
-* On the IFX modem, AT+CPIN? can return READY too
-