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 <[email protected]> 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 _______________________________________________ networkmanager-list mailing list [email protected] https://mail.gnome.org/mailman/listinfo/networkmanager-list
