[PATCH v2] telit-plugin: handle QSS unsolicited due to power state transitions
Aleksander Morgado
aleksander at aleksander.es
Fri Sep 1 07:01:50 UTC 2017
Hey Carlo!
On Wed, Aug 30, 2017 at 2:40 PM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> When transitioning between power-low and power-on modes, Telit modems
> switch the SIM off/on, which leads to the emission of #QSS unsolicited not
> related to an actual SIM swap.
>
> To handle this use case, this patch:
>
> * disables reacting on #QSS unsolicited when modem_power_down is received
> * implements modem_after_power_up that:
> - checks whether the SIM has been changed, matching cached SIM Identifier
> with the value in the current SIM. If SIM Identifier, is different,
> sim_hot_swap_ports_detected is called.
> - re-enables reacting on #QSS unsolicited
>
> ---
>
> Hi Aleksander,
>
> here is my second patch version for the problem.
> Not 100% sure this is what you expected, please let me know what do you think.
>
See comments below. I think it makes sense to check SIM ID like this,
yes, the logic does sound good.
> ---
> plugins/telit/mm-broadband-modem-telit.c | 200 ++++++++++++++++++++++++++++++-
> 1 file changed, 196 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c
> index a9fc5f0..2f3dcca 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -58,6 +58,7 @@ struct _MMBroadbandModemTelitPrivate {
> MMTelitCsimLockState csim_lock_state;
> GTask *csim_lock_task;
> guint csim_lock_timeout_id;
> + gboolean parse_qss;
> };
>
> /*****************************************************************************/
> @@ -151,10 +152,15 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,
> }
>
> if (cur_qss_status != prev_qss_status)
> - mm_dbg ("QSS handler: status changed '%s -> %s",
> + mm_dbg ("QSS handler: status changed '%s -> %s'",
> mm_telit_qss_status_get_string (prev_qss_status),
> mm_telit_qss_status_get_string (cur_qss_status));
>
> + if (self->priv->parse_qss == FALSE) {
> + mm_dbg ("QSS: ignore QSS is enabled");
How about a less cryptic message like "QSS message ignored"?
> + return;
> + }
> +
> if ((prev_qss_status == QSS_STATUS_SIM_REMOVED && cur_qss_status != QSS_STATUS_SIM_REMOVED) ||
> (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status == QSS_STATUS_SIM_REMOVED)) {
> mm_info ("QSS handler: SIM swap detected");
> @@ -903,14 +909,193 @@ modem_load_unlock_retries (MMIfaceModem *self,
> }
>
> /*****************************************************************************/
> +/* Modem power up (Modem interface) */
> +static void
> +modem_power_up (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + mm_base_modem_at_command (MM_BASE_MODEM (self),
> + "+CFUN=1",
> + 5,
> + FALSE,
> + (GAsyncReadyCallback) callback,
> + user_data);
> +}
> +
Not sure you need to subclass "modem_power_up", I'd suggest you remove this.
> +/*****************************************************************************/
> +/* 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 {
> + MMBroadbandModemTelit *self;
When using GTask, we don't store the "self" pointer in the context
struct. Instead, you can retrieve it from the GTask directly with
"g_task_get_source_object" when needed.
> + gboolean has_sim_changed;
> + guint retries;
> + ModemAfterPowerUpStep step;
> + GError *error;
> +} AfterPowerUpContext;
> +
> +static void after_power_up_step (GTask *task);
> +
> +static void
> +after_power_up_context_free (AfterPowerUpContext *ctx)
> +{
> + g_clear_object (&(ctx->error));
A GError is not a GObject! Use g_clear_error() instead. Actually... I
don't see you're setting this GError anyway, so I'd suggest you just
remove it.
> + g_object_unref (ctx->self);
> + g_slice_free (AfterPowerUpContext, ctx);
If you remove both the "self" pointer and the GError you end up with a
struct that doesn't need anything else to be freed, so instead of
using a GSlice you can just g_new0() to allocate and use g_free as
GDestroyNotify in the g_task_set_task_data() call below, i.e. no need
for a custom after_power_up_context_free() method.
> +}
> +
> +static void
> +load_sim_identifier_ready (MMBaseSim *self,
Don't call it "self" if you're implementing a modem object source
file, call it "sim" instead. "self" is the modem in this source file.
> + GAsyncResult *res,
> + GTask *task)
> +{
> + AfterPowerUpContext *ctx;
> + GError *error = NULL;
> + gchar *current_simid;
> + gchar *cached_simid;
> +
> + ctx = g_task_get_task_data (task);
> +
> + cached_simid = (gchar *)mm_gdbus_sim_get_sim_identifier (MM_GDBUS_SIM (self));
> + current_simid = MM_BASE_SIM_GET_CLASS (self)->load_sim_identifier_finish (self, res, &error);
Instead of calling the MMBaseSim class method from within the modem
object implementation, I'd suggest you implement a pair of
mm_base_sim_load_sim_identifier() and
mm_base_sim_load_sim_identifier_finish() methods in mm-base-sim.[h|c].
These two new methods would be the ones calling MM_BASE_SIM_GET_CLASS
(self)->load_sim_identifier() and MM_BASE_SIM_GET_CLASS
(self)->load_sim_identifier_finish(), ONLY if they're implemented.
> + if (error) {
> + if (ctx->retries > 0) {
> + ctx->retries--;
> + mm_warn ("Couldn't read 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 ("Couldn't read 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);
> + g_object_unref (self);
You don't need to unref (self) here if you did it after calling the
async method, see below the comment about the refcount balancing. It
is guaranteed that the object for which the completion handler is
being called will be valid as long as this completion handler is run
(i.e. as long as this ready() method is being run).
> + ctx->step++;
> + after_power_up_step (task);
> +}
> +
> +static void
> +after_power_up_step (GTask *task)
> +{
> + AfterPowerUpContext *ctx;
> + MMBaseSim *sim;
You're going to set the pointer by reference later on with the
g_object_get() call, so initialize it to NULL here just in case. Truth
be told, g_object_get() will do it for you, but just for consistency.
> +
> + ctx = g_task_get_task_data (task);
> +
> + g_object_get (MM_BROADBAND_MODEM_TELIT (ctx->self),
> + MM_IFACE_MODEM_SIM, &sim,
> + NULL);
> + if (!sim) {
> + mm_dbg ("Could not acquire MMBaseSim object");
> + g_task_return_new_error (task,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Could not acquire MMBaseSim object");
Don't mm_dbg() and return error at the same time, probably not needed.
Also, don't say "MMBaseSim", just "sim" would be enough.
> + return;
> + }
> +
> + switch (ctx->step) {
> + case AFTER_POWER_UP_STEP_FIRST:
> + /* Fall back on next step */
> + ctx->step++;
> + case AFTER_POWER_UP_STEP_GET_SIM_IDENTIFIER:
> + MM_BASE_SIM_GET_CLASS (sim)->load_sim_identifier (sim,
As said above, better a mm_base_sim_load_sim_identifier() method call.
> + (GAsyncReadyCallback)load_sim_identifier_ready,
> + task);
You're missing a g_object_unref (sim) here. The async operation itself
should take care a reference to the sim object itself, so you balance
out your sim refcount here.
> + return;
> + case AFTER_POWER_UP_STEP_ENABLE_QSS_PARSING:
> + mm_dbg ("Stop ignoring #QSS unsolicited");
> + ctx->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 (ctx->self));
> +
> + g_task_return_boolean (task, TRUE);
> + g_object_unref (task);
> + break;
> + default:
> + g_assert_not_reached ();
> + }
> +}
> +
> +static gboolean
> +modem_after_power_up_finish (MMIfaceModem *self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + return g_task_propagate_boolean (G_TASK (res), error);
> +}
Move the finish method up in the source code, above the
load_sim_identifier_ready() method. This is to keep the flow of
reading async methods from bottom to top.
> +
> +static void
> +modem_after_power_up (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + GTask *task;
> + AfterPowerUpContext *ctx;
> +
> + ctx = g_slice_new0 (AfterPowerUpContext);
As said before, just g_new0()
> + ctx->self = g_object_ref (MM_BROADBAND_MODEM_TELIT (self));
As said before, no need for the self pointer in the context.
> + ctx->step = AFTER_POWER_UP_STEP_FIRST;
> + ctx->has_sim_changed = FALSE;
> + ctx->retries = 3;
> +
> + task = g_task_new (self, NULL, callback, user_data);
> + g_task_set_task_data (task, ctx, (GDestroyNotify) after_power_up_context_free);
As said before, just g_free as GDestroyNotify if the context doesn't
have any other thing to free inside.
> +
> + after_power_up_step (task);
> +}
> +
> +
> +/*****************************************************************************/
> /* Modem power down (Modem interface) */
>
> +static void
> +telit_modem_power_down_ready (MMBaseModem *self,
> + GAsyncResult *res,
> + GTask *task)
> +{
> + GError *error = NULL;
> +
> + mm_base_modem_at_command_finish (self, res, &error);
> + /* By default, errors in the power up/down command are ignored in parent broadband modem.
> + * The Telit plugin keeps the same behavior, but ignore next QSS only when the command
> + * did not fail. */
> + if (!error) {
> + mm_dbg ("Ignore #QSS unsolicited during power down/low");
> + MM_BROADBAND_MODEM_TELIT (self)->priv->parse_qss = FALSE;
> + }
The GError is leaking in the "else" branch. If you're not even
printing the error message, you can probably just do:
if (!mm_base_modem_at_command_finish (self, res, NULL)) {
mm_dbg ("Ignore #QSS unsolicited during power down/low");
MM_BROADBAND_MODEM_TELIT (self)->priv->parse_qss = FALSE;
}
> +
> + g_task_return_boolean (task, TRUE);
> + g_object_unref (task);
> +}
> +
> static gboolean
> modem_power_down_finish (MMIfaceModem *self,
> GAsyncResult *res,
> GError **error)
> {
> - return !!mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, error);
> + return g_task_propagate_boolean (G_TASK (res), error);
> }
>
> static void
> @@ -918,12 +1103,15 @@ modem_power_down (MMIfaceModem *self,
> GAsyncReadyCallback callback,
> gpointer user_data)
> {
> + GTask *task;
> +
> + task = g_task_new (self, NULL, callback, user_data);
> mm_base_modem_at_command (MM_BASE_MODEM (self),
> "+CFUN=4",
> 20,
> FALSE,
> - callback,
> - user_data);
> + (GAsyncReadyCallback) telit_modem_power_down_ready,
> + task);
> }
>
> /*****************************************************************************/
> @@ -1457,6 +1645,7 @@ mm_broadband_modem_telit_init (MMBroadbandModemTelit *self)
> self->priv->csim_lock_support = FEATURE_SUPPORT_UNKNOWN;
> self->priv->csim_lock_state = CSIM_LOCK_STATE_UNKNOWN;
> self->priv->qss_status = QSS_STATUS_UNKNOWN;
> + self->priv->parse_qss = TRUE;
> }
>
> static void
> @@ -1474,6 +1663,9 @@ iface_modem_init (MMIfaceModem *iface)
> iface->load_unlock_retries = modem_load_unlock_retries;
> iface->reset = modem_reset;
> iface->reset_finish = modem_reset_finish;
> + iface->modem_power_up = modem_power_up;
As said, no need to subclass modem_power_up. Plus, hint: whenever you
subclass an async method, you need to subclass also it's associated
finish() method or you would relying on the parent method to have the
same kind of implementation that you want to have, and this may not be
true (and if it is, it may not be true in the future).
> + iface->modem_after_power_up = modem_after_power_up;
> + iface->modem_after_power_up_finish = modem_after_power_up_finish;
> iface->modem_power_down = modem_power_down;
> iface->modem_power_down_finish = modem_power_down_finish;
> iface->load_access_technologies = load_access_technologies;
> --
> 2.9.3
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list