[PATCH 0/4] support MC7430 Sierra modem

[PATCH 0/4] support MC7430 Sierra modem

[Christophe Ronco]

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

Hi,

I have made some patch to add the support of the MC7430 Sierra modem.
This is a QMI modem.
I chose to use qmimodem sim driver instead of qmimodem simlegacy driver
because:
 - a QMI command used in sim-legacy is not supported by MC7430
 - UIM commands are more powerfull than DMS commands

I had some other problems fixed in following patches.
With these patches I am able to see the MC7430 modem as a service in
connman over ofono and to connect and disconnect it.

Patch can be applied to ofono 1.19.

Christophe

Christophe Ronco (4):
  MC7430: use qmimodem sim driver
  MC7430: add read_imsi feature to qmimodem sim driver
  MC7430: fix get signal strength
  MC7430: fix QMI notification messages handling

 drivers/qmimodem/nas.h |  2 +-
 drivers/qmimodem/qmi.c |  2 +-
 drivers/qmimodem/sim.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++----
 plugins/gobi.c         |  2 +-
 plugins/udevng.c       |  2 --
 5 files changed, 82 insertions(+), 12 deletions(-)

-- 
2.11.0

[PATCH 1/4] MC7430: use qmimodem sim driver

[Christophe Ronco]

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

QMI modem sim-legacy driver uses a command not supported by MC7430 (QMI_DMS_GET_PIN_STATUS)

plugins/udevng.c
 QMI Sierra modems use sim driver.
drivers/qmimodem/sim.c
 In get_card_status_cb, call ofono_sim_register after ofono_sim_inserted_notify (as it is done in sim_legacy).
 Otherwise process crash (sim->driver->query_facility_lock called and not defined in this driver) with  MC7430 modem using this driver.
---
 drivers/qmimodem/sim.c | 3 +--
 plugins/udevng.c       | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 197da509..fa7bef58 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -403,8 +403,6 @@ static void get_card_status_cb(struct qmi_result *result, void *user_data)
 	}
 
 done:
-	ofono_sim_register(sim);
-
 	switch (data->card_state) {
 	case 0x00:	/* Absent */
 	case 0x02:	/* Error */
@@ -413,6 +411,7 @@ done:
 		ofono_sim_inserted_notify(sim, TRUE);
 		break;
 	}
+	ofono_sim_register(sim);
 }
 
 static void event_registration_cb(struct qmi_result *result, void *user_data)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index 933bf4fa..a2866b64 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -252,8 +252,6 @@ static gboolean setup_sierra(struct modem_info *modem)
 
 	if (qmi != NULL && net != NULL) {
 		ofono_modem_set_driver(modem->modem, "gobi");
-		/* Fixup SIM interface for Sierra QMI devices */
-		ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
 		goto done;
 	}
 
-- 
2.11.0

[PATCH 2/4] MC7430: add read_imsi feature to qmimodem sim driver

[Christophe Ronco]

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

drivers/qmimodem/sim.c
 Add read_imsi feature to this driver. This is based on DMS service.
 This is mandatory to be able to use this driver for GPRS connection
plugins/gobi.c
 DMS feature is needed for SIM driver (to be able to read IMSI)
---
 drivers/qmimodem/sim.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++---
 plugins/gobi.c         |  2 +-
 2 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index fa7bef58..f61466d5 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -30,6 +30,7 @@
 #include <ofono/sim.h>
 
 #include "qmi.h"
+#include "dms.h"
 #include "uim.h"
 
 #include "qmimodem.h"
@@ -39,6 +40,8 @@
 #define EF_STATUS_VALID 1
 
 struct sim_data {
+	struct qmi_device *qmi_dev;
+	struct qmi_service *dms;
 	struct qmi_service *uim;
 	uint32_t event_mask;
 	uint8_t card_state;
@@ -295,6 +298,47 @@ error:
 	g_free(cbd);
 }
 
+static void get_imsi_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_sim_imsi_cb_t cb = cbd->cb;
+	char *str;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, NULL)) {
+		CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
+		return;
+	}
+
+	str = qmi_result_get_string(result, QMI_DMS_RESULT_IMSI);
+	if (!str) {
+		CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
+		return;
+	}
+
+	CALLBACK_WITH_SUCCESS(cb, str, cbd->data);
+
+	qmi_free(str);
+}
+
+static void qmi_read_imsi(struct ofono_sim *sim,
+				ofono_sim_imsi_cb_t cb, void *user_data)
+{
+	struct sim_data *data = ofono_sim_get_data(sim);
+	struct cb_data *cbd = cb_data_new(cb, user_data);
+
+	DBG("");
+
+	if (qmi_service_send(data->dms, QMI_DMS_GET_IMSI, NULL,
+					get_imsi_cb, cbd, g_free) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
+
+	g_free(cbd);
+}
+
 static void qmi_query_passwd_state(struct ofono_sim *sim,
 				ofono_sim_passwd_cb_t cb, void *user_data)
 {
@@ -464,11 +508,30 @@ static void create_uim_cb(struct qmi_service *service, void *user_data)
 		return;
 
 error:
-	qmi_service_unref(data->uim);
+	qmi_service_unref(data->dms);
 
 	ofono_sim_remove(sim);
 }
 
+static void create_dms_cb(struct qmi_service *service, void *user_data)
+{
+	struct ofono_sim *sim = user_data;
+	struct sim_data *data = ofono_sim_get_data(sim);
+	struct qmi_param *param;
+
+	DBG("");
+
+	if (!service) {
+		ofono_error("Failed to request DMS service");
+		ofono_sim_remove(sim);
+		return;
+	}
+
+	data->dms = qmi_service_ref(service);
+
+	qmi_service_create(data->qmi_dev, QMI_SERVICE_UIM, create_uim_cb, sim, NULL);
+}
+
 static int qmi_sim_probe(struct ofono_sim *sim,
 				unsigned int vendor, void *user_data)
 {
@@ -485,9 +548,12 @@ static int qmi_sim_probe(struct ofono_sim *sim,
 	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
 		data->retries[i] = -1;
 
+	data->qmi_dev = device;
+
 	ofono_sim_set_data(sim, data);
 
-	qmi_service_create(device, QMI_SERVICE_UIM, create_uim_cb, sim, NULL);
+	qmi_service_create_shared(device, QMI_SERVICE_DMS,
+						create_dms_cb, sim, NULL);
 
 	return 0;
 }
@@ -500,9 +566,15 @@ static void qmi_sim_remove(struct ofono_sim *sim)
 
 	ofono_sim_set_data(sim, NULL);
 
-	qmi_service_unregister_all(data->uim);
-
-	qmi_service_unref(data->uim);
+	if (data->uim) {
+		qmi_service_unregister_all(data->uim);
+		qmi_service_unref(data->uim);
+		data->uim = NULL;
+	}
+	if (data->dms) {
+		qmi_service_unregister_all(data->dms);
+		qmi_service_unref(data->dms);
+	}
 
 	g_free(data);
 }
@@ -515,6 +587,7 @@ static struct ofono_sim_driver driver = {
 	.read_file_transparent	= qmi_read_transparent,
 	.read_file_linear	= qmi_read_record,
 	.read_file_cyclic	= qmi_read_record,
+	.read_imsi		= qmi_read_imsi,
 	.query_passwd_state	= qmi_query_passwd_state,
 	.query_pin_retries	= qmi_query_pin_retries,
 };
diff --git a/plugins/gobi.c b/plugins/gobi.c
index 6a789411..84a98279 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -420,7 +420,7 @@ static void gobi_pre_sim(struct ofono_modem *modem)
 
 	ofono_devinfo_create(modem, 0, "qmimodem", data->device);
 
-	if (data->features & GOBI_UIM)
+	if ((data->features & GOBI_UIM) && (data->features & GOBI_DMS))
 		sim_driver = "qmimodem";
 	else if (data->features & GOBI_DMS)
 		sim_driver = "qmimodem-legacy";
-- 
2.11.0

[PATCH 3/4] MC7430: fix get signal strength

[Christophe Ronco]

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

drivers/qmimodem/nas.h
 Get current signal strength (type: 0x01), not list of other signals strength (0x10)
 Without this fix:
  - I can't get a signal strength on MC7430 because list does not exist (only one signal strength).
  - On MC7304, result is wrong
---
 drivers/qmimodem/nas.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/qmimodem/nas.h b/drivers/qmimodem/nas.h
index efc28735..dee9d701 100644
--- a/drivers/qmimodem/nas.h
+++ b/drivers/qmimodem/nas.h
@@ -63,7 +63,7 @@ struct qmi_nas_rf_info {
 } __attribute__((__packed__));
 
 /* Get the signal strength */
-#define QMI_NAS_RESULT_SIGNAL_STRENGTH		0x10
+#define QMI_NAS_RESULT_SIGNAL_STRENGTH		0x01
 
 /* Scan for visible network */
 #define QMI_NAS_PARAM_NETWORK_MASK		0x10	/* uint8 bitmask */
-- 
2.11.0

[PATCH 4/4] MC7430: fix QMI notification messages handling

[Christophe Ronco]

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

QMI notification messages handlers are never called on MC7430 without this fix.

drivers/qmimodem/qmi.c
 Do not test transaction id before calling notification handler.
 On MC7430, notification messages contain a not null transaction id
 (starts with 1, increased at each message for a particular client).
 On MC7304 transaction id in notification messages is always 0.
---
 drivers/qmimodem/qmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 3389bb1f..0080f250 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -758,7 +758,7 @@ static void handle_packet(struct qmi_device *device,
 
 		tid = GUINT16_FROM_LE(service->transaction);
 
-		if (service->type == 0x04 && tid == 0x0000) {
+		if (service->type == 0x04) {
 			handle_indication(device, hdr->service, hdr->client,
 							message, length, data);
 			return;
-- 
2.11.0

Re: [PATCH 2/4] MC7430: add read_imsi feature to qmimodem sim driver

[Denis Kenzior]

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

Hi Christophe,

On 12/28/2016 09:04 AM, Christophe Ronco wrote:
> drivers/qmimodem/sim.c
>  Add read_imsi feature to this driver. This is based on DMS service.
>  This is mandatory to be able to use this driver for GPRS connection
> plugins/gobi.c
>  DMS feature is needed for SIM driver (to be able to read IMSI)

Its been a while since I looked into QMI.  Marcel did most of the work, 
but I remember the basics.  So correct me if some of my understanding is 
incorrect.

My understanding was that the legacy driver was used for modems that 
didn't support the UIM service.  Since UIM doesn't have a dedicated IMSI 
getter, we added support to read EFimsi directly via low-level SIM 
reads.  See commit bc38ef91cd35935dc7a0d6eec31d9880e39a6bd5

This worked on a bunch of QMI based devices, so I have to assume that 
this fails on Sierra?  If so, then this probably should be quirked to be 
Sierra-specific behavior.  E.g. via a mechanism similar to 
drivers/atmodem/vendor.h

> ---
>  drivers/qmimodem/sim.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++---
>  plugins/gobi.c         |  2 +-
>  2 files changed, 79 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
> index fa7bef58..f61466d5 100644
> --- a/drivers/qmimodem/sim.c
> +++ b/drivers/qmimodem/sim.c
> @@ -30,6 +30,7 @@
>  #include <ofono/sim.h>
>
>  #include "qmi.h"
> +#include "dms.h"
>  #include "uim.h"
>
>  #include "qmimodem.h"
> @@ -39,6 +40,8 @@
>  #define EF_STATUS_VALID 1
>
>  struct sim_data {
> +	struct qmi_device *qmi_dev;
> +	struct qmi_service *dms;
>  	struct qmi_service *uim;
>  	uint32_t event_mask;
>  	uint8_t card_state;
> @@ -295,6 +298,47 @@ error:
>  	g_free(cbd);
>  }
>
> +static void get_imsi_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_sim_imsi_cb_t cb = cbd->cb;
> +	char *str;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, NULL)) {
> +		CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
> +		return;
> +	}
> +
> +	str = qmi_result_get_string(result, QMI_DMS_RESULT_IMSI);
> +	if (!str) {
> +		CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
> +		return;
> +	}
> +
> +	CALLBACK_WITH_SUCCESS(cb, str, cbd->data);
> +
> +	qmi_free(str);
> +}
> +
> +static void qmi_read_imsi(struct ofono_sim *sim,
> +				ofono_sim_imsi_cb_t cb, void *user_data)
> +{
> +	struct sim_data *data = ofono_sim_get_data(sim);
> +	struct cb_data *cbd = cb_data_new(cb, user_data);
> +
> +	DBG("");
> +
> +	if (qmi_service_send(data->dms, QMI_DMS_GET_IMSI, NULL,
> +					get_imsi_cb, cbd, g_free) > 0)
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
> +
> +	g_free(cbd);
> +}
> +
>  static void qmi_query_passwd_state(struct ofono_sim *sim,
>  				ofono_sim_passwd_cb_t cb, void *user_data)
>  {
> @@ -464,11 +508,30 @@ static void create_uim_cb(struct qmi_service *service, void *user_data)
>  		return;
>
>  error:
> -	qmi_service_unref(data->uim);
> +	qmi_service_unref(data->dms);
>
>  	ofono_sim_remove(sim);

ofono_sim_remove will call .remove method.  So this is likely to cause a 
crash since uim and dms have already been unref-ed.

>  }
>
> +static void create_dms_cb(struct qmi_service *service, void *user_data)
> +{
> +	struct ofono_sim *sim = user_data;
> +	struct sim_data *data = ofono_sim_get_data(sim);
> +	struct qmi_param *param;
> +
> +	DBG("");
> +
> +	if (!service) {
> +		ofono_error("Failed to request DMS service");
> +		ofono_sim_remove(sim);
> +		return;
> +	}
> +
> +	data->dms = qmi_service_ref(service);
> +
> +	qmi_service_create(data->qmi_dev, QMI_SERVICE_UIM, create_uim_cb, sim, NULL);
> +}
> +
>  static int qmi_sim_probe(struct ofono_sim *sim,
>  				unsigned int vendor, void *user_data)
>  {
> @@ -485,9 +548,12 @@ static int qmi_sim_probe(struct ofono_sim *sim,
>  	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
>  		data->retries[i] = -1;
>
> +	data->qmi_dev = device;
> +
>  	ofono_sim_set_data(sim, data);
>
> -	qmi_service_create(device, QMI_SERVICE_UIM, create_uim_cb, sim, NULL);
> +	qmi_service_create_shared(device, QMI_SERVICE_DMS,
> +						create_dms_cb, sim, NULL);
>
>  	return 0;
>  }
> @@ -500,9 +566,15 @@ static void qmi_sim_remove(struct ofono_sim *sim)
>
>  	ofono_sim_set_data(sim, NULL);
>
> -	qmi_service_unregister_all(data->uim);
> -
> -	qmi_service_unref(data->uim);
> +	if (data->uim) {
> +		qmi_service_unregister_all(data->uim);
> +		qmi_service_unref(data->uim);
> +		data->uim = NULL;
> +	}
> +	if (data->dms) {
> +		qmi_service_unregister_all(data->dms);
> +		qmi_service_unref(data->dms);
> +	}
>
>  	g_free(data);
>  }
> @@ -515,6 +587,7 @@ static struct ofono_sim_driver driver = {
>  	.read_file_transparent	= qmi_read_transparent,
>  	.read_file_linear	= qmi_read_record,
>  	.read_file_cyclic	= qmi_read_record,
> +	.read_imsi		= qmi_read_imsi,
>  	.query_passwd_state	= qmi_query_passwd_state,
>  	.query_pin_retries	= qmi_query_pin_retries,
>  };
> diff --git a/plugins/gobi.c b/plugins/gobi.c
> index 6a789411..84a98279 100644
> --- a/plugins/gobi.c
> +++ b/plugins/gobi.c

This should be in a separate patch, see HACKING, 'Submitting patches' 
section

> @@ -420,7 +420,7 @@ static void gobi_pre_sim(struct ofono_modem *modem)
>
>  	ofono_devinfo_create(modem, 0, "qmimodem", data->device);
>
> -	if (data->features & GOBI_UIM)
> +	if ((data->features & GOBI_UIM) && (data->features & GOBI_DMS))
>  		sim_driver = "qmimodem";
>  	else if (data->features & GOBI_DMS)
>  		sim_driver = "qmimodem-legacy";
>

Regards,
-Denis

Re: [PATCH 3/4] MC7430: fix get signal strength

[Denis Kenzior]

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

Hi Christophe,

On 12/28/2016 09:04 AM, Christophe Ronco wrote:
> drivers/qmimodem/nas.h
>  Get current signal strength (type: 0x01), not list of other signals strength (0x10)
>  Without this fix:
>   - I can't get a signal strength on MC7430 because list does not exist (only one signal strength).
>   - On MC7304, result is wrong
> ---
>  drivers/qmimodem/nas.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Been a while since I looked at QMI, but this looks correct to me. 
Applied, thanks!

Regards,
-Denis

Re: [PATCH 4/4] MC7430: fix QMI notification messages handling

[Denis Kenzior]

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

Hi Christophe,

On 12/28/2016 09:04 AM, Christophe Ronco wrote:
> QMI notification messages handlers are never called on MC7430 without this fix.
>
> drivers/qmimodem/qmi.c
>  Do not test transaction id before calling notification handler.
>  On MC7430, notification messages contain a not null transaction id
>  (starts with 1, increased at each message for a particular client).
>  On MC7304 transaction id in notification messages is always 0.
> ---
>  drivers/qmimodem/qmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Applied, thanks!

Regards,
-Denis

Re: [PATCH 1/4] MC7430: use qmimodem sim driver

[Denis Kenzior]

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

Hi Christophe,

On 12/28/2016 09:03 AM, Christophe Ronco wrote:
> QMI modem sim-legacy driver uses a command not supported by MC7430 (QMI_DMS_GET_PIN_STATUS)
>
> plugins/udevng.c
>  QMI Sierra modems use sim driver.
> drivers/qmimodem/sim.c
>  In get_card_status_cb, call ofono_sim_register after ofono_sim_inserted_notify (as it is done in sim_legacy).
>  Otherwise process crash (sim->driver->query_facility_lock called and not defined in this driver) with  MC7430 modem using this driver.

This seems to be caused by a bug in the core.  However, looking at 
src/sim.c ofono_sim_inserted_notify(), I see that the 
query_facility_lock call is guarded:

                 if (sim->driver->query_facility_lock) {
                         sim->driver->query_facility_lock(sim,
                                         OFONO_SIM_PASSWORD_PHSIM_PIN,
                                         sim_query_fac_imsilock_cb, sim);

                 } else {
                         sim_initialize(sim);
                 }

Are you using latest upstream code?  Any chance you can provide a backtrace?

> ---
>  drivers/qmimodem/sim.c | 3 +--
>  plugins/udevng.c       | 2 --

These need to be 2 separate commits.  See HACKING, 'Submitting patches' 
section.

>  2 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
> index 197da509..fa7bef58 100644
> --- a/drivers/qmimodem/sim.c
> +++ b/drivers/qmimodem/sim.c
> @@ -403,8 +403,6 @@ static void get_card_status_cb(struct qmi_result *result, void *user_data)
>  	}
>
>  done:
> -	ofono_sim_register(sim);
> -
>  	switch (data->card_state) {
>  	case 0x00:	/* Absent */
>  	case 0x02:	/* Error */
> @@ -413,6 +411,7 @@ done:
>  		ofono_sim_inserted_notify(sim, TRUE);
>  		break;
>  	}
> +	ofono_sim_register(sim);

This seems to be working around a bug in the core (src/sim.c).  See above.

>  }
>
>  static void event_registration_cb(struct qmi_result *result, void *user_data)
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index 933bf4fa..a2866b64 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -252,8 +252,6 @@ static gboolean setup_sierra(struct modem_info *modem)
>
>  	if (qmi != NULL && net != NULL) {
>  		ofono_modem_set_driver(modem->modem, "gobi");
> -		/* Fixup SIM interface for Sierra QMI devices */
> -		ofono_modem_set_boolean(modem->modem, "ForceSimLegacy", TRUE);
>  		goto done;
>  	}
>
>

Regards,
-Denis

Re: [PATCH 2/4] MC7430: add read_imsi feature to qmimodem sim driver

[Christophe Ronco]

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

Hello Denis,

Thanks for reading my patches. I resend this mail (previous was sent 
with a wrong from address so it did not go to the list).

On 12/31/2016 01:34 AM, Denis Kenzior wrote:
> Hi Christophe,
>
> On 12/28/2016 09:04 AM, Christophe Ronco wrote:
>> drivers/qmimodem/sim.c
>>  Add read_imsi feature to this driver. This is based on DMS service.
>>  This is mandatory to be able to use this driver for GPRS connection
>> plugins/gobi.c
>>  DMS feature is needed for SIM driver (to be able to read IMSI)
>
> Its been a while since I looked into QMI.  Marcel did most of the 
> work, but I remember the basics.  So correct me if some of my 
> understanding is incorrect.
>
> My understanding was that the legacy driver was used for modems that 
> didn't support the UIM service.  Since UIM doesn't have a dedicated 
> IMSI getter, we added support to read EFimsi directly via low-level 
> SIM reads.  See commit bc38ef91cd35935dc7a0d6eec31d9880e39a6bd5
>
> This worked on a bunch of QMI based devices, so I have to assume that 
> this fails on Sierra?  If so, then this probably should be quirked to 
> be Sierra-specific behavior.  E.g. via a mechanism similar to 
> drivers/atmodem/vendor.h
IMSI reading on my MC7430 modem do fail without this patch.
I have the following traces in this case:
Jan  5 11:08:41 klk-lpbs_040070 daemon.debug ofonod[869]: 
../ofono-1.19/drivers/qmimodem/sim.c:qmi_read_transparent() file id 
0x6f07 path len 0
Jan  5 11:08:41 klk-lpbs_040070 daemon.debug ofonod[869]: 
../ofono-1.19/drivers/qmimodem/sim.c:read_generic_cb()
Jan  5 11:08:41 klk-lpbs_040070 daemon.err ofonod[869]: Unable to read 
IMSI, emergency calls only

So read_file_transparent is asked by sim_retrieve_imsi and it seems to 
fail. I don't know why.

After a quick look at the code:
All QMI modems supported by Ofono support DMS feature. There are all 
created by GOBI plugin and in this plugin, DMS feature is mandatory 
(discover_cb will shutdown device if DMS feature is not said to be 
supported).
So I assume that most of the QMI modems will support QMI_DMS_GET_IMSI.

What I would like to do for QMI modem is to always try QMI_DMS_GET_IMSI 
(not only for Sierra modems) and if it fails, try to obtain IMSI via EF 
reads.

I have done a patch to do that (see diff above). Let me know if you 
think that's a good idea. In this case, I will send two patches:
  - backup read_imsi by IMSI via EF read
  - add read_imsi to qmimodem sim driver

diff --git a/src/sim.c b/src/sim.c
index edd7c763..66794305 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1472,19 +1472,6 @@ static void sim_imsi_obtained(struct ofono_sim 
*sim, const char *imsi)

  }

-static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,
-                       void *data)
-{
-       struct ofono_sim *sim = data;
-
-       if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-               ofono_error("Unable to read IMSI, emergency calls only");
-               return;
-       }
-
-       sim_imsi_obtained(sim, imsi);
-}
-
  static void sim_efimsi_cb(const struct ofono_error *error,
                                 const unsigned char *data, int len, 
void *user)
  {
@@ -1524,6 +1511,26 @@ error:
         ofono_error("Unable to read IMSI, emergency calls only");
  }

+static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,
+                       void *data)
+{
+       struct ofono_sim *sim = data;
+
+       if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
+               sim_imsi_obtained(sim, imsi);
+               return;
+       }
+
+       /* Driver function failed, try via EF reads if possible */
+       if (sim->driver->read_file_transparent == NULL) {
+               ofono_error("Unable to read IMSI, emergency calls only");
+               return;
+       }
+
+       sim->driver->read_file_transparent(sim, SIM_EFIMSI_FILEID, 0, 9,
+                                               NULL, 0, sim_efimsi_cb, 
sim);
+}
+
  static void sim_retrieve_imsi(struct ofono_sim *sim)
  {
         if (sim->driver->read_imsi) {



>
>> ---
>>  drivers/qmimodem/sim.c | 83 
>> +++++++++++++++++++++++++++++++++++++++++++++++---
>>  plugins/gobi.c         |  2 +-
>>  2 files changed, 79 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
>> index fa7bef58..f61466d5 100644
>> --- a/drivers/qmimodem/sim.c
>> +++ b/drivers/qmimodem/sim.c
>> @@ -30,6 +30,7 @@
>>  #include <ofono/sim.h>
>>
>>  #include "qmi.h"
>> +#include "dms.h"
>>  #include "uim.h"
>>
>>  #include "qmimodem.h"
>> @@ -39,6 +40,8 @@
>>  #define EF_STATUS_VALID 1
>>
>>  struct sim_data {
>> +    struct qmi_device *qmi_dev;
>> +    struct qmi_service *dms;
>>      struct qmi_service *uim;
>>      uint32_t event_mask;
>>      uint8_t card_state;
>> @@ -295,6 +298,47 @@ error:
>>      g_free(cbd);
>>  }
>>
>> +static void get_imsi_cb(struct qmi_result *result, void *user_data)
>> +{
>> +    struct cb_data *cbd = user_data;
>> +    ofono_sim_imsi_cb_t cb = cbd->cb;
>> +    char *str;
>> +
>> +    DBG("");
>> +
>> +    if (qmi_result_set_error(result, NULL)) {
>> +        CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
>> +        return;
>> +    }
>> +
>> +    str = qmi_result_get_string(result, QMI_DMS_RESULT_IMSI);
>> +    if (!str) {
>> +        CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
>> +        return;
>> +    }
>> +
>> +    CALLBACK_WITH_SUCCESS(cb, str, cbd->data);
>> +
>> +    qmi_free(str);
>> +}
>> +
>> +static void qmi_read_imsi(struct ofono_sim *sim,
>> +                ofono_sim_imsi_cb_t cb, void *user_data)
>> +{
>> +    struct sim_data *data = ofono_sim_get_data(sim);
>> +    struct cb_data *cbd = cb_data_new(cb, user_data);
>> +
>> +    DBG("");
>> +
>> +    if (qmi_service_send(data->dms, QMI_DMS_GET_IMSI, NULL,
>> +                    get_imsi_cb, cbd, g_free) > 0)
>> +        return;
>> +
>> +    CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
>> +
>> +    g_free(cbd);
>> +}
>> +
>>  static void qmi_query_passwd_state(struct ofono_sim *sim,
>>                  ofono_sim_passwd_cb_t cb, void *user_data)
>>  {
>> @@ -464,11 +508,30 @@ static void create_uim_cb(struct qmi_service 
>> *service, void *user_data)
>>          return;
>>
>>  error:
>> -    qmi_service_unref(data->uim);
>> +    qmi_service_unref(data->dms);
>>
>>      ofono_sim_remove(sim);
>
> ofono_sim_remove will call .remove method.  So this is likely to cause 
> a crash since uim and dms have already been unref-ed.
OK, I have not tested this error case. The 
"qmi_service_unref(data->uim)" that's here in this function without the 
patch should cause the same crash.
>
>>  }
>>
>> +static void create_dms_cb(struct qmi_service *service, void *user_data)
>> +{
>> +    struct ofono_sim *sim = user_data;
>> +    struct sim_data *data = ofono_sim_get_data(sim);
>> +    struct qmi_param *param;
>> +
>> +    DBG("");
>> +
>> +    if (!service) {
>> +        ofono_error("Failed to request DMS service");
>> +        ofono_sim_remove(sim);
>> +        return;
>> +    }
>> +
>> +    data->dms = qmi_service_ref(service);
>> +
>> +    qmi_service_create(data->qmi_dev, QMI_SERVICE_UIM, 
>> create_uim_cb, sim, NULL);
>> +}
>> +
>>  static int qmi_sim_probe(struct ofono_sim *sim,
>>                  unsigned int vendor, void *user_data)
>>  {
>> @@ -485,9 +548,12 @@ static int qmi_sim_probe(struct ofono_sim *sim,
>>      for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
>>          data->retries[i] = -1;
>>
>> +    data->qmi_dev = device;
>> +
>>      ofono_sim_set_data(sim, data);
>>
>> -    qmi_service_create(device, QMI_SERVICE_UIM, create_uim_cb, sim, 
>> NULL);
>> +    qmi_service_create_shared(device, QMI_SERVICE_DMS,
>> +                        create_dms_cb, sim, NULL);
>>
>>      return 0;
>>  }
>> @@ -500,9 +566,15 @@ static void qmi_sim_remove(struct ofono_sim *sim)
>>
>>      ofono_sim_set_data(sim, NULL);
>>
>> -    qmi_service_unregister_all(data->uim);
>> -
>> -    qmi_service_unref(data->uim);
>> +    if (data->uim) {
>> +        qmi_service_unregister_all(data->uim);
>> +        qmi_service_unref(data->uim);
>> +        data->uim = NULL;
>> +    }
>> +    if (data->dms) {
>> +        qmi_service_unregister_all(data->dms);
>> +        qmi_service_unref(data->dms);
>> +    }
>>
>>      g_free(data);
>>  }
>> @@ -515,6 +587,7 @@ static struct ofono_sim_driver driver = {
>>      .read_file_transparent    = qmi_read_transparent,
>>      .read_file_linear    = qmi_read_record,
>>      .read_file_cyclic    = qmi_read_record,
>> +    .read_imsi        = qmi_read_imsi,
>>      .query_passwd_state    = qmi_query_passwd_state,
>>      .query_pin_retries    = qmi_query_pin_retries,
>>  };
>> diff --git a/plugins/gobi.c b/plugins/gobi.c
>> index 6a789411..84a98279 100644
>> --- a/plugins/gobi.c
>> +++ b/plugins/gobi.c
>
> This should be in a separate patch, see HACKING, 'Submitting patches' 
> section
OK.

>
>> @@ -420,7 +420,7 @@ static void gobi_pre_sim(struct ofono_modem *modem)
>>
>>      ofono_devinfo_create(modem, 0, "qmimodem", data->device);
>>
>> -    if (data->features & GOBI_UIM)
>> +    if ((data->features & GOBI_UIM) && (data->features & GOBI_DMS))
>>          sim_driver = "qmimodem";
>>      else if (data->features & GOBI_DMS)
>>          sim_driver = "qmimodem-legacy";
>>
>
> Regards,
> -Denis
>

Re: [PATCH 2/4] MC7430: add read_imsi feature to qmimodem sim driver

[Denis Kenzior]

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

Hi Christophe,

>
> What I would like to do for QMI modem is to always try QMI_DMS_GET_IMSI
> (not only for Sierra modems) and if it fails, try to obtain IMSI via EF
> reads.

So you want the core to try both read_imsi and read_transparent in 
sequence to obtain the IMSI.  This isn't really ideal, as the driver 
should really implement one method or the other.

Do we know whether the Sierra devices actually support raw SIM reads? 
Maybe the QMI commands need to be formatted specifically for Sierra 
devices?  Ideal situation would be to get read_transparent to work for 
obtaining the IMSI.

If the above can't be made to work, then we should probably just add 
another QMI based sim driver for these devices.  E.g. one that supports 
read_imsi via DMS and PIN operations via UIM.

>
> I have done a patch to do that (see diff above). Let me know if you
> think that's a good idea. In this case, I will send two patches:
>  - backup read_imsi by IMSI via EF read

Can you send this patch properly via git send-email.  I might be 
convinced to apply it.

Regards,
-Denis

[PATCH 0/2] Fix Read Imsi feature for MC7430 modem

[Christophe Ronco]

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

Done in 2 patches:
 - Backup read imsi modem driver funtion by IMSI via EF read (what is done
now when read_insi is not defined in modem driver)
 - Implement read_imsi in qmimodem sim driver

Christophe Ronco (2):
  sim: backup driver read_imsi by IMSI via EF read
  qmimodem: Add read_imsi to qmimodem sim driver

 drivers/qmimodem/sim.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++----
 src/sim.c              | 33 ++++++++++++--------
 2 files changed, 97 insertions(+), 19 deletions(-)

-- 
2.11.0

[PATCH 1/2] sim: backup driver read_imsi by IMSI via EF read

[Christophe Ronco]

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

If read_imsi driver function fails, try to obtain IMSI via EF read
---
 src/sim.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index edd7c763..66794305 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1472,19 +1472,6 @@ static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
 
 }
 
-static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,
-			void *data)
-{
-	struct ofono_sim *sim = data;
-
-	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-		ofono_error("Unable to read IMSI, emergency calls only");
-		return;
-	}
-
-	sim_imsi_obtained(sim, imsi);
-}
-
 static void sim_efimsi_cb(const struct ofono_error *error,
 				const unsigned char *data, int len, void *user)
 {
@@ -1524,6 +1511,26 @@ error:
 	ofono_error("Unable to read IMSI, emergency calls only");
 }
 
+static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,
+			void *data)
+{
+	struct ofono_sim *sim = data;
+
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
+		sim_imsi_obtained(sim, imsi);
+		return;
+	}
+
+	/* Driver function failed, try via EF reads if possible */
+	if (sim->driver->read_file_transparent == NULL) {
+		ofono_error("Unable to read IMSI, emergency calls only");
+		return;
+	}
+
+	sim->driver->read_file_transparent(sim, SIM_EFIMSI_FILEID, 0, 9,
+						NULL, 0, sim_efimsi_cb, sim);
+}
+
 static void sim_retrieve_imsi(struct ofono_sim *sim)
 {
 	if (sim->driver->read_imsi) {
-- 
2.11.0

[PATCH 2/2] qmimodem: Add read_imsi to qmimodem sim driver

[Christophe Ronco]

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

Add read_imsi feature to qmimodem sim driver.
This is based on DMS service.
On MC7430, this is mandatory to be able to use this driver for GPRS
connection because reading IMSI via EF reads fails.
---
 drivers/qmimodem/sim.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 77 insertions(+), 6 deletions(-)

diff --git a/drivers/qmimodem/sim.c b/drivers/qmimodem/sim.c
index 197da509..db012bcf 100644
--- a/drivers/qmimodem/sim.c
+++ b/drivers/qmimodem/sim.c
@@ -30,6 +30,7 @@
 #include <ofono/sim.h>
 
 #include "qmi.h"
+#include "dms.h"
 #include "uim.h"
 
 #include "qmimodem.h"
@@ -39,6 +40,8 @@
 #define EF_STATUS_VALID 1
 
 struct sim_data {
+	struct qmi_device *qmi_dev;
+	struct qmi_service *dms;
 	struct qmi_service *uim;
 	uint32_t event_mask;
 	uint8_t card_state;
@@ -295,6 +298,47 @@ error:
 	g_free(cbd);
 }
 
+static void get_imsi_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_sim_imsi_cb_t cb = cbd->cb;
+	char *str;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, NULL)) {
+		CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
+		return;
+	}
+
+	str = qmi_result_get_string(result, QMI_DMS_RESULT_IMSI);
+	if (!str) {
+		CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
+		return;
+	}
+
+	CALLBACK_WITH_SUCCESS(cb, str, cbd->data);
+
+	qmi_free(str);
+}
+
+static void qmi_read_imsi(struct ofono_sim *sim,
+				ofono_sim_imsi_cb_t cb, void *user_data)
+{
+	struct sim_data *data = ofono_sim_get_data(sim);
+	struct cb_data *cbd = cb_data_new(cb, user_data);
+
+	DBG("");
+
+	if (qmi_service_send(data->dms, QMI_DMS_GET_IMSI, NULL,
+					get_imsi_cb, cbd, g_free) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
+
+	g_free(cbd);
+}
+
 static void qmi_query_passwd_state(struct ofono_sim *sim,
 				ofono_sim_passwd_cb_t cb, void *user_data)
 {
@@ -465,11 +509,28 @@ static void create_uim_cb(struct qmi_service *service, void *user_data)
 		return;
 
 error:
-	qmi_service_unref(data->uim);
-
 	ofono_sim_remove(sim);
 }
 
+static void create_dms_cb(struct qmi_service *service, void *user_data)
+{
+	struct ofono_sim *sim = user_data;
+	struct sim_data *data = ofono_sim_get_data(sim);
+
+	DBG("");
+
+	if (!service) {
+		ofono_error("Failed to request DMS service");
+		ofono_sim_remove(sim);
+		return;
+	}
+
+	data->dms = qmi_service_ref(service);
+
+	qmi_service_create(data->qmi_dev, QMI_SERVICE_UIM, create_uim_cb, sim,
+					NULL);
+}
+
 static int qmi_sim_probe(struct ofono_sim *sim,
 				unsigned int vendor, void *user_data)
 {
@@ -486,9 +547,12 @@ static int qmi_sim_probe(struct ofono_sim *sim,
 	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
 		data->retries[i] = -1;
 
+	data->qmi_dev = device;
+
 	ofono_sim_set_data(sim, data);
 
-	qmi_service_create(device, QMI_SERVICE_UIM, create_uim_cb, sim, NULL);
+	qmi_service_create_shared(device, QMI_SERVICE_DMS,
+						create_dms_cb, sim, NULL);
 
 	return 0;
 }
@@ -501,9 +565,15 @@ static void qmi_sim_remove(struct ofono_sim *sim)
 
 	ofono_sim_set_data(sim, NULL);
 
-	qmi_service_unregister_all(data->uim);
-
-	qmi_service_unref(data->uim);
+	if (data->uim) {
+		qmi_service_unregister_all(data->uim);
+		qmi_service_unref(data->uim);
+		data->uim = NULL;
+	}
+	if (data->dms) {
+		qmi_service_unregister_all(data->dms);
+		qmi_service_unref(data->dms);
+	}
 
 	g_free(data);
 }
@@ -516,6 +586,7 @@ static struct ofono_sim_driver driver = {
 	.read_file_transparent	= qmi_read_transparent,
 	.read_file_linear	= qmi_read_record,
 	.read_file_cyclic	= qmi_read_record,
+	.read_imsi		= qmi_read_imsi,
 	.query_passwd_state	= qmi_query_passwd_state,
 	.query_pin_retries	= qmi_query_pin_retries,
 };
-- 
2.11.0

Re: [PATCH 0/2] Fix Read Imsi feature for MC7430 modem

[Denis Kenzior]

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

Hi Christophe,

On 01/09/2017 02:34 AM, Christophe Ronco wrote:
> Done in 2 patches:
>  - Backup read imsi modem driver funtion by IMSI via EF read (what is done
> now when read_insi is not defined in modem driver)
>  - Implement read_imsi in qmimodem sim driver
>
> Christophe Ronco (2):
>   sim: backup driver read_imsi by IMSI via EF read
>   qmimodem: Add read_imsi to qmimodem sim driver
>
>  drivers/qmimodem/sim.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++----
>  src/sim.c              | 33 ++++++++++++--------
>  2 files changed, 97 insertions(+), 19 deletions(-)
>

Both applied, thanks!

Regards,
-Denis