[PATCH] sim: fix crash in case of invalid sim password type

[PATCH] sim: fix crash in case of invalid sim password type

[Christophe Ronco]

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

Hi,

I have an old  Swedish SIM card here that I tried to put in my MC7304 modem.
My ofono version is 1.20 (with some additional patches).
It sometimes return an invalid SIM password type.
After that, ofono crashes. Here is an extract of debug traces when this happens.
Ofono is just starting, modem was here before ofono starts.

Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_attributes() file id 0x6fb7 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/devinfo.c:string_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/devinfo.c:qmi_query_serial() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:get_file_attributes_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.err ofonod[1120]: Requested file structure differs from SIM: 6fb7
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/voicecall.c:ecc_g2_read_cb() 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_attributes() file id 0x6fb7 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/devinfo.c:get_ids_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:get_file_attributes_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_record() file id 0x6fb7 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/voicecall.c:ecc_g3_read_cb() 1
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_record() file id 0x6fb7 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/voicecall.c:ecc_g3_read_cb() 1
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_record() file id 0x6fb7 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/voicecall.c:ecc_g3_read_cb() 1
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_record() file id 0x6fb7 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/voicecall.c:ecc_g3_read_cb() 1
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_record() file id 0x6fb7 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/voicecall.c:ecc_g3_read_cb() 1
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_attributes() file id 0x2fe2 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:get_file_attributes_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_transparent() file id 0x2fe2 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/simfs.c:sim_fs_op_read_block_cb() bufoff: 0, dataoff: 0, tocopy: 10
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_attributes() file id 0x6f05 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:get_file_attributes_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_transparent() file id 0x6f05 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/simfs.c:sim_fs_op_read_block_cb() bufoff: 0, dataoff: 0, tocopy: 6
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_attributes() file id 0x2f05 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:get_file_attributes_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_read_transparent() file id 0x2f05 path len 0
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:read_generic_cb() 
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/simfs.c:sim_fs_op_read_block_cb() bufoff: 0, dataoff: 0, tocopy: 6
Jun 27 15:28:41 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:qmi_query_passwd_state() 
Jun 27 15:28:42 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:get_card_status() info1->app_state:0x6: OFONO_SIM_PASSWORD_INVALID
Jun 27 15:28:42 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/drivers/qmimodem/sim.c:query_passwd_state_cb() passwd state 16
Jun 27 15:28:42 klk-lpbs-0504B4 daemon.debug ofonod[1120]: ../git/src/sim.c:sim_pin_query_cb() sim->pin_type: 0, pin_type: 16
Jun 27 15:28:42 klk-lpbs-0504B4 daemon.err ofonod[1120]: Aborting (signal 11) [/usr/sbin/ofonod]

Problem is just that we don't have a string corresponding to this password type.

Christophe Ronco (1):
  sim: fix crash in case of invalid sim password type

 src/sim.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.7.4

[PATCH] sim: fix crash in case of invalid sim password type

[Christophe Ronco]

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

Crash seen in sim_pin_query_cb if pin type is OFONO_SIM_PASSWORD_INVALID
---
 src/sim.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/sim.c b/src/sim.c
index ac5b6fd..aa4190f 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -145,6 +145,7 @@ static const char *const passwd_name[] = {
 	[OFONO_SIM_PASSWORD_PHSP_PUK] = "servicepuk",
 	[OFONO_SIM_PASSWORD_PHCORP_PIN] = "corp",
 	[OFONO_SIM_PASSWORD_PHCORP_PUK] = "corppuk",
+	[OFONO_SIM_PASSWORD_INVALID] = "invalid",
 };
 
 static void sim_own_numbers_update(struct ofono_sim *sim);
-- 
2.7.4

Re: [PATCH] sim: fix crash in case of invalid sim password type

[Denis Kenzior]

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

Hi Christophe,

On 06/28/2017 02:46 AM, Christophe Ronco wrote:
> Crash seen in sim_pin_query_cb if pin type is OFONO_SIM_PASSWORD_INVALID
> ---
>  src/sim.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/sim.c b/src/sim.c
> index ac5b6fd..aa4190f 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -145,6 +145,7 @@ static const char *const passwd_name[] = {
>  	[OFONO_SIM_PASSWORD_PHSP_PUK] = "servicepuk",
>  	[OFONO_SIM_PASSWORD_PHCORP_PIN] = "corp",
>  	[OFONO_SIM_PASSWORD_PHCORP_PUK] = "corppuk",
> +	[OFONO_SIM_PASSWORD_INVALID] = "invalid",

This seems wrong, the driver should not be reporting an invalid PIN type.

>  };
>
>  static void sim_own_numbers_update(struct ofono_sim *sim);
>

Regards,
-Denis

[PATCHV2] fix crash in case of invalid sim password type

[Christophe Ronco]

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

Hi Denis,

I fixed qmimodem driver instead of src/sim.c.
It will now report a failure to get SIM pin type instead of SIM pin type
OFONO_SIM_PASSWORD_INVALID.

I 've tested drivers/qmimodem/sim.c but not drivers/qmimodem/sim-legacy.c.

With my old Swedish SIM card, I now see following trace (which is OK I think):
ofonod[974]: Querying PIN authentication state failed

SimManager properties tells me that "PinRequired" property is "none".
I don't know if that's what we want to see in this case.

Best Regards and sorry for the delay,

Christophe Ronco

Christophe Ronco (1):
  qmimodem: report failure in case of invalid pin type

 drivers/qmimodem/sim-legacy.c | 5 ++++-
 drivers/qmimodem/sim.c        | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH] qmimodem: report failure in case of invalid pin type

[Christophe Ronco]

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

This will avoid a crash seen in sim_pin_query_cb (src/sim.c) if pin type
is OFONO_SIM_PASSWORD_INVALID.
---
 drivers/qmimodem/sim-legacy.c | 5 ++++-
 drivers/qmimodem/sim.c        | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/qmimodem/sim-legacy.c b/drivers/qmimodem/sim-legacy.c
index 318b1ae..d16b2eb 100644
--- a/drivers/qmimodem/sim-legacy.c
+++ b/drivers/qmimodem/sim-legacy.c
@@ -211,7 +211,10 @@ static void get_pin_status_cb(struct qmi_result *result, void *user_data)
 	data->retries[OFONO_SIM_PASSWORD_SIM_PUK2] = pin->unblock_retries;
 
 done:
-	CALLBACK_WITH_SUCCESS(cb, pin_type, cbd->data);
+	if (pin_type == OFONO_SIM_PASSWORD_INVALID)
+		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+	else
+		CALLBACK_WITH_SUCCESS(cb, pin_type, cbd->data);
 }
 
 static void qmi_query_passwd_state(struct ofono_sim *sim,
diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 8d40028..7425d5e 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -483,7 +483,11 @@ static void query_passwd_state_cb(struct qmi_result *result,
 	case GET_CARD_STATUS_RESULT_OK:
 		DBG("passwd state %d", sim_stat.passwd_state);
 		data->retry_count = 0;
-		CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state, cbd->data);
+		if (sim_stat.passwd_state == OFONO_SIM_PASSWORD_INVALID)
+			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+		else
+			CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state,
+								cbd->data);
 		break;
 	case GET_CARD_STATUS_RESULT_TEMP_ERROR:
 		data->retry_count++;
-- 
2.7.4

Re: [PATCH] qmimodem: report failure in case of invalid pin type

[Denis Kenzior]

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

Hi Christophe,

On 08/29/2017 07:29 AM, Christophe Ronco wrote:
> This will avoid a crash seen in sim_pin_query_cb (src/sim.c) if pin type
> is OFONO_SIM_PASSWORD_INVALID.
> ---
>   drivers/qmimodem/sim-legacy.c | 5 ++++-
>   drivers/qmimodem/sim.c        | 6 +++++-
>   2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/qmimodem/sim-legacy.c b/drivers/qmimodem/sim-legacy.c
> index 318b1ae..d16b2eb 100644
> --- a/drivers/qmimodem/sim-legacy.c
> +++ b/drivers/qmimodem/sim-legacy.c
> @@ -211,7 +211,10 @@ static void get_pin_status_cb(struct qmi_result *result, void *user_data)
>   	data->retries[OFONO_SIM_PASSWORD_SIM_PUK2] = pin->unblock_retries;
>   
>   done:
> -	CALLBACK_WITH_SUCCESS(cb, pin_type, cbd->data);
> +	if (pin_type == OFONO_SIM_PASSWORD_INVALID)
> +		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> +	else
> +		CALLBACK_WITH_SUCCESS(cb, pin_type, cbd->data);
>   }
>   
>   static void qmi_query_passwd_state(struct ofono_sim *sim,
> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
> index 8d40028..7425d5e 100644
> --- a/drivers/qmimodem/sim.c
> +++ b/drivers/qmimodem/sim.c
> @@ -483,7 +483,11 @@ static void query_passwd_state_cb(struct qmi_result *result,
>   	case GET_CARD_STATUS_RESULT_OK:
>   		DBG("passwd state %d", sim_stat.passwd_state);
>   		data->retry_count = 0;
> -		CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state, cbd->data);
> +		if (sim_stat.passwd_state == OFONO_SIM_PASSWORD_INVALID)
> +			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> +		else
> +			CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state,
> +								cbd->data);

So we get back a CARD_STATUS_RESULT_OK, yet the password state is not 
valid.  It would seem there's a more serious issue here?  Is is that we 
need to keep retrying to get the password state? Is this related to 
Personalization state?

>   		break;
>   	case GET_CARD_STATUS_RESULT_TEMP_ERROR:
>   		data->retry_count++;
> 

Regards,
-Denis

Re: [PATCH] qmimodem: report failure in case of invalid pin type

[Christophe Ronco]

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

Hi Denis,

I wasn't able to reproduce yesterday.
 From traces I kept, with problematic SIM card, I had this trace when 
problem appeared:
../git/drivers/qmimodem/sim.c:get_card_status() info1->app_state:0x6: 
OFONO_SIM_PASSWORD_INVALID

I had a look at QMI lib and app_state to 0x6 means 
QMI_UIM_CARD_APPLICATION_STATE_ILLEGAL. Other unhandled states are :
  - QMI_UIM_CARD_APPLICATION_STATE_UNKNOWN
  - QMI_UIM_CARD_APPLICATION_STATE_DETECTED
  - QMI_UIM_CARD_APPLICATION_STATE_PIN1_BLOCKED

With current driver code, all these states will lead to 
OFONO_SIM_PASSWORD_INVALID. I don't know if a retry will help if we have 
any of these states. It would be easy to make a retry for these states 
as this is already implemented for 
QMI_UIM_CARD_APPLICATION_STATE_CHECK_PERSONALIZATION_STATE state.

Do you want me to change the patch to implement that?

On my side, my concern is more: what "PinRequired" property (in 
SimManager properties) should be if we fail to get PIN state (in QMI 
driver or any other driver (with AT+CPIN? replying ERROR for example))? 
I think that with current code it will be "none" and I would prefer 
something like "unknown", "error" or "invalid" to distinguish a SIM with 
entered or disabled PIN from a SIM where driver is not able to get PIN 
state. What do you think?

Best Regards,

Christophe Ronco


On 08/30/2017 08:29 PM, Denis Kenzior wrote:
> Hi Christophe,
>
> On 08/29/2017 07:29 AM, Christophe Ronco wrote:
>> This will avoid a crash seen in sim_pin_query_cb (src/sim.c) if pin type
>> is OFONO_SIM_PASSWORD_INVALID.
>> ---
>>   drivers/qmimodem/sim-legacy.c | 5 ++++-
>>   drivers/qmimodem/sim.c        | 6 +++++-
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/qmimodem/sim-legacy.c 
>> b/drivers/qmimodem/sim-legacy.c
>> index 318b1ae..d16b2eb 100644
>> --- a/drivers/qmimodem/sim-legacy.c
>> +++ b/drivers/qmimodem/sim-legacy.c
>> @@ -211,7 +211,10 @@ static void get_pin_status_cb(struct qmi_result 
>> *result, void *user_data)
>>       data->retries[OFONO_SIM_PASSWORD_SIM_PUK2] = pin->unblock_retries;
>>     done:
>> -    CALLBACK_WITH_SUCCESS(cb, pin_type, cbd->data);
>> +    if (pin_type == OFONO_SIM_PASSWORD_INVALID)
>> +        CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
>> +    else
>> +        CALLBACK_WITH_SUCCESS(cb, pin_type, cbd->data);
>>   }
>>     static void qmi_query_passwd_state(struct ofono_sim *sim,
>> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
>> index 8d40028..7425d5e 100644
>> --- a/drivers/qmimodem/sim.c
>> +++ b/drivers/qmimodem/sim.c
>> @@ -483,7 +483,11 @@ static void query_passwd_state_cb(struct 
>> qmi_result *result,
>>       case GET_CARD_STATUS_RESULT_OK:
>>           DBG("passwd state %d", sim_stat.passwd_state);
>>           data->retry_count = 0;
>> -        CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state, cbd->data);
>> +        if (sim_stat.passwd_state == OFONO_SIM_PASSWORD_INVALID)
>> +            CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
>> +        else
>> +            CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state,
>> +                                cbd->data);
>
> So we get back a CARD_STATUS_RESULT_OK, yet the password state is not 
> valid.  It would seem there's a more serious issue here?  Is is that 
> we need to keep retrying to get the password state? Is this related to 
> Personalization state?
>
>>           break;
>>       case GET_CARD_STATUS_RESULT_TEMP_ERROR:
>>           data->retry_count++;
>>
>
> Regards,
> -Denis
>

Re: [PATCH] qmimodem: report failure in case of invalid pin type

[Denis Kenzior]

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

Hi Christophe,

On 09/06/2017 03:31 AM, Christophe Ronco wrote:
> Hi Denis,
> 
> I wasn't able to reproduce yesterday.
>  From traces I kept, with problematic SIM card, I had this trace when 
> problem appeared:
> ../git/drivers/qmimodem/sim.c:get_card_status() info1->app_state:0x6: 
> OFONO_SIM_PASSWORD_INVALID
> 
> I had a look at QMI lib and app_state to 0x6 means 
> QMI_UIM_CARD_APPLICATION_STATE_ILLEGAL. Other unhandled states are :

This would probably mean that the SIM card is not inserted properly. 
One way to handle this would be to mark the SIM as not inserted...

>   - QMI_UIM_CARD_APPLICATION_STATE_UNKNOWN

No idea what we can do here, same as above?

>   - QMI_UIM_CARD_APPLICATION_STATE_DETECTED

Sounds like a transient state and we should retry

>   - QMI_UIM_CARD_APPLICATION_STATE_PIN1_BLOCKED

Doesn't this mean PUK1 entry is required?

> 
> With current driver code, all these states will lead to 
> OFONO_SIM_PASSWORD_INVALID. I don't know if a retry will help if we have 
> any of these states. It would be easy to make a retry for these states 
> as this is already implemented for 
> QMI_UIM_CARD_APPLICATION_STATE_CHECK_PERSONALIZATION_STATE state.

We do have a query_facility_lock sim atom driver method.  Perhaps this 
needs to be implemented for QMI?  If the driver doesn't handle this, 
then there's no sense in even trying devices which might be locked...

> 
> Do you want me to change the patch to implement that?
> 

Sure, patches are always welcome

> On my side, my concern is more: what "PinRequired" property (in 
> SimManager properties) should be if we fail to get PIN state (in QMI 
> driver or any other driver (with AT+CPIN? replying ERROR for example))? 
> I think that with current code it will be "none" and I would prefer 
> something like "unknown", "error" or "invalid" to distinguish a SIM with 
> entered or disabled PIN from a SIM where driver is not able to get PIN 
> state. What do you think?

AT modem drivers that use at_util_sim_state_query* set the SIM as 
inserted only if the CPIN query succeeds.  Otherwise the SIM is marked 
as not present and PinRequired property isn't even used.

> 
> Best Regards,
> 
> Christophe Ronco
> 

Regards,
-Denis

Re: [PATCH] qmimodem: report failure in case of invalid pin type

[Christophe Ronco]

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


Hi Denis,

I didn't work on that because I wasn't able to reproduce anymore (and I 
am still not able to reproduce on my desk).
But I also receive logs from gateways on field when some problems are 
discovered.
So I am able to say that this problem has been reported on some modems 
(let's say 1 out of 1000). Even on these modems, this happens only from 
time to time.
On Ofono version deployed on field, I still have the first version of 
patch sent in this thread (adding a string corresponding to 
OFONO_SIM_PASSWORD_INVALID). So, hopefully, I don't have a crash of 
Ofono when problem happens and other modem (there are 2 modems on each 
deployed gateway) can work normally.

I am pretty sure that the root cause is an hardware problem in the SIM 
connector. I say that because I have other boards where SIM detection 
sometimes fails and this always comes from a SIM connector soldering 
problem. Our SIM connector is definitely not bullet proof.

I have no debug on these boards, so I can't say what is the precise 
answer to QMI_UIM_GET_CARD_STATUS command.
My point is that this problem do not exist only with an exotic SIM on my 
desk and Ofono should not crash when it occurs.

On 09/06/2017 05:35 PM, Denis Kenzior wrote:
> Hi Christophe,
>
> On 09/06/2017 03:31 AM, Christophe Ronco wrote:
>> Hi Denis,
>>
>> I wasn't able to reproduce yesterday.
>>  From traces I kept, with problematic SIM card, I had this trace when 
>> problem appeared:
>> ../git/drivers/qmimodem/sim.c:get_card_status() info1->app_state:0x6: 
>> OFONO_SIM_PASSWORD_INVALID
>>
>> I had a look at QMI lib and app_state to 0x6 means 
>> QMI_UIM_CARD_APPLICATION_STATE_ILLEGAL. Other unhandled states are :
>
> This would probably mean that the SIM card is not inserted properly. 
> One way to handle this would be to mark the SIM as not inserted...
We just said during driver probe that SIM was inserted. We could say 
that finally it is not.
>
>>   - QMI_UIM_CARD_APPLICATION_STATE_UNKNOWN
>
> No idea what we can do here, same as above?
>
>>   - QMI_UIM_CARD_APPLICATION_STATE_DETECTED
>
> Sounds like a transient state and we should retry
>
>>   - QMI_UIM_CARD_APPLICATION_STATE_PIN1_BLOCKED
>
> Doesn't this mean PUK1 entry is required?
I assume answer is no. There is another state to say that: 
QMI_UIM_CARD_APPLICATION_STATE_PUK1_OR_UPIN_PUK_REQUIRED
>
>>
>> With current driver code, all these states will lead to 
>> OFONO_SIM_PASSWORD_INVALID. I don't know if a retry will help if we 
>> have any of these states. It would be easy to make a retry for these 
>> states as this is already implemented for 
>> QMI_UIM_CARD_APPLICATION_STATE_CHECK_PERSONALIZATION_STATE state.
>
> We do have a query_facility_lock sim atom driver method.  Perhaps this 
> needs to be implemented for QMI?  If the driver doesn't handle this, 
> then there's no sense in even trying devices which might be locked...
>
>>
>> Do you want me to change the patch to implement that?
>>
>
> Sure, patches are always welcome
>
>> On my side, my concern is more: what "PinRequired" property (in 
>> SimManager properties) should be if we fail to get PIN state (in QMI 
>> driver or any other driver (with AT+CPIN? replying ERROR for 
>> example))? I think that with current code it will be "none" and I 
>> would prefer something like "unknown", "error" or "invalid" to 
>> distinguish a SIM with entered or disabled PIN from a SIM where 
>> driver is not able to get PIN state. What do you think?
>
> AT modem drivers that use at_util_sim_state_query* set the SIM as 
> inserted only if the CPIN query succeeds.  Otherwise the SIM is marked 
> as not present and PinRequired property isn't even used.
I had a look at two cases (I just read code, I didn't try anything so I 
might be wrong...):
1) Telit: SIM insertion detected in plugin through #QSS notifications.
2) Huawei: SIM insertion detected in plugin with answer to AT^SYSINFO AT 
command.
If we have a AT+CPIN that fails with a SIM detected, I think (once 
again, that's just code reading, I have not tried) SIM will be marked 
present and PinRequired will be "none".
But maybe, in real life, it never happens to have #QSS or AT^SYSINFO 
saying that SIM is inserted and AT+CPIN returning ERROR.

I will send patches to :
1) implement a retry for all PIN states described above and report 
failure when retries fail
2) say that SIM is not inserted when we fail to retrieve PIN status

Best Regards,

Christophe Ronco

[PATCH 1/2] qmi: report failure or retry in case of invalid pin type

[Christophe Ronco]

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

QMI_UIM_GET_CARD_STATUS is retried in more error cases
when trying to get password type.
In case of failure, driver report an error instead of
OFONO_SIM_PASSWORD_INVALID. This avoids a crash.
---
 drivers/qmimodem/sim.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 1d9befc..6633c13 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -493,8 +493,15 @@ static bool get_card_status(const struct qmi_uim_slot_info *slot,
 	case 0x03:	/* PUK1 or PUK for UPIN is required */
 		sim_stat->passwd_state = OFONO_SIM_PASSWORD_SIM_PUK;
 		break;
+	case 0x00:	/* Unknown */
+	case 0x01:	/* Detected */
 	case 0x04:	/* Personalization state must be checked. */
-		/* This is temporary, we could retry and get another result */
+	case 0x05:	/* PIN1 blocked */
+	case 0x06:	/* Illegal */
+		/*
+		 * This could be temporary, we should retry and
+		 * expect another result
+		 */
 		sim_stat->passwd_state = OFONO_SIM_PASSWORD_INVALID;
 		need_retry = true;
 		break;
@@ -605,12 +612,18 @@ static void query_passwd_state_cb(struct qmi_result *result,
 	case GET_CARD_STATUS_RESULT_OK:
 		DBG("passwd state %d", sim_stat.passwd_state);
 		data->retry_count = 0;
-		CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state, cbd->data);
+		if (sim_stat.passwd_state == OFONO_SIM_PASSWORD_INVALID)
+			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+		else
+			CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state,
+								cbd->data);
 		break;
 	case GET_CARD_STATUS_RESULT_TEMP_ERROR:
 		data->retry_count++;
 		if (data->retry_count > MAX_RETRY_COUNT) {
-			DBG("Failed after %d attempts", data->retry_count);
+			DBG("Failed after %d attempts. Card state:%d",
+							data->retry_count,
+							sim_stat.card_state);
 			data->retry_count = 0;
 			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
 		} else {
-- 
2.7.4

[PATCH 2/2] qmi: report SIM not inserted when unable to get PIN type

[Christophe Ronco]

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

---
 drivers/qmimodem/sim.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 6633c13..9aa943b 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -612,9 +612,10 @@ static void query_passwd_state_cb(struct qmi_result *result,
 	case GET_CARD_STATUS_RESULT_OK:
 		DBG("passwd state %d", sim_stat.passwd_state);
 		data->retry_count = 0;
-		if (sim_stat.passwd_state == OFONO_SIM_PASSWORD_INVALID)
+		if (sim_stat.passwd_state == OFONO_SIM_PASSWORD_INVALID) {
 			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-		else
+			ofono_sim_inserted_notify(sim, FALSE);
+		} else
 			CALLBACK_WITH_SUCCESS(cb, sim_stat.passwd_state,
 								cbd->data);
 		break;
@@ -626,6 +627,7 @@ static void query_passwd_state_cb(struct qmi_result *result,
 							sim_stat.card_state);
 			data->retry_count = 0;
 			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+			ofono_sim_inserted_notify(sim, FALSE);
 		} else {
 			DBG("Retry command");
 			retry_cbd = cb_data_new(cb, cbd->data);
@@ -639,6 +641,7 @@ static void query_passwd_state_cb(struct qmi_result *result,
 		DBG("Command failed");
 		data->retry_count = 0;
 		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+		ofono_sim_inserted_notify(sim, FALSE);
 		break;
 	}
 }
-- 
2.7.4

Re: [PATCH 1/2] qmi: report failure or retry in case of invalid pin type

[Denis Kenzior]

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

Hi Christophe,

On 04/13/2018 08:58 AM, Christophe Ronco wrote:
> QMI_UIM_GET_CARD_STATUS is retried in more error cases
> when trying to get password type.
> In case of failure, driver report an error instead of
> OFONO_SIM_PASSWORD_INVALID. This avoids a crash.
> ---
>   drivers/qmimodem/sim.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 

Both applied, thanks.

Regards,
-Denis