[PATCH 1/2] Core logic to support SIM hot swap
Aleksander Morgado
aleksander at aleksander.es
Tue Jul 26 12:38:34 UTC 2016
Hey Carlo,
>
> this is the second attempt for this patch.
> I should have fixed all the problems observed in the first code review.
>
Sorry for the late recheck on this. I do like how this looks like, but I
believe there are a couple minor things to consider still. See comments
below.
>
> ---
>
> BaseModem
> added reprobe property.
>
> MMDevice
> added logic to recreate the modem if it is set invalid and "to reprobe"
>
> MMBroadbandModem
> * added initialization step for SIM hot swap:
> 1. keep dedicated ports open to listen to modem's unsolicited
> 2. dedicated error management in case of initialization failure due to SIM missing
> * added function to be called in order to act upon SIM insertion/removal:
> 1. close dedicated ports
> 2. set the modem to be reprobed
> 3. disable modem
> * added SIM HOT SWAP boolean property
>
> MMIfaceModem
> * added initialization step for SIM hot swap, if supported by the plugin
> * dedicated error management in case of initialization failure due to SIM missing
> ---
> src/mm-base-modem.c | 26 +++++++++
> src/mm-base-modem.h | 4 ++
> src/mm-broadband-modem.c | 141 +++++++++++++++++++++++++++++++++++++++++------
> src/mm-broadband-modem.h | 5 ++
> src/mm-device.c | 15 +++++
> src/mm-iface-modem.c | 56 +++++++++++++++++++
> src/mm-iface-modem.h | 17 ++++--
> 7 files changed, 243 insertions(+), 21 deletions(-)
>
> diff --git a/src/mm-base-modem.c b/src/mm-base-modem.c
> index 9cd7c5f..ede014b 100644
> --- a/src/mm-base-modem.c
> +++ b/src/mm-base-modem.c
> @@ -47,6 +47,7 @@ enum {
> PROP_VENDOR_ID,
> PROP_PRODUCT_ID,
> PROP_CONNECTION,
> + PROP_REPROBE,
> PROP_LAST
> };
>
> @@ -70,6 +71,7 @@ struct _MMBaseModemPrivate {
>
> gboolean hotplugged;
> gboolean valid;
> + gboolean reprobe;
>
> guint max_timeouts;
>
> @@ -392,6 +394,24 @@ mm_base_modem_set_valid (MMBaseModem *self,
> }
> }
>
> +void
> +mm_base_modem_set_reprobe (MMBaseModem *self,
> + gboolean reprobe)
> +{
> + g_return_if_fail (MM_IS_BASE_MODEM (self));
> +
> + self->priv->reprobe = reprobe;
> +}
> +
> +gboolean
> +mm_base_modem_get_reprobe (MMBaseModem *self)
> +{
> + g_return_val_if_fail (MM_IS_BASE_MODEM (self), FALSE);
> +
> + return self->priv->reprobe;
> +}
> +
> +
> gboolean
> mm_base_modem_get_valid (MMBaseModem *self)
> {
> @@ -1295,6 +1315,9 @@ set_property (GObject *object,
> case PROP_VALID:
> mm_base_modem_set_valid (self, g_value_get_boolean (value));
> break;
> + case PROP_REPROBE:
> + mm_base_modem_set_reprobe (self, g_value_get_boolean (value));
> + break;
> case PROP_MAX_TIMEOUTS:
> self->priv->max_timeouts = g_value_get_uint (value);
> break;
> @@ -1338,6 +1361,9 @@ get_property (GObject *object,
> case PROP_VALID:
> g_value_set_boolean (value, self->priv->valid);
> break;
> + case PROP_REPROBE:
> + g_value_set_boolean (value, self->priv->reprobe);
> + break;
> case PROP_MAX_TIMEOUTS:
> g_value_set_uint (value, self->priv->max_timeouts);
> break;
> diff --git a/src/mm-base-modem.h b/src/mm-base-modem.h
> index 3c0d16f..c4bcdce 100644
> --- a/src/mm-base-modem.h
> +++ b/src/mm-base-modem.h
> @@ -166,6 +166,10 @@ void mm_base_modem_set_valid (MMBaseModem *self,
> gboolean valid);
> gboolean mm_base_modem_get_valid (MMBaseModem *self);
>
> +void mm_base_modem_set_reprobe (MMBaseModem *self,
> + gboolean reprobe);
> +gboolean mm_base_modem_get_reprobe (MMBaseModem *self);
> +
> const gchar *mm_base_modem_get_device (MMBaseModem *self);
> const gchar **mm_base_modem_get_drivers (MMBaseModem *self);
> const gchar *mm_base_modem_get_plugin (MMBaseModem *self);
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index e1fd7ca..f1a062c 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -112,6 +112,7 @@ enum {
> PROP_MODEM_MESSAGING_SMS_DEFAULT_STORAGE,
> PROP_MODEM_VOICE_CALL_LIST,
> PROP_MODEM_SIMPLE_STATUS,
> + PROP_MODEM_SIM_HOT_SWAP_SUPPORTED,
> PROP_LAST
> };
>
> @@ -124,7 +125,9 @@ typedef struct _PortsContext PortsContext;
> struct _MMBroadbandModemPrivate {
> /* Broadband modem specific implementation */
> PortsContext *enabled_ports_ctx;
> + PortsContext *sim_hot_swap_ports_ctx;
> gboolean modem_init_run;
> + gboolean sim_hot_swap_supported;
>
> /*<--- Modem interface --->*/
> /* Properties */
> @@ -8317,6 +8320,11 @@ disabling_stopped (MMBroadbandModem *self,
> ports_context_unref (self->priv->enabled_ports_ctx);
> self->priv->enabled_ports_ctx = NULL;
> }
> +
> + if (self->priv->sim_hot_swap_ports_ctx) {
> + ports_context_unref (self->priv->sim_hot_swap_ports_ctx);
> + self->priv->sim_hot_swap_ports_ctx = NULL;
> + }
> return TRUE;
> }
>
> @@ -9435,6 +9443,7 @@ typedef enum {
> INITIALIZE_STEP_IFACE_SIGNAL,
> INITIALIZE_STEP_IFACE_OMA,
> INITIALIZE_STEP_IFACE_FIRMWARE,
> + INITIALIZE_STEP_SIM_HOT_SWAP,
> INITIALIZE_STEP_IFACE_SIMPLE,
> INITIALIZE_STEP_LAST,
> } InitializeStep;
> @@ -9776,6 +9785,38 @@ initialize_step (InitializeContext *ctx)
> ctx);
> return;
>
> + case INITIALIZE_STEP_SIM_HOT_SWAP:
> + {
> + gboolean is_sim_hot_swap_supported = FALSE;
> +
> + g_object_get (ctx->self,
> + MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED,
> + &is_sim_hot_swap_supported,
> + NULL);
> +
> + if (!is_sim_hot_swap_supported) {
> + ctx->self->priv->sim_hot_swap_ports_ctx = NULL;
> + } else {
> + PortsContext *ports;
> + GError *error = NULL;
> +
> + mm_dbg ("Creating PortsContext for SIM hot swap.");
> +
> + ports = g_new0 (PortsContext, 1);
> + ports->ref_count = 1;
> +
> + 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);
> + }
> + }
> + /* Fall down to next step */
> + ctx->step++;
> +
> case INITIALIZE_STEP_IFACE_SIMPLE:
> if (ctx->self->priv->modem_state != MM_MODEM_STATE_FAILED)
> mm_iface_modem_simple_initialize (MM_IFACE_MODEM_SIMPLE (ctx->self));
> @@ -9796,23 +9837,45 @@ initialize_step (InitializeContext *ctx)
> "cannot fully initialize");
> } else {
> /* Fatal SIM, firmware, or modem failure :-( */
> - g_simple_async_result_set_error (ctx->result,
> - MM_CORE_ERROR,
> - MM_CORE_ERROR_WRONG_STATE,
> - "Modem is unusable, "
> - "cannot fully initialize");
> - /* 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.
> - */
> - mm_iface_modem_3gpp_shutdown (MM_IFACE_MODEM_3GPP (ctx->self));
> - mm_iface_modem_3gpp_ussd_shutdown (MM_IFACE_MODEM_3GPP_USSD (ctx->self));
> - mm_iface_modem_cdma_shutdown (MM_IFACE_MODEM_CDMA (ctx->self));
> - mm_iface_modem_location_shutdown (MM_IFACE_MODEM_LOCATION (ctx->self));
> - mm_iface_modem_messaging_shutdown (MM_IFACE_MODEM_MESSAGING (ctx->self));
> - mm_iface_modem_voice_shutdown (MM_IFACE_MODEM_VOICE (ctx->self));
> - mm_iface_modem_time_shutdown (MM_IFACE_MODEM_TIME (ctx->self));
> - mm_iface_modem_simple_shutdown (MM_IFACE_MODEM_SIMPLE (ctx->self));
> + gboolean is_sim_hot_swap_supported = 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) {
This check should also look for self->priv->sim_hot_swap_ports_ctx
being set I think. i.e. if SIM missing, and SIM hot swap is supported,
and SIM hot swap has been initialized (we have ports ctx set), then....
> + mm_info ("SIM is missing, but the modem supports SIM hot swap. 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");
> + /* 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.
> + */
> + mm_iface_modem_3gpp_shutdown (MM_IFACE_MODEM_3GPP (ctx->self));
> + mm_iface_modem_3gpp_ussd_shutdown (MM_IFACE_MODEM_3GPP_USSD (ctx->self));
> + mm_iface_modem_cdma_shutdown (MM_IFACE_MODEM_CDMA (ctx->self));
> + mm_iface_modem_location_shutdown (MM_IFACE_MODEM_LOCATION (ctx->self));
> + mm_iface_modem_messaging_shutdown (MM_IFACE_MODEM_MESSAGING (ctx->self));
> + mm_iface_modem_voice_shutdown (MM_IFACE_MODEM_VOICE (ctx->self));
> + mm_iface_modem_time_shutdown (MM_IFACE_MODEM_TIME (ctx->self));
> + mm_iface_modem_simple_shutdown (MM_IFACE_MODEM_SIMPLE (ctx->self));
I'm not sure, but why do we want to avoid shutting down the interfaces
if sim hot swap is supported? If SIM hot swap is supported, but we don't
have a SIM card yet, we should still avoid exposing all the interfaces
except for Modem, OMA and Firmware.
> + }
> }
> initialize_context_complete_and_free (ctx);
> return;
> @@ -9960,6 +10023,39 @@ mm_broadband_modem_create_device_identifier (MMBroadbandModem *self,
> MM_GDBUS_MODEM (MM_BROADBAND_MODEM (self)->priv->modem_dbus_skeleton))));
> }
>
> +
> +/*****************************************************************************/
> +static void
> +after_hotswap_event_disable_ready (MMBaseModem *self,
> + GAsyncResult *res,
> + gpointer user_data)
There's some wrong indentation in the previous lines.
> +{
> + GError *error = NULL;
> + mm_base_modem_disable_finish (self, res, &error);
> + if (error) {
> + mm_err ("Disable modem error: %s", error->message);
> + g_error_free (error);
> + } else {
> + mm_base_modem_set_valid (MM_BASE_MODEM (self), FALSE);
There's some wrong indentation in the previous line.
> + }
> +}
> +
> +
> +void mm_broadband_modem_update_sim_hot_swap_status (MMBroadbandModem *self,
> + gboolean is_sim_missing)
The "void" should go in its own line.
Also, is_sim_missing isn't used anywere. Maybe rename the method to
"sim_hot_swap_detected" so that it is used whenever we're waiting for a
SIM and the SIM is detected-
> +{
> + if (self->priv->sim_hot_swap_ports_ctx) {
> + mm_dbg ("Releasing SIM hot swap ports context");
> + ports_context_unref (self->priv->sim_hot_swap_ports_ctx);
> + self->priv->sim_hot_swap_ports_ctx = NULL;
> + }
> +
> + mm_base_modem_set_reprobe (MM_BASE_MODEM (self), TRUE);
> + mm_base_modem_disable (MM_BASE_MODEM (self),
> + (GAsyncReadyCallback) after_hotswap_event_disable_ready,
> + NULL);
> +}
> +
> /*****************************************************************************/
>
> MMBroadbandModem *
> @@ -10091,6 +10187,9 @@ set_property (GObject *object,
> g_clear_object (&self->priv->modem_simple_status);
> self->priv->modem_simple_status = g_value_dup_object (value);
> break;
> + case PROP_MODEM_SIM_HOT_SWAP_SUPPORTED:
> + self->priv->sim_hot_swap_supported = g_value_get_boolean (value);
> + break;
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> break;
> @@ -10193,6 +10292,9 @@ get_property (GObject *object,
> case PROP_MODEM_SIMPLE_STATUS:
> g_value_set_object (value, self->priv->modem_simple_status);
> break;
> + case PROP_MODEM_SIM_HOT_SWAP_SUPPORTED:
> + g_value_set_boolean (value, self->priv->sim_hot_swap_supported);
> + break;
> default:
> G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> break;
> @@ -10221,6 +10323,7 @@ mm_broadband_modem_init (MMBroadbandModem *self)
> self->priv->modem_messaging_sms_default_storage = MM_SMS_STORAGE_UNKNOWN;
> self->priv->current_sms_mem1_storage = MM_SMS_STORAGE_UNKNOWN;
> self->priv->current_sms_mem2_storage = MM_SMS_STORAGE_UNKNOWN;
> + self->priv->sim_hot_swap_supported = FALSE;
> }
>
> static void
> @@ -10671,4 +10774,8 @@ mm_broadband_modem_class_init (MMBroadbandModemClass *klass)
> g_object_class_override_property (object_class,
> PROP_MODEM_SIMPLE_STATUS,
> MM_IFACE_MODEM_SIMPLE_STATUS);
> +
> + g_object_class_override_property (object_class,
> + PROP_MODEM_SIM_HOT_SWAP_SUPPORTED,
> + MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED);
> }
> diff --git a/src/mm-broadband-modem.h b/src/mm-broadband-modem.h
> index 93ffeb5..ca5bab3 100644
> --- a/src/mm-broadband-modem.h
> +++ b/src/mm-broadband-modem.h
> @@ -127,5 +127,10 @@ gboolean mm_broadband_modem_lock_sms_storages_finish (MMBroadbandModem *self,
> void mm_broadband_modem_unlock_sms_storages (MMBroadbandModem *self,
> gboolean mem1,
> gboolean mem2);
> +/* Helper to update SIM hot swap */
> +void mm_broadband_modem_update_sim_hot_swap_status (MMBroadbandModem *self,
> + gboolean is_sim_missing);
> +
> +
>
> #endif /* MM_BROADBAND_MODEM_H */
> diff --git a/src/mm-device.c b/src/mm-device.c
> index 2456cc8..8435cf1 100644
> --- a/src/mm-device.c
> +++ b/src/mm-device.c
> @@ -489,8 +489,23 @@ modem_valid (MMBaseModem *modem,
> MMDevice *self)
> {
> if (!mm_base_modem_get_valid (modem)) {
> + GDBusObjectManagerServer *object_manager = self->priv->object_manager;
> +
> /* Modem no longer valid */
> mm_device_remove_modem (self);
> +
> + if (mm_base_modem_get_reprobe (modem)) {
> + GError *error = NULL;
> +
> + if (!mm_device_create_modem (self, object_manager, &error)) {
> + mm_warn ("Could not recreate modem for device at '%s': %s",
> + mm_device_get_path (self),
> + error ? error->message : "unknown");
> + g_error_free (error);
> + } else {
> + mm_dbg ("Modem recreated for device '%s'", mm_device_get_path (self));
> + }
> + }
> } else {
> /* Modem now valid, export it, but only if we really have it around.
> * It may happen that the initialization sequence fails because the
> diff --git a/src/mm-iface-modem.c b/src/mm-iface-modem.c
> index 516ed78..3581c7b 100644
> --- a/src/mm-iface-modem.c
> +++ b/src/mm-iface-modem.c
> @@ -3747,6 +3747,7 @@ typedef enum {
> INITIALIZATION_STEP_SUPPORTED_BANDS,
> INITIALIZATION_STEP_SUPPORTED_IP_FAMILIES,
> INITIALIZATION_STEP_POWER_STATE,
> + INITIALIZATION_STEP_SIM_HOT_SWAP,
> INITIALIZATION_STEP_UNLOCK_REQUIRED,
> INITIALIZATION_STEP_SIM,
> INITIALIZATION_STEP_OWN_NUMBERS,
> @@ -4203,6 +4204,27 @@ load_current_bands_ready (MMIfaceModem *self,
> interface_initialization_step (ctx);
> }
>
> +/*****************************************************************************/
> +/* Setup SIM hot swap (Modem interface) */
> +static void
> +setup_sim_hot_swap_ready (MMIfaceModem *self,
> + GAsyncResult *res,
> + InitializationContext *ctx) {
> + GError *error = NULL;
> +
> + MM_IFACE_MODEM_GET_INTERFACE (self)->setup_sim_hot_swap_finish (self, res, &error);
> + if (error) {
> + mm_warn ("Iface modem: SIM hot swap setup failed: '%s'", error->message);
> + g_error_free (error);
> + } else {
> + mm_dbg ("Iface modem: SIM hot swap setup succeded");
> + }
> +
> + /* Go on to next step */
> + ctx->step++;
> + interface_initialization_step (ctx);
> +}
> +
> static void
> interface_initialization_step (InitializationContext *ctx)
> {
> @@ -4544,6 +4566,18 @@ interface_initialization_step (InitializationContext *ctx)
> /* Fall down to next step */
> ctx->step++;
>
> + case INITIALIZATION_STEP_SIM_HOT_SWAP:
> + if (MM_IFACE_MODEM_GET_INTERFACE (ctx->self)->setup_sim_hot_swap &&
> + MM_IFACE_MODEM_GET_INTERFACE (ctx->self)->setup_sim_hot_swap_finish) {
> + MM_IFACE_MODEM_GET_INTERFACE (ctx->self)->setup_sim_hot_swap (
> + MM_IFACE_MODEM (ctx->self),
> + (GAsyncReadyCallback) setup_sim_hot_swap_ready,
> + ctx);
> + return;
> + }
> + /* Fall down to next step */
> + ctx->step++;
> +
> case INITIALIZATION_STEP_UNLOCK_REQUIRED:
> /* Only check unlock required if we were previously not unlocked */
> if (mm_gdbus_modem_get_unlock_required (ctx->skeleton) != MM_MODEM_LOCK_NONE) {
> @@ -4707,6 +4741,21 @@ interface_initialization_step (InitializationContext *ctx)
> ctx->self);
>
> if (ctx->fatal_error) {
> + if (g_error_matches (ctx->fatal_error,
> + MM_MOBILE_EQUIPMENT_ERROR,
> + MM_MOBILE_EQUIPMENT_ERROR_SIM_NOT_INSERTED)) {
> + gboolean is_sim_hot_swap_supported = FALSE;
> +
> + g_object_get (ctx->self,
> + MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED,
> + &is_sim_hot_swap_supported,
> + NULL);
> +
> + if (is_sim_hot_swap_supported) {
> + mm_iface_modem_update_failed_state (ctx->self,
> + MM_MODEM_STATE_FAILED_REASON_SIM_MISSING);
> + }
> + }
> g_simple_async_result_take_error (ctx->result, ctx->fatal_error);
> ctx->fatal_error = NULL;
> } else {
> @@ -5119,6 +5168,13 @@ 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,
> + "Sim Hot Swap Supported",
> + "Whether the modem supports sim hot swap or not.",
> + TRUE,
FALSE by default.
> + G_PARAM_READWRITE));
This property should be construct-only.
>
> initialized = TRUE;
> }
> diff --git a/src/mm-iface-modem.h b/src/mm-iface-modem.h
> index 74ea9f9..517c651 100644
> --- a/src/mm-iface-modem.h
> +++ b/src/mm-iface-modem.h
> @@ -32,10 +32,11 @@
> #define MM_IS_IFACE_MODEM(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MM_TYPE_IFACE_MODEM))
> #define MM_IFACE_MODEM_GET_INTERFACE(obj) (G_TYPE_INSTANCE_GET_INTERFACE ((obj), MM_TYPE_IFACE_MODEM, MMIfaceModem))
>
> -#define MM_IFACE_MODEM_DBUS_SKELETON "iface-modem-dbus-skeleton"
> -#define MM_IFACE_MODEM_STATE "iface-modem-state"
> -#define MM_IFACE_MODEM_SIM "iface-modem-sim"
> -#define MM_IFACE_MODEM_BEARER_LIST "iface-modem-bearer-list"
> +#define MM_IFACE_MODEM_DBUS_SKELETON "iface-modem-dbus-skeleton"
> +#define MM_IFACE_MODEM_STATE "iface-modem-state"
> +#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"
>
> typedef struct _MMIfaceModem MMIfaceModem;
>
> @@ -327,6 +328,14 @@ struct _MMIfaceModem {
> MMBaseBearer * (*create_bearer_finish) (MMIfaceModem *self,
> GAsyncResult *res,
> GError **error);
> + /* Setup SIM hot swap */
> + void (*setup_sim_hot_swap) (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data);
> +
> + gboolean (*setup_sim_hot_swap_finish) (MMIfaceModem *self,
> + GAsyncResult *res,
> + GError **error);
> };
>
> GType mm_iface_modem_get_type (void);
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list