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

[Aleksander Morgado]

Hey!

On 03/08/17 22:56, 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 otherwise gets very confused.
> ---

Ideally mbim-proxy should never crash :) Do you have any issue that ends up in the proxy crashing, or is this just a nice to have recovery method?

But looking at this in a more generic way, if the MbimDevice reports "device-removed" then we should really try to handle that in ModemManager as an indication that the port is no longer valid, so let's try to support this as well.

BTW; I'd love to see a patch also for QMI doing the same thing... :)

See comments below.


>  src/mm-broadband-modem-mbim.c | 104 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 100 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
> index c69893f6..33f58302 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,6 +1586,87 @@ parent_initialization_started (GTask *task)
>  }
>  
>  static void
> +mbim_device_removed_disable_ready (MMBaseModem *self,
> +                                   GAsyncResult *res,
> +                                   gpointer unused)
> +{
> +    GError *error = NULL;
> +
> +    if (!mm_base_modem_disable_finish (self, res, &error)) {
> +        mm_warn ("Failed to disable modem %s: %s",
> +                 mm_base_modem_get_device (self), error->message);
> +        g_error_free (error);
> +    }
> +
> +    mm_base_modem_set_valid (self, FALSE);
> +}
> +
> +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_disable (MM_BASE_MODEM (self),
> +                           (GAsyncReadyCallback) mbim_device_removed_disable_ready,
> +                           NULL);

Is this disable() even needed here? We already lost communication with the proxy, wouldn't all next commands just timeout or error out?
i.e. can't we just set_valid(self,FALSE) here?

> +}
> +
> +static void
> +mbim_device_connect_to_signal (MMPortMbim *mbim,
> +                               GTask *task)

This method name may be improved explicitly stating what the signal connected does, something like "track_mbim_device_removed".
Also, there is no point in getting the GTask here as input. Just pass the MMBroadbandModemMbim *self object as first parameter and the MMPortMbim as second.

> +{
> +    MMBroadbandModemMbim *self;
> +    MbimDevice *device;
> +
> +    self = MM_BROADBAND_MODEM_MBIM (g_task_get_source_object (task));
> +    device = mm_port_mbim_peek_device (mbim);

How about g_assert (device); and you remove the if/else() below?

> +    if (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);
> +    } else {
> +        mm_err ("MBIM port for %s not actually open?",
> +                mm_port_get_device (MM_PORT (mbim)));
> +    }
> +
> +    /* Done we are, launch parent's callback */
> +    parent_initialization_started (task);

This is not right; this method should only connect the signal, it shouldn't keep on to the next step, whatever it is. Do that in the caller better.

> +}
> +
> +static void
> +mbim_device_disconnect_from_signal (MMBroadbandModemMbim *self)
> +{

How about naming it "untrack_mbim_device_removed"?

> +    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)
> @@ -1595,8 +1679,9 @@ mbim_port_open_ready (MMPortMbim *mbim,
>          return;
>      }
>  
> -    /* Done we are, launch parent's callback */
> -    parent_initialization_started (task);
> +    /* Make sure we know if mbim-proxy dies on us, and then do the parent's
> +     * initialization */
> +    mbim_device_connect_to_signal (mbim, task);

As said before, call parent_initialization_started() here.

>  }
>  
>  static void
> @@ -1624,8 +1709,9 @@ initialization_started (MMBroadbandModem *self,
>      }
>  
>      if (mm_port_mbim_is_open (ctx->mbim)) {
> -        /* Nothing to be done, just launch parent's callback */
> -        parent_initialization_started (task);
> +        /* Nothing to be done, just connect to a signal and launch parent's
> +         * callback */
> +        mbim_device_connect_to_signal (ctx->mbim, task);

As said before, call parent_initialization_started() here.

>          return;
>      }
>  
> @@ -3167,6 +3253,15 @@ mm_broadband_modem_mbim_init (MMBroadbandModemMbim *self)
>  }
>  
>  static void
> +dispose (GObject *object)
> +{
> +    /* Disconnect signal handler for mbim-proxy disappearing, if it exists */
> +    mbim_device_disconnect_from_signal (MM_BROADBAND_MODEM_MBIM (object));
> +
> +    G_OBJECT_CLASS (mm_broadband_modem_mbim_parent_class)->dispose (object);
> +}
> +
> +static void
>  finalize (GObject *object)
>  {
>      MMPortMbim *mbim;
> @@ -3333,6 +3428,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