[PATCH v2] mm-broadband-modem-mbim: reprobe on mbim-proxy death

[Aleksander Morgado]

Hey Eric,

On 04/08/17 19:15, Eric Caruso wrote:
> In case mbim-proxy crashes, ModemManager needs to be able to
> recognize this and respawn the proxy so we don't lose access
> to the modem. Do this by subscribing to the device removal
> signal on MbimDevice and reprobing the modem when we lose the
> connection to mbim-proxy.
> 
> We can't just restart mbim-proxy because the reopened mbim-proxy
> will give us a different client ID, and so unsolicitied
> notifications will fail and MM gets very confused otherwise.
> ---

I've tested this and it does look good. The modem is reprobed as expected and allows connectivity right away again.
One minor comment below.

>  src/mm-broadband-modem-mbim.c | 78 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
> index c69893f6..72c81879 100644
> --- a/src/mm-broadband-modem-mbim.c
> +++ b/src/mm-broadband-modem-mbim.c
> @@ -90,6 +90,9 @@ struct _MMBroadbandModemMbimPrivate {
>      MbimDataClass highest_available_data_class;
> 
>      MbimSubscriberReadyState last_ready_state;
> +
> +    /* For notifying when the mbim-proxy connection is dead */
> +    gulong mbim_device_removed_id;
>  };
>  
>  /*****************************************************************************/
> @@ -1583,10 +1586,66 @@ parent_initialization_started (GTask *task)
>  }
>  
>  static void
> +mbim_device_removed_cb (MbimDevice *device,
> +                        MMBroadbandModemMbim *self)
> +{
> +    /* We have to do a full re-probe here because simply reopening the device
> +     * and restarting mbim-proxy will leave us without MBIM notifications. */
> +    mm_info ("Connection to mbim-proxy for %s lost, reprobing",
> +             mbim_device_get_path_display (device));
> +
> +    g_signal_handler_disconnect (device,
> +                                 self->priv->mbim_device_removed_id);
> +    self->priv->mbim_device_removed_id = 0;
> +
> +    mm_base_modem_set_reprobe (MM_BASE_MODEM (self), TRUE);
> +    mm_base_modem_set_valid (MM_BASE_MODEM (self), FALSE);
> +}
> +
> +static void
> +track_mbim_device_removed (MMBroadbandModemMbim *self,
> +                           MMPortMbim *mbim)
> +{
> +    MbimDevice *device;
> +
> +    device = mm_port_mbim_peek_device (mbim);
> +    g_assert (device);
> +
> +    /* Register removal handler so we can handle mbim-proxy crashes */
> +    self->priv->mbim_device_removed_id = g_signal_connect (
> +        device,
> +        MBIM_DEVICE_SIGNAL_REMOVED,
> +        G_CALLBACK (mbim_device_removed_cb),
> +        self);
> +}
> +
> +static void
> +untrack_mbim_device_removed (MMBroadbandModemMbim *self)
> +{
> +    MMPortMbim *mbim;
> +    MbimDevice *device;
> +
> +    if (self->priv->mbim_device_removed_id == 0)
> +        return;
> +
> +    mbim = mm_base_modem_peek_port_mbim (MM_BASE_MODEM (self));
> +    if (!mbim)
> +        return;
> +
> +    device = mm_port_mbim_peek_device (mbim);
> +    if (!device)
> +        return;
> +
> +    g_signal_handler_disconnect (device, self->priv->mbim_device_removed_id);
> +    self->priv->mbim_device_removed_id = 0;
> +}
> +
> +static void
>  mbim_port_open_ready (MMPortMbim *mbim,
>                        GAsyncResult *res,
>                        GTask *task)
>  {
> +    MMBroadbandModemMbim *self;
>      GError *error = NULL;
>  
>      if (!mm_port_mbim_open_finish (mbim, res, &error)) {
> @@ -1595,7 +1654,10 @@ mbim_port_open_ready (MMPortMbim *mbim,
>          return;
>      }
>  
> -    /* Done we are, launch parent's callback */
> +    /* Make sure we know if mbim-proxy dies on us, and then do the parent's
> +     * initialization */
> +    self = MM_BROADBAND_MODEM_MBIM (g_task_get_source_object (task));
> +    track_mbim_device_removed (self, mbim);
>      parent_initialization_started (task);
>  }
>  
> @@ -1624,7 +1686,9 @@ initialization_started (MMBroadbandModem *self,
>      }
>  
>      if (mm_port_mbim_is_open (ctx->mbim)) {
> -        /* Nothing to be done, just launch parent's callback */
> +        /* Nothing to be done, just connect to a signal and launch parent's
> +         * callback */
> +        track_mbim_device_removed (MM_BROADBAND_MODEM_MBIM (self), ctx->mbim);
>          parent_initialization_started (task);
>          return;
>      }
> @@ -3167,6 +3231,15 @@ mm_broadband_modem_mbim_init (MMBroadbandModemMbim *self)
>  }
>  
>  static void
> +dispose (GObject *object)
> +{
> +    /* Disconnect signal handler for mbim-proxy disappearing, if it exists */
> +    untrack_mbim_device_removed (MM_BROADBAND_MODEM_MBIM (object));
> +

Can we move the untrack_() method to finalize() instead? E.g. just after common_setup_cleanup_unsolicited_events_sync(). The untrack() method could also receive the MMPortMbim device as well as input.

> +    G_OBJECT_CLASS (mm_broadband_modem_mbim_parent_class)->dispose (object);
> +}
> +
> +static void
>  finalize (GObject *object)
>  {
>      MMPortMbim *mbim;
> @@ -3333,6 +3406,7 @@ mm_broadband_modem_mbim_class_init (MMBroadbandModemMbimClass *klass)
>  
>      g_type_class_add_private (object_class, sizeof (MMBroadbandModemMbimPrivate));
>  
> +    object_class->dispose = dispose;
>      object_class->finalize = finalize;
>  
>      broadband_modem_class->initialization_started = initialization_started;
> 


-- 
Aleksander
https://aleksander.es