RE: [PATCH] telit: lock/unlock CSIM operations by default

[Penalva, Salvador]

Thank you all,

You have all been really helpful and quick and the patch seems to do its job 
perfectly!

Salvador

-Original Message-
From: Aleksander Morgado [mailto:aleksan...@aleksander.es] 
Sent: jueves, 16 de marzo de 2017 22:35
To: Carlo Lobrano; Penalva, Salvador; Daniele Palmas
Cc: ModemManager (development); Aleksander Morgado
Subject: Re: [PATCH] telit: lock/unlock CSIM operations by default

On Wed, Mar 15, 2017 at 12:19 AM, Aleksander Morgado <aleksan...@aleksander.es> 
wrote:
> Wrap the AT+CSIM=XX commands between lock (CSIM=1) and unlock (CSIM=0) 
> operations.
>
> This seems to avoid the TTY lockup seen in several different Telit 
> modules.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=100205
>
> Reported-by: Penalva, Salvador <salvador.pena...@digi.com>
> ---
>
> Hey Salvador, Carlo and Daniele,
>
> Can you guys review and test this patch? It looks like it solved the issue I 
> saw myself.
>
> The AT command reference for the Telit LE866 does say that the AT+CSIM 
> commands need to be enclosed within lock/unlock operations like these.
>

Pushed to git master and mm-1-6.

> ---
>  plugins/telit/mm-broadband-modem-telit.c | 60 
> 
>  1 file changed, 60 insertions(+)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c 
> b/plugins/telit/mm-broadband-modem-telit.c
> index b1679ae4..8bc9d1e5 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -452,6 +452,8 @@ modem_load_supported_bands (MMIfaceModem *self,  
> /*
> /
>  /* Load unlock retries (Modem interface) */
>
> +#define CSIM_LOCK_STR   "+CSIM=1"
> +#define CSIM_UNLOCK_STR "+CSIM=0"
>  #define CSIM_QUERY_PIN_RETRIES_STR  "+CSIM=10,002100"
>  #define CSIM_QUERY_PUK_RETRIES_STR  "+CSIM=10,002C000100"
>  #define CSIM_QUERY_PIN2_RETRIES_STR "+CSIM=10,0020008100"
> @@ -460,10 +462,12 @@ modem_load_supported_bands (MMIfaceModem *self,
>
>  typedef enum {
>  LOAD_UNLOCK_RETRIES_STEP_FIRST,
> +LOAD_UNLOCK_RETRIES_STEP_LOCK,
>  LOAD_UNLOCK_RETRIES_STEP_PIN,
>  LOAD_UNLOCK_RETRIES_STEP_PUK,
>  LOAD_UNLOCK_RETRIES_STEP_PIN2,
>  LOAD_UNLOCK_RETRIES_STEP_PUK2,
> +LOAD_UNLOCK_RETRIES_STEP_UNLOCK,
>  LOAD_UNLOCK_RETRIES_STEP_LAST
>  } LoadUnlockRetriesStep;
>
> @@ -500,6 +504,25 @@ modem_load_unlock_retries_finish (MMIfaceModem 
> *self,  }
>
>  static void
> +csim_unlock_ready (MMBaseModem  *self,
> +   GAsyncResult *res,
> +   LoadUnlockRetriesContext *ctx) {
> +const gchar *response;
> +GError  *error = NULL;
> +
> +/* Ignore errors */
> +response = mm_base_modem_at_command_finish (self, res, );
> +if (!response) {
> +mm_warn ("Couldn't unlock SIM card: %s", error->message);
> +g_error_free (error);
> +}
> +
> +ctx->step++;
> +load_unlock_retries_step (ctx);
> +}
> +
> +static void
>  csim_query_ready (MMBaseModem *self,
>GAsyncResult *res,
>LoadUnlockRetriesContext *ctx) @@ -542,6 +565,7 @@ 
> csim_query_ready (MMBaseModem *self,
>  mm_unlock_retries_set (ctx->retries, MM_MODEM_LOCK_SIM_PUK2, 
> unlock_retries);
>  break;
>  default:
> +g_assert_not_reached ();
>  break;
>  }
>
> @@ -551,12 +575,40 @@ next_step:
>  }
>
>  static void
> +csim_lock_ready (MMBaseModem  *self,
> + GAsyncResult *res,
> + LoadUnlockRetriesContext *ctx) {
> +const gchar *response;
> +GError  *error = NULL;
> +
> +response = mm_base_modem_at_command_finish (self, res, );
> +if (!response) {
> +g_prefix_error (, "Couldn't lock SIM card: ");
> +g_simple_async_result_take_error (ctx->result, error);
> +load_unlock_retries_context_complete_and_free (ctx);
> +return;
> +}
> +
> +ctx->step++;
> +load_unlock_retries_step (ctx);
> +}
> +
> +static void
>  load_unlock_retries_step (LoadUnlockRetriesContext *ctx)  {
>  switch (ctx->step) {
>  case LOAD_UNLOCK_RETRIES_STEP_FIRST:
>  /* 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,
> +  

Re: [PATCH] telit: lock/unlock CSIM operations by default

[Aleksander Morgado]

On Wed, Mar 15, 2017 at 12:19 AM, Aleksander Morgado
 wrote:
> Wrap the AT+CSIM=XX commands between lock (CSIM=1) and
> unlock (CSIM=0) operations.
>
> This seems to avoid the TTY lockup seen in several different Telit
> modules.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=100205
>
> Reported-by: Penalva, Salvador 
> ---
>
> Hey Salvador, Carlo and Daniele,
>
> Can you guys review and test this patch? It looks like it solved the issue I 
> saw myself.
>
> The AT command reference for the Telit LE866 does say that the AT+CSIM 
> commands need to be enclosed within lock/unlock operations like these.
>

Pushed to git master and mm-1-6.

> ---
>  plugins/telit/mm-broadband-modem-telit.c | 60 
> 
>  1 file changed, 60 insertions(+)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c 
> b/plugins/telit/mm-broadband-modem-telit.c
> index b1679ae4..8bc9d1e5 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -452,6 +452,8 @@ modem_load_supported_bands (MMIfaceModem *self,
>  
> /*/
>  /* Load unlock retries (Modem interface) */
>
> +#define CSIM_LOCK_STR   "+CSIM=1"
> +#define CSIM_UNLOCK_STR "+CSIM=0"
>  #define CSIM_QUERY_PIN_RETRIES_STR  "+CSIM=10,002100"
>  #define CSIM_QUERY_PUK_RETRIES_STR  "+CSIM=10,002C000100"
>  #define CSIM_QUERY_PIN2_RETRIES_STR "+CSIM=10,0020008100"
> @@ -460,10 +462,12 @@ modem_load_supported_bands (MMIfaceModem *self,
>
>  typedef enum {
>  LOAD_UNLOCK_RETRIES_STEP_FIRST,
> +LOAD_UNLOCK_RETRIES_STEP_LOCK,
>  LOAD_UNLOCK_RETRIES_STEP_PIN,
>  LOAD_UNLOCK_RETRIES_STEP_PUK,
>  LOAD_UNLOCK_RETRIES_STEP_PIN2,
>  LOAD_UNLOCK_RETRIES_STEP_PUK2,
> +LOAD_UNLOCK_RETRIES_STEP_UNLOCK,
>  LOAD_UNLOCK_RETRIES_STEP_LAST
>  } LoadUnlockRetriesStep;
>
> @@ -500,6 +504,25 @@ modem_load_unlock_retries_finish (MMIfaceModem *self,
>  }
>
>  static void
> +csim_unlock_ready (MMBaseModem  *self,
> +   GAsyncResult *res,
> +   LoadUnlockRetriesContext *ctx)
> +{
> +const gchar *response;
> +GError  *error = NULL;
> +
> +/* Ignore errors */
> +response = mm_base_modem_at_command_finish (self, res, );
> +if (!response) {
> +mm_warn ("Couldn't unlock SIM card: %s", error->message);
> +g_error_free (error);
> +}
> +
> +ctx->step++;
> +load_unlock_retries_step (ctx);
> +}
> +
> +static void
>  csim_query_ready (MMBaseModem *self,
>GAsyncResult *res,
>LoadUnlockRetriesContext *ctx)
> @@ -542,6 +565,7 @@ csim_query_ready (MMBaseModem *self,
>  mm_unlock_retries_set (ctx->retries, MM_MODEM_LOCK_SIM_PUK2, 
> unlock_retries);
>  break;
>  default:
> +g_assert_not_reached ();
>  break;
>  }
>
> @@ -551,12 +575,40 @@ next_step:
>  }
>
>  static void
> +csim_lock_ready (MMBaseModem  *self,
> + GAsyncResult *res,
> + LoadUnlockRetriesContext *ctx)
> +{
> +const gchar *response;
> +GError  *error = NULL;
> +
> +response = mm_base_modem_at_command_finish (self, res, );
> +if (!response) {
> +g_prefix_error (, "Couldn't lock SIM card: ");
> +g_simple_async_result_take_error (ctx->result, error);
> +load_unlock_retries_context_complete_and_free (ctx);
> +return;
> +}
> +
> +ctx->step++;
> +load_unlock_retries_step (ctx);
> +}
> +
> +static void
>  load_unlock_retries_step (LoadUnlockRetriesContext *ctx)
>  {
>  switch (ctx->step) {
>  case LOAD_UNLOCK_RETRIES_STEP_FIRST:
>  /* 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);
> +break;
>  case LOAD_UNLOCK_RETRIES_STEP_PIN:
>  mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
>CSIM_QUERY_PIN_RETRIES_STR,
> @@ -589,6 +641,14 @@ load_unlock_retries_step (LoadUnlockRetriesContext *ctx)
>(GAsyncReadyCallback) csim_query_ready,
>ctx);
>  break;
> +case LOAD_UNLOCK_RETRIES_STEP_UNLOCK:
> +mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> +  CSIM_UNLOCK_STR,
> +  CSIM_QUERY_TIMEOUT,
> +  

Re: [PATCH] telit: lock/unlock CSIM operations by default

[Dan Williams]

On Wed, 2017-03-15 at 10:28 +0100, Aleksander Morgado wrote:
> On Wed, Mar 15, 2017 at 2:40 AM, Dan Williams 
> wrote:
> > > 
> > > Can you guys review and test this patch? It looks like it solved
> > > the
> > > issue I saw myself.
> > > 
> > > The AT command reference for the Telit LE866 does say that the
> > > AT+CSIM commands need to be enclosed within lock/unlock
> > > operations
> > > like these.
> > 
> > If the LOCK succeeds, then don't we need to UNLOCK if some
> > intermediate
> > step fails?  It looks like we'll just leave it in locked state if
> > any
> > step like say PIN or PIN2 calls
> > load_unlock_retries_context_complete_and_free().
> 
> But all the other intermediate steps never fail; i.e.
> csim_query_ready() always goes to the next step, never completes the
> GSimpleAsyncResult. So effectively, if any intermediate AT command
> fails, we just go on to the next one, and after the last one we have
> the CSIM=0 to unlock.

Ok then :)  I didn't look at the actual code, so I thought this could
be the case, but looking at the code you're entirely correct.  But
could we put a comment that the unlock behavior depends on all
intermediate steps not erroring out?  Otherwise later if anything there
does get changed, it'll be easy to screw up the lock balancing.

Otherwise LGTM.

Dan
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

RE: [PATCH] telit: lock/unlock CSIM operations by default

[Penalva, Salvador]

Hi all, 

I tried Aleksander Patch with an older version of telit HE910 FW and it works 
perfect. 

I just have one question, Is this CSIM command supported in all telit Modems? 
It wouldn't be good if by implementing this change on telit-plugin to fix this 
issue on telit HE910 and the other modems that can use this command, other 
modems  stop working.

Thank you,

Salvador

-Original Message-
From: Aleksander Morgado [mailto:aleksan...@aleksander.es] 
Sent: miércoles, 15 de marzo de 2017 10:28
To: Dan Williams
Cc: Carlo Lobrano; Penalva, Salvador; Daniele Palmas; ModemManager (development)
Subject: Re: [PATCH] telit: lock/unlock CSIM operations by default

On Wed, Mar 15, 2017 at 2:40 AM, Dan Williams <d...@redhat.com> wrote:
>>
>> Can you guys review and test this patch? It looks like it solved the 
>> issue I saw myself.
>>
>> The AT command reference for the Telit LE866 does say that the
>> AT+CSIM commands need to be enclosed within lock/unlock operations
>> like these.
>
> If the LOCK succeeds, then don't we need to UNLOCK if some 
> intermediate step fails?  It looks like we'll just leave it in locked 
> state if any step like say PIN or PIN2 calls 
> load_unlock_retries_context_complete_and_free().

But all the other intermediate steps never fail; i.e.
csim_query_ready() always goes to the next step, never completes the 
GSimpleAsyncResult. So effectively, if any intermediate AT command fails, we 
just go on to the next one, and after the last one we have the CSIM=0 to unlock.

--
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] telit: lock/unlock CSIM operations by default

[Dan Williams]

On Wed, 2017-03-15 at 00:19 +0100, Aleksander Morgado wrote:
> Wrap the AT+CSIM=XX commands between lock (CSIM=1) and
> unlock (CSIM=0) operations.
> 
> This seems to avoid the TTY lockup seen in several different Telit
> modules.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=100205
> 
> Reported-by: Penalva, Salvador 
> ---
> 
> Hey Salvador, Carlo and Daniele,
> 
> Can you guys review and test this patch? It looks like it solved the
> issue I saw myself.
> 
> The AT command reference for the Telit LE866 does say that the
> AT+CSIM commands need to be enclosed within lock/unlock operations
> like these.

If the LOCK succeeds, then don't we need to UNLOCK if some intermediate
step fails?  It looks like we'll just leave it in locked state if any
step like say PIN or PIN2 calls
load_unlock_retries_context_complete_and_free().

Dan

> ---
>  plugins/telit/mm-broadband-modem-telit.c | 60
> 
>  1 file changed, 60 insertions(+)
> 
> diff --git a/plugins/telit/mm-broadband-modem-telit.c
> b/plugins/telit/mm-broadband-modem-telit.c
> index b1679ae4..8bc9d1e5 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -452,6 +452,8 @@ modem_load_supported_bands (MMIfaceModem *self,
>  /***
> **/
>  /* Load unlock retries (Modem interface) */
> 
> +#define CSIM_LOCK_STR   "+CSIM=1"
> +#define CSIM_UNLOCK_STR "+CSIM=0"
>  #define CSIM_QUERY_PIN_RETRIES_STR  "+CSIM=10,002100"
>  #define CSIM_QUERY_PUK_RETRIES_STR  "+CSIM=10,002C000100"
>  #define CSIM_QUERY_PIN2_RETRIES_STR "+CSIM=10,0020008100"
> @@ -460,10 +462,12 @@ modem_load_supported_bands (MMIfaceModem *self,
> 
>  typedef enum {
>  LOAD_UNLOCK_RETRIES_STEP_FIRST,
> +LOAD_UNLOCK_RETRIES_STEP_LOCK,
>  LOAD_UNLOCK_RETRIES_STEP_PIN,
>  LOAD_UNLOCK_RETRIES_STEP_PUK,
>  LOAD_UNLOCK_RETRIES_STEP_PIN2,
>  LOAD_UNLOCK_RETRIES_STEP_PUK2,
> +LOAD_UNLOCK_RETRIES_STEP_UNLOCK,
>  LOAD_UNLOCK_RETRIES_STEP_LAST
>  } LoadUnlockRetriesStep;
> 
> @@ -500,6 +504,25 @@ modem_load_unlock_retries_finish (MMIfaceModem
> *self,
>  }
> 
>  static void
> +csim_unlock_ready (MMBaseModem  *self,
> +   GAsyncResult *res,
> +   LoadUnlockRetriesContext *ctx)
> +{
> +const gchar *response;
> +GError  *error = NULL;
> +
> +/* Ignore errors */
> +response = mm_base_modem_at_command_finish (self, res, );
> +if (!response) {
> +mm_warn ("Couldn't unlock SIM card: %s", error->message);
> +g_error_free (error);
> +}
> +
> +ctx->step++;
> +load_unlock_retries_step (ctx);
> +}
> +
> +static void
>  csim_query_ready (MMBaseModem *self,
>    GAsyncResult *res,
>    LoadUnlockRetriesContext *ctx)
> @@ -542,6 +565,7 @@ csim_query_ready (MMBaseModem *self,
>  mm_unlock_retries_set (ctx->retries,
> MM_MODEM_LOCK_SIM_PUK2, unlock_retries);
>  break;
>  default:
> +g_assert_not_reached ();
>  break;
>  }
> 
> @@ -551,12 +575,40 @@ next_step:
>  }
> 
>  static void
> +csim_lock_ready (MMBaseModem  *self,
> + GAsyncResult *res,
> + LoadUnlockRetriesContext *ctx)
> +{
> +const gchar *response;
> +GError  *error = NULL;
> +
> +response = mm_base_modem_at_command_finish (self, res, );
> +if (!response) {
> +g_prefix_error (, "Couldn't lock SIM card: ");
> +g_simple_async_result_take_error (ctx->result, error);
> +load_unlock_retries_context_complete_and_free (ctx);
> +return;
> +}
> +
> +ctx->step++;
> +load_unlock_retries_step (ctx);
> +}
> +
> +static void
>  load_unlock_retries_step (LoadUnlockRetriesContext *ctx)
>  {
>  switch (ctx->step) {
>  case LOAD_UNLOCK_RETRIES_STEP_FIRST:
>  /* 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);
> +break;
>  case LOAD_UNLOCK_RETRIES_STEP_PIN:
>  mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
>    CSIM_QUERY_PIN_RETRIES_STR,
> @@ -589,6 +641,14 @@ load_unlock_retries_step
> (LoadUnlockRetriesContext *ctx)
>    (GAsyncReadyCallback)
> csim_query_ready,
>    ctx);
>  break;
> +case LOAD_UNLOCK_RETRIES_STEP_UNLOCK:
> +