<div dir="ltr"><div class="inbox-inbox-uyb8Gf"><div><div class="inbox-inbox-i3">>> + csim_unlock_complete (self->priv->csim_lock_task); </div></div></div><div class="inbox-inbox-uyb8Gf"><div><div class="inbox-inbox-F3hlO"> > Reset the csim_lock_task pointer here to NULL, please. </div><div class="inbox-inbox-F3hlO">Can I do this inside csim_unlock_complete or there is a reason to do it outside this function? </div></div></div></div> <div class="gmail_quote"><div dir="ltr">On Tue, 25 Jul 2017 at 14:28 Carlo Lobrano <<a href="mailto:c.lobrano@gmail.com">c.lobrano@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>> 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. </div></div><div dir="ltr">Totally agree. I was unsure how to manage the two references and I didn't think I actually don't need it.</div><div dir="ltr"> <div> <div class="gmail_quote"><div dir="ltr">On Tue, 25 Jul 2017 at 14:10 Aleksander Morgado <<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey, On Mon, Jul 24, 2017 at 6:23 PM, Carlo Lobrano <<a href="mailto:c.lobrano@gmail.com" target="_blank">c.lobrano@gmail.com</a>> 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 > <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> </blockquote></div></div></div></blockquote></div>