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

[Carlo Lobrano]

Ciao Aleksander,​

On 22 May 2017 at 13:18, Aleksander Morgado <aleksander at aleksander.es>
wrote:

> ---
>
> 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!
>

​​This is weird, but I couldn't reproduce the "modem not recognized" part
myself.

What I can reproduce is the crash, and I can also confirm that this patch
fix it very well. Thank you!
​



>
> ---
>  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170523/94c7708e/attachment.html>