[PATCH v2] telit: unsupported CSIM lock should not skip loading unlock retries

Aleksander Morgado aleksander at aleksander.es
Mon Apr 10 11:25:24 UTC 2017


On 10/04/17 12:25, Carlo Lobrano wrote:
> Some modems do not support CSIM lock/unlock, but they do support
> querying SIM unlock retries through +CSIM command.
> 
> If CSIM lock returns with "unsupported command" do not propagate
> the error and continue with the other CSIM queries instead, moreover the
> CSIM lock feature is signed as FEATURE_UNSUPPORTED in Telit modem
> private structure to prevent being sent again (e.g. calling CSIM
> unlock AT command).
> ---
> 
> Hi Aleksander,
> 
> I added private structure to store whether csim lock is supported or not (and so
> skip the command). Let me know if this is ok now.
> 

See below.

> Carlo
> 
> ---
>  plugins/telit/mm-broadband-modem-telit.c | 86 ++++++++++++++++++++++++++------
>  plugins/telit/mm-broadband-modem-telit.h |  2 +
>  2 files changed, 72 insertions(+), 16 deletions(-)
> 
> diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c
> index cce0229..841a48b 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -42,6 +42,15 @@ 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));
>  
> +typedef enum {
> +    FEATURE_SUPPORT_UNKNOWN,
> +    FEATURE_NOT_SUPPORTED,
> +    FEATURE_SUPPORTED
> +} FeatureSupport;
> +
> +struct _MMBroadbandModemTelitPrivate {
> +    FeatureSupport csim_lock_support;
> +};
>  
>  /*****************************************************************************/
>  /* After Sim Unlock (Modem interface) */
> @@ -521,6 +530,11 @@ csim_unlock_ready (MMBaseModem              *self,
>      /* Ignore errors */
>      response = mm_base_modem_at_command_finish (self, res, &error);
>      if (!response) {
> +        if (g_error_matches (error,
> +                             MM_MOBILE_EQUIPMENT_ERROR,
> +                             MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED)) {
> +            ctx->self->priv->csim_lock_support = FEATURE_NOT_SUPPORTED;
> +        }

Is this previous part really needed? Is there any case in which a LOCK
command would succeed but then UNLOCK command report a "not supported"
error? I guess it doesn't harm to have this, though, your call.

>          mm_warn ("Couldn't unlock SIM card: %s", error->message);
>          g_error_free (error);
>      }
> @@ -591,10 +605,18 @@ csim_lock_ready (MMBaseModem              *self,
>  
>      response = mm_base_modem_at_command_finish (self, res, &error);
>      if (!response) {
> -        g_prefix_error (&error, "Couldn't lock SIM card: ");
> -        g_simple_async_result_take_error (ctx->result, error);
> -        load_unlock_retries_context_complete_and_free (ctx);
> -        return;
> +        if (g_error_matches (error,
> +                             MM_MOBILE_EQUIPMENT_ERROR,
> +                             MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED)) {
> +            ctx->self->priv->csim_lock_support = FEATURE_NOT_SUPPORTED;
> +            mm_warn ("Couldn't lock SIM card: %s. Continuing without CSIM lock.", error->message);
> +            g_error_free (error);
> +        } else {
> +            g_prefix_error (&error, "Couldn't lock SIM card: ");
> +            g_simple_async_result_take_error (ctx->result, error);
> +            load_unlock_retries_context_complete_and_free (ctx);
> +            return;
> +        }
>      }


Looks like csim_lock_support isn't set anywhere to FEATURE_SUPPORTED? It
should be set here I believe.

>  
>      ctx->step++;
> @@ -602,6 +624,40 @@ csim_lock_ready (MMBaseModem              *self,
>  }
>  
>  static void
> +handle_csim_locking (LoadUnlockRetriesContext *ctx, gboolean is_lock)
> +{
> +    switch (ctx->self->priv->csim_lock_support) {
> +        case FEATURE_SUPPORT_UNKNOWN:
> +        case FEATURE_SUPPORTED:
> +            if (is_lock) {
> +                mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> +                                          CSIM_LOCK_STR,
> +                                          CSIM_QUERY_TIMEOUT,
> +                                          FALSE,
> +                                          (GAsyncReadyCallback) csim_lock_ready,
> +                                          ctx);
> +            } else {
> +                mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> +                                          CSIM_UNLOCK_STR,
> +                                          CSIM_QUERY_TIMEOUT,
> +                                          FALSE,
> +                                          (GAsyncReadyCallback) csim_unlock_ready,
> +                                          ctx);
> +            }
> +            break;
> +        case FEATURE_NOT_SUPPORTED:
> +            mm_dbg ("CSIM lock not supported by this modem. Skipping %s command",
> +                    is_lock ? "lock" : "unlock");
> +            ctx->step++;
> +            load_unlock_retries_step (ctx);
> +            break;
> +        default:
> +            g_assert_not_reached ();
> +            break;
> +    }
> +}
> +
> +static void
>  load_unlock_retries_step (LoadUnlockRetriesContext *ctx)
>  {
>      switch (ctx->step) {
> @@ -609,12 +665,7 @@ load_unlock_retries_step (LoadUnlockRetriesContext *ctx)
>              /* Fall back on next step */
>              ctx->step++;
>          case LOAD_UNLOCK_RETRIES_STEP_LOCK:
> -            mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> -                                      CSIM_LOCK_STR,
> -                                      CSIM_QUERY_TIMEOUT,
> -                                      FALSE,
> -                                      (GAsyncReadyCallback) csim_lock_ready,
> -                                      ctx);
> +            handle_csim_locking (ctx, TRUE);
>              break;
>          case LOAD_UNLOCK_RETRIES_STEP_PIN:
>              mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> @@ -649,12 +700,7 @@ load_unlock_retries_step (LoadUnlockRetriesContext *ctx)
>                                        ctx);
>              break;
>          case LOAD_UNLOCK_RETRIES_STEP_UNLOCK:
> -            mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> -                                      CSIM_UNLOCK_STR,
> -                                      CSIM_QUERY_TIMEOUT,
> -                                      FALSE,
> -                                      (GAsyncReadyCallback) csim_unlock_ready,
> -                                      ctx);
> +            handle_csim_locking (ctx, FALSE);
>              break;
>          case LOAD_UNLOCK_RETRIES_STEP_LAST:
>              if (ctx->succeded_requests == 0) {
> @@ -1220,6 +1266,11 @@ mm_broadband_modem_telit_new (const gchar *device,
>  static void
>  mm_broadband_modem_telit_init (MMBroadbandModemTelit *self)
>  {
> +    self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self,
> +                                              MM_TYPE_BROADBAND_MODEM_TELIT,
> +                                              MMBroadbandModemTelitPrivate);
> +
> +    self->priv->csim_lock_support = FEATURE_SUPPORT_UNKNOWN;
>  }
>  
>  static void
> @@ -1263,4 +1314,7 @@ iface_modem_3gpp_init (MMIfaceModem3gpp *iface)
>  static void
>  mm_broadband_modem_telit_class_init (MMBroadbandModemTelitClass *klass)
>  {
> +    GObjectClass *object_class = G_OBJECT_CLASS (klass);
> +
> +    g_type_class_add_private (object_class, sizeof (MMBroadbandModemTelitPrivate));
>  }
> diff --git a/plugins/telit/mm-broadband-modem-telit.h b/plugins/telit/mm-broadband-modem-telit.h
> index 50e6365..f68465e 100644
> --- a/plugins/telit/mm-broadband-modem-telit.h
> +++ b/plugins/telit/mm-broadband-modem-telit.h
> @@ -29,9 +29,11 @@
>  
>  typedef struct _MMBroadbandModemTelit MMBroadbandModemTelit;
>  typedef struct _MMBroadbandModemTelitClass MMBroadbandModemTelitClass;
> +typedef struct _MMBroadbandModemTelitPrivate MMBroadbandModemTelitPrivate;
>  
>  struct _MMBroadbandModemTelit {
>      MMBroadbandModem parent;
> +    MMBroadbandModemTelitPrivate *priv;
>  };
>  
>  struct _MMBroadbandModemTelitClass{
> 


-- 
Aleksander
https://aleksander.es


More information about the ModemManager-devel mailing list