Re: [MM] [PATCH] sim: retry SIM operations during initialization
- From: Aleksander Morgado <aleksander lanedo com>
- To: Ben Chan <benchan chromium org>
- Cc: networkmanager-list gnome org
- Subject: Re: [MM] [PATCH] sim: retry SIM operations during initialization
- Date: Thu, 23 Aug 2012 14:36:34 +0200
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
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
