[PATCH] telit-plugin: ignore QSS when SIM-ME interface is locked
Carlo Lobrano
c.lobrano at gmail.com
Tue Jul 25 12:28:01 UTC 2017
> 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.
Totally agree. I was unsure how to manage the two references and I didn't
think I actually don't need it.
On Tue, 25 Jul 2017 at 14:10 Aleksander Morgado <aleksander at aleksander.es>
wrote:
> Hey,
>
> On Mon, Jul 24, 2017 at 6:23 PM, Carlo Lobrano <c.lobrano at gmail.com>
> 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
> > 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/20170725/87a3f4be/attachment-0001.html>
More information about the ModemManager-devel
mailing list