Re: [PATCH] port-probe: make sure stored task pointer is set to NULL before completing
On Wed, Apr 13, 2016 at 10:26 AM, Carlo Lobranowrote: > 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
On Mon, Apr 11, 2016 at 4:56 PM, Carlo Lobranowrote: > > 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
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 Palmaswrote: > 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
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
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
On Fri, Apr 8, 2016 at 4:55 PM, Daniele Palmaswrote: > 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
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, > +