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

[Eric Caruso]

Thanks for taking a look!

On Fri, Aug 4, 2017 at 5:39 AM, Aleksander Morgado
 wrote:
> 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?

Mostly this is a nice-to-have, but on Chrome OS it would improve the
user experience since mbim-proxy crashing causes MM to lose contact
with the modem but not really do anything about it. Instead of the
user having to realize this and toggle a bunch of switches until it
works again it would be cool if MM just tried to recover.

We turned off mbim-proxy on Chrome OS back when all the modem stuff
was still running as root, and I wanted to turn it back on so we can
concurrently use mbimcli but Ben also wanted us to have some handling
in case it disappears on us.

> 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... :)

I don't have any QMI modems to test on at the moment, so I can write a
patch, but no guarantees on whether it works ;)

-Eric
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [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, )) {
> +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));

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

[Eric Caruso]

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.
---
 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, )) {
+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);
+}
+
+static void
+mbim_device_connect_to_signal (MMPortMbim *mbim,
+   GTask *task)
+{
+MMBroadbandModemMbim *self;
+MbimDevice *device;
+
+self = MM_BROADBAND_MODEM_MBIM (g_task_get_source_object (task));
+device = mm_port_mbim_peek_device (mbim);
+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);
+}
+
+static void
+mbim_device_disconnect_from_signal (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)
@@ -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);
 }
 
 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);
 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