Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-13 Thread Aleksander Morgado 
On Wed, Apr 13, 2016 at 10:26 AM, Carlo Lobrano  wrote:
> No worries :)
> I double checked and I can confirm the issue is gone.

Thanks.

I'll try to prepare a new rc today or tomorrow.

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

Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-13 Thread Aleksander Morgado 
On Mon, Apr 11, 2016 at 4:56 PM, Carlo Lobrano  wrote:
>
> it looks like the last commit introduced a some kind of problem. HE910
> initialization ends with error
>
>> Modem couldn't be initialized: couldn't load current capabilities: Failed
>> to determine modem capabilities
>
> Here's the pastebin link of the logs of this test,
>
>> http://pastebin.com/h5PkSb92
>
> while the following's the logs of a good run made with version
> 8a386218690aeff7e2c923a14f91da7bbc046ed2 (HEAD~1)
>
>> http://pastebin.com/0fqrW52k
>
> Apparently, the parser mm-broadband-modem.c:parse_caps_gcap isn't receiving
> the reply from the serial device (the last line is a debug log I added,
> which prints out the content of the "response" that the function is
> parsing).
>
>  [1460381746.474901] [mm-port-serial-at.c:459] debug_log():
> (ttyACM0): --> 'AT+GCAP'
>  [1460381746.521723] [mm-port-serial-at.c:459] debug_log():
> (ttyACM0): <-- ''
>  [1460381746.550323] [mm-port-serial-at.c:459] debug_log():
> (ttyACM0): <-- '+GCAP: +CGSM,+DS,+FCLASS,+MS,+ESOK'
>  [1460381746.583914] [mm-broadband-modem.c:448] parse_caps_gcap():
> response is ''
>
> On LE910 SV V2 the behavior's the same.

Fixed it in git master now, sorry for that. It was actually
interesting to see what was the outcome of having missed some GTask
unrefs during probing... :)

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

Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-11 Thread Carlo Lobrano 
Hi Daniele,

it looks like the last commit introduced a some kind of problem. HE910
initialization ends with error

> Modem couldn't be initialized: couldn't load current capabilities: Failed
to determine modem capabilities

Here's the pastebin link of the logs of this test,

> http://pastebin.com/h5PkSb92

while the following's the logs of a good run made with version
8a386218690aeff7e2c923a14f91da7bbc046ed2 (HEAD~1)

> http://pastebin.com/0fqrW52k

Apparently, the parser mm-broadband-modem.c:parse_caps_gcap isn't receiving
the reply from the serial device (the last line is a debug log I added,
which prints out the content of the "response" that the function is
parsing).

 [1460381746.474901] [mm-port-serial-at.c:459] debug_log():
(ttyACM0): --> 'AT+GCAP'
 [1460381746.521723] [mm-port-serial-at.c:459] debug_log():
(ttyACM0): <-- ''
 [1460381746.550323] [mm-port-serial-at.c:459] debug_log():
(ttyACM0): <-- '+GCAP: +CGSM,+DS,+FCLASS,+MS,+ESOK'
 [1460381746.583914] [mm-broadband-modem.c:448] parse_caps_gcap():
response is ''

On LE910 SV V2 the behavior's the same.

Carlo



On Mon, 11 Apr 2016 at 15:52 Daniele Palmas  wrote:

> Hi Carlo,
>
> 2016-04-11 10:55 GMT+02:00 Daniele Palmas :
> > Hi Aleksander,
> >
> > 2016-04-08 17:14 GMT+02:00 Aleksander Morgado  >:
> >> On Fri, Apr 8, 2016 at 4:55 PM, Daniele Palmas 
> wrote:
> >>> 2016-04-08 16:42 GMT+02:00 Aleksander Morgado <
> aleksan...@aleksander.es>:
>  When we were completing tasks in idle, the logic was like this:
> 
>   * Schedule task completion in idle
>   * self->priv->task = NULL
>   * (idle) Task completion callback called
> 
>  This meant that the self->priv->task was always set to NULL before the
>  completion callback was called, which is what we wanted as a new task
> may be
>  scheduled in the callback itself.
> 
>  Now, without completing in idle, we were wrongly doing:
> 
>   * Task completion callback called
>   * self->priv->task = NULL
> 
>  This commit fixes the logic by making sure self->priv->task = NULL
> before any
>  task completion.
>  ---
> 
>  Daniele, can you validate that this patch fixes your issue?
> 
> >>>
> >>> According to a quick test I did, it seems to fix the issue on LE910.
> >>
> >> I'll get the patch merged to git master, and if your tests on Monday
> >> go well, I'll release a new rc with this fix.
> >>
> >
> > I confirm that the issue I was seeing has been fixed by your patch.
> > Thanks again!
> >
>
> Can you please also test with HE910 and LE910 V2?
>
> Thanks,
> Daniele
>
> > Regards,
> > Daniele
> >
> >> --
> >> Aleksander
> >> https://aleksander.es
>
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-11 Thread Daniele Palmas 
Hi Carlo,

2016-04-11 10:55 GMT+02:00 Daniele Palmas :
> Hi Aleksander,
>
> 2016-04-08 17:14 GMT+02:00 Aleksander Morgado :
>> On Fri, Apr 8, 2016 at 4:55 PM, Daniele Palmas  wrote:
>>> 2016-04-08 16:42 GMT+02:00 Aleksander Morgado :
 When we were completing tasks in idle, the logic was like this:

  * Schedule task completion in idle
  * self->priv->task = NULL
  * (idle) Task completion callback called

 This meant that the self->priv->task was always set to NULL before the
 completion callback was called, which is what we wanted as a new task may 
 be
 scheduled in the callback itself.

 Now, without completing in idle, we were wrongly doing:

  * Task completion callback called
  * self->priv->task = NULL

 This commit fixes the logic by making sure self->priv->task = NULL before 
 any
 task completion.
 ---

 Daniele, can you validate that this patch fixes your issue?

>>>
>>> According to a quick test I did, it seems to fix the issue on LE910.
>>
>> I'll get the patch merged to git master, and if your tests on Monday
>> go well, I'll release a new rc with this fix.
>>
>
> I confirm that the issue I was seeing has been fixed by your patch.
> Thanks again!
>

Can you please also test with HE910 and LE910 V2?

Thanks,
Daniele

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

Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-11 Thread Daniele Palmas 
Hi Aleksander,

2016-04-08 17:14 GMT+02:00 Aleksander Morgado :
> On Fri, Apr 8, 2016 at 4:55 PM, Daniele Palmas  wrote:
>> 2016-04-08 16:42 GMT+02:00 Aleksander Morgado :
>>> When we were completing tasks in idle, the logic was like this:
>>>
>>>  * Schedule task completion in idle
>>>  * self->priv->task = NULL
>>>  * (idle) Task completion callback called
>>>
>>> This meant that the self->priv->task was always set to NULL before the
>>> completion callback was called, which is what we wanted as a new task may be
>>> scheduled in the callback itself.
>>>
>>> Now, without completing in idle, we were wrongly doing:
>>>
>>>  * Task completion callback called
>>>  * self->priv->task = NULL
>>>
>>> This commit fixes the logic by making sure self->priv->task = NULL before 
>>> any
>>> task completion.
>>> ---
>>>
>>> Daniele, can you validate that this patch fixes your issue?
>>>
>>
>> According to a quick test I did, it seems to fix the issue on LE910.
>
> I'll get the patch merged to git master, and if your tests on Monday
> go well, I'll release a new rc with this fix.
>

I confirm that the issue I was seeing has been fixed by your patch.
Thanks again!

Regards,
Daniele

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

Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-08 Thread Aleksander Morgado 
On Fri, Apr 8, 2016 at 4:55 PM, Daniele Palmas  wrote:
> 2016-04-08 16:42 GMT+02:00 Aleksander Morgado :
>> When we were completing tasks in idle, the logic was like this:
>>
>>  * Schedule task completion in idle
>>  * self->priv->task = NULL
>>  * (idle) Task completion callback called
>>
>> This meant that the self->priv->task was always set to NULL before the
>> completion callback was called, which is what we wanted as a new task may be
>> scheduled in the callback itself.
>>
>> Now, without completing in idle, we were wrongly doing:
>>
>>  * Task completion callback called
>>  * self->priv->task = NULL
>>
>> This commit fixes the logic by making sure self->priv->task = NULL before any
>> task completion.
>> ---
>>
>> Daniele, can you validate that this patch fixes your issue?
>>
>
> According to a quick test I did, it seems to fix the issue on LE910.

I'll get the patch merged to git master, and if your tests on Monday
go well, I'll release a new rc with this fix.

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

Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-08 Thread Daniele Palmas 
Hi Aleksander,

2016-04-08 16:42 GMT+02:00 Aleksander Morgado :
> When we were completing tasks in idle, the logic was like this:
>
>  * Schedule task completion in idle
>  * self->priv->task = NULL
>  * (idle) Task completion callback called
>
> This meant that the self->priv->task was always set to NULL before the
> completion callback was called, which is what we wanted as a new task may be
> scheduled in the callback itself.
>
> Now, without completing in idle, we were wrongly doing:
>
>  * Task completion callback called
>  * self->priv->task = NULL
>
> This commit fixes the logic by making sure self->priv->task = NULL before any
> task completion.
> ---
>
> Daniele, can you validate that this patch fixes your issue?
>

According to a quick test I did, it seems to fix the issue on LE910.

On Monday I will do more testing.

Thank you very much for your help!

Regards,
Daniele

> ---
>  src/mm-port-probe.c | 184 
> 
>  1 file changed, 100 insertions(+), 84 deletions(-)
>
> diff --git a/src/mm-port-probe.c b/src/mm-port-probe.c
> index a1ca8ba..5d2428a 100644
> --- a/src/mm-port-probe.c
> +++ b/src/mm-port-probe.c
> @@ -95,6 +95,50 @@ struct _MMPortProbePrivate {
>  };
>
>  
> /*/
> +/* Probe task completions.
> + * Always make sure that the stored task is NULL when the task is completed.
> + */
> +
> +static gboolean
> +port_probe_task_return_error_if_cancelled (MMPortProbe *self)
> +{
> +GTask *task;
> +
> +task = self->priv->task;
> +self->priv->task = NULL;
> +
> +if (g_task_return_error_if_cancelled (task)) {
> +g_object_unref (task);
> +return TRUE;
> +}
> +
> +self->priv->task = task;
> +return FALSE;
> +}
> +
> +static void
> +port_probe_task_return_error (MMPortProbe *self,
> +  GError  *error)
> +{
> +GTask *task;
> +
> +task = self->priv->task;
> +self->priv->task = NULL;
> +g_task_return_error (task, error);
> +}
> +
> +static void
> +port_probe_task_return_boolean (MMPortProbe *self,
> +gboolean result)
> +{
> +GTask *task;
> +
> +task = self->priv->task;
> +self->priv->task = NULL;
> +g_task_return_boolean (task, result);
> +}
> +
> +/*/
>
>  void
>  mm_port_probe_set_result_at (MMPortProbe *self,
> @@ -527,10 +571,8 @@ wdm_probe (MMPortProbe *self)
>  ctx->source_id = 0;
>
>  /* If already cancelled, do nothing else */
> -if (g_task_return_error_if_cancelled (self->priv->task)) {
> -g_clear_object (>priv->task);
> +if (port_probe_task_return_error_if_cancelled (self))
>  return G_SOURCE_REMOVE;
> -}
>
>  /* QMI probing needed? */
>  if ((ctx->flags & MM_PORT_PROBE_QMI) &&
> @@ -547,8 +589,7 @@ wdm_probe (MMPortProbe *self)
>  }
>
>  /* All done now */
> -g_task_return_boolean (self->priv->task, TRUE);
> -g_clear_object (>priv->task);
> +port_probe_task_return_boolean (self, TRUE);
>  return G_SOURCE_REMOVE;
>  }
>
> @@ -571,10 +612,8 @@ serial_probe_qcdm_parse_response (MMPortSerialQcdm *port,
>  ctx = g_task_get_task_data (self->priv->task);
>
>  /* If already cancelled, do nothing else */
> -if (g_task_return_error_if_cancelled (self->priv->task)) {
> -g_clear_object (>priv->task);
> +if (port_probe_task_return_error_if_cancelled (self))
>  return;
> -}
>
>  response = mm_port_serial_qcdm_command_finish (port, res, );
>  if (!error) {
> @@ -642,10 +681,8 @@ serial_probe_qcdm (MMPortProbe *self)
>  ctx->source_id = 0;
>
>  /* If already cancelled, do nothing else */
> -if (g_task_return_error_if_cancelled (self->priv->task)) {
> -g_clear_object (>priv->task);
> +if (port_probe_task_return_error_if_cancelled (self))
>  return G_SOURCE_REMOVE;
> -}
>
>  mm_dbg ("(%s/%s) probing QCDM...",
>  g_udev_device_get_subsystem (self->priv->port),
> @@ -665,27 +702,25 @@ serial_probe_qcdm (MMPortProbe *self)
>  /* Open the QCDM port */
>  ctx->serial = MM_PORT_SERIAL (mm_port_serial_qcdm_new 
> (g_udev_device_get_name (self->priv->port)));
>  if (!ctx->serial) {
> -g_task_return_new_error (self->priv->task,
> - MM_CORE_ERROR,
> - MM_CORE_ERROR_FAILED,
> - "(%s/%s) Couldn't create QCDM port",
> - g_udev_device_get_subsystem 
> (self->priv->port),
> - g_udev_device_get_name (self->priv->port));
> -g_clear_object (>priv->task);
> +port_probe_task_return_error (self,
> +  g_error_new (MM_CORE_ERROR,
> +

[PATCH] port-probe: make sure stored task pointer is set to NULL before completing

2016-04-08 Thread Aleksander Morgado 
When we were completing tasks in idle, the logic was like this:

 * Schedule task completion in idle
 * self->priv->task = NULL
 * (idle) Task completion callback called

This meant that the self->priv->task was always set to NULL before the
completion callback was called, which is what we wanted as a new task may be
scheduled in the callback itself.

Now, without completing in idle, we were wrongly doing:

 * Task completion callback called
 * self->priv->task = NULL

This commit fixes the logic by making sure self->priv->task = NULL before any
task completion.
---

Daniele, can you validate that this patch fixes your issue?

---
 src/mm-port-probe.c | 184 
 1 file changed, 100 insertions(+), 84 deletions(-)

diff --git a/src/mm-port-probe.c b/src/mm-port-probe.c
index a1ca8ba..5d2428a 100644
--- a/src/mm-port-probe.c
+++ b/src/mm-port-probe.c
@@ -95,6 +95,50 @@ struct _MMPortProbePrivate {
 };

 /*/
+/* Probe task completions.
+ * Always make sure that the stored task is NULL when the task is completed.
+ */
+
+static gboolean
+port_probe_task_return_error_if_cancelled (MMPortProbe *self)
+{
+GTask *task;
+
+task = self->priv->task;
+self->priv->task = NULL;
+
+if (g_task_return_error_if_cancelled (task)) {
+g_object_unref (task);
+return TRUE;
+}
+
+self->priv->task = task;
+return FALSE;
+}
+
+static void
+port_probe_task_return_error (MMPortProbe *self,
+  GError  *error)
+{
+GTask *task;
+
+task = self->priv->task;
+self->priv->task = NULL;
+g_task_return_error (task, error);
+}
+
+static void
+port_probe_task_return_boolean (MMPortProbe *self,
+gboolean result)
+{
+GTask *task;
+
+task = self->priv->task;
+self->priv->task = NULL;
+g_task_return_boolean (task, result);
+}
+
+/*/

 void
 mm_port_probe_set_result_at (MMPortProbe *self,
@@ -527,10 +571,8 @@ wdm_probe (MMPortProbe *self)
 ctx->source_id = 0;

 /* If already cancelled, do nothing else */
-if (g_task_return_error_if_cancelled (self->priv->task)) {
-g_clear_object (>priv->task);
+if (port_probe_task_return_error_if_cancelled (self))
 return G_SOURCE_REMOVE;
-}

 /* QMI probing needed? */
 if ((ctx->flags & MM_PORT_PROBE_QMI) &&
@@ -547,8 +589,7 @@ wdm_probe (MMPortProbe *self)
 }

 /* All done now */
-g_task_return_boolean (self->priv->task, TRUE);
-g_clear_object (>priv->task);
+port_probe_task_return_boolean (self, TRUE);
 return G_SOURCE_REMOVE;
 }

@@ -571,10 +612,8 @@ serial_probe_qcdm_parse_response (MMPortSerialQcdm *port,
 ctx = g_task_get_task_data (self->priv->task);

 /* If already cancelled, do nothing else */
-if (g_task_return_error_if_cancelled (self->priv->task)) {
-g_clear_object (>priv->task);
+if (port_probe_task_return_error_if_cancelled (self))
 return;
-}

 response = mm_port_serial_qcdm_command_finish (port, res, );
 if (!error) {
@@ -642,10 +681,8 @@ serial_probe_qcdm (MMPortProbe *self)
 ctx->source_id = 0;

 /* If already cancelled, do nothing else */
-if (g_task_return_error_if_cancelled (self->priv->task)) {
-g_clear_object (>priv->task);
+if (port_probe_task_return_error_if_cancelled (self))
 return G_SOURCE_REMOVE;
-}

 mm_dbg ("(%s/%s) probing QCDM...",
 g_udev_device_get_subsystem (self->priv->port),
@@ -665,27 +702,25 @@ serial_probe_qcdm (MMPortProbe *self)
 /* Open the QCDM port */
 ctx->serial = MM_PORT_SERIAL (mm_port_serial_qcdm_new 
(g_udev_device_get_name (self->priv->port)));
 if (!ctx->serial) {
-g_task_return_new_error (self->priv->task,
- MM_CORE_ERROR,
- MM_CORE_ERROR_FAILED,
- "(%s/%s) Couldn't create QCDM port",
- g_udev_device_get_subsystem 
(self->priv->port),
- g_udev_device_get_name (self->priv->port));
-g_clear_object (>priv->task);
+port_probe_task_return_error (self,
+  g_error_new (MM_CORE_ERROR,
+   MM_CORE_ERROR_FAILED,
+   "(%s/%s) Couldn't create 
QCDM port",
+   g_udev_device_get_subsystem 
(self->priv->port),
+   g_udev_device_get_name 
(self->priv->port)));
 return G_SOURCE_REMOVE;
 }

 /* Try to open the port */
 if (!mm_port_serial_open (ctx->serial, )) {
-g_task_return_new_error (self->priv->task,
-