Merge pull request #290 from tkenakka/pin

Refactor rilmodem SIM PIN handling

ofono/drivers/rilmodem/phonebook.c

@@ -292,20 +292,17 @@ void handle_sne(size_t len, const unsigned char *msg, char *sne)
 				list_entry->data;
 
 			if (entry) {
-				/* If one already exists,
-						delete it */
-				if (entry->sne)
-					g_free(entry->sne);
-
 				DBG("Adding SNE to entry %d",
 					phonebook_entry_nbr);
 				DBG("name %s", entry->name);
 
+				g_free(entry->sne);
 				entry->sne = sne;
-			} else {
-				g_free(sne);
+				return;
 			}
 		}
+
+		g_free(sne);
 	}
 }
 

ofono/drivers/rilmodem/rilutil.c

@@ -401,7 +401,6 @@ gboolean ril_util_parse_sim_status(GRil *gril,
 	}
 
 	for (i = 0; i < status->num_apps; i++) {
-		DBG("processing app[%d]", i);
 		apps[i] = g_try_new0(struct sim_app, 1);
 		if (apps[i] == NULL) {
 			ofono_error("Can't allocate app_data");
@@ -411,8 +410,6 @@ gboolean ril_util_parse_sim_status(GRil *gril,
 		apps[i]->app_type = parcel_r_int32(&rilp);
 		apps[i]->app_state = parcel_r_int32(&rilp);
 
-		DBG("app[%d]: app_type: %d, app_state: %d", i,
-				apps[i]->app_type, apps[i]->app_state);
 		/*
 		 * Consider RIL_APPSTATE_ILLEGAL also READY. Even if app state
 		 * is  RIL_APPSTATE_ILLEGAL (-1), ICC operations must be

ofono/drivers/rilmodem/sim.c

@@ -519,44 +519,6 @@ static void ril_read_imsi(struct ofono_sim *sim, ofono_sim_imsi_cb_t cb,
 	}
 }
 
-void set_pin_lock_state(struct ofono_sim *sim, struct sim_app *app)
-{
-	DBG("pin1:%u,pin2:%u", app->pin1_state, app->pin2_state);
-	/*
-	 * Updates only pin and pin2 state. Other locks are not dealt here. For
-	 * that a RIL_REQUEST_QUERY_FACILITY_LOCK request should be used.
-	 */
-	switch (app->pin1_state) {
-	case RIL_PINSTATE_ENABLED_NOT_VERIFIED:
-	case RIL_PINSTATE_ENABLED_VERIFIED:
-	case RIL_PINSTATE_ENABLED_BLOCKED:
-	case RIL_PINSTATE_ENABLED_PERM_BLOCKED:
-		ofono_set_pin_lock_state(sim, OFONO_SIM_PASSWORD_SIM_PIN, TRUE);
-		break;
-	case RIL_PINSTATE_DISABLED:
-		ofono_set_pin_lock_state(
-			sim, OFONO_SIM_PASSWORD_SIM_PIN, FALSE);
-		break;
-	default:
-		break;
-	}
-	switch (app->pin2_state) {
-	case RIL_PINSTATE_ENABLED_NOT_VERIFIED:
-	case RIL_PINSTATE_ENABLED_VERIFIED:
-	case RIL_PINSTATE_ENABLED_BLOCKED:
-	case RIL_PINSTATE_ENABLED_PERM_BLOCKED:
-		ofono_set_pin_lock_state(
-			sim, OFONO_SIM_PASSWORD_SIM_PIN2, TRUE);
-		break;
-	case RIL_PINSTATE_DISABLED:
-		ofono_set_pin_lock_state(
-			sim, OFONO_SIM_PASSWORD_SIM_PIN2, FALSE);
-		break;
-	default:
-		break;
-	}
-}
-
 static void configure_active_app(struct sim_data *sd,
 					struct sim_app *app,
 					guint index)
@@ -627,6 +589,19 @@ static void configure_active_app(struct sim_data *sd,
 	}
 }
 
+static void free_sim_state(struct sim_data *sd)
+{
+	guint i = 0;
+
+	sd->passwd_state = OFONO_SIM_PASSWORD_INVALID;
+
+	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
+		sd->retries[i] = -1;
+
+	sd->removed = TRUE;
+	sd->initialized = FALSE;
+}
+
 static void sim_status_cb(struct ril_msg *message, gpointer user_data)
 {
 	struct ofono_sim *sim = user_data;
@@ -637,7 +612,7 @@ static void sim_status_cb(struct ril_msg *message, gpointer user_data)
 	guint search_index = -1;
 	struct parcel rilp;
 
-	DBG("%p", message);
+	DBG("");
 
 	if (ril_util_parse_sim_status(sd->ril, message, &status, apps) &&
 		status.num_apps) {
@@ -652,30 +627,30 @@ static void sim_status_cb(struct ril_msg *message, gpointer user_data)
 				apps[i]->app_type != RIL_APPTYPE_UNKNOWN) {
 				current_active_app = apps[i]->app_type;
 				configure_active_app(sd, apps[i], i);
-				set_pin_lock_state(sim, apps[i]);
 				break;
 			}
 		}
 
-		/*
-		 * ril_util_parse_sim_status returns true only when
-		 * card status is RIL_CARDSTATE_PRESENT so notify TRUE always.
-		 *
-		 * ofono_sim_inserted_notify skips and returns if
-		 * present/not_present status doesn't change from previous.
-		 */
-		ofono_sim_inserted_notify(sim, TRUE);
-
 		sd->removed = FALSE;
 
-		/* TODO: There doesn't seem to be any other
-		 * way to force the core SIM code to
-		 * recheck the PIN.
-		 * Wouldn't __ofono_sim_refresh be
-		 * more appropriate call here??
-		 * __ofono_sim_refresh(sim, NULL, TRUE, TRUE);
-		 */
-		__ofono_sim_recheck_pin(sim);
+		if (sd->passwd_state != OFONO_SIM_PASSWORD_INVALID) {
+			/*
+			 * ril_util_parse_sim_status returns true only when
+			 * card status is RIL_CARDSTATE_PRESENT,
+			 * ofono_sim_inserted_notify returns if status doesn't
+			 * change. So can notify core always in this branch.
+			 */
+			ofono_sim_inserted_notify(sim, TRUE);
+
+			/* TODO: There doesn't seem to be any other
+			 * way to force the core SIM code to
+			 * recheck the PIN.
+			 * Wouldn't __ofono_sim_refresh be
+			 * more appropriate call here??
+			 * __ofono_sim_refresh(sim, NULL, TRUE, TRUE);
+			 */
+			__ofono_sim_recheck_pin(sim);
+		}
 
 		if (current_online_state == RIL_ONLINE_PREF) {
 
@@ -703,10 +678,9 @@ static void sim_status_cb(struct ril_msg *message, gpointer user_data)
 		if (status.card_state == RIL_CARDSTATE_ABSENT) {
 			ofono_info("%s: RIL_CARDSTATE_ABSENT", __func__);
 
-			ofono_sim_inserted_notify(sim, FALSE);
+			free_sim_state(sd);
 
-			sd->removed = TRUE;
-			sd->initialized = FALSE;
+			ofono_sim_inserted_notify(sim, FALSE);
 		}
 	}
 }
@@ -771,7 +745,6 @@ static void ril_query_passwd_state_cb(struct ril_msg *message, gpointer user_dat
 				apps[i]->app_type != RIL_APPTYPE_UNKNOWN) {
 				current_active_app = apps[i]->app_type;
 				configure_active_app(sd, apps[i], i);
-				set_pin_lock_state(sim, apps[i]);
 				break;
 			}
 		}
@@ -826,8 +799,9 @@ static void ril_pin_change_state_cb(struct ril_msg *message, gpointer user_data)
 	struct sim_data *sd = cbd->user;
 	struct parcel rilp;
 	int retry_count;
-	int retries[OFONO_SIM_PASSWORD_INVALID];
 	int passwd_type;
+	int i;
+
 	/* There is no reason to ask SIM status until
 	 * unsolicited sim status change indication
 	 * Looks like state does not change before that.
@@ -837,8 +811,11 @@ static void ril_pin_change_state_cb(struct ril_msg *message, gpointer user_data)
 	ril_util_init_parcel(message, &rilp);
 	parcel_r_int32(&rilp);
 	retry_count = parcel_r_int32(&rilp);
-	retries[passwd_type] = retry_count;
-	sd->retries[passwd_type] = retries[passwd_type];
+
+	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
+		sd->retries[i] = -1;
+
+	sd->retries[passwd_type] = retry_count;
 
 	DBG("result=%d passwd_type=%d retry_count=%d",
 		message->error, passwd_type, retry_count);
@@ -1110,7 +1087,7 @@ static gboolean ril_sim_register(gpointer user)
 	send_get_sim_status(sim);
 
 	sd->idle_id = 0;
-	sd->idle_id = g_ril_register(sd->ril,
+	g_ril_register(sd->ril,
 			RIL_UNSOL_RESPONSE_SIM_STATUS_CHANGED,
 			(GRilNotifyFunc) ril_sim_status_changed, sim);
 	return FALSE;
@@ -1153,12 +1130,15 @@ static int ril_sim_probe(struct ofono_sim *sim, unsigned int vendor,
 
 static void ril_sim_remove(struct ofono_sim *sim)
 {
+	DBG("");
 	struct sim_data *sd = ofono_sim_get_data(sim);
 
 	ofono_sim_set_data(sim, NULL);
 
-	if (sd->idle_id > 0)
+	if (sd->idle_id > 0) {
 		g_source_remove(sd->idle_id);
+		sd->idle_id = 0;
+	}
 
 	g_free(sd->aid_str);
 	g_free(sd->app_str);

ofono/include/sim.h

@@ -199,10 +199,6 @@ const unsigned char *ofono_sim_get_cphs_service_table(struct ofono_sim *sim);
 
 enum ofono_sim_password_type ofono_sim_get_password_type(struct ofono_sim *sim);
 
-void ofono_set_pin_lock_state(struct ofono_sim *sim,
-				enum ofono_sim_password_type type,
-				ofono_bool_t state);
-
 unsigned int ofono_sim_add_state_watch(struct ofono_sim *sim,
 					ofono_sim_state_event_cb_t cb,
 					void *data, ofono_destroy_func destroy);

ofono/src/modem.c

@@ -68,6 +68,7 @@ struct ofono_modem {
 	unsigned int		call_ids;
 	DBusMessage		*pending;
 	guint			interface_update;
+	ofono_bool_t		features_update;
 	ofono_bool_t		powered;
 	ofono_bool_t		powered_pending;
 	ofono_bool_t		get_online;
@@ -1247,6 +1248,9 @@ static gboolean trigger_interface_update(void *data)
 						&interfaces);
 	g_free(interfaces);
 
+	if (!modem->features_update)
+		goto out;
+
 	features = g_new0(char *, g_slist_length(modem->feature_list) + 1);
 	for (i = 0, l = modem->feature_list; l; l = l->next, i++)
 		features[i] = l->data;
@@ -1255,7 +1259,9 @@ static gboolean trigger_interface_update(void *data)
 						"Features", DBUS_TYPE_STRING,
 						&features);
 	g_free(features);
+	modem->features_update = FALSE;
 
+out:
 	modem->interface_update = 0;
 
 	return FALSE;
@@ -1299,9 +1305,11 @@ void ofono_modem_add_interface(struct ofono_modem *modem,
 						g_strdup(interface));
 
 	feature = get_feature(interface);
-	if (feature)
+	if (feature) {
+		modem->features_update = TRUE;
 		modem->feature_list = g_slist_prepend(modem->feature_list,
 							g_strdup(feature));
+	}
 
 	if (modem->interface_update != 0)
 		return;
@@ -1333,6 +1341,7 @@ void ofono_modem_remove_interface(struct ofono_modem *modem,
 						(GCompareFunc) strcmp);
 		if (found) {
 			g_free(found->data);
+			modem->features_update = TRUE;
 			modem->feature_list =
 				g_slist_remove(modem->feature_list,
 						found->data);
@@ -2071,6 +2080,7 @@ static void modem_unregister(struct ofono_modem *modem)
 	if (modem->interface_update) {
 		g_source_remove(modem->interface_update);
 		modem->interface_update = 0;
+		modem->features_update = FALSE;
 	}
 
 	if (modem->lock_watch) {

ofono/src/sim.c

@@ -2296,13 +2296,6 @@ enum ofono_sim_password_type ofono_sim_get_password_type(struct ofono_sim *sim)
 	return sim->pin_type;
 }
 
-void ofono_set_pin_lock_state(struct ofono_sim *sim,
-				enum ofono_sim_password_type type,
-				ofono_bool_t state)
-{
-	sim->locked_pins[type] = state;
-}
-
 const unsigned char *ofono_sim_get_cphs_service_table(struct ofono_sim *sim)
 {
 	if (sim == NULL)
@@ -2513,8 +2506,18 @@ void ofono_sim_inserted_notify(struct ofono_sim *sim, ofono_bool_t inserted)
 
 	if (inserted)
 		sim_initialize(sim);
-	else
+	else {
+		/*
+		 * Reset type to trigger property change signal after sim is
+		 * removed and inserted.
+		 * Can't reset in sim_free_main_state because it's called also
+		 * when sim state changes to OFONO_SIM_STATE_LOCKED_OUT
+		 * (PUK lock) if user fails to change PIN.
+		 */
+		sim->pin_type = OFONO_SIM_PASSWORD_INVALID;
+
 		sim_free_state(sim);
+	}
 }
 
 unsigned int ofono_sim_add_state_watch(struct ofono_sim *sim,
@@ -2763,6 +2766,8 @@ static void sim_pin_query_cb(const struct ofono_error *error,
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(sim->atom);
 	const char *pin_name;
+	char **locked_pins;
+	gboolean lock_changed;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		ofono_error("Querying PIN authentication state failed");
@@ -2777,9 +2782,21 @@ static void sim_pin_query_cb(const struct ofono_error *error,
 				password_is_pin(pin_type) == FALSE)
 			pin_type = puk2pin(pin_type);
 
-		if (pin_type != OFONO_SIM_PASSWORD_INVALID)
+		if (pin_type != OFONO_SIM_PASSWORD_INVALID) {
+			lock_changed = !sim->locked_pins[pin_type];
 			sim->locked_pins[pin_type] = TRUE;
 
+			if (lock_changed) {
+				locked_pins = get_locked_pins(sim);
+				ofono_dbus_signal_array_property_changed(conn,
+						path,
+						OFONO_SIM_MANAGER_INTERFACE,
+						"LockedPins", DBUS_TYPE_STRING,
+						&locked_pins);
+				g_strfreev(locked_pins);
+				locked_pins = NULL;
+			}
+		}
 		ofono_dbus_signal_property_changed(conn, path,
 						OFONO_SIM_MANAGER_INTERFACE,
 						"PinRequired", DBUS_TYPE_STRING,