* [PATCH] sim: fix crash in case of invalid sim password type @ 2017-06-28 7:46 Christophe Ronco 2017-06-28 7:46 ` Christophe Ronco 0 siblings, 1 reply; 12+ messages in thread From: Christophe Ronco @ 2017-06-28 7:46 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] sim: fix crash in case of invalid sim password type 2017-06-28 7:46 [PATCH] sim: fix crash in case of invalid sim password type Christophe Ronco @ 2017-06-28 7:46 ` Christophe Ronco 2017-08-07 19:19 ` Denis Kenzior 0 siblings, 1 reply; 12+ messages in thread From: Christophe Ronco @ 2017-06-28 7:46 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] sim: fix crash in case of invalid sim password type 2017-06-28 7:46 ` Christophe Ronco @ 2017-08-07 19:19 ` Denis Kenzior 2017-08-29 12:29 ` [PATCHV2] " Christophe Ronco 0 siblings, 1 reply; 12+ messages in thread From: Denis Kenzior @ 2017-08-07 19:19 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHV2] fix crash in case of invalid sim password type 2017-08-07 19:19 ` Denis Kenzior @ 2017-08-29 12:29 ` Christophe Ronco 2017-08-29 12:29 ` [PATCH] qmimodem: report failure in case of invalid pin type Christophe Ronco 0 siblings, 1 reply; 12+ messages in thread From: Christophe Ronco @ 2017-08-29 12:29 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] qmimodem: report failure in case of invalid pin type 2017-08-29 12:29 ` [PATCHV2] " Christophe Ronco @ 2017-08-29 12:29 ` Christophe Ronco 2017-08-30 18:29 ` Denis Kenzior 0 siblings, 1 reply; 12+ messages in thread From: Christophe Ronco @ 2017-08-29 12:29 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] qmimodem: report failure in case of invalid pin type 2017-08-29 12:29 ` [PATCH] qmimodem: report failure in case of invalid pin type Christophe Ronco @ 2017-08-30 18:29 ` Denis Kenzior 2017-09-06 8:31 ` Christophe Ronco 0 siblings, 1 reply; 12+ messages in thread From: Denis Kenzior @ 2017-08-30 18:29 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] qmimodem: report failure in case of invalid pin type 2017-08-30 18:29 ` Denis Kenzior @ 2017-09-06 8:31 ` Christophe Ronco 2017-09-06 15:35 ` Denis Kenzior 0 siblings, 1 reply; 12+ messages in thread From: Christophe Ronco @ 2017-09-06 8:31 UTC (permalink / raw) To: ofono [-- 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] qmimodem: report failure in case of invalid pin type 2017-09-06 8:31 ` Christophe Ronco @ 2017-09-06 15:35 ` Denis Kenzior 2018-04-13 13:55 ` Christophe Ronco 0 siblings, 1 reply; 12+ messages in thread From: Denis Kenzior @ 2017-09-06 15:35 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] qmimodem: report failure in case of invalid pin type 2017-09-06 15:35 ` Denis Kenzior @ 2018-04-13 13:55 ` Christophe Ronco 2018-04-13 13:58 ` [PATCH 1/2] qmi: report failure or retry " Christophe Ronco 0 siblings, 1 reply; 12+ messages in thread From: Christophe Ronco @ 2018-04-13 13:55 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] qmi: report failure or retry in case of invalid pin type 2018-04-13 13:55 ` Christophe Ronco @ 2018-04-13 13:58 ` Christophe Ronco 2018-04-13 13:58 ` [PATCH 2/2] qmi: report SIM not inserted when unable to get PIN type Christophe Ronco 2018-04-19 15:30 ` [PATCH 1/2] qmi: report failure or retry in case of invalid pin type Denis Kenzior 0 siblings, 2 replies; 12+ messages in thread From: Christophe Ronco @ 2018-04-13 13:58 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] qmi: report SIM not inserted when unable to get PIN type 2018-04-13 13:58 ` [PATCH 1/2] qmi: report failure or retry " Christophe Ronco @ 2018-04-13 13:58 ` Christophe Ronco 2018-04-19 15:30 ` [PATCH 1/2] qmi: report failure or retry in case of invalid pin type Denis Kenzior 1 sibling, 0 replies; 12+ messages in thread From: Christophe Ronco @ 2018-04-13 13:58 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] qmi: report failure or retry in case of invalid pin type 2018-04-13 13:58 ` [PATCH 1/2] qmi: report failure or retry " Christophe Ronco 2018-04-13 13:58 ` [PATCH 2/2] qmi: report SIM not inserted when unable to get PIN type Christophe Ronco @ 2018-04-19 15:30 ` Denis Kenzior 1 sibling, 0 replies; 12+ messages in thread From: Denis Kenzior @ 2018-04-19 15:30 UTC (permalink / raw) To: ofono [-- 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-04-19 15:30 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-28 7:46 [PATCH] sim: fix crash in case of invalid sim password type Christophe Ronco 2017-06-28 7:46 ` Christophe Ronco 2017-08-07 19:19 ` Denis Kenzior 2017-08-29 12:29 ` [PATCHV2] " Christophe Ronco 2017-08-29 12:29 ` [PATCH] qmimodem: report failure in case of invalid pin type Christophe Ronco 2017-08-30 18:29 ` Denis Kenzior 2017-09-06 8:31 ` Christophe Ronco 2017-09-06 15:35 ` Denis Kenzior 2018-04-13 13:55 ` Christophe Ronco 2018-04-13 13:58 ` [PATCH 1/2] qmi: report failure or retry " Christophe Ronco 2018-04-13 13:58 ` [PATCH 2/2] qmi: report SIM not inserted when unable to get PIN type Christophe Ronco 2018-04-19 15:30 ` [PATCH 1/2] qmi: report failure or retry in case of invalid pin type Denis Kenzior
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.