<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">> Of course it looks great, I just want to have a try tomorrow on a modem to double check it. </div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Double checked, all green. Thanks a lot! </div></div><div class="gmail_extra"> <div class="gmail_quote">On 4 September 2017 at 18:41, Carlo Lobrano <<a href="mailto:c.lobrano@gmail.com" target="_blank">c.lobrano@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Of course it looks great, I just want to have a try tomorrow on a modem to double check it. </div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"> <div class="gmail_quote">On 4 September 2017 at 18:29, Carlo Lobrano <<a href="mailto:c.lobrano@gmail.com" target="_blank">c.lobrano@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">> Using its own GTask ends up requiring the extra _ready() method, but that > makes the flow much more clear and consistent I think.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"> </div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">yeah, I agree too</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"> </div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">> I've pushed your patch to the "clobrano/qss-unsolicited-power" branch > in my github repo: > <a href="https://github.com/aleksander0m/ModemManager/commits/clobrano/qss-unsolicited-power" target="_blank">https://github.com/aleksander0m/ModemManager/commits/clobrano/qss-unsolicited-power</a> > > 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! </div></div><div class="m_-3560426087727553143HOEnZb"><div class="m_-3560426087727553143h5"><div class="gmail_extra"> <div class="gmail_quote">On 4 September 2017 at 18:09, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey! On Mon, Sep 4, 2017 at 8:13 AM, Carlo Lobrano <<a href="mailto:c.lobrano@gmail.com" target="_blank">c.lobrano@gmail.com</a>> 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: <a href="https://github.com/aleksander0m/ModemManager/commits/clobrano/qss-unsolicited-power" rel="noreferrer" target="_blank">https://github.com/aleksander0m/ModemManager/commits/clobrano/qss-unsolicited-power</a> 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. <div class="m_-3560426087727553143m_4760795883497439400HOEnZb"><div class="m_-3560426087727553143m_4760795883497439400h5"> > --- > 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 > </div></div><div class="m_-3560426087727553143m_4760795883497439400HOEnZb"><div class="m_-3560426087727553143m_4760795883497439400h5">> _______________________________________________ > ModemManager-devel mailing list > <a href="mailto:ModemManager-devel@lists.freedesktop.org" target="_blank">ModemManager-devel@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel</a> -- Aleksander <a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a> </div></div></blockquote></div> </div> </div></div></blockquote></div> </div> </div></div></blockquote></div> </div>