> Pushed to git master, thanks.
Thanks!
> I suppose there is a bit of a race in the code, because:
>
> First the current SIM insertion state is checked (with AT#QSS?), and
> stored, and afterwards unsolicited notifications are enabled "AT#QSS=1".
>
> If a SIM eject happens between the two, then the SIM removed event
> wouldn't be caught, and the internal stored state would be incorrect -
> the fix would be to issue and parse the AT#QSS? command after the
> AT#QSS=1 command. This could also serve to verify that the modem did
> enable hot swap notifiation. In practise this is highly unlikely, but
> just thought I'd mention it.
and thanks Tim, you're right, I'll work on this as well
On 1 August 2017 at 10:34, Aleksander Morgado <aleksan...@aleksander.es>
wrote:
> On 25/07/17 09:03, Carlo Lobrano 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
> >
> > Finally, this patch replaces the MBIM's sim_hot_swap_on private property
> > with the new property MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED, since
> they have the
> > same meaning.
> > ---
> >
> > Please ignore the previous patch, since it was rebased on the wrong
> branch.
> > This one is rebased on master branch at commit
> > aa0a6bed363419659101084b3861a51144eee859
> >
>
> Pushed to git master, thanks.
>
> > ---
> > plugins/telit/mm-broadband-modem-telit.c | 1 +
> > src/mm-broadband-modem-mbim.c | 21 ++++---
> > src/mm-broadband-modem.c | 94
> ++++++++++++++++++++++----------
> > src/mm-iface-modem.c | 14 ++++-
> > src/mm-iface-modem.h | 1 +
> > 5 files changed, 93 insertions(+), 38 deletions(-)
> >
> > diff --git a/plugins/telit/mm-broadband-modem-telit.c
> b/plugins/telit/mm-broadband-modem-telit.c
> > index e7650c0..edd9093 100644
> > --- a/plugins/telit/mm-broadband-modem-telit.c
> > +++ b/plugins/telit/mm-broadband-modem-telit.c
> > @@ -1377,6 +1377,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-mbim.c
> b/src/mm-broadband-modem-mbim.c
> > index c69893f..7f5d2fc 100644
> > --- a/src/mm-broadband-modem-mbim.c
> > +++ b/src/mm-broadband-modem-mbim.c
> > @@ -89,8 +89,6 @@ struct _MMBroadbandModemMbimPrivate {
> > MbimDataClass available_data_classes;
> > MbimDataClass highest_available_data_class;
> >
> > - /* For checking whether the SIM has been hot swapped */
> > - gboolean sim_hot_swap_on;
> > MbimSubscriberReadyState last_ready_state;
> > };
> >
> > @@ -1990,6 +1988,7 @@ basic_connect_notification_subscriber_ready_status
> (MMBroadbandModemMbim *self,
> > (self->priv->last_ready_state ==
> > MBIM_SUBSCRIBER_READY_STATE_SIM_NOT_INSERTED
> &&
> > ready_state !=
> > MBIM_SUBSCRIBER_READY_STATE_SIM_NOT_INSERTED))
> {
> > /* SIM has been removed or reinserted, re-probe to ensure
> correct interfaces are exposed */
> > + mm_dbg ("SIM hot swap detected");
> > mm_broadband_modem_update_sim_hot_swap_detected
> (MM_BROADBAND_MODEM (self));
> > }
> >
> > @@ -2286,10 +2285,15 @@ cleanup_unsolicited_events_3gpp
> (MMIfaceModem3gpp *_self,
> > gpointer user_data)
> > {
> > MMBroadbandModemMbim *self = MM_BROADBAND_MODEM_MBIM (_self);
> > + gboolean is_sim_hot_swap_configured = FALSE;
> > +
> > + g_object_get (self,
> > + MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED,
> &is_sim_hot_swap_configured,
> > + NULL);
> >
> > self->priv->setup_flags &= ~PROCESS_NOTIFICATION_FLAG_
> SIGNAL_QUALITY;
> > self->priv->setup_flags &= ~PROCESS_NOTIFICATION_FLAG_CONNECT;
> > - if (self->priv->sim_hot_swap_on)
> > + if (is_sim_hot_swap_configured)
> > self->priv->setup_flags &= ~PROCESS_NOTIFICATION_FLAG_
> SUBSCRIBER_INFO;
> > self->priv->setup_flags &= ~PROCESS_NOTIFICATION_FLAG_
> PACKET_SERVICE;
> > common_setup_cleanup_unsolicited_events (self, FALSE, callback,
> user_data);
> > @@ -2500,7 +2504,6 @@ enable_subscriber_info_unsolicited_events_ready
> (MMBroadbandModemMbim *self,
> >
> > if (!common_enable_disable_unsolicited_events_finish (self, res,
> &error)) {
> > mm_dbg ("Failed to enable subscriber info events: %s",
> error->message);
> > - self->priv->sim_hot_swap_on = FALSE;
> > g_task_return_error (task, error);
> > g_object_unref (task);
> > return;
> > @@ -2519,7 +2522,6 @@ setup_subscriber_info_unsolicited_events_ready
> (MMBroadbandModemMbim *self,
> >
> > if (!common_setup_cleanup_unsolicited_events_finish (self, res,
> &error)) {
> > mm_dbg ("Failed to set up subscriber info events: %s",
> error->message);
> > - self->priv->sim_hot_swap_on = FALSE;
> > g_task_return_error (task, error);
> > g_object_unref (task);
> > return;
> > @@ -2541,7 +2543,6 @@ modem_setup_sim_hot_swap (MMIfaceModem *_self,
> >
> > task = g_task_new (self, NULL, callback, user_data);
> >
> > - self->priv->sim_hot_swap_on = TRUE;
> > self->priv->setup_flags |= PROCESS_NOTIFICATION_FLAG_
> SUBSCRIBER_INFO;
> > common_setup_cleanup_unsolicited_events (self,
> > TRUE,
> > @@ -2566,10 +2567,15 @@ modem_3gpp_disable_unsolicited_events
> (MMIfaceModem3gpp *_self,
> > gpointer user_data)
> > {
> > MMBroadbandModemMbim *self = MM_BROADBAND_MODEM_MBIM (_self);
> > + gboolean is_sim_hot_swap_configured = FALSE;
> > +
> > + g_object_get (self,
> > + MM_IFACE_MODEM_SIM_HOT_SWAP_CONFIGURED,
> &is_sim_hot_swap_configured,
> > + NULL);
> >
> > self->priv->enable_flags &= ~PROCESS_NOTIFICATION_FLAG_
> SIGNAL_QUALITY;
> > self->priv->enable_flags &= ~PROCESS_NOTIFICATION_FLAG_CONNECT;
> > - if (!self->priv->sim_hot_swap_on)
> > + if (is_sim_hot_swap_configured)
> > self->priv->enable_flags &= ~PROCESS_NOTIFICATION_FLAG_
> SUBSCRIBER_INFO;
> > self->priv->enable_flags &= ~PROCESS_NOTIFICATION_FLAG_
> PACKET_SERVICE;
> > common_enable_disable_unsolicited_events (self, callback,
> user_data);
> > @@ -3154,6 +3160,7 @@ mm_broadband_modem_mbim_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 d3d349e..2d2f650 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 */
> > @@ -10098,27 +10100,38 @@ initialize_step (GTask *task)
> > * (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");
> > @@ -10146,32 +10159,44 @@ initialize_step (GTask *task)
> > } 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...");
> > - error = g_error_new (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");
> > - error = g_error_new (MM_CORE_ERROR,
> > - MM_CORE_ERROR_WRONG_STATE,
> > - "Modem is unusable, "
> > - "cannot fully initialize");
> > + 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...");
> > + error = g_error_new (MM_CORE_ERROR,
> > + MM_CORE_ERROR_WRONG_STATE,
> > + "Modem is unusable due to
> SIM missing, "
> > + "cannot fully initialize,
> waiting for SIM insertion.");
> > + goto sim_hot_swap_enabled;
> > + }
> > }
> >
> > + error = g_error_new (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.
> > @@ -10498,6 +10523,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;
> > @@ -10603,6 +10631,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;
> > @@ -11102,6 +11133,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 d59303a..ec453a8 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;
> >
> >
>
>
> --
> Aleksander
> https://aleksander.es
>
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
