[PATCH] sim hot swap: improved error management
Eric Caruso
ejcaruso at chromium.org
Wed Jul 12 17:25:35 UTC 2017
That sounds fine to me.
The improved error message for SIM hot swap being configured but the
ports context failing to open is good too -- the logs could be
misleading if MM couldn't open AT ports but enabling hot swap didn't
require them (as is true for MBIM modems).
On Wed, Jul 12, 2017 at 6:37 AM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
> + Eric for comments
>
> On Wed, Jul 12, 2017 at 2:28 PM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
>> Currently, when SIM hot swap fails in either mm-iface or plugin, the
>> ModemManager still opens ports context and prints a message saying that
>> SIM hot swap is supported and that it's waiting for SIM insertion,
>> instead of clearly saying that SIM hot swap is not working.
>>
>> This patch:
>>
>> 1. introduces a new property MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED
>> which is FALSE by default and set to TRUE only when
>> setup_sim_hot_swap_finish() succeded.
>> 2. subordinates the completion of SIM hot swap setup (in
>> mm-broadband-modem) and the related messages to the the value of
>> MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED
>
> It looks to me that the "sim_hot_swap_on" in mm-broadband-modem-mbim.c
> would serve the same purpose of this new
> MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED property, isn't that right?
>
> i.e. if SIM how swap enabling fails, both those booleans get set to
> FALSE, so that the code afterwards skips steps as sim hot swap isn't
> configured. What do you guys think? Maybe we should also update the
> MBIM implementation to use this new
> MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED property?
>
>
>> ---
>> plugins/telit/mm-broadband-modem-telit.c | 1 +
>> src/mm-broadband-modem.c | 91 ++++++++++++++++++++++----------
>> src/mm-iface-modem.c | 14 ++++-
>> src/mm-iface-modem.h | 1 +
>> 4 files changed, 78 insertions(+), 29 deletions(-)
>>
>> diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c
>> index a8ee886..9f5b588 100644
>> --- a/plugins/telit/mm-broadband-modem-telit.c
>> +++ b/plugins/telit/mm-broadband-modem-telit.c
>> @@ -1373,6 +1373,7 @@ mm_broadband_modem_telit_new (const gchar *device,
>> MM_BASE_MODEM_VENDOR_ID, vendor_id,
>> MM_BASE_MODEM_PRODUCT_ID, product_id,
>> MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED, TRUE,
>> + MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED, FALSE,
>> NULL);
>> }
>>
>> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
>> index 08aa23a..9f842c9 100644
>> --- a/src/mm-broadband-modem.c
>> +++ b/src/mm-broadband-modem.c
>> @@ -116,6 +116,7 @@ enum {
>> PROP_MODEM_VOICE_CALL_LIST,
>> PROP_MODEM_SIMPLE_STATUS,
>> PROP_MODEM_SIM_HOT_SWAP_SUPPORTED,
>> + PROP_MODEM_SIM_HOT_SWAP_CONFIGURED,
>> PROP_FLOW_CONTROL,
>> PROP_LAST
>> };
>> @@ -134,6 +135,7 @@ struct _MMBroadbandModemPrivate {
>> PortsContext *sim_hot_swap_ports_ctx;
>> gboolean modem_init_run;
>> gboolean sim_hot_swap_supported;
>> + gboolean sim_hot_swap_configured;
>>
>> /*<--- Modem interface --->*/
>> /* Properties */
>> @@ -10242,27 +10244,38 @@ initialize_step (InitializeContext *ctx)
>> * (we may be re-running the initialization step after SIM-PIN unlock) */
>> if (!ctx->self->priv->sim_hot_swap_ports_ctx) {
>> gboolean is_sim_hot_swap_supported = FALSE;
>> + gboolean is_sim_hot_swap_configured = FALSE;
>>
>> g_object_get (ctx->self,
>> MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED, &is_sim_hot_swap_supported,
>> NULL);
>>
>> + g_object_get (ctx->self,
>> + MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED, &is_sim_hot_swap_configured,
>> + NULL);
>> +
>> if (is_sim_hot_swap_supported) {
>> - PortsContext *ports;
>> - GError *error = NULL;
>>
>> - mm_dbg ("Creating ports context for SIM hot swap");
>> + if (!is_sim_hot_swap_configured) {
>> + mm_warn ("SIM hot swap supported but not configured. Skipping opening ports");
>> + } else {
>> + PortsContext *ports;
>> + GError *error = NULL;
>>
>> - ports = g_new0 (PortsContext, 1);
>> - ports->ref_count = 1;
>> + mm_dbg ("Creating ports context for SIM hot swap");
>>
>> - if (!open_ports_enabling (ctx->self, ports, FALSE, &error)) {
>> - mm_warn ("Couldn't open ports during Modem SIM hot swap enabling: %s", error? error->message : "unknown reason");
>> - g_error_free (error);
>> - } else
>> - ctx->self->priv->sim_hot_swap_ports_ctx = ports_context_ref (ports);
>> + ports = g_new0 (PortsContext, 1);
>> + ports->ref_count = 1;
>>
>> - ports_context_unref (ports);
>> + if (!open_ports_enabling (ctx->self, ports, FALSE, &error)) {
>> + mm_warn ("Couldn't open ports during Modem SIM hot swap enabling: %s", error? error->message : "unknown reason");
>> + g_error_free (error);
>> + } else {
>> + ctx->self->priv->sim_hot_swap_ports_ctx = ports_context_ref (ports);
>> + }
>> +
>> + ports_context_unref (ports);
>> + }
>> }
>> } else
>> mm_dbg ("Ports context for SIM hot swap already available");
>> @@ -10291,34 +10304,47 @@ initialize_step (InitializeContext *ctx)
>> } else {
>> /* Fatal SIM, firmware, or modem failure :-( */
>> gboolean is_sim_hot_swap_supported = FALSE;
>> + gboolean is_sim_hot_swap_configured = FALSE;
>> +
>> MMModemStateFailedReason reason =
>> mm_gdbus_modem_get_state_failed_reason (
>> (MmGdbusModem*)ctx->self->priv->modem_dbus_skeleton);
>>
>> g_object_get (ctx->self,
>> - MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED,
>> - &is_sim_hot_swap_supported,
>> - NULL);
>> -
>> - if (reason == MM_MODEM_STATE_FAILED_REASON_SIM_MISSING &&
>> - is_sim_hot_swap_supported &&
>> - ctx->self->priv->sim_hot_swap_ports_ctx) {
>> - mm_info ("SIM is missing, but the modem supports SIM hot swap. Waiting for SIM...");
>> + MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED,
>> + &is_sim_hot_swap_supported,
>> + NULL);
>> +
>> + g_object_get (ctx->self,
>> + MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED,
>> + &is_sim_hot_swap_configured,
>> + NULL);
>> +
>> + if (reason == MM_MODEM_STATE_FAILED_REASON_SIM_MISSING) {
>> + if (!is_sim_hot_swap_supported) {
>> + mm_dbg ("SIM is missing, but this modem does not support SIM hot swap.");
>> + } else if (!is_sim_hot_swap_configured) {
>> + mm_warn ("SIM is missing, but SIM hot swap could not be configured.");
>> + } else if (!ctx->self->priv->sim_hot_swap_ports_ctx) {
>> + mm_err ("SIM is missing and SIM hot swap is configured, but ports are not opened.");
>> + } else {
>> + mm_dbg ("SIM is missing, but SIM hot swap is enabled. Waiting for SIM...");
>> g_simple_async_result_set_error (ctx->result,
>> MM_CORE_ERROR,
>> MM_CORE_ERROR_WRONG_STATE,
>> "Modem is unusable due to SIM missing, "
>> - "cannot fully initialize, "
>> - "waiting for SIM insertion.");
>> - } else {
>> - mm_dbg ("SIM is missing and Modem does not support SIM Hot Swap");
>> - g_simple_async_result_set_error (ctx->result,
>> - MM_CORE_ERROR,
>> - MM_CORE_ERROR_WRONG_STATE,
>> - "Modem is unusable, "
>> - "cannot fully initialize");
>> + "cannot fully initialize, waiting for SIM insertion.");
>> + goto sim_hot_swap_enabled;
>> + }
>> +
>> }
>>
>> + g_simple_async_result_set_error (ctx->result,
>> + MM_CORE_ERROR,
>> + MM_CORE_ERROR_WRONG_STATE,
>> + "Modem is unusable, "
>> + "cannot fully initialize");
>> +sim_hot_swap_enabled:
>> /* Ensure we only leave the Modem, OMA, and Firmware interfaces
>> * around. A failure could be caused by firmware issues, which
>> * a firmware update, switch, or provisioning could fix.
>> @@ -10644,6 +10670,9 @@ set_property (GObject *object,
>> case PROP_MODEM_SIM_HOT_SWAP_SUPPORTED:
>> self->priv->sim_hot_swap_supported = g_value_get_boolean (value);
>> break;
>> + case PROP_MODEM_SIM_HOT_SWAP_CONFIGURED:
>> + self->priv->sim_hot_swap_configured = g_value_get_boolean (value);
>> + break;
>> default:
>> G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>> break;
>> @@ -10749,6 +10778,9 @@ get_property (GObject *object,
>> case PROP_MODEM_SIM_HOT_SWAP_SUPPORTED:
>> g_value_set_boolean (value, self->priv->sim_hot_swap_supported);
>> break;
>> + case PROP_MODEM_SIM_HOT_SWAP_CONFIGURED:
>> + g_value_set_boolean (value, self->priv->sim_hot_swap_configured);
>> + break;
>> case PROP_FLOW_CONTROL:
>> g_value_set_flags (value, self->priv->flow_control);
>> break;
>> @@ -11248,6 +11280,9 @@ mm_broadband_modem_class_init (MMBroadbandModemClass *klass)
>> PROP_MODEM_SIM_HOT_SWAP_SUPPORTED,
>> MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED);
>>
>> + g_object_class_override_property (object_class,
>> + PROP_MODEM_SIM_HOT_SWAP_CONFIGURED,
>> + MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED);
>> properties[PROP_FLOW_CONTROL] =
>> g_param_spec_flags (MM_BROADBAND_MODEM_FLOW_CONTROL,
>> "Flow control",
>> diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c
>> index e79e55c..73183c0 100644
>> --- a/src/mm-iface-modem.c
>> +++ b/src/mm-iface-modem.c
>> @@ -4335,6 +4335,9 @@ setup_sim_hot_swap_ready (MMIfaceModem *self,
>> g_error_free (error);
>> } else {
>> mm_dbg ("Iface modem: SIM hot swap setup succeded");
>> + g_object_set (self,
>> + MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED, TRUE,
>> + NULL);
>> }
>>
>> /* Go on to next step */
>> @@ -4880,8 +4883,9 @@ interface_initialization_step (GTask *task)
>> MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED, &is_sim_hot_swap_supported,
>> NULL);
>>
>> - if (is_sim_hot_swap_supported)
>> + if (is_sim_hot_swap_supported) {
>> mm_iface_modem_update_failed_state (self, MM_MODEM_STATE_FAILED_REASON_SIM_MISSING);
>> + }
>> }
>> } else {
>> /* We are done without errors!
>> @@ -5284,6 +5288,7 @@ iface_modem_init (gpointer g_iface)
>> "List of bearers handled by the modem",
>> MM_TYPE_BEARER_LIST,
>> G_PARAM_READWRITE));
>> +
>> g_object_interface_install_property
>> (g_iface,
>> g_param_spec_boolean (MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED,
>> @@ -5292,6 +5297,13 @@ iface_modem_init (gpointer g_iface)
>> FALSE,
>> G_PARAM_READWRITE));
>>
>> + g_object_interface_install_property
>> + (g_iface,
>> + g_param_spec_boolean (MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED,
>> + "Sim Hot Swap Configured",
>> + "Whether the sim hot swap support is configured correctly.",
>> + FALSE,
>> + G_PARAM_READWRITE));
>> initialized = TRUE;
>> }
>>
>> diff --git a/src/mm-iface-modem.h b/src/mm-iface-modem.h
>> index 17ded5b..711d2f2 100644
>> --- a/src/mm-iface-modem.h
>> +++ b/src/mm-iface-modem.h
>> @@ -37,6 +37,7 @@
>> #define MM_IFACE_MODEM_SIM "iface-modem-sim"
>> #define MM_IFACE_MODEM_BEARER_LIST "iface-modem-bearer-list"
>> #define MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED "iface-modem-sim-hot-swap-supported"
>> +#define MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED "iface-modem-sim-hot-swap-configured"
>>
>> typedef struct _MMIfaceModem MMIfaceModem;
>>
>> --
>> 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