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

2017-05-23 Thread Aleksander Morgado 
On 23/05/17 18:40, Aleksander Morgado wrote:
>> What I can reproduce is the crash, and I can also confirm that this patch
>> fix it very well. Thank you!
> Thanks for checking!

Pushed to git master, btw.

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

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

2017-05-23 Thread Aleksander Morgado 
On Tue, May 23, 2017 at 5:58 PM, Carlo Lobrano  wrote:
> What I can reproduce is the crash, and I can also confirm that this patch
> fix it very well. Thank you!

Thanks for checking!


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

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

2017-05-23 Thread Carlo Lobrano 
Ciao Aleksander,​

On 22 May 2017 at 13:18, Aleksander Morgado 
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:
>  (ttyACM3) device open count is 1 (close)
>  (ttyACM0) device open count is 0 (close)
>  (ttyACM0) closing serial port...
>  (ttyACM0) serial port closed
>  (ttyACM3) device open count is 0 (close)
>  (ttyACM3) closing serial port...
>  (ttyACM3) serial port closed
>   Modem /org/freedesktop/ModemManager1/Modem/0: state changed
> (disabling -> disabled)
>  [device /sys/devices/pci:00/:00:1a.7/usb1/1-3]
> unexported modem from path '/org/freedesktop/ModemManager1/Modem/0'
>  (ttyACM3) forced to close port
>  (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:
>   Modem /org/freedesktop/ModemManager1/Modem/0: 3GPP
> Registration state changed (home -> unknown)
>  (ttyACM3) device open count is 2 (close)
>  (ttyACM0) device open count is 1 (close)
>  (ttyACM3) device open count is 1 (close)
>   Modem /org/freedesktop/ModemManager1/Modem/0: state changed
> (disabling -> disabled)
>  [device /sys/devices/pci:00/: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
>
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel