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

Daniele Palmas dnlplm at gmail.com
Fri Apr 8 14:55:23 UTC 2016


Hi Aleksander,

2016-04-08 16:42 GMT+02:00 Aleksander Morgado <aleksander at 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.

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 (&self->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 (&self->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 (&self->priv->task);
> +    if (port_probe_task_return_error_if_cancelled (self))
>          return;
> -    }
>
>      response = mm_port_serial_qcdm_command_finish (port, res, &error);
>      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 (&self->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 (&self->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, &error)) {
> -        g_task_return_new_error (self->priv->task,
> -                                 MM_SERIAL_ERROR,
> -                                 MM_SERIAL_ERROR_OPEN_FAILED,
> -                                 "(%s/%s) Failed to open QCDM port: %s",
> -                                 g_udev_device_get_subsystem (self->priv->port),
> -                                 g_udev_device_get_name (self->priv->port),
> -                                 (error ? error->message : "unknown error"));
> +        port_probe_task_return_error (self,
> +                                      g_error_new (MM_SERIAL_ERROR,
> +                                                   MM_SERIAL_ERROR_OPEN_FAILED,
> +                                                   "(%s/%s) Failed to open QCDM port: %s",
> +                                                   g_udev_device_get_subsystem (self->priv->port),
> +                                                   g_udev_device_get_name (self->priv->port),
> +                                                   (error ? error->message : "unknown error")));
>          g_clear_error (&error);
> -        g_clear_object (&self->priv->task);
>          return G_SOURCE_REMOVE;
>      }
>
> @@ -699,13 +734,12 @@ serial_probe_qcdm (MMPortProbe *self)
>      len = qcdm_cmd_version_info_new ((char *) (verinfo->data + 1), 9);
>      if (len <= 0) {
>          g_byte_array_unref (verinfo);
> -        g_task_return_new_error (self->priv->task,
> -                                 MM_SERIAL_ERROR,
> -                                 MM_SERIAL_ERROR_OPEN_FAILED,
> -                                 "(%s/%s) Failed to create QCDM versin info command",
> -                                 g_udev_device_get_subsystem (self->priv->port),
> -                                 g_udev_device_get_name (self->priv->port));
> -        g_clear_object (&self->priv->task);
> +        port_probe_task_return_error (self,
> +                                      g_error_new (MM_SERIAL_ERROR,
> +                                                   MM_SERIAL_ERROR_OPEN_FAILED,
> +                                                   "(%s/%s) Failed to create QCDM versin info command",
> +                                                   g_udev_device_get_subsystem (self->priv->port),
> +                                                   g_udev_device_get_name (self->priv->port)));
>          return G_SOURCE_REMOVE;
>      }
>      verinfo->len = len + 1;
> @@ -857,10 +891,8 @@ serial_probe_at_parse_response (MMPortSerialAt *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 (&self->priv->task);
> +    if (port_probe_task_return_error_if_cancelled (self))
>          return;
> -    }
>
>      /* If AT probing cancelled, end this partial probing */
>      if (g_cancellable_is_cancelled (ctx->at_probing_cancellable)) {
> @@ -882,14 +914,13 @@ serial_probe_at_parse_response (MMPortSerialAt *port,
>                                                 &result_error)) {
>          /* Were we told to abort the whole probing? */
>          if (result_error) {
> -            g_task_return_new_error (self->priv->task,
> -                                     MM_CORE_ERROR,
> -                                     MM_CORE_ERROR_UNSUPPORTED,
> -                                     "(%s/%s) error while probing AT features: %s",
> -                                     g_udev_device_get_subsystem (self->priv->port),
> -                                     g_udev_device_get_name (self->priv->port),
> -                                     result_error->message);
> -            g_clear_object (&self->priv->task);
> +            port_probe_task_return_error (self,
> +                                          g_error_new (MM_CORE_ERROR,
> +                                                       MM_CORE_ERROR_UNSUPPORTED,
> +                                                       "(%s/%s) error while probing AT features: %s",
> +                                                       g_udev_device_get_subsystem (self->priv->port),
> +                                                       g_udev_device_get_name (self->priv->port),
> +                                                       result_error->message));
>              goto out;
>          }
>
> @@ -940,10 +971,8 @@ serial_probe_at (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 (&self->priv->task);
> +    if (port_probe_task_return_error_if_cancelled (self))
>          return G_SOURCE_REMOVE;
> -    }
>
>      /* If AT probing cancelled, end this partial probing */
>      if (g_cancellable_is_cancelled (ctx->at_probing_cancellable)) {
> @@ -1007,8 +1036,7 @@ at_custom_init_ready (MMPortProbe *self,
>
>      if (!ctx->at_custom_init_finish (self, res, &error)) {
>          /* All errors propagated up end up forcing an UNSUPPORTED result */
> -        g_task_return_error (self->priv->task, error);
> -        g_clear_object (&self->priv->task);
> +        port_probe_task_return_error (self, error);
>          return;
>      }
>
> @@ -1023,16 +1051,13 @@ static void
>  serial_probe_schedule (MMPortProbe *self)
>  {
>      PortProbeRunContext *ctx;
> -    GTask *task;
>
>      g_assert (self->priv->task);
>      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 (&self->priv->task);
> +    if (port_probe_task_return_error_if_cancelled (self))
>          return;
> -    }
>
>      /* If we got some custom initialization setup requested, go on with it
>       * first. */
> @@ -1101,10 +1126,7 @@ serial_probe_schedule (MMPortProbe *self)
>      }
>
>      /* All done! */
> -    task = self->priv->task;
> -    self->priv->task = NULL;
> -    g_task_return_boolean (task, TRUE);
> -    g_object_unref (task);
> +    port_probe_task_return_boolean (self, TRUE);
>  }
>
>  static void
> @@ -1168,10 +1190,8 @@ serial_open_at (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 (&self->priv->task);
> +    if (port_probe_task_return_error_if_cancelled (self))
>          return G_SOURCE_REMOVE;
> -    }
>
>      /* Create AT serial port if not done before */
>      if (!ctx->serial) {
> @@ -1183,13 +1203,12 @@ serial_open_at (MMPortProbe *self)
>
>          ctx->serial = MM_PORT_SERIAL (mm_port_serial_at_new (g_udev_device_get_name (self->priv->port), subsys));
>          if (!ctx->serial) {
> -            g_task_return_new_error (self->priv->task,
> -                                     MM_CORE_ERROR,
> -                                     MM_CORE_ERROR_FAILED,
> -                                     "(%s/%s) couldn't create AT port",
> -                                     g_udev_device_get_subsystem (self->priv->port),
> -                                     g_udev_device_get_name (self->priv->port));
> -            g_clear_object (&self->priv->task);
> +            port_probe_task_return_error (self,
> +                                          g_error_new (MM_CORE_ERROR,
> +                                                       MM_CORE_ERROR_FAILED,
> +                                                       "(%s/%s) couldn't create AT port",
> +                                                       g_udev_device_get_subsystem (self->priv->port),
> +                                                       g_udev_device_get_name (self->priv->port)));
>              return G_SOURCE_REMOVE;
>          }
>
> @@ -1215,13 +1234,12 @@ serial_open_at (MMPortProbe *self)
>          /* Abort if maximum number of open tries reached */
>          if (++ctx->at_open_tries > 4) {
>              /* took too long to open the port; give up */
> -            g_task_return_new_error (self->priv->task,
> -                                     MM_CORE_ERROR,
> -                                     MM_CORE_ERROR_FAILED,
> -                                     "(%s/%s) failed to open port after 4 tries",
> -                                     g_udev_device_get_subsystem (self->priv->port),
> -                                     g_udev_device_get_name (self->priv->port));
> -            g_clear_object (&self->priv->task);
> +            port_probe_task_return_error (self,
> +                                          g_error_new (MM_CORE_ERROR,
> +                                                       MM_CORE_ERROR_FAILED,
> +                                                       "(%s/%s) failed to open port after 4 tries",
> +                                                       g_udev_device_get_subsystem (self->priv->port),
> +                                                       g_udev_device_get_name (self->priv->port)));
>              g_clear_error (&error);
>              return G_SOURCE_REMOVE;
>          }
> @@ -1233,14 +1251,13 @@ serial_open_at (MMPortProbe *self)
>              return G_SOURCE_REMOVE;
>          }
>
> -        g_task_return_new_error (self->priv->task,
> -                                 MM_SERIAL_ERROR,
> -                                 MM_SERIAL_ERROR_OPEN_FAILED,
> -                                 "(%s/%s) failed to open port: %s",
> -                                 g_udev_device_get_subsystem (self->priv->port),
> -                                 g_udev_device_get_name (self->priv->port),
> -                                 (error ? error->message : "unknown error"));
> -        g_clear_object (&self->priv->task);
> +        port_probe_task_return_error (self,
> +                                      g_error_new (MM_SERIAL_ERROR,
> +                                                   MM_SERIAL_ERROR_OPEN_FAILED,
> +                                                   "(%s/%s) failed to open port: %s",
> +                                                   g_udev_device_get_subsystem (self->priv->port),
> +                                                   g_udev_device_get_name (self->priv->port),
> +                                                   (error ? error->message : "unknown error")));
>          g_clear_error (&error);
>          return G_SOURCE_REMOVE;
>      }
> @@ -1346,8 +1363,7 @@ mm_port_probe_run (MMPortProbe                *self,
>          mm_dbg ("(%s/%s) port probing finished: no more probings needed",
>                  g_udev_device_get_subsystem (self->priv->port),
>                  g_udev_device_get_name (self->priv->port));
> -        g_task_return_boolean (self->priv->task, TRUE);
> -        g_clear_object (&self->priv->task);
> +        port_probe_task_return_boolean (self, TRUE);
>          return;
>      }
>
> --
> 2.8.0


More information about the ModemManager-devel mailing list