port-mbim: implement the new generic 'removed' signal

Letting the MMBroadbandModemMbim listen for the notifications of the
MbimDevice was not a good idea, especially since no explicit reference
to the device object was hold in the modem object. This leads to race
conditions in which the MbimDevice outlives the MMBroadbandModemMbim and
the MMPortMbim, and when the "device-removed" signal is triggered, the
program crashes.

The MMPortMbim will now emit its own 'removed' signals when the
underlying MbimDevice emits "device-removed', ensuring that the signal
handler is properly cleared up during the MMPortMbim disposal.

Fixes https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/issues/518

2 files changed, 27 insertions, 66 deletions

diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
index 06d82a82..9a13a88b 100644
--- a/src/mm-broadband-modem-mbim.c
+++ b/src/mm-broadband-modem-mbim.c
@@ -3225,57 +3225,6 @@ query_device_services (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_obj_msg (self, "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;
-
-    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)
@@ -3288,9 +3237,6 @@ mbim_port_open_ready (MMPortMbim   *mbim,
         return;
     }
 
-    /* Make sure we know if mbim-proxy dies on us, and then do the parent's
-     * initialization */
-    track_mbim_device_removed (MM_BROADBAND_MODEM_MBIM (g_task_get_source_object (task)), mbim);
     query_device_services (task);
 }
 
@@ -3383,7 +3329,6 @@ initialization_started (MMBroadbandModem    *self,
     if (mm_port_mbim_is_open (ctx->mbim)) {
         /* 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);
         query_device_services (task);
         return;
     }
@@ -9230,8 +9175,7 @@ dispose (GObject *object)
         /* Explicitly remove notification handler */
         self->priv->setup_flags = PROCESS_NOTIFICATION_FLAG_NONE;
         common_setup_cleanup_unsolicited_events_sync (self, mm_port_mbim_peek_device (mbim), FALSE);
-        /* Disconnect signal handler for mbim-proxy disappearing, if it exists */
-        untrack_mbim_device_removed (self, mbim);
+
         /* If we did open the MBIM port during initialization, close it now */
         if (mm_port_mbim_is_open (mbim))
             mm_port_mbim_close (mbim, NULL, NULL);
diff --git a/src/mm-port-mbim.c b/src/mm-port-mbim.c
index 7dcdda59..ae1021a0 100644
--- a/src/mm-port-mbim.c
+++ b/src/mm-port-mbim.c
@@ -35,8 +35,9 @@ struct _MMPortMbimPrivate {
     gboolean    in_progress;
     MbimDevice *mbim_device;
 
-    /* timeout monitoring */
+    /* monitoring */
     gulong timeout_monitoring_id;
+    gulong removed_monitoring_id;
 
 #if defined WITH_QMI && QMI_MBIM_QMUX_SUPPORTED
     gboolean    qmi_supported;
@@ -392,13 +393,17 @@ mm_port_mbim_reset (MMPortMbim          *self,
 /*****************************************************************************/
 
 static void
-reset_timeout_monitoring (MMPortMbim *self,
-                          MbimDevice *mbim_device)
+reset_monitoring (MMPortMbim *self,
+                  MbimDevice *mbim_device)
 {
     if (self->priv->timeout_monitoring_id && mbim_device) {
         g_signal_handler_disconnect (mbim_device, self->priv->timeout_monitoring_id);
         self->priv->timeout_monitoring_id = 0;
     }
+    if (self->priv->removed_monitoring_id && mbim_device) {
+        g_signal_handler_disconnect (mbim_device, self->priv->removed_monitoring_id);
+        self->priv->removed_monitoring_id = 0;
+    }
 }
 
 static void
@@ -410,18 +415,30 @@ consecutive_timeouts_updated_cb (MMPortMbim *self,
 }
 
 static void
-setup_timeout_monitoring (MMPortMbim *self,
-                          MbimDevice *mbim_device)
+device_removed_cb (MMPortMbim  *self)
+{
+    g_signal_emit_by_name (self, MM_PORT_SIGNAL_REMOVED);
+}
+
+static void
+setup_monitoring (MMPortMbim *self,
+                  MbimDevice *mbim_device)
 {
     g_assert (mbim_device);
 
-    reset_timeout_monitoring (self, mbim_device);
+    reset_monitoring (self, mbim_device);
 
     g_assert (!self->priv->timeout_monitoring_id);
     self->priv->timeout_monitoring_id = g_signal_connect_swapped (mbim_device,
                                                                   "notify::" MBIM_DEVICE_CONSECUTIVE_TIMEOUTS,
                                                                   G_CALLBACK (consecutive_timeouts_updated_cb),
                                                                   self);
+
+    g_assert (!self->priv->removed_monitoring_id);
+    self->priv->removed_monitoring_id = g_signal_connect_swapped (mbim_device,
+                                                                  MBIM_DEVICE_SIGNAL_REMOVED,
+                                                                  G_CALLBACK (device_removed_cb),
+                                                                  self);
 }
 
 /*****************************************************************************/
@@ -600,7 +617,7 @@ mbim_device_open_ready (MbimDevice   *mbim_device,
     }
 
     mm_obj_dbg (self, "MBIM device is now open");
-    setup_timeout_monitoring (self, mbim_device);
+    setup_monitoring (self, mbim_device);
 
 #if defined WITH_QMI && QMI_MBIM_QMUX_SUPPORTED
     if (self->priv->qmi_supported) {
@@ -739,7 +756,7 @@ mbim_device_close_ready (MbimDevice   *mbim_device,
 
     g_assert (!self->priv->mbim_device);
     self->priv->in_progress = FALSE;
-    reset_timeout_monitoring (self, mbim_device);
+    reset_monitoring (self, mbim_device);
 
     if (!mbim_device_close_finish (mbim_device, res, &error))
         g_task_return_error (task, error);
@@ -896,7 +913,7 @@ dispose (GObject *object)
 #endif
 
     /* Clear device object */
-    reset_timeout_monitoring (self, self->priv->mbim_device);
+    reset_monitoring (self, self->priv->mbim_device);
     g_clear_object (&self->priv->mbim_device);
 
     G_OBJECT_CLASS (mm_port_mbim_parent_class)->dispose (object);