[PATCH v4] telit-plugin: handle QSS unsolicited due to power state transitions

Aleksander Morgado aleksander at aleksander.es
Mon Sep 4 16:09:37 UTC 2017


Hey!

On Mon, Sep 4, 2017 at 8:13 AM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> When transitioning between power-low and power-on modes, Telit modems
> switch the SIM off/on, which leads to the emission of #QSS unsolicited not
> related to actual SIM swaps.
>
> To handle this #QSS unsolicited, this patch:
>
> * disables reacting on #QSS unsolicited when modem_power_down is received
> * implements modem_after_power_up that:
>     - checks whether the SIM has been changed, matching cached SIM
>       Identifier with the value in the current SIM. If SIM Identifier,
>       is different, sim hot swap ports detected is called.
>     - re-enables reacting on #QSS unsolicited
>
> ---
>
> Hi Aleksander,
>
> I should have fixed all the problems, but I'm still not sure about
> base-sim's implementation of mm_base_sim_load_sim_identifier and
> mm_base_sim_load_sim_identifier_finish.
>
> I saw that, generally, similar functions have their own ..._ready counterpart,
> and maybe a GTask that takes care of caller's callback,
> but I think I shouldn't need it and let the caller's callback do all the
> job. However, as I said, I'm not really sure this is right.
>

What you did is technically right, although I always prefer to avoid
putting much logic in the finish() method (as it's really duplicating
logic), so I always have the async method manage its own GTask. In
this way, the finish() method doesn't need to do extra checks like "do
I need to do special things based on how the GTask was created?" Using
its own GTask ends up requiring the extra _ready() method, but that
makes the flow much more clear and consistent I think.

I've pushed your patch to the "clobrano/qss-unsolicited-power" branch
in my github repo:
  https://github.com/aleksander0m/ModemManager/commits/clobrano/qss-unsolicited-power

I've fixed up several whitespace issues in your patch, and then pushed
several other commits on top. Could you give them a look to make sure
they're ok? The last patch does the own-GTask think discussed above.


> ---
>  plugins/telit/mm-broadband-modem-telit.c | 181 ++++++++++++++++++++++++++++++-
>  src/mm-base-sim.c                        |  51 +++++++++
>  src/mm-base-sim.h                        |  80 +++++++-------
>  3 files changed, 271 insertions(+), 41 deletions(-)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c
> index a9fc5f0..b30e7c9 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -58,6 +58,7 @@ struct _MMBroadbandModemTelitPrivate {
>      MMTelitCsimLockState csim_lock_state;
>      GTask *csim_lock_task;
>      guint csim_lock_timeout_id;
> +    gboolean parse_qss;
>  };
>
>  /*****************************************************************************/
> @@ -151,10 +152,15 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,
>      }
>
>      if (cur_qss_status != prev_qss_status)
> -        mm_dbg ("QSS handler: 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 (self->priv->parse_qss == FALSE) {
> +        mm_dbg ("QSS: message ignored");
> +        return;
> +    }
> +
>      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 handler: SIM swap detected");
> @@ -903,14 +909,175 @@ modem_load_unlock_retries (MMIfaceModem *self,
>  }
>
>  /*****************************************************************************/
> +/* Modem after power up (Modem interface) */
> +typedef enum {
> +    AFTER_POWER_UP_STEP_FIRST,
> +    AFTER_POWER_UP_STEP_GET_SIM_IDENTIFIER,
> +    AFTER_POWER_UP_STEP_ENABLE_QSS_PARSING,
> +    AFTER_POWER_UP_STEP_LAST
> +} ModemAfterPowerUpStep;
> +
> +typedef struct {
> +    gboolean has_sim_changed;
> +    guint retries;
> +    ModemAfterPowerUpStep step;
> +} AfterPowerUpContext;
> +
> +static void after_power_up_step (GTask *task);
> +
> +static gboolean
> +modem_after_power_up_finish (MMIfaceModem *self,
> +                             GAsyncResult *res,
> +                             GError **error)
> +{
> +    return g_task_propagate_boolean (G_TASK (res), error);
> +}
> +
> +static void
> +load_sim_identifier_ready (MMBaseSim *sim,
> +                           GAsyncResult *res,
> +                           GTask *task)
> +{
> +    AfterPowerUpContext *ctx;
> +    GError *error = NULL;
> +    gchar *current_simid;
> +    gchar *cached_simid;
> +
> +    ctx = g_task_get_task_data (task);
> +
> +    cached_simid = (gchar *)mm_gdbus_sim_get_sim_identifier (MM_GDBUS_SIM (sim));
> +    current_simid = mm_base_sim_load_sim_identifier_finish (sim, res, &error);
> +
> +    if (error) {
> +        if (g_error_matches (error, MM_CORE_ERROR, MM_CORE_ERROR_UNSUPPORTED)) {
> +            mm_warn ("could not load SIM identifier: %s", error->message);
> +            g_clear_error (&error);
> +            goto out;
> +        }
> +
> +        if (ctx->retries-- > 0) {
> +            mm_warn ("could not load SIM identifier: %s (%d retries left)",
> +                     error->message, ctx->retries);
> +            g_clear_error (&error);
> +            g_timeout_add_seconds (1, (GSourceFunc) after_power_up_step, task);
> +            return;
> +        }
> +
> +        mm_warn ("could not load SIM identifier: %s", error->message);
> +        g_clear_error (&error);
> +        goto out;
> +    }
> +
> +    if (g_strcmp0 (current_simid, cached_simid) != 0) {
> +        mm_warn ("sim identifier has changed: possible SIM swap during power down/low");
> +        ctx->has_sim_changed = TRUE;
> +    }
> +
> +out:
> +    g_free (current_simid);
> +    ctx->step++;
> +    after_power_up_step (task);
> +}
> +
> +static void
> +after_power_up_step (GTask *task)
> +{
> +    AfterPowerUpContext *ctx;
> +    MMBroadbandModemTelit *self;
> +    MMBaseSim *sim = NULL;
> +
> +    ctx = g_task_get_task_data (task);
> +    self = g_task_get_source_object (task);
> +
> +    g_object_get (MM_BROADBAND_MODEM_TELIT (self),
> +                  MM_IFACE_MODEM_SIM, &sim,
> +                  NULL);
> +    if (!sim) {
> +        g_task_return_new_error (task,
> +                                 MM_CORE_ERROR,
> +                                 MM_CORE_ERROR_FAILED,
> +                                 "could not acquire sim object");
> +        return;
> +    }
> +
> +    switch (ctx->step) {
> +    case AFTER_POWER_UP_STEP_FIRST:
> +        /* Fall back on next step */
> +        ctx->step++;
> +    case AFTER_POWER_UP_STEP_GET_SIM_IDENTIFIER:
> +        mm_base_sim_load_sim_identifier (sim,
> +                                         (GAsyncReadyCallback)load_sim_identifier_ready,
> +                                         task);
> +        g_object_unref (sim);
> +        return;
> +    case AFTER_POWER_UP_STEP_ENABLE_QSS_PARSING:
> +        mm_dbg ("Stop ignoring #QSS");
> +        self->priv->parse_qss = TRUE;
> +
> +        /* Fall back on next step */
> +        ctx->step++;
> +    case AFTER_POWER_UP_STEP_LAST:
> +        if (ctx->has_sim_changed)
> +            mm_broadband_modem_update_sim_hot_swap_detected (MM_BROADBAND_MODEM (self));
> +
> +        g_task_return_boolean (task, TRUE);
> +        g_object_unref (task);
> +        break;
> +    default:
> +        g_assert_not_reached ();
> +    }
> +}
> +
> +static void
> +modem_after_power_up (MMIfaceModem *self,
> +                      GAsyncReadyCallback callback,
> +                      gpointer user_data)
> +{
> +    GTask *task;
> +    AfterPowerUpContext *ctx;
> +
> +    ctx = g_new0 (AfterPowerUpContext, 1);
> +    ctx->step = AFTER_POWER_UP_STEP_FIRST;
> +    ctx->has_sim_changed = FALSE;
> +    ctx->retries = 3;
> +
> +    task = g_task_new (self, NULL, callback, user_data);
> +    g_task_set_task_data (task, ctx, (GDestroyNotify) g_free);
> +
> +    after_power_up_step (task);
> +}
> +
> +
> +/*****************************************************************************/
>  /* Modem power down (Modem interface) */
>
> +static void
> +telit_modem_power_down_ready (MMBaseModem *self,
> +                              GAsyncResult *res,
> +                              GTask *task)
> +{
> +    GError *error = NULL;
> +
> +    if (mm_base_modem_at_command_finish (self, res, &error)) {
> +        mm_dbg ("Ignore #QSS unsolicited during power down/low");
> +        MM_BROADBAND_MODEM_TELIT (self)->priv->parse_qss = FALSE;
> +    }
> +
> +    if (error) {
> +        mm_err ("modem power down: %s", error->message);
> +        g_clear_error (&error);
> +    }
> +
> +    g_task_return_boolean (task, TRUE);
> +    g_object_unref (task);
> +}
> +
>  static gboolean
>  modem_power_down_finish (MMIfaceModem *self,
>                           GAsyncResult *res,
>                           GError **error)
>  {
> -    return !!mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, error);
> +    return g_task_propagate_boolean (G_TASK (res), error);
>  }
>
>  static void
> @@ -918,12 +1085,15 @@ modem_power_down (MMIfaceModem *self,
>                    GAsyncReadyCallback callback,
>                    gpointer user_data)
>  {
> +    GTask *task;
> +
> +    task = g_task_new (self, NULL, callback, user_data);
>      mm_base_modem_at_command (MM_BASE_MODEM (self),
>                                "+CFUN=4",
>                                20,
>                                FALSE,
> -                              callback,
> -                              user_data);
> +                              (GAsyncReadyCallback) telit_modem_power_down_ready,
> +                              task);
>  }
>
>  /*****************************************************************************/
> @@ -1457,6 +1627,7 @@ mm_broadband_modem_telit_init (MMBroadbandModemTelit *self)
>      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->parse_qss = TRUE;
>  }
>
>  static void
> @@ -1474,6 +1645,8 @@ iface_modem_init (MMIfaceModem *iface)
>      iface->load_unlock_retries = modem_load_unlock_retries;
>      iface->reset = modem_reset;
>      iface->reset_finish = modem_reset_finish;
> +    iface->modem_after_power_up = modem_after_power_up;
> +    iface->modem_after_power_up_finish = modem_after_power_up_finish;
>      iface->modem_power_down = modem_power_down;
>      iface->modem_power_down_finish = modem_power_down_finish;
>      iface->load_access_technologies = load_access_technologies;
> diff --git a/src/mm-base-sim.c b/src/mm-base-sim.c
> index f3525e0..d03f342 100644
> --- a/src/mm-base-sim.c
> +++ b/src/mm-base-sim.c
> @@ -701,6 +701,57 @@ mm_base_sim_send_puk (MMBaseSim *self,
>  }
>
>  /*****************************************************************************/
> +/* LOAD SIM IDENTIFIER */
> +gchar *
> +mm_base_sim_load_sim_identifier_finish (MMBaseSim *self,
> +                                        GAsyncResult *res,
> +                                        GError **error) {
> +    GError *inner_error = NULL;
> +    gchar *simid;
> +
> +    if (g_async_result_is_tagged (res, mm_base_sim_load_sim_identifier) ||
> +        !MM_BASE_SIM_GET_CLASS (self)->load_sim_identifier_finish) {
> +        inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_UNSUPPORTED,
> +                                   "not implemented");
> +        g_propagate_error (error, inner_error);
> +        return NULL;
> +    }
> +
> +    simid = MM_BASE_SIM_GET_CLASS (self)->load_sim_identifier_finish (self, res, &inner_error);
> +    if (inner_error) {
> +        g_propagate_error (error, inner_error);
> +        return NULL;
> +    }
> +
> +    return simid;
> +}
> +
> +void
> +mm_base_sim_load_sim_identifier (MMBaseSim *self,
> +                                 GAsyncReadyCallback callback,
> +                                 gpointer user_data)
> +{
> +    if (!MM_BASE_SIM_GET_CLASS (self)->load_sim_identifier &&
> +        !MM_BASE_SIM_GET_CLASS (self)->load_sim_identifier_finish) {
> +        g_task_report_new_error (self,
> +                                 callback,
> +                                 user_data,
> +                                 mm_base_sim_load_sim_identifier,
> +                                 MM_CORE_ERROR,
> +                                 MM_CORE_ERROR_UNSUPPORTED,
> +                                 "not implemented");
> +        return;
> +    }
> +
> +    MM_BASE_SIM_GET_CLASS (self)->load_sim_identifier (
> +            self,
> +            (GAsyncReadyCallback)callback,
> +            user_data);
> +
> +    return;
> +}
> +
> +/*****************************************************************************/
>  /* SEND PIN (DBus call handling) */
>
>  typedef struct {
> diff --git a/src/mm-base-sim.h b/src/mm-base-sim.h
> index 1903994..22f02c8 100644
> --- a/src/mm-base-sim.h
> +++ b/src/mm-base-sim.h
> @@ -50,8 +50,7 @@ struct _MMBaseSim {
>
>  struct _MMBaseSimClass {
>      MmGdbusSimSkeletonClass parent;
> -
> -    /* Load SIM identifier (async) */
> +/* Load SIM identifier (async) */
>      void    (* load_sim_identifier)        (MMBaseSim *self,
>                                              GAsyncReadyCallback callback,
>                                              gpointer user_data);
> @@ -129,40 +128,47 @@ struct _MMBaseSimClass {
>
>  GType mm_base_sim_get_type (void);
>
> -void         mm_base_sim_new               (MMBaseModem *modem,
> -                                            GCancellable *cancellable,
> -                                            GAsyncReadyCallback callback,
> -                                            gpointer user_data);
> -MMBaseSim   *mm_base_sim_new_finish        (GAsyncResult  *res,
> -                                            GError       **error);
> -
> -void         mm_base_sim_initialize        (MMBaseSim *self,
> -                                            GCancellable *cancellable,
> -                                            GAsyncReadyCallback callback,
> -                                            gpointer user_data);
> -gboolean     mm_base_sim_initialize_finish (MMBaseSim *self,
> -                                            GAsyncResult *result,
> -                                            GError **error);
> -
> -void         mm_base_sim_send_pin          (MMBaseSim *self,
> -                                            const gchar *pin,
> -                                            GAsyncReadyCallback callback,
> -                                            gpointer user_data);
> -gboolean     mm_base_sim_send_pin_finish   (MMBaseSim *self,
> -                                            GAsyncResult *res,
> -                                            GError **error);
> -
> -void         mm_base_sim_send_puk          (MMBaseSim *self,
> -                                            const gchar *puk,
> -                                            const gchar *new_pin,
> -                                            GAsyncReadyCallback callback,
> -                                            gpointer user_data);
> -gboolean     mm_base_sim_send_puk_finish   (MMBaseSim *self,
> -                                            GAsyncResult *res,
> -                                            GError **error);
> -
> -void         mm_base_sim_export            (MMBaseSim *self);
> -
> -const gchar *mm_base_sim_get_path          (MMBaseSim *sim);
> +void         mm_base_sim_new                        (MMBaseModem *modem,
> +                                                     GCancellable *cancellable,
> +                                                     GAsyncReadyCallback callback,
> +                                                     gpointer user_data);
> +MMBaseSim   *mm_base_sim_new_finish                 (GAsyncResult  *res,
> +                                                     GError       **error);
> +
> +void         mm_base_sim_initialize                 (MMBaseSim *self,
> +                                                     GCancellable *cancellable,
> +                                                     GAsyncReadyCallback callback,
> +                                                     gpointer user_data);
> +gboolean     mm_base_sim_initialize_finish          (MMBaseSim *self,
> +                                                     GAsyncResult *result,
> +                                                     GError **error);
> +
> +void         mm_base_sim_send_pin                   (MMBaseSim *self,
> +                                                     const gchar *pin,
> +                                                     GAsyncReadyCallback callback,
> +                                                     gpointer user_data);
> +gboolean     mm_base_sim_send_pin_finish            (MMBaseSim *self,
> +                                                     GAsyncResult *res,
> +                                                     GError **error);
> +
> +void         mm_base_sim_send_puk                   (MMBaseSim *self,
> +                                                     const gchar *puk,
> +                                                     const gchar *new_pin,
> +                                                     GAsyncReadyCallback callback,
> +                                                     gpointer user_data);
> +gboolean     mm_base_sim_send_puk_finish            (MMBaseSim *self,
> +                                                     GAsyncResult *res,
> +                                                     GError **error);
> +
> +void         mm_base_sim_export                     (MMBaseSim *self);
> +
> +const gchar *mm_base_sim_get_path                   (MMBaseSim *sim);
> +
> +void          mm_base_sim_load_sim_identifier        (MMBaseSim *self,
> +                                                     GAsyncReadyCallback callback,
> +                                                     gpointer user_data);
> +gchar       *mm_base_sim_load_sim_identifier_finish (MMBaseSim *self,
> +                                                     GAsyncResult *res,
> +                                                     GError **error);
>
>  #endif /* MM_BASE_SIM_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