Re: [MM] [PATCH] sim: retry SIM operations during initialization

[Ben Chan <benchan chromium org>]

The timeout + retries really tries to address flaky SIM operations on
some modems. We could limit the retries to certain error codes, but
that won't address flakiness, unfortunately :(

We could let individual plugins to configure whether it should retry
on failed SIM operations. For a modem that does not support reading
IMSI (AT+CIMI), the plugin would set load_imsi / load_imsi_finish to
NULL to skip INITIALIZATION_STEP_IMSI altogether.

- Ben

On Thu, Aug 23, 2012 at 5:36 AM, Aleksander Morgado
<aleksander lanedo com> wrote:
>
> On 08/22/2012 07:28 AM, Ben Chan wrote:
> > This patch modifies the MMSim interface to validate the IMSI value
> > reported by AT+CIMI and retry the command upon an error. It also
> > modifies other SIM operations during the interface initialization to
> > retry upon an error.
>
> I'm a bit worried about the kind of errors that cause the retries to get
> executed. Currently it seems that every error will trigger the 1s wait
> and retry. I think there needs to be a clear distinction between the
> errors that are considered retry-able and those which are not, if that
> is even possible. Adding the retries in the generic implementation will
> mean that if a modem *always* fails to run the IMSI loading because it
> doesn't support the specific command used, it will always get the 1s
> delays and retries. Are you able to narrow down the cases where the
> retries should be executed?
>
> > ---
> >  src/mm-sim.c |  124
> > ++++++++++++++++++++++++++++++++++-----------------------
> >  1 files changed, 74 insertions(+), 50 deletions(-)
> >
> > diff --git a/src/mm-sim.c b/src/mm-sim.c
> > index 9cd31cc..eb0bb00 100644
> > --- a/src/mm-sim.c
> > +++ b/src/mm-sim.c
> > @@ -1066,15 +1066,48 @@ load_sim_identifier (MMSim *self,
> >  /* IMSI */
> >
> >  static gchar *
> > +parse_imsi (const gchar *response,
> > +            GError **error)
> > +{
> > +    const gchar *s;
> > +    int len;
> > +    gboolean success;
> > +
> > +    if (response) {
> > +        success = TRUE;
> > +        for (s = response, len = 0; *s; ++s, ++len) {
> > +            /* IMSI is a number with 15 or less decimal digits. */
> > +            if (!isdigit (*s) || len > 15) {
> > +                success = FALSE;
> > +                break;
> > +            }
> > +        }
> > +        if (success)
> > +            return g_strdup (response);
> > +    }
> > +
> > +    g_set_error (error,
> > +                 MM_CORE_ERROR,
> > +                 MM_CORE_ERROR_FAILED,
> > +                 "Invalid +CIMI response '%s'", response ? response :
> > "<null>");
> > +    return NULL;
> > +}
> > +
> > +static gchar *
> >  load_imsi_finish (MMSim *self,
> >                    GAsyncResult *res,
> >                    GError **error)
> >  {
> > +    const gchar *result;
> >      gchar *imsi;
> >
> >      if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT
> > (res), error))
> >          return NULL;
> > -    imsi = g_strdup (g_simple_async_result_get_op_res_gpointer
> > (G_SIMPLE_ASYNC_RESULT (res)));
> > +    result = g_simple_async_result_get_op_res_gpointer
> > (G_SIMPLE_ASYNC_RESULT (res));
> > +
> > +    imsi = parse_imsi (result, error);
> > +    if (!imsi)
> > +        return NULL;
> >
> >      mm_dbg ("loaded IMSI: %s", imsi);
> >      return imsi;
> > @@ -1093,7 +1126,7 @@ load_imsi (MMSim *self,
> >          MM_BASE_MODEM (self->priv->modem),
> >          "+CIMI",
> >          3,
> > -        TRUE,
> > +        FALSE,
> >          (GAsyncReadyCallback)load_imsi_command_ready,
> >          g_simple_async_result_new (G_OBJECT (self),
> >                                     callback,
> > @@ -1368,7 +1401,7 @@ struct _InitAsyncContext {
> >      GCancellable *cancellable;
> >      MMSim *self;
> >      InitializationStep step;
> > -    guint sim_identifier_tries;
> > +    guint step_tries;
> >  };
> >
> >  static void
> > @@ -1381,6 +1414,21 @@ init_async_context_free (InitAsyncContext *ctx)
> >      g_free (ctx);
> >  }
> >
> > +static void
> > +init_async_context_next_step (InitAsyncContext *ctx)
> > +{
> > +    ctx->step++;
> > +    ctx->step_tries = 0;
> > +}
> > +
> > +static gboolean
> > +interface_initialization_step_again (InitAsyncContext *ctx)
> > +{
> > +    interface_initialization_step (ctx);
> > +
> > +    return FALSE;
> > +}
> > +
> >  MMSim *
> >  mm_sim_new_finish (GAsyncResult  *res,
> >                     GError       **error)
> > @@ -1409,41 +1457,6 @@ initable_init_finish (GAsyncInitable  *initable,
> >      return !g_simple_async_result_propagate_error
> > (G_SIMPLE_ASYNC_RESULT (result), error);
> >  }
> >
> > -static void
> > -load_sim_identifier_ready (MMSim *self,
> > -                           GAsyncResult *res,
> > -                           InitAsyncContext *ctx)
> > -{
> > -    GError *error = NULL;
> > -    gchar *simid;
> > -
> > -    simid  = MM_SIM_GET_CLASS (ctx->self)->load_sim_identifier_finish
> > (self, res, &error);
> > -    if (!simid) {
> > -        /* TODO: make the retries gobi-specific? */
> > -
> > -        /* Try one more time... Gobi 1K cards may reply to the first
> > -         * request with '+CRSM: 106,134,""' which is bogus because
> > -         * subsequent requests work fine.
> > -         */
> > -        if (++ctx->sim_identifier_tries < 2) {
> > -            g_clear_error (&error);
> > -            interface_initialization_step (ctx);
> > -            return;
> > -        }
> > -
> > -        mm_warn ("couldn't load SIM identifier: '%s'",
> > -                 error ? error->message : "unknown error");
> > -        g_clear_error (&error);
> > -    }
> > -
> > -    mm_gdbus_sim_set_sim_identifier (MM_GDBUS_SIM (self), simid);
> > -    g_free (simid);
> > -
> > -    /* Go on to next step */
> > -    ctx->step++;
> > -    interface_initialization_step (ctx);
> > -}
> > -
> >  #undef STR_REPLY_READY_FN
> >  #define STR_REPLY_READY_FN(NAME,DISPLAY)
> > \
> >      static void
> > \
> > @@ -1455,19 +1468,30 @@ load_sim_identifier_ready (MMSim *self,
> >          gchar *val;
> > \
> >
> > \
> >          val = MM_SIM_GET_CLASS (ctx->self)->load_##NAME##_finish (self,
> > res, &error); \
> > -        mm_gdbus_sim_set_##NAME (MM_GDBUS_SIM (self), val);
> > \
> > -        g_free (val);
> > \
> > +        if (!val) {
> > \
> > +            if (++ctx->step_tries < 3) {
> > \
> > +                g_clear_error (&error);
> > \
> > +                g_timeout_add_seconds (1,
> > \
> > +
> > (GSourceFunc)interface_initialization_step_again, \
> > +                                       ctx);
> > \
> > +                return;
> > \
> > +            }
> > \
> >
> > \
> > -        if (error) {
> > \
> > -            mm_warn ("couldn't load %s: '%s'", DISPLAY,
> > error->message); \
> > -            g_error_free (error);
> > \
> > +            mm_warn ("couldn't load %s: '%s'",
> > \
> > +                     DISPLAY,
> > \
> > +                     error ? error->message : "unknown error");
> > \
> > +            g_clear_error (&error);
> > \
> >          }
> > \
> >
> > \
> > +        mm_gdbus_sim_set_##NAME (MM_GDBUS_SIM (self), val);
> > \
> > +        g_free (val);
> > \
> > +
> > \
> >          /* Go on to next step */
> > \
> > -        ctx->step++;
> > \
> > +        init_async_context_next_step (ctx);
> > \
> >          interface_initialization_step (ctx);
> > \
> >      }
> >
> > +STR_REPLY_READY_FN (sim_identifier, "SIM identifier")
> >  STR_REPLY_READY_FN (imsi, "IMSI")
> >  STR_REPLY_READY_FN (operator_identifier, "Operator identifier")
> >  STR_REPLY_READY_FN (operator_name, "Operator name")
> > @@ -1478,7 +1502,7 @@ interface_initialization_step (InitAsyncContext
> > *ctx)
> >      switch (ctx->step) {
> >      case INITIALIZATION_STEP_FIRST:
> >          /* Fall down to next step */
> > -        ctx->step++;
> > +        init_async_context_next_step (ctx);
> >
> >      case INITIALIZATION_STEP_SIM_IDENTIFIER:
> >          /* SIM ID is meant to be loaded only once during the whole
> > @@ -1494,7 +1518,7 @@ interface_initialization_step (InitAsyncContext
> > *ctx)
> >              return;
> >          }
> >          /* Fall down to next step */
> > -        ctx->step++;
> > +        init_async_context_next_step (ctx);
> >
> >      case INITIALIZATION_STEP_IMSI:
> >          /* IMSI is meant to be loaded only once during the whole
> > @@ -1510,7 +1534,7 @@ interface_initialization_step (InitAsyncContext
> > *ctx)
> >              return;
> >          }
> >          /* Fall down to next step */
> > -        ctx->step++;
> > +        init_async_context_next_step (ctx);
> >
> >      case INITIALIZATION_STEP_OPERATOR_ID:
> >          /* Operator ID is meant to be loaded only once during the whole
> > @@ -1526,7 +1550,7 @@ interface_initialization_step (InitAsyncContext
> > *ctx)
> >              return;
> >          }
> >          /* Fall down to next step */
> > -        ctx->step++;
> > +        init_async_context_next_step (ctx);
> >
> >      case INITIALIZATION_STEP_OPERATOR_NAME:
> >          /* Operator Name is meant to be loaded only once during the
> > whole
> > @@ -1542,7 +1566,7 @@ interface_initialization_step (InitAsyncContext
> > *ctx)
> >              return;
> >          }
> >          /* Fall down to next step */
> > -        ctx->step++;
> > +        init_async_context_next_step (ctx);
> >
> >      case INITIALIZATION_STEP_LAST:
> >          /* We are done without errors! */
> > @@ -1575,7 +1599,7 @@ common_init_async (GAsyncInitable *initable,
> >                          g_object_ref (cancellable) :
> >                          NULL);
> >      ctx->step = INITIALIZATION_STEP_FIRST;
> > -    ctx->sim_identifier_tries = 0;
> > +    ctx->step_tries = 0;
> >
> >      interface_initialization_step (ctx);
> >  }
> >
>
>
> --
> Aleksander