[PATCH v2] telit: unsupported CSIM lock should not skip loading unlock retries
Carlo Lobrano
c.lobrano at gmail.com
Thu Apr 13 06:47:48 UTC 2017
> 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.
Right, there is no real need for this, but just for completeness I prefer
to keep it, if you don't mind
>
Looks like csim_lock_support isn't set anywhere to FEATURE_SUPPORTED? It
should be set here I believe.
Doh!
I'll fix it :D
On 10 April 2017 at 13:25, Aleksander Morgado <aleksander at aleksander.es>
wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170413/0c70c868/attachment-0001.html>
More information about the ModemManager-devel
mailing list