[PATCH] broadband-modem: don't create sim hot swap ports context multiple times

[Aleksander Morgado]

---

Hey Carlo,

It's never too late :)

I couldn't reproduce the issue with the SIM hot swap changes you were trying, because none of my Telit modems here were able to support it properly, so I ended up re-reviewing the logs you sent and I believe I found the issue.

When the SIM is already unlocked, the "initialization" sequence is run only once. When the SIM is locked, though, the "initialization" sequence is run twice: one until it detects that the SIM is locked, and the second one after the SIM is unlocked. The INITIALIZE_STEP_SIM_HOT_SWAP step was therefore running twice and unconditionally creating a new PortsContext each time, and that would have a nice side-effect apart from the memleak: the "port open count" was incorrectly increased by one for the TTYs...

When SIM was removed ModemManager would close and remove all ports. These are the logs when we have run the initialization sequence only once, you can see that the open count goes to 0 for both ttyACM0 and ttyACM3:
    <debug> (ttyACM3) device open count is 1 (close)
    <debug> (ttyACM0) device open count is 0 (close)
    <debug> (ttyACM0) closing serial port...
    <debug> (ttyACM0) serial port closed
    <debug> (ttyACM3) device open count is 0 (close)
    <debug> (ttyACM3) closing serial port...
    <debug> (ttyACM3) serial port closed
    <info>  Modem /org/freedesktop/ModemManager1/Modem/0: state changed (disabling -> disabled)
    <debug> [device /sys/devices/pci0000:00/0000:00:1a.7/usb1/1-3] unexported modem from path '/org/freedesktop/ModemManager1/Modem/0'
    <debug> (ttyACM3) forced to close port
    <debug> (ttyACM0) forced to close port

These are the logs when we have run the initialization sequence twice and the PortsContext was created twice, you can see how the open count didn't go to 0:
    <info>  Modem /org/freedesktop/ModemManager1/Modem/0: 3GPP Registration state changed (home -> unknown)
    <debug> (ttyACM3) device open count is 2 (close)
    <debug> (ttyACM0) device open count is 1 (close)
    <debug> (ttyACM3) device open count is 1 (close)
    <info>  Modem /org/freedesktop/ModemManager1/Modem/0: state changed (disabling -> disabled)
    <debug> [device /sys/devices/pci0000:00/0000:00:1a.7/usb1/1-3] unexported modem from path '/org/freedesktop/ModemManager1/Modem/0'

When the modem was re-created you said you would get NULL strings as response in the AT commands sent, even if you saw the responses in the logs.. well, it was the port objects from the previous modem run, which were not fully closed, the ones receiving the responses :) Your new serial port objects in the new modem run would fail and get NULL strings as response as you said.

Could you take a look at the patch, which just avoids to re-create the ports context, and see if it really fixes the issue you were seeing in your tests?

Cheers!

---
 src/mm-broadband-modem.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
index 4a14d33d..68242e5c 100644
--- a/src/mm-broadband-modem.c
+++ b/src/mm-broadband-modem.c
@@ -10154,7 +10154,9 @@ initialize_step (InitializeContext *ctx)
         return;

     case INITIALIZE_STEP_SIM_HOT_SWAP:
-        {
+        /* Create the SIM hot swap ports context only if not already done before
+         * (we may be re-running the initialization step after SIM-PIN unlock) */
+        if (!ctx->self->priv->sim_hot_swap_ports_ctx) {
             gboolean is_sim_hot_swap_supported = FALSE;

             g_object_get (ctx->self,
@@ -10165,7 +10167,7 @@ initialize_step (InitializeContext *ctx)
                 PortsContext *ports;
                 GError *error = NULL;

-                mm_dbg ("Creating PortsContext for SIM hot swap.");
+                mm_dbg ("Creating ports context for SIM hot swap");

                 ports = g_new0 (PortsContext, 1);
                 ports->ref_count = 1;
@@ -10178,7 +10180,9 @@ initialize_step (InitializeContext *ctx)

                 ports_context_unref (ports);
             }
-        }
+        } else
+            mm_dbg ("Ports context for SIM hot swap already available");
+
         /* Fall down to next step */
         ctx->step++;

--
2.12.2