Hey Eric, See comments inline.
On Tue, Nov 28, 2017 at 2:39 AM, Eric Caruso <[email protected]> wrote: > When in low-power mode, some modems will not dispatch unsolicited > notifications, such as for SIM hot swapping. There is code in > MMBroadbandModemTelit to handle this by checking the SIM identifier > during modem power up against the identifier cached in the SIM > D-Bus object. If they're different, the SIM has likely been > swapped while we were powered down. > > We can move this code out to MMBroadbandModem because it doesn't > actually rely on any Telit-specific details, and invoke it from > MMIfaceModem via a new method. > --- > plugins/telit/mm-broadband-modem-telit.c | 128 ++--------------------------- > src/mm-broadband-modem.c | 134 > +++++++++++++++++++++++++++++++ > src/mm-iface-modem.c | 33 ++++++++ > src/mm-iface-modem.h | 10 +++ > 4 files changed, 183 insertions(+), 122 deletions(-) > > diff --git a/plugins/telit/mm-broadband-modem-telit.c > b/plugins/telit/mm-broadband-modem-telit.c > index 995ae6a9..08917ca4 100644 > --- a/plugins/telit/mm-broadband-modem-telit.c > +++ b/plugins/telit/mm-broadband-modem-telit.c > @@ -911,20 +911,6 @@ modem_load_unlock_retries (MMIfaceModem *self, > > > /*****************************************************************************/ > /* Modem after power up (Modem interface) */ > -typedef enum { > - AFTER_POWER_UP_STEP_FIRST, > - AFTER_POWER_UP_STEP_GET_SIM_IDENTIFIER, > - AFTER_POWER_UP_STEP_ENABLE_QSS_PARSING, > - AFTER_POWER_UP_STEP_LAST > -} ModemAfterPowerUpStep; > - > -typedef struct { > - gboolean has_sim_changed; > - guint retries; > - ModemAfterPowerUpStep step; > -} AfterPowerUpContext; > - > -static void after_power_up_step (GTask *task); > > static gboolean > modem_after_power_up_finish (MMIfaceModem *self, > @@ -934,123 +920,21 @@ modem_after_power_up_finish (MMIfaceModem *self, > return g_task_propagate_boolean (G_TASK (res), error); > } > > -static void > -load_sim_identifier_ready (MMBaseSim *sim, > - GAsyncResult *res, > - GTask *task) > -{ > - AfterPowerUpContext *ctx; > - GError *error = NULL; > - gchar *current_simid; > - const gchar *cached_simid; > - > - ctx = g_task_get_task_data (task); > - > - cached_simid = (gchar *)mm_gdbus_sim_get_sim_identifier (MM_GDBUS_SIM > (sim)); > - current_simid = mm_base_sim_load_sim_identifier_finish (sim, res, > &error); > - > - if (error) { > - if (g_error_matches (error, MM_CORE_ERROR, > MM_CORE_ERROR_UNSUPPORTED)) { > - mm_warn ("could not load SIM identifier: %s", error->message); > - g_clear_error (&error); > - goto out; > - } > - > - if (ctx->retries-- > 0) { > - mm_warn ("could not load SIM identifier: %s (%d retries left)", > - error->message, ctx->retries); > - g_clear_error (&error); > - g_timeout_add_seconds (1, (GSourceFunc) after_power_up_step, > task); > - return; > - } > - > - mm_warn ("could not load SIM identifier: %s", error->message); > - g_clear_error (&error); > - goto out; > - } > - > - if (g_strcmp0 (current_simid, cached_simid) != 0) { > - mm_warn ("sim identifier has changed: possible SIM swap during power > down/low"); > - ctx->has_sim_changed = TRUE; > - } > - > -out: > - g_free (current_simid); > - ctx->step++; > - after_power_up_step (task); > -} > - > -static void > -after_power_up_step (GTask *task) > -{ > - AfterPowerUpContext *ctx; > - MMBroadbandModemTelit *self; > - > - ctx = g_task_get_task_data (task); > - self = g_task_get_source_object (task); > - > - switch (ctx->step) { > - case AFTER_POWER_UP_STEP_FIRST: > - /* Fall back on next step */ > - ctx->step++; > - > - case AFTER_POWER_UP_STEP_GET_SIM_IDENTIFIER: { > - MMBaseSim *sim = NULL; > - > - g_object_get (MM_BROADBAND_MODEM_TELIT (self), > - MM_IFACE_MODEM_SIM, &sim, > - NULL); > - if (!sim) { > - g_task_return_new_error (task, > - MM_CORE_ERROR, > - MM_CORE_ERROR_FAILED, > - "could not acquire sim object"); > - g_object_unref (task); > - return; > - } > - > - mm_base_sim_load_sim_identifier (sim, > - > (GAsyncReadyCallback)load_sim_identifier_ready, > - task); > - g_object_unref (sim); > - return; > - } > - > - case AFTER_POWER_UP_STEP_ENABLE_QSS_PARSING: > - mm_dbg ("Stop ignoring #QSS"); > - self->priv->parse_qss = TRUE; > - > - /* Fall back on next step */ > - ctx->step++; > - case AFTER_POWER_UP_STEP_LAST: > - if (ctx->has_sim_changed) > - mm_broadband_modem_update_sim_hot_swap_detected > (MM_BROADBAND_MODEM (self)); > - > - g_task_return_boolean (task, TRUE); > - g_object_unref (task); > - break; > - default: > - g_assert_not_reached (); > - } > -} > - > static void > modem_after_power_up (MMIfaceModem *self, > GAsyncReadyCallback callback, > gpointer user_data) > { > GTask *task; > - AfterPowerUpContext *ctx; > - > - ctx = g_new0 (AfterPowerUpContext, 1); > - ctx->step = AFTER_POWER_UP_STEP_FIRST; > - ctx->has_sim_changed = FALSE; > - ctx->retries = 3; > + MMBroadbandModemTelit *modem = MM_BROADBAND_MODEM_TELIT (self); > > task = g_task_new (self, NULL, callback, user_data); > - g_task_set_task_data (task, ctx, (GDestroyNotify) g_free); > > - after_power_up_step (task); > + mm_dbg ("Stop ignoring #QSS"); > + modem->priv->parse_qss = TRUE; > + > + g_task_return_boolean (task, TRUE); > + g_object_unref (task); > } > > > /*****************************************************************************/ > diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c > index 3d96372a..62d8329e 100644 > --- a/src/mm-broadband-modem.c > +++ b/src/mm-broadband-modem.c > @@ -3346,6 +3346,138 @@ modem_power_up (MMIfaceModem *self, > g_object_unref (task); > } > > +/*****************************************************************************/ > +/* Reprobing the modem if the SIM changed across a power-off or power-down */ > + > +typedef struct { > + MMBaseSim *sim; > + guint retries; > +} SimSwapContext; > + > +static void load_sim_identifier (GTask *task); > + > +static void > +sim_swap_context_free (SimSwapContext *ctx) > +{ > + g_clear_object (&ctx->sim); > + g_slice_free (SimSwapContext, ctx); > +} > + > +static gboolean > +modem_check_for_sim_swap_finish (MMIfaceModem *self, > + GAsyncResult *res, > + GError **error) > +{ > + return g_task_propagate_boolean (G_TASK (res), error); > +} > + > +static void > +load_sim_identifier_ready (MMBaseSim *sim, > + GAsyncResult *res, > + GTask *task) > +{ > + MMBroadbandModem *self; > + SimSwapContext *ctx; > + const gchar *cached_simid; > + gchar *current_simid; > + gboolean ret = FALSE; > + GError *error = NULL; > + > + self = MM_BROADBAND_MODEM (g_task_get_source_object (task)); > + ctx = g_task_get_task_data (task); > + cached_simid = mm_gdbus_sim_get_sim_identifier (MM_GDBUS_SIM (sim)); > + current_simid = MM_BASE_SIM_GET_CLASS > (ctx->sim)->load_sim_identifier_finish (sim, res, &error); No ctx->sim here, just sim (you're getting it as input). > + > + if (error) { > + if (g_error_matches (error, MM_CORE_ERROR, > MM_CORE_ERROR_UNSUPPORTED)) { > + mm_warn ("could not load SIM identifier: %s", error->message); > + g_clear_error (&error); I think you should g_task_return_error() here instead of ignoring the error in the implementation, and then we'll mm_warn() in the MMIfaceModem. > + goto out; > + } > + > + if (ctx->retries > 0) { > + mm_warn ("could not load SIM identifier: %s (%d retries left)", > + error->message, ctx->retries); > + --ctx->retries; > + g_clear_error (&error); > + g_timeout_add_seconds (1, (GSourceFunc) load_sim_identifier, > task); > + return; > + } > + > + mm_warn ("could not load SIM identifier: %s", error->message); > + g_clear_error (&error); I think you should g_task_return_error() here instead of ignoring the error in the implementation, and then we'll mm_warn() in the MMIfaceModem. > + goto out; > + } > + > + if (g_strcmp0 (current_simid, cached_simid) != 0) { > + mm_info ("sim identifier has changed: possible SIM swap during power > down/low"); > + mm_broadband_modem_update_sim_hot_swap_detected (self); > + } > + > + ret = TRUE; > + > +out: > + g_free (current_simid); > + g_task_return_boolean (task, ret); > + g_object_unref (task); > +} > + > +static void > +load_sim_identifier (GTask *task) load_sim_identifier() is a GSourceFunc that you're setting in a timeout, and therefore the method must return a gboolean and the code should "return G_SOURCE_REMOVE" always. > +{ > + MMBroadbandModem *self = MM_BROADBAND_MODEM (g_task_get_source_object > (task)); > + SimSwapContext *ctx = g_task_get_task_data (task); > + > + if (!ctx->sim) { > + g_object_get (self, > + MM_IFACE_MODEM_SIM, &ctx->sim, > + NULL); > + } > + > + if (!ctx->sim) { > + g_task_return_new_error (task, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "could not acquire sim object"); > + g_object_unref (task); > + return; > + } This whole if() check should go inside the first if (!ctx->sim) check done. > + > + if (!MM_BASE_SIM_GET_CLASS (ctx->sim)->load_sim_identifier || > + !MM_BASE_SIM_GET_CLASS (ctx->sim)->load_sim_identifier_finish) { > + g_task_return_new_error (task, > + MM_CORE_ERROR, > + MM_CORE_ERROR_FAILED, > + "sim identifier could not be loaded"); > + g_object_unref (task); > + return; > + } This whole if() check should go inside the first if (!ctx->sim) check done. > + > + MM_BASE_SIM_GET_CLASS (ctx->sim)->load_sim_identifier ( > + ctx->sim, > + (GAsyncReadyCallback)load_sim_identifier_ready, > + task); > +} > + > +static void > +modem_check_for_sim_swap (MMIfaceModem *self, > + GAsyncReadyCallback callback, > + gpointer user_data) > +{ > + GTask *task; > + SimSwapContext *ctx; > + > + mm_dbg ("Checking if SIM needs to be reinitialized..."); > + > + task = g_task_new (self, NULL, callback, user_data); > + ctx = g_slice_new (SimSwapContext); > + ctx->sim = NULL; > + ctx->retries = 3; Just use g_slice_new0() and avoid initializing the NULL pointer. > + g_task_set_task_data (task, ctx, (GDestroyNotify)sim_swap_context_free); > + > + load_sim_identifier (task); There is code in load_sim_identifier() that is going to be called just once, e.g. the one setting ctx->sim and doing the additional checks over it. Maybe just add that code in this same method here? There is no need to rerun all those things on every load_sim_identifier() iteration. > +} > + > > /*****************************************************************************/ > /* Sending a command to the modem (Modem interface) */ > > @@ -10757,6 +10889,8 @@ iface_modem_init (MMIfaceModem *iface) > /* Enabling steps */ > iface->modem_power_up = modem_power_up; > iface->modem_power_up_finish = modem_power_up_finish; > + iface->check_for_sim_swap = modem_check_for_sim_swap; > + iface->check_for_sim_swap_finish = modem_check_for_sim_swap_finish; > iface->setup_flow_control = modem_setup_flow_control; > iface->setup_flow_control_finish = modem_setup_flow_control_finish; > iface->load_supported_charsets = modem_load_supported_charsets; > diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c > index afb3be32..c3ecc278 100644 > --- a/src/mm-iface-modem.c > +++ b/src/mm-iface-modem.c > @@ -3557,6 +3557,7 @@ static void interface_enabling_step (GTask *task); > typedef enum { > ENABLING_STEP_FIRST, > ENABLING_STEP_SET_POWER_STATE, > + ENABLING_STEP_CHECK_FOR_SIM_SWAP, > ENABLING_STEP_FLOW_CONTROL, > ENABLING_STEP_SUPPORTED_CHARSETS, > ENABLING_STEP_CHARSET, > @@ -3606,6 +3607,26 @@ enabling_set_power_state_ready (MMIfaceModem *self, > interface_enabling_step (task); > } > > +static void > +check_for_sim_swap_ready (MMIfaceModem *self, > + GAsyncResult *res, > + GTask *task) > +{ > + EnablingContext *ctx; > + GError *error = NULL; A whiteline here is missing. > + MM_IFACE_MODEM_GET_INTERFACE (self)->check_for_sim_swap_finish (self, > res, &error); > + if (error) { This shouldn't be fatal. There may be modems that don't have load_sim_identifier() implemented, and enabling shouldn't fail due to that. Even if loading SIM iD fails for whatever reason, it's just better to ignore here the error at this point. > + g_task_return_error (task, error); > + g_object_unref (task); > + return; > + } > + > + /* Go on to next step */ > + ctx = g_task_get_task_data (task); > + ctx->step++; > + interface_enabling_step (task); > +} > + > static void > setup_flow_control_ready (MMIfaceModem *self, > GAsyncResult *res, > @@ -3709,6 +3730,18 @@ interface_enabling_step (GTask *task) > task); > return; > > + case ENABLING_STEP_CHECK_FOR_SIM_SWAP: > + if (MM_IFACE_MODEM_GET_INTERFACE (self)->check_for_sim_swap && > + MM_IFACE_MODEM_GET_INTERFACE (self)->check_for_sim_swap_finish) { > + MM_IFACE_MODEM_GET_INTERFACE (self)->check_for_sim_swap ( > + self, > + (GAsyncReadyCallback)check_for_sim_swap_ready, > + task); > + return; > + } > + /* Fall down to next step */ > + ctx->step++; > + > case ENABLING_STEP_FLOW_CONTROL: > if (MM_IFACE_MODEM_GET_INTERFACE (self)->setup_flow_control && > MM_IFACE_MODEM_GET_INTERFACE (self)->setup_flow_control_finish) { > diff --git a/src/mm-iface-modem.h b/src/mm-iface-modem.h > index d25fbd49..177bf910 100644 > --- a/src/mm-iface-modem.h > +++ b/src/mm-iface-modem.h > @@ -281,6 +281,16 @@ struct _MMIfaceModem { > GAsyncResult *res, > GError **error); > > + /* Asynchronous check to see if the SIM was swapped. > + * Useful for when the modem changes power states since we might > + * not get the relevant notifications from the modem. */ > + void (*check_for_sim_swap) (MMIfaceModem *self, > + GAsyncReadyCallback callback, > + gpointer user_data); > + gboolean (*check_for_sim_swap_finish) (MMIfaceModem *self, > + GAsyncResult *res, > + GError **error); > + > /* Asynchronous flow control setup */ > void (*setup_flow_control) (MMIfaceModem *self, > GAsyncReadyCallback callback, > -- > 2.15.0.417.g466bffb3ac-goog > -- Aleksander https://aleksander.es _______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
