[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