[PATCH v4] telit-plugin: handle QSS unsolicited due to power state transitions
Carlo Lobrano
c.lobrano at gmail.com
Tue Sep 5 07:19:39 UTC 2017
> Of course it looks great, I just want to have a try tomorrow on a modem
to double check it.
Double checked, all green. Thanks a lot!
On 4 September 2017 at 18:41, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> Of course it looks great, I just want to have a try tomorrow on a modem to
> double check it.
>
> On 4 September 2017 at 18:29, Carlo Lobrano <c.lobrano at gmail.com> wrote:
>
>> > Using its own GTask ends up requiring the extra _ready() method, but
>> that
>> > makes the flow much more clear and consistent I think.
>>
>> yeah, I agree too
>>
>> > 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?
>>
>> sure, I'm going to check it, thanks for the explanation and the fixes!
>>
>>
>>
>> On 4 September 2017 at 18:09, Aleksander Morgado <
>> aleksander at aleksander.es> wrote:
>>
>>> 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/clobran
>>> o/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
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170905/5fbc847b/attachment-0001.html>
More information about the ModemManager-devel
mailing list