Hey Eric,
Looks very good to me, just some minor coding style issues, see below.
Also, I wouldn't mind debug logs when inserting the sim and removing
the sim, for future reference.
On Tue, Jul 11, 2017 at 3:09 AM, Eric Caruso <ejcar...@chromium.org> wrote:
> If an MBIM modem supports unsolicited notifications for
> subscriber ready status, we can use it to detect when SIM cards
> have been removed and reinserted. Upon detection we should re-
> probe the modem so that we can configure it for the new SIM.
> ---
> src/mm-broadband-modem-mbim.c | 94
> +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
> index 0d1126d4..422f8ee4 100644
> --- a/src/mm-broadband-modem-mbim.c
> +++ b/src/mm-broadband-modem-mbim.c
> @@ -88,6 +88,10 @@ struct _MMBroadbandModemMbimPrivate {
> /* Access technology updates */
> 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;
> };
>
>
> /*****************************************************************************/
> @@ -627,6 +631,8 @@ unlock_required_subscriber_ready_state_ready (MbimDevice
> *device,
> }
> }
>
> + ctx->self->priv->last_ready_state = ready_state;
> +
> /* Fatal errors are reported right away */
> if (error) {
> g_simple_async_result_take_error (ctx->result, error);
> @@ -2041,8 +2047,15 @@ basic_connect_notification_subscriber_ready_status
> (MMBroadbandModemMbim *self,
> if (ready_state == MBIM_SUBSCRIBER_READY_STATE_INITIALIZED)
> mm_iface_modem_update_own_numbers (MM_IFACE_MODEM (self),
> telephone_numbers);
>
> - /* TODO: handle SIM removal using
> MBIM_SUBSCRIBER_READY_STATE_SIM_NOT_INSERTED */
> + if ((self->priv->last_ready_state !=
> MBIM_SUBSCRIBER_READY_STATE_SIM_NOT_INSERTED &&
> + ready_state == MBIM_SUBSCRIBER_READY_STATE_SIM_NOT_INSERTED) ||
> + (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_broadband_modem_update_sim_hot_swap_detected (MM_BROADBAND_MODEM
> (self));
> + }
>
> + self->priv->last_ready_state = ready_state;
> g_strfreev (telephone_numbers);
> }
>
> @@ -2340,7 +2353,8 @@ cleanup_unsolicited_events_3gpp (MMIfaceModem3gpp *self,
> {
> MM_BROADBAND_MODEM_MBIM (self)->priv->setup_flags &=
> ~PROCESS_NOTIFICATION_FLAG_SIGNAL_QUALITY;
> MM_BROADBAND_MODEM_MBIM (self)->priv->setup_flags &=
> ~PROCESS_NOTIFICATION_FLAG_CONNECT;
> - MM_BROADBAND_MODEM_MBIM (self)->priv->setup_flags &=
> ~PROCESS_NOTIFICATION_FLAG_SUBSCRIBER_INFO;
> + if (!MM_BROADBAND_MODEM_MBIM (self)->priv->sim_hot_swap_on)
> + MM_BROADBAND_MODEM_MBIM (self)->priv->setup_flags &=
> ~PROCESS_NOTIFICATION_FLAG_SUBSCRIBER_INFO;
> MM_BROADBAND_MODEM_MBIM (self)->priv->setup_flags &=
> ~PROCESS_NOTIFICATION_FLAG_PACKET_SERVICE;
> common_setup_cleanup_unsolicited_events (MM_BROADBAND_MODEM_MBIM (self),
> FALSE, callback, user_data);
> }
> @@ -2526,6 +2540,74 @@ modem_3gpp_enable_unsolicited_registration_events
> (MMIfaceModem3gpp *self,
> }
>
>
> /*****************************************************************************/
> +/* Setup SIM hot swap */
> +
> +static gboolean
> +modem_setup_sim_hot_swap_finish (MMIfaceModem *self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + return g_task_propagate_boolean (G_TASK (res), error);
> +}
> +
> +static void
> +enable_subscriber_info_unsolicited_events_ready (MMIfaceModem *self,
common_enable_disable_unsolicited_events() receives a
MMBroadbandModemMbim self pointer, and so the callback passed should
have that as first argument. This will avoid the explicit cast when
calling finish().
> + GAsyncResult *res,
> + GTask* task)
The asterisk should be attached to the variable name above.
> +{
> + GError *error = NULL;
> +
> + if (!common_enable_disable_unsolicited_events_finish
> (MM_BROADBAND_MODEM_MBIM (self), res, &error)) {
> + mm_dbg ("Failed to enable subscriber info events: %s",
> error->message);
> + MM_BROADBAND_MODEM_MBIM (self)->priv->sim_hot_swap_on = FALSE;
> + g_task_return_error (task, error);
> + g_object_unref (task);
> + return;
> + }
> +
> + g_task_return_boolean (task, TRUE);
> + g_object_unref (task);
> +}
> +
> +static void
> +setup_subscriber_info_unsolicited_events_ready (MMIfaceModem *self,
common_setup_cleanup_unsolicited_events() receives a
MMBroadbandModemMbim self pointer, and so the callback passed should
have that as first argument. This will avoid the explicit cast when
calling finish() and also all the other casts you have in the method.
> + GAsyncResult *res,
> + GTask *task)
> +{
> + GError *error = NULL;
> +
> + if (!common_setup_cleanup_unsolicited_events_finish
> (MM_BROADBAND_MODEM_MBIM (self), res, &error)) {
> + mm_dbg ("Failed to set up subscriber info events: %s",
> error->message);
> + MM_BROADBAND_MODEM_MBIM (self)->priv->sim_hot_swap_on = FALSE;
> + g_task_return_error (task, error);
> + g_object_unref (task);
> + return;
> + }
> +
> + MM_BROADBAND_MODEM_MBIM (self)->priv->enable_flags |=
> PROCESS_NOTIFICATION_FLAG_SUBSCRIBER_INFO;
> + common_enable_disable_unsolicited_events (MM_BROADBAND_MODEM_MBIM (self),
> +
> (GAsyncReadyCallback)enable_subscriber_info_unsolicited_events_ready,
> + task);
> +}
> +
> +static void
> +modem_setup_sim_hot_swap (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + GTask *task;
> +
> + task = g_task_new (self, NULL, callback, user_data);
> +
> + MM_BROADBAND_MODEM_MBIM (self)->priv->sim_hot_swap_on = TRUE;
> + MM_BROADBAND_MODEM_MBIM (self)->priv->setup_flags |=
> PROCESS_NOTIFICATION_FLAG_SUBSCRIBER_INFO;
> + common_setup_cleanup_unsolicited_events (MM_BROADBAND_MODEM_MBIM (self),
> + TRUE,
> +
> (GAsyncReadyCallback)setup_subscriber_info_unsolicited_events_ready,
> + task);
> +}
> +
> +/*****************************************************************************/
> /* Enable/Disable unsolicited events (3GPP interface) */
>
> static gboolean
> @@ -2543,7 +2625,8 @@ modem_3gpp_disable_unsolicited_events (MMIfaceModem3gpp
> *self,
> {
> MM_BROADBAND_MODEM_MBIM (self)->priv->enable_flags &=
> ~PROCESS_NOTIFICATION_FLAG_SIGNAL_QUALITY;
> MM_BROADBAND_MODEM_MBIM (self)->priv->enable_flags &=
> ~PROCESS_NOTIFICATION_FLAG_CONNECT;
> - MM_BROADBAND_MODEM_MBIM (self)->priv->enable_flags &=
> ~PROCESS_NOTIFICATION_FLAG_SUBSCRIBER_INFO;
> + if (!MM_BROADBAND_MODEM_MBIM (self)->priv->sim_hot_swap_on)
> + MM_BROADBAND_MODEM_MBIM (self)->priv->enable_flags &=
> ~PROCESS_NOTIFICATION_FLAG_SUBSCRIBER_INFO;
> MM_BROADBAND_MODEM_MBIM (self)->priv->enable_flags &=
> ~PROCESS_NOTIFICATION_FLAG_PACKET_SERVICE;
> common_enable_disable_unsolicited_events (MM_BROADBAND_MODEM_MBIM
> (self), callback, user_data);
> }
> @@ -3165,6 +3248,7 @@ mm_broadband_modem_mbim_new (const gchar *device,
> MM_BASE_MODEM_PLUGIN, plugin,
> MM_BASE_MODEM_VENDOR_ID, vendor_id,
> MM_BASE_MODEM_PRODUCT_ID, product_id,
> + MM_IFACE_MODEM_SIM_HOT_SWAP_SUPPORTED, TRUE,
> NULL);
> }
>
> @@ -3251,6 +3335,10 @@ iface_modem_init (MMIfaceModem *iface)
> /* Create MBIM-specific bearer */
> iface->create_bearer = modem_create_bearer;
> iface->create_bearer_finish = modem_create_bearer_finish;
> +
> + /* SIM hot swapping */
> + iface->setup_sim_hot_swap = modem_setup_sim_hot_swap;
> + iface->setup_sim_hot_swap_finish = modem_setup_sim_hot_swap_finish;
> }
>
> static void
> --
> 2.12.2
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
--
Aleksander
https://aleksander.es
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
