Skip to contentSkip to conversation

ofono_sim_ready_notify

PATCH In review Kristen Carlson Accardi 3 participants 7 messages Aug 25, 2010 – Aug 26, 2010

lore
Share & tools
lore
#0 lore raw

cover ofono_sim_ready_notify

When entering a PIN, wait for the driver to notify us that the sim is ready before querying the password again and proceeding with the rest of sim initialization

Kristen Carlson Accardi (2):
sim: add ofono_sim_ready_notify() support
atmodem: call sim_ready_notify when epev is received

 drivers/atmodem/sim.c |   12 ++++++++----
 include/sim.h         |    2 ++
 src/sim.c             |   25 +++++++++++++++++++++++--
 3 files changed, 33 insertions(+), 6 deletions(-)
Signature · 1 line
1.7.2.1
#1 lore raw

PATCH 1/2 sim: add ofono_sim_ready_notify() support

---
 include/sim.h |    2 ++
 src/sim.c     |   25 +++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)
include/sim.h +2 -0
index 36a99b9..9b7d52e 100644
@@ -187,6 +187,8 @@ enum ofono_sim_state ofono_sim_get_state(struct ofono_sim *sim);
 
 void ofono_sim_inserted_notify(struct ofono_sim *sim, ofono_bool_t inserted);
 
+void ofono_sim_ready_notify(struct ofono_sim *sim);
+
 /* This will queue an operation to read all available records with id from the
  * SIM.  Callback cb will be called every time a record has been read, or once
  * if an error has occurred.  For transparent files, the callback will only
src/sim.c +23 -2
index 04a708b..b76a263 100644
@@ -56,6 +56,9 @@ static void sim_own_numbers_update(struct ofono_sim *sim);
 static void sim_pin_check(struct ofono_sim *sim);
 static void sim_set_ready(struct ofono_sim *sim);
 
+#define SIM_STATUS_READY		1
+#define SIM_STATUS_WAITING_FOR_READY	(1 << 1)
+
 struct sim_file_op {
 	int id;
 	gboolean cache;
@@ -78,6 +81,7 @@ struct ofono_sim {
 	GSList *new_numbers;
 	GSList *service_numbers;
 	gboolean sdn_ready;
+	unsigned int status;
 	enum ofono_sim_state state;
 	enum ofono_sim_password_type pin_type;
 	gboolean locked_pins[OFONO_SIM_PASSWORD_SIM_PUK]; /* Number of PINs */
@@ -686,15 +690,20 @@ static void sim_enter_pin_cb(const struct ofono_error *error, void *data)
 {
 	struct ofono_sim *sim = data;
 	DBusMessage *reply;
+	gboolean ok = (error->type == OFONO_ERROR_TYPE_NO_ERROR);
 
-	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
+	if (!ok)
 		reply = __ofono_error_failed(sim->pending);
 	else
 		reply = dbus_message_new_method_return(sim->pending);
 
 	__ofono_dbus_pending_reply(&sim->pending, reply);
 
-	sim_pin_check(sim);
+	if (ok && (sim->status & SIM_STATUS_READY)) {
+		sim->status &= ~SIM_STATUS_WAITING_FOR_READY;
+		sim_pin_check(sim);
+	} else
+		sim->status |= SIM_STATUS_WAITING_FOR_READY;
 }
 
 static DBusMessage *sim_enter_pin(DBusConnection *conn, DBusMessage *msg,
@@ -2002,6 +2011,18 @@ static void sim_free_state(struct ofono_sim *sim)
 	}
 
 	sim->mnc_length = 0;
+
+	sim->status = 0;
+}
+
+void ofono_sim_ready_notify(struct ofono_sim *sim)
+{
+	sim->status |= SIM_STATUS_READY;
+
+	if (sim->status & SIM_STATUS_WAITING_FOR_READY) {
+		sim->status &= ~SIM_STATUS_WAITING_FOR_READY;
+		sim_pin_check(sim);
+	}
 }
 
 void ofono_sim_inserted_notify(struct ofono_sim *sim, ofono_bool_t inserted)
Signature · 1 line
1.7.2.1
#2 lore raw

Re: [PATCH 1/2] sim: add ofono_sim_ready_notify() support

Hi Kristen and all,

I've been toying PUKs and PIN2s and it seems to me that insted of ofono_sim_ready_notify() driver needs a method with which it can announce which PIN code SIM might need. If that is none, then we proceed bit like after ofono_sim_ready_notify() in Kristen's patch. If on the other hand user somehow gets his SIM PUKed, the driver can indicate that to core, and we can change the SIM state back to PRESENT.

More comments to Kristen below:

2010/8/25 Kristen Carlson Accardi <[email protected]>:

Quoted 28 lines
src/sim.c +3 -0
+#define SIM_STATUS_READY               1
+#define SIM_STATUS_WAITING_FOR_READY   (1 << 1)
+

 struct sim_file_op {
       int id;
       gboolean cache;

src/sim.c +1 -0
@@ -78,6 +81,7 @@ struct ofono_sim {
        GSList *new_numbers;
        GSList *service_numbers;
        gboolean sdn_ready;
+       unsigned int status;
        enum ofono_sim_state state;
        enum ofono_sim_password_type pin_type;

       gboolean locked_pins[OFONO_SIM_PASSWORD_SIM_PUK]; /* Number of PINs */

src/sim.c +2 -1
@@ -686,15 +690,20 @@ static void sim_enter_pin_cb(const struct ofono_error *error, void *data)
  {
        struct ofono_sim *sim = data;
        DBusMessage *reply;
+       gboolean ok = (error->type == OFONO_ERROR_TYPE_NO_ERROR);
 
-       if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
+       if (!ok)
                reply = __ofono_error_failed(sim->pending);

       else
               reply = dbus_message_new_method_return(sim->pending);

       __ofono_dbus_pending_reply(&sim->pending, reply);

Quoted 7 lines

-       sim_pin_check(sim);
+       if (ok && (sim->status & SIM_STATUS_READY)) {
+               sim->status &= ~SIM_STATUS_WAITING_FOR_READY;
+               sim_pin_check(sim);
+       } else
+               sim->status |= SIM_STATUS_WAITING_FOR_READY; }

This does not work if modem 1) does not implement pin check or pin query 2) does not get ready immediately. isimodem comes to mind.

I'd guess you should set the bit sim->status |= SIM_STATUS_WAITING_FOR_READY from sim_efpl_read_cb.

Perhaps renaming the sim_pin_query() as ofono_sim_ready_notify() and check for the WAITING_FOR_READY bit there? You could also set another bit in ofono_sim_ready_notify, just prevent it from running sim_initialize_after_pin() twice.

Signature · 1 line
Pekka.Pessi mail at nokia.com
#3 lore raw

Re: [PATCH 1/2] sim: add ofono_sim_ready_notify() support

Hi Pekka,

Quoted 9 lines · Pekka Pessi

Hi Kristen and all,

I've been toying PUKs and PIN2s and it seems to me that insted of ofono_sim_ready_notify() driver needs a method with which it can announce which PIN code SIM might need. If that is none, then we proceed bit like after ofono_sim_ready_notify() in Kristen's patch. If on the other hand user somehow gets his SIM PUKed, the driver can indicate that to core, and we can change the SIM state back to PRESENT.

Seems like these are two separate issues. We need sim_ready_notify for the driver to tell us it is safe to proceed with modem initialization. calypso, hso send us unsolicited notifications when sim, phonebook and sms bits are ready. Huawei (if we're lucky) tells us that the sim is ready. MBM does not do anything unless PIN was required, in which case it uses EPEE/EPEV.

We need a separate case for sim dead / sim puked.

Quoted 32 lines

More comments to Kristen below:

2010/8/25 Kristen Carlson Accardi <[email protected]>:

Quoted 28 lines
src/sim.c +6 -1
+#define SIM_STATUS_READY               1
+#define SIM_STATUS_WAITING_FOR_READY   (1 << 1)
+
 struct sim_file_op {
       int id;
       gboolean cache;
@@ -78,6 +81,7 @@ struct ofono_sim {
       GSList *new_numbers;
       GSList *service_numbers;
       gboolean sdn_ready;
+       unsigned int status;
       enum ofono_sim_state state;
       enum ofono_sim_password_type pin_type;
       gboolean locked_pins[OFONO_SIM_PASSWORD_SIM_PUK]; /* Number of PINs */
@@ -686,15 +690,20 @@ static void sim_enter_pin_cb(const struct ofono_error *error, void *data)
 {
       struct ofono_sim *sim = data;
       DBusMessage *reply;
+       gboolean ok = (error->type == OFONO_ERROR_TYPE_NO_ERROR);
 
-       if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
+       if (!ok)
               reply = __ofono_error_failed(sim->pending);
       else
               reply = dbus_message_new_method_return(sim->pending);
 
       __ofono_dbus_pending_reply(&sim->pending, reply);
 
Quoted 10 lines

src/sim.c static void sim_enter_pin_cb(const struct ofono_error *error, void *data) · line 707

Quoted 7 lines
src/sim.c +5 -1
-       sim_pin_check(sim);
+       if (ok && (sim->status & SIM_STATUS_READY)) {
+               sim->status &= ~SIM_STATUS_WAITING_FOR_READY;
+               sim_pin_check(sim);
+       } else
+               sim->status |= SIM_STATUS_WAITING_FOR_READY;
 }

This does not work if modem 1) does not implement pin check or pin query 2) does not get ready immediately. isimodem comes to mind.

Yes, see my comment above. The sim_ready needs to work with modems that inform us they're ready, even if there's no PIN enabled.

Quoted 9 lines

I'd guess you should set the bit sim->status |= SIM_STATUS_WAITING_FOR_READY from sim_efpl_read_cb.

Perhaps renaming the sim_pin_query() as ofono_sim_ready_notify() and check for the WAITING_FOR_READY bit there? You could also set another bit in ofono_sim_ready_notify, just prevent it from running sim_initialize_after_pin() twice.

Regards,
-Denis

#4 lore raw

Re: [PATCH 1/2] sim: add ofono_sim_ready_notify() support

In reply to Pekka Pessi

Hi Kristen and all,

This is what I have in mind:
- no sim_pin_check after return from sim_pin_enter() but driver calls sim_ready_notify()
- if driver has no pin check, retry initialization after sim_ready_notify()
- when SIM requires PUK, change state back to SIM_INSERTED

There is no sim_set_state() yet, it has to be added

PIN2 and PUK2 results from pin query should be ignored, they are transient and do not prevent SIM from being ready.

--Pekka

src/sim.c +9 -0
@@ -80,6 +80,15 @@ struct ofono_sim {
 	gboolean sdn_ready;
 	enum ofono_sim_state state;
 	enum ofono_sim_password_type pin_type;
+	union {
+		struct {
+			unsigned pre_pin_started:1;
+			unsigned pre_pin_done:1;
+			unsigned post_pin_started:1;
+			unsigned ready_notified:1;
+		};
+		unsigned all;
+	} init;
 	gboolean locked_pins[OFONO_SIM_PASSWORD_SIM_PUK]; /* Number of PINs */
 	char **language_prefs;
 	GQueue *simop_q;
@@ -693,8 +702,6 @@ static void sim_enter_pin_cb(const struct

ofono_error *error, void *data)
reply = dbus_message_new_method_return(sim->pending);

__ofono_dbus_pending_reply(&sim->pending, reply);
-
- sim_pin_check(sim);
}

static DBusMessage *sim_enter_pin(DBusConnection *conn, DBusMessage *msg,
@@ -980,6 +987,12 @@ static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,

if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
ofono_error("Unable to read IMSI, emergency calls only");
+
+ sim->init.post_pin_started = 0;
+
+ if (sim->init.ready_notified)
+ sim_pin_check(sim);
+
return;
}

@@ -1130,6 +1143,9 @@ static void sim_efphase_read_cb(int ok, int length, int record,

static void sim_initialize_after_pin(struct ofono_sim *sim) {
+ sim->init.post_pin_started = 1;
+ sim->init.ready_notified = 0;
+
ofono_sim_read(sim, SIM_EFPHASE_FILEID,
OFONO_SIM_FILE_STRUCTURE_TRANSPARENT, sim_efphase_read_cb, sim);
@@ -1167,6 +1183,10 @@ static void sim_pin_query_cb(const struct ofono_error *error,
goto checkdone;
}

+ if (pin_type == OFONO_SIM_PASSWORD_PIN2 ||
+ pin_type == OFONO_SIM_PASSWORD_PUK2)
+ pin_type = OFONO_SIM_PASSWORD_NONE;
+
if (sim->pin_type != pin_type) {
sim->pin_type = pin_type;
pin_name = sim_passwd_name(pin_type);
@@ -1185,7 +1205,18 @@ static void sim_pin_query_cb(const struct ofono_error *error,
}

checkdone:
- if (pin_type == OFONO_SIM_PASSWORD_NONE)
+ if (pin_type != OFONO_SIM_PASSWORD_NONE) {
+ int ready_notified = sim->init.ready_notified;
+
+ sim->init.post_pin_started = 0;
+ sim->init.ready_notified = 0;
+
+ sim_set_state(sim, OFONO_SIM_STATE_INSERTED);
+
+ return;
+ }
+
+ if (sim->init.pre_pin_done && !sim->init.post_pin_started)
sim_initialize_after_pin(sim);
}

@@ -1199,6 +1230,22 @@ static void sim_pin_check(struct ofono_sim *sim)
sim->driver->query_passwd_state(sim, sim_pin_query_cb, sim);
}

+void ofono_sim_ready_notify(struct ofono_sim *sim)
+{
+ if (sim == NULL)
+ return;
+
+ sim->init.ready_notified = 1;
+
+ if (sim->state == OFONO_SIM_STATE_NOT_PRESENT)
+ return;
+
+ if (!sim->init.pre_pin_done)
+ return;
+
+ sim_pin_check(sim);
+}
+
static void sim_efli_read_cb(int ok, int length, int record,
const unsigned char *data, int record_length, void *userdata)
@@ -1379,6 +1426,8 @@ skip_efpl:
DBUS_TYPE_STRING, &sim->language_prefs);

+ sim->init.pre_pin_done = 1;
+
sim_pin_check(sim);
}

@@ -1407,6 +1456,8 @@ static void sim_iccid_read_cb(int ok, int length, int record,

static void sim_initialize(struct ofono_sim *sim)
{
+ sim->init.pre_pin_started = 1;
+
/* Perform SIM initialization according to 3GPP 31.102 Section 5.1.1.2
* The assumption here is that if sim manager is being initialized,
* then sim commands are implemented, and the sim manager is then
static void sim_free_state(struct ofono_sim *sim)
@@ -2007,6 +2060,8 @@ static void sim_free_state(struct ofono_sim *sim)
g_free(sim->iccid);
sim->iccid = NULL;
}
+
+ sim->init.all = 0;
}

void ofono_sim_inserted_notify(struct ofono_sim *sim, ofono_bool_t inserted)

#5 lore raw

Re: [PATCH 1/2] sim: add ofono_sim_ready_notify() support

Hi Pekka,

Quoted 7 lines · Pekka Pessi

Hi Kristen and all,

This is what I have in mind:
- no sim_pin_check after return from sim_pin_enter() but driver calls sim_ready_notify()
- if driver has no pin check, retry initialization after sim_ready_notify()
- when SIM requires PUK, change state back to SIM_INSERTED

Isn't it already in state SIM_INSERTED? We go to SIM_INSERTED as soon as the driver calls sim_inserted_notify()

Quoted 27 lines

There is no sim_set_state() yet, it has to be added

PIN2 and PUK2 results from pin query should be ignored, they are transient and do not prevent SIM from being ready.

--Pekka

src/sim.c +9 -0
@@ -80,6 +80,15 @@ struct ofono_sim {
 	gboolean sdn_ready;
 	enum ofono_sim_state state;
 	enum ofono_sim_password_type pin_type;
+	union {
+		struct {
+			unsigned pre_pin_started:1;
+			unsigned pre_pin_done:1;
+			unsigned post_pin_started:1;
+			unsigned ready_notified:1;
+		};
+		unsigned all;
+	} init;
 	gboolean locked_pins[OFONO_SIM_PASSWORD_SIM_PUK]; /* Number of PINs */
 	char **language_prefs;
 	GQueue *simop_q;

src/sim.c static void sim_enter_pin_cb(const struct · line 698

Quoted 6 lines
src/sim.c +0 -2
@@ -693,8 +702,6 @@ static void sim_enter_pin_cb(const struct
 ofono_error *error, void *data)
 		reply = dbus_message_new_method_return(sim->pending);
 
 	__ofono_dbus_pending_reply(&sim->pending, reply);
-
-	sim_pin_check(sim);

You do want to check the PIN if this is not PIN/PIN2. E.g. some weird carrier/corp/etc lock.

Quoted 4 lines
src/sim.c +0 -0
 }
 

static DBusMessage *sim_enter_pin(DBusConnection *conn, DBusMessage *msg,

src/sim.c static void sim_imsi_cb(const struct ofono_error · line 996

Quoted 10 lines
src/sim.c +6 -0
@@ -980,6 +987,12 @@ static void sim_imsi_cb(const struct ofono_error
 *error, const char *imsi,
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		ofono_error("Unable to read IMSI, emergency calls only");
+
+		sim->init.post_pin_started = 0;
+
+		if (sim->init.ready_notified)
+			sim_pin_check(sim);
+

I am lost, are we polling the IMSI here? What is the point of sim_ready_notify then?

Quoted 15 lines
src/sim.c +3 -0
 		return;
 	}
@@ -1130,6 +1143,9 @@ static void sim_efphase_read_cb(int ok, int
 length, int record,
 
 static void sim_initialize_after_pin(struct ofono_sim *sim)
 {
+	sim->init.post_pin_started = 1;
+	sim->init.ready_notified = 0;
+
 	ofono_sim_read(sim, SIM_EFPHASE_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
src/sim.c +0 -0
 			sim_efphase_read_cb, sim);

src/sim.c static void sim_pin_query_cb(const struct · line 1190

Quoted 8 lines
src/sim.c +4 -0
@@ -1167,6 +1183,10 @@ static void sim_pin_query_cb(const struct
 ofono_error *error,
 		goto checkdone;
 	}
 
+	if (pin_type == OFONO_SIM_PASSWORD_PIN2 ||
+			pin_type == OFONO_SIM_PASSWORD_PUK2)
+		pin_type = OFONO_SIM_PASSWORD_NONE;
+

Again, not sure you can really assume that if there are other PINs present.

Quoted 12 lines
src/sim.c +0 -0
 	if (sim->pin_type != pin_type) {
 		sim->pin_type = pin_type;
src/sim.c +3 -1
 		pin_name = sim_passwd_name(pin_type);
@@ -1185,7 +1205,18 @@ static void sim_pin_query_cb(const struct
 ofono_error *error,
 	}
 
 checkdone:
-	if (pin_type == OFONO_SIM_PASSWORD_NONE)
+	if (pin_type != OFONO_SIM_PASSWORD_NONE) {
+		int ready_notified = sim->init.ready_notified;
+

src/sim.c static void sim_pin_query_cb(const struct · line 1221

Quoted 10 lines
src/sim.c +9 -0
+		sim->init.post_pin_started = 0;
+		sim->init.ready_notified = 0;
+
+		sim_set_state(sim, OFONO_SIM_STATE_INSERTED);
+
+		return;
+	}
+
+	if (sim->init.pre_pin_done && !sim->init.post_pin_started)
 		sim_initialize_after_pin(sim);

I am lost here :) I think we need to talk this over on IRC.

Quoted 54 lines
src/sim.c +20 -0
 }
@@ -1199,6 +1230,22 @@ static void sim_pin_check(struct ofono_sim *sim)
 	sim->driver->query_passwd_state(sim, sim_pin_query_cb, sim);
 }
 
+void ofono_sim_ready_notify(struct ofono_sim *sim)
+{
+	if (sim == NULL)
+		return;
+
+	sim->init.ready_notified = 1;
+
+	if (sim->state == OFONO_SIM_STATE_NOT_PRESENT)
+		return;
+
+	if (!sim->init.pre_pin_done)
+		return;
+
+	sim_pin_check(sim);
+}
+
 static void sim_efli_read_cb(int ok, int length, int record,
 				const unsigned char *data,
 				int record_length, void *userdata)
@@ -1379,6 +1426,8 @@ skip_efpl:
 						DBUS_TYPE_STRING,
 						&sim->language_prefs);
 
+	sim->init.pre_pin_done = 1;
+
 	sim_pin_check(sim);
 }
 
@@ -1407,6 +1456,8 @@ static void sim_iccid_read_cb(int ok, int
 length, int record,
 
 static void sim_initialize(struct ofono_sim *sim)
 {
+	sim->init.pre_pin_started = 1;
+
 	/* Perform SIM initialization according to 3GPP 31.102 Section 5.1.1.2
 	 * The assumption here is that if sim manager is being initialized,
src/sim.c +2 -0
 	 * then sim commands are implemented, and the sim manager is then
 static void sim_free_state(struct ofono_sim *sim)
@@ -2007,6 +2060,8 @@ static void sim_free_state(struct ofono_sim *sim)
 		g_free(sim->iccid);
 		sim->iccid = NULL;
 	}
+
+	sim->init.all = 0;
 }
 
 void ofono_sim_inserted_notify(struct ofono_sim *sim, ofono_bool_t inserted)
Quoted 4 lines

_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono

Regards,
-Denis

#6 lore raw

PATCH 2/2 atmodem: call sim_ready_notify when epev is received

In reply to Kristen Carlson Accardi
---
 drivers/atmodem/sim.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)
drivers/atmodem/sim.c +8 -4
index 21bc933..44ecc1a 100644
@@ -537,13 +537,16 @@ static gboolean at_epev_unregister(gpointer user_data)
 static void at_epev_notify(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 = { .type = OFONO_ERROR_TYPE_NO_ERROR };
 
 	if (sd->epev_source)
 		return;
 
+	ofono_sim_ready_notify(sim);
+
 	cb(&error, cbd->data);
 
 	sd->epev_source = g_timeout_add(0, at_epev_unregister, sd);
@@ -553,7 +556,8 @@ 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;
 
@@ -590,15 +594,15 @@ static void at_lock_unlock_cb(gboolean ok, GAtResult *result,
 static void at_pin_send(struct ofono_sim *sim, const char *passwd,
 			ofono_sim_lock_unlock_cb_t cb, void *data)
 {
-	struct sim_data *sd = ofono_sim_get_data(sim);
 	struct cb_data *cbd = cb_data_new(cb, data);
+	struct sim_data *sd = ofono_sim_get_data(sim);
 	char buf[64];
 	int ret;
 
 	if (!cbd)
 		goto error;
 
-	cbd->user = sd;
+	cbd->user = sim;
 
 	snprintf(buf, sizeof(buf), "AT+CPIN=\"%s\"", passwd);
 
Signature · 1 line
1.7.2.1