[PATCH] telit-plugin: ignore QSS when SIM-ME interface is locked
Carlo Lobrano
c.lobrano at gmail.com
Wed Jul 26 08:36:32 UTC 2017
>> + csim_unlock_complete (self->priv->csim_lock_task);
> Reset the csim_lock_task pointer here to NULL, please.
Can I do this inside csim_unlock_complete or there is a reason to do it
outside this function?
On Tue, 25 Jul 2017 at 14:28 Carlo Lobrano <c.lobrano at gmail.com> wrote:
> > 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/20170726/99e7d78b/attachment-0001.html>
More information about the ModemManager-devel
mailing list