[PATCH_v2] atmodem: Poll SIM state after having entered PIN

[PATCH_v2] atmodem: Poll SIM state after having entered PIN

[Guillaume Zajac]

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

Some modems use SIM notification to check SIM state.
If not do some poll using atutil helper.
---
 drivers/atmodem/sim.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index c51b1d4..269079a 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -51,6 +51,7 @@ struct sim_data {
 	GAtChat *chat;
 	unsigned int vendor;
 	guint ready_id;
+	struct at_util_sim_state_query *sim_state_query;
 };
 
 static const char *crsm_prefix[] = { "+CRSM:", NULL };
@@ -972,6 +973,23 @@ static void at_epev_notify(GAtResult *result, gpointer user_data)
 	sd->ready_id = 0;
 }
 
+static void sim_state_cb(gboolean present, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	struct sim_data *sd = cbd->user;
+	ofono_sim_lock_unlock_cb_t cb = cbd->cb;
+
+	at_util_sim_state_query_free(sd->sim_state_query);
+	sd->sim_state_query = NULL;
+
+	if (present == 1)
+		CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	else
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+	g_free(cbd);
+}
+
 static void at_pin_send_cb(gboolean ok, GAtResult *result,
 				gpointer user_data)
 {
@@ -1005,6 +1023,13 @@ static void at_pin_send_cb(gboolean ok, GAtResult *result,
 		sd->ready_id = g_at_chat_register(sd->chat, "*EPEV",
 							at_epev_notify,
 							FALSE, cbd, g_free);
+		/*
+		 * After pin is entered, SIM state is check by doing
+		 * some polling if modem doesn't use notification.
+		 */
+	default:
+		sd->sim_state_query = at_util_sim_state_query_new(sd->chat,
+						2, 20, sim_state_cb, cbd);
 		return;
 	}
 
@@ -1246,6 +1271,9 @@ static void at_sim_remove(struct ofono_sim *sim)
 {
 	struct sim_data *sd = ofono_sim_get_data(sim);
 
+	/* Cleanup potential SIM state polling */
+	at_util_sim_state_query_free(sd->sim_state_query);
+
 	ofono_sim_set_data(sim, NULL);
 
 	g_at_chat_unref(sd->chat);
-- 
1.7.4.1

Re: [PATCH_v2] atmodem: Poll SIM state after having entered PIN

[Denis Kenzior]

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

Hi Guillaume,

>> Who owns the cbd data in this case? Are we not leaking that here?
>>
> 
> Yes, we are. I see 2 options here:
> a- Tweaking current at_util_sim_state_query_new(GAtChat *chat,
>                         guint interval, guint num_times,
>                         at_util_sim_inserted_cb_t cb,
>                         void *userdata, GDestroyNotify data_destroy); in
> adding a destroyer for user data
> 
> b- Implementing new gpointer at_util_sim_state_query_get_data(struct
> at_util_sim_state_query *req); and free user_data during remove
> 

Option a would be fine.

Regards,
-Denis

Re: [PATCH_v2] atmodem: Poll SIM state after having entered PIN

[Marcel Holtmann]

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

Hi Guillaume,

> Some modems use SIM notification to check SIM state.
> If not do some poll using atutil helper.
> ---
>  drivers/atmodem/sim.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
> index c51b1d4..269079a 100644
> --- a/drivers/atmodem/sim.c
> +++ b/drivers/atmodem/sim.c
> @@ -51,6 +51,7 @@ struct sim_data {
>  	GAtChat *chat;
>  	unsigned int vendor;
>  	guint ready_id;
> +	struct at_util_sim_state_query *sim_state_query;
>  };
>  
>  static const char *crsm_prefix[] = { "+CRSM:", NULL };
> @@ -972,6 +973,23 @@ static void at_epev_notify(GAtResult *result, gpointer user_data)
>  	sd->ready_id = 0;
>  }
>  
> +static void sim_state_cb(gboolean present, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	struct sim_data *sd = cbd->user;
> +	ofono_sim_lock_unlock_cb_t cb = cbd->cb;
> +
> +	at_util_sim_state_query_free(sd->sim_state_query);
> +	sd->sim_state_query = NULL;
> +
> +	if (present == 1)
> +		CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +	else
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +
> +	g_free(cbd);
> +}
> +
>  static void at_pin_send_cb(gboolean ok, GAtResult *result,
>  				gpointer user_data)
>  {
> @@ -1005,6 +1023,13 @@ static void at_pin_send_cb(gboolean ok, GAtResult *result,
>  		sd->ready_id = g_at_chat_register(sd->chat, "*EPEV",
>  							at_epev_notify,
>  							FALSE, cbd, g_free);
> +		/*
> +		 * After pin is entered, SIM state is check by doing
> +		 * some polling if modem doesn't use notification.
> +		 */
> +	default:
> +		sd->sim_state_query = at_util_sim_state_query_new(sd->chat,
> +						2, 20, sim_state_cb, cbd);
>  		return;
>  	}

this part just looks wrong. You are now also doing CPIN polling for the
Ericsson and ST-Ericsson cards.

>  
> @@ -1246,6 +1271,9 @@ static void at_sim_remove(struct ofono_sim *sim)
>  {
>  	struct sim_data *sd = ofono_sim_get_data(sim);
>  
> +	/* Cleanup potential SIM state polling */
> +	at_util_sim_state_query_free(sd->sim_state_query);
> +

Who owns the cbd data in this case? Are we not leaking that here?

>  	ofono_sim_set_data(sim, NULL);
>  
>  	g_at_chat_unref(sd->chat);

Regards

Marcel

Re: [PATCH_v2] atmodem: Poll SIM state after having entered PIN

[Guillaume Zajac]

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

Hi Marcel,

On 17/04/2012 11:36, Marcel Holtmann wrote:
> Hi Guillaume,
>
>> Some modems use SIM notification to check SIM state.
>> If not do some poll using atutil helper.
>> ---
>>   drivers/atmodem/sim.c |   28 ++++++++++++++++++++++++++++
>>   1 files changed, 28 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
>> index c51b1d4..269079a 100644
>> --- a/drivers/atmodem/sim.c
>> +++ b/drivers/atmodem/sim.c
>> @@ -51,6 +51,7 @@ struct sim_data {
>>   	GAtChat *chat;
>>   	unsigned int vendor;
>>   	guint ready_id;
>> +	struct at_util_sim_state_query *sim_state_query;
>>   };
>>
>>   static const char *crsm_prefix[] = { "+CRSM:", NULL };
>> @@ -972,6 +973,23 @@ static void at_epev_notify(GAtResult *result, gpointer user_data)
>>   	sd->ready_id = 0;
>>   }
>>
>> +static void sim_state_cb(gboolean present, gpointer user_data)
>> +{
>> +	struct cb_data *cbd = user_data;
>> +	struct sim_data *sd = cbd->user;
>> +	ofono_sim_lock_unlock_cb_t cb = cbd->cb;
>> +
>> +	at_util_sim_state_query_free(sd->sim_state_query);
>> +	sd->sim_state_query = NULL;
>> +
>> +	if (present == 1)
>> +		CALLBACK_WITH_SUCCESS(cb, cbd->data);
>> +	else
>> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
>> +
>> +	g_free(cbd);
>> +}
>> +
>>   static void at_pin_send_cb(gboolean ok, GAtResult *result,
>>   				gpointer user_data)
>>   {
>> @@ -1005,6 +1023,13 @@ static void at_pin_send_cb(gboolean ok, GAtResult *result,
>>   		sd->ready_id = g_at_chat_register(sd->chat, "*EPEV",
>>   							at_epev_notify,
>>   							FALSE, cbd, g_free);
>> +		/*
>> +		 * After pin is entered, SIM state is check by doing
>> +		 * some polling if modem doesn't use notification.
>> +		 */
>> +	default:
>> +		sd->sim_state_query = at_util_sim_state_query_new(sd->chat,
>> +						2, 20, sim_state_cb, cbd);
>>   		return;
>>   	}
> this part just looks wrong. You are now also doing CPIN polling for the
> Ericsson and ST-Ericsson cards.
>

Ok I can fix it easily.

>>
>> @@ -1246,6 +1271,9 @@ static void at_sim_remove(struct ofono_sim *sim)
>>   {
>>   	struct sim_data *sd = ofono_sim_get_data(sim);
>>
>> +	/* Cleanup potential SIM state polling */
>> +	at_util_sim_state_query_free(sd->sim_state_query);
>> +
> Who owns the cbd data in this case? Are we not leaking that here?
>

Yes, we are. I see 2 options here:
a- Tweaking current at_util_sim_state_query_new(GAtChat *chat,
                         guint interval, guint num_times,
                         at_util_sim_inserted_cb_t cb,
                         void *userdata, GDestroyNotify data_destroy); 
in adding a destroyer for user data

b- Implementing new gpointer at_util_sim_state_query_get_data(struct 
at_util_sim_state_query *req); and free user_data during remove

Or maybe you have another solution?

>>   	ofono_sim_set_data(sim, NULL);
>>
>>   	g_at_chat_unref(sd->chat);

Kind regards,
Guillaume