[PATCH] telit-plugin: ignore QSS when SIM-ME interface is locked
Aleksander Morgado
aleksander at aleksander.es
Tue Jul 25 12:10:32 UTC 2017
Hey,
On Mon, Jul 24, 2017 at 6:23 PM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> With some modems, the lock/unlock of the SIM-ME interface with +CSIM=1/0
> command is followed by #QSS unsolicited messages. With the current
> implementation, this messages are mistaken for SIM swap events and so the
> modem is first dropped and then re-probed.
>
> With this patch, the plugin takes into account the SIM-ME lock state when
> parsing #QSS unsolicited, so that the QSS handler can correctly
> elaborate the messages that are not related to SIM swap events.
> ---
> plugins/telit/mm-broadband-modem-telit.c | 90 ++++++++++++++++++++++++++++----
> plugins/telit/mm-modem-helpers-telit.h | 9 ++++
> 2 files changed, 90 insertions(+), 9 deletions(-)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c
> index e7650c0..3facc3e 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -44,6 +44,8 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemTelit, mm_broadband_modem_telit, MM_TYPE
> G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM, iface_modem_init)
> G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_3GPP, iface_modem_3gpp_init));
>
> +#define CSIM_UNLOCK_MAX_TIMEOUT 3
> +
> typedef enum {
> FEATURE_SUPPORT_UNKNOWN,
> FEATURE_NOT_SUPPORTED,
> @@ -53,6 +55,9 @@ typedef enum {
> struct _MMBroadbandModemTelitPrivate {
> FeatureSupport csim_lock_support;
> MMTelitQssStatus qss_status;
> + MMTelitCsimLockState csim_lock_state;
> + GTask *csim_lock_task;
> + guint csim_lock_timeout_id;
> };
>
> /*****************************************************************************/
> @@ -107,6 +112,7 @@ typedef struct {
> } QssSetupContext;
>
> static void qss_setup_step (GTask *task);
> +static void csim_unlock_complete (GTask *task);
>
> static void
> telit_qss_unsolicited_handler (MMPortSerialAt *port,
> @@ -122,14 +128,36 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,
> prev_qss_status = self->priv->qss_status;
> self->priv->qss_status = cur_qss_status;
>
> + if (self->priv->csim_lock_state >= CSIM_LOCK_STATE_LOCK_REQUESTED) {
> +
> + if (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status == QSS_STATUS_SIM_REMOVED) {
> + mm_dbg ("QSS handler: #QSS=0 after +CSIM=1 -> CSIM locked!");
> + self->priv->csim_lock_state = CSIM_LOCK_STATE_LOCKED;
> + }
> +
> + if (prev_qss_status == QSS_STATUS_SIM_REMOVED && cur_qss_status != QSS_STATUS_SIM_REMOVED) {
> + mm_dbg ("QSS handler: #QSS>=1 after +CSIM=0 -> CSIM unlocked!");
> + self->priv->csim_lock_state = CSIM_LOCK_STATE_UNLOCKED;
> +
> + if (self->priv->csim_lock_timeout_id) {
> + g_source_remove (self->priv->csim_lock_timeout_id);
> + self->priv->csim_lock_timeout_id = 0;
> + }
> +
> + csim_unlock_complete (self->priv->csim_lock_task);
Reset the csim_lock_task pointer here to NULL, please.
> + }
> +
> + return;
> + }
> +
> if (cur_qss_status != prev_qss_status)
> - mm_dbg ("QSS: status changed '%s -> %s",
> + mm_dbg ("QSS handler: status changed '%s -> %s",
> mm_telit_qss_status_get_string (prev_qss_status),
> mm_telit_qss_status_get_string (cur_qss_status));
>
> if ((prev_qss_status == QSS_STATUS_SIM_REMOVED && cur_qss_status != QSS_STATUS_SIM_REMOVED) ||
> (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status == QSS_STATUS_SIM_REMOVED)) {
> - mm_info ("QSS: SIM swap detected");
> + mm_info ("QSS handler: SIM swap detected");
> mm_broadband_modem_update_sim_hot_swap_detected (MM_BROADBAND_MODEM (self));
> }
> }
> @@ -610,8 +638,9 @@ csim_unlock_ready (MMBaseModem *_self,
> if (!response) {
> if (g_error_matches (error,
> MM_MOBILE_EQUIPMENT_ERROR,
> - MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED))
> + MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED)) {
> self->priv->csim_lock_support = FEATURE_NOT_SUPPORTED;
> + }
> mm_warn ("Couldn't unlock SIM card: %s", error->message);
> g_error_free (error);
> }
> @@ -703,6 +732,8 @@ csim_lock_ready (MMBaseModem *_self,
> g_object_unref (task);
> return;
> }
> + } else {
> + self->priv->csim_lock_state = CSIM_LOCK_STATE_LOCK_REQUESTED;
> }
>
> if (self->priv->csim_lock_support != FEATURE_NOT_SUPPORTED) {
> @@ -755,6 +786,42 @@ handle_csim_locking (GTask *task,
> }
>
> static void
> +csim_unlock_complete (GTask *task)
> +{
> + MMBroadbandModemTelit *self;
> + LoadUnlockRetriesContext *ctx;
> +
> + self = MM_BROADBAND_MODEM_TELIT (g_task_get_source_object (task));
> +
> + ctx = g_task_get_task_data (task);
> + if (ctx->succeded_requests == 0) {
> + g_task_return_new_error (task, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not get any of the SIM unlock retries values");
> + } else {
> + g_task_return_pointer (task, g_object_ref (ctx->retries), g_object_unref);
> + }
> +
> + g_object_unref (task);
> +}
> +
> +static gboolean
> +csim_unlock_periodic_check (GTask *task)
> +{
> + MMBroadbandModemTelit *self;
Not sure you need to receive the GTask here from the timeout
user_data, as you already have it in the private info. Instead, you
could pass a reference to the MMBroadbandModemTelit object as
user_data and receive "self" here.
> +
> + self = MM_BROADBAND_MODEM_TELIT (g_task_get_source_object (task));
> +
> + if (self->priv->csim_lock_state != CSIM_LOCK_STATE_UNLOCKED) {
> + mm_warn ("CSIM is still locked after %d seconds. Trying to continue anyway", CSIM_UNLOCK_MAX_TIMEOUT);
> + }
> +
> + self->priv->csim_lock_timeout_id = 0;
> + csim_unlock_complete (task);
Reset the self->priv->csim_lock_task pointer here to NULL, please.
> +
If you passed a full reference to the modem object, you would unref it here.
> + return G_SOURCE_REMOVE;
> +}
> +
> +static void
> load_unlock_retries_step (GTask *task)
> {
> MMBroadbandModemTelit *self;
> @@ -805,12 +872,14 @@ load_unlock_retries_step (GTask *task)
> handle_csim_locking (task, FALSE);
> break;
> case LOAD_UNLOCK_RETRIES_STEP_LAST:
> - if (ctx->succeded_requests == 0)
> - g_task_return_new_error (task, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> - "Could not get any of the SIM unlock retries values");
> - else
> - g_task_return_pointer (task, g_object_ref (ctx->retries), g_object_unref);
> - g_object_unref (task);
> + if (self->priv->csim_lock_state == CSIM_LOCK_STATE_LOCKED) {
> + mm_dbg ("CSIM is locked. Waiting for #QSS=1");
> + self->priv->csim_lock_task = task;
> + self->priv->csim_lock_timeout_id = g_timeout_add_seconds (CSIM_UNLOCK_MAX_TIMEOUT, (GSourceFunc) csim_unlock_periodic_check, task);
As said above this logic is right, but given that you're storing the
GTask in the private info, lets keep there the only valid reference to
the GTask, otherwise the ownership of the GTask would be shared
between the timeout and the private info
So, instead, why not pass a full reference to the Modem object (i.e.
g_object_ref (self)) as user_data in the timeout, and then unref that
explicitly before returning G_SOURCE_REMOVE in
csim_unlock_periodic_check(). This logic makes sure that the modem
object will exist as long as the timeout exists (well, this was
implicit already as we're using a GTask with a reference to the modem
object...).
> + } else {
> + self->priv->csim_lock_state = CSIM_LOCK_STATE_UNLOCKED;
> + csim_unlock_complete (task);
> + }
> break;
> default:
> break;
> @@ -1388,7 +1457,10 @@ mm_broadband_modem_telit_init (MMBroadbandModemTelit *self)
> MMBroadbandModemTelitPrivate);
>
> self->priv->csim_lock_support = FEATURE_SUPPORT_UNKNOWN;
> + self->priv->csim_lock_state = CSIM_LOCK_STATE_UNKNOWN;
> self->priv->qss_status = QSS_STATUS_UNKNOWN;
> + self->priv->csim_lock_task = NULL;
> + self->priv->csim_lock_timeout_id = 0;
No need to explicitly initialize the values to 0/NULL, just remove
those, the private info is always initially initialized to 0. Note
that for enums the approach is different, reading the code above we
don't know if the UNKNOWN was 0 or -1 or what, so it makes sense to
initialize the enum explicitly to a value even if that value may be 0.
> }
>
I was going to suggest adding a pair of g_assert()s in e.g. finalize()
to make sure that the timeout_id is 0 and the task pointer is NULL, as
otherwise it would mean there's a logic error somewhere, but we don't
have a finalize() defined, and just for this it probably doesn't make
much sense, so nevermind.
> static void
> diff --git a/plugins/telit/mm-modem-helpers-telit.h b/plugins/telit/mm-modem-helpers-telit.h
> index 24c39e0..1cf76f0 100644
> --- a/plugins/telit/mm-modem-helpers-telit.h
> +++ b/plugins/telit/mm-modem-helpers-telit.h
> @@ -110,4 +110,13 @@ typedef enum { /*< underscore_name=mm_telit_qss_status >*/
>
> MMTelitQssStatus mm_telit_parse_qss_query (const gchar *response, GError **error);
>
> +/* CSIM lock state */
> +typedef enum { /*< underscore_name=mm_telit_csim_lock_state >*/
> + CSIM_LOCK_STATE_UNKNOWN,
> + CSIM_LOCK_STATE_UNLOCKED,
> + CSIM_LOCK_STATE_LOCK_REQUESTED,
> + CSIM_LOCK_STATE_LOCKED,
> +} MMTelitCsimLockState;
> +
> +
> #endif /* MM_MODEM_HELPERS_TELIT_H */
> --
> 2.9.3
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list