[PATCH] Hardening PORTCFG parse reply

[Dan Williams]

On Wed, 2016-06-01 at 14:51 +0200, Carlo Lobrano wrote:
> A brief explaination of the issue that this patch solves.
> I observed a probing failure at resume from memory (S3), caused by a
> missing or wrong reply to #PORTCFG? AT command sent during port
> probing.
> This error is probably due to the overposition of multiple messages
> and replies,
> since at the same time the ModemManager is trying and close the last
> connection,
> and probing a new modem device, and the PORTCFG parser receive an
> empty string to
> parse, causing the failure.
> Therefore the fix consists in the hardening of the reply to this
> command and
> in providing a default port configuration in case of the retries are
> not enough.

Hmm, this seems like a hackish workaround...  What suspend/resume
manager are you using?  Ideally MM would either block suspend until it
had cleaned up and disposed the modem, or on resume it would just
force-close everything, dispose the modem, and reprobe.

Dan

> ---
> 
> - Increase PORTCFG retries during initialization from 3 to 5 retries.
> - Provide a default PORTCFG configuration in case of missing or wrong
> reply.
> ---
>  plugins/telit/mm-plugin-telit.c | 44 +++++++++++++++++++++++++++--
> ------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/plugins/telit/mm-plugin-telit.c b/plugins/telit/mm-
> plugin-telit.c
> index a3620e7..19eed8f 100644
> --- a/plugins/telit/mm-plugin-telit.c
> +++ b/plugins/telit/mm-plugin-telit.c
> @@ -165,8 +165,10 @@ cache_port_mode (MMDevice *device,
>      r = g_regex_new ("#PORTCFG:\\s*(\\d+),(\\d+)", flags, 0, NULL);
>      g_assert (r != NULL);
>  
> -    if (!g_regex_match_full (r, reply, strlen (reply), 0, 0,
> &match_info, &error))
> +    if (!g_regex_match_full (r, reply, strlen (reply), 0, 0,
> &match_info, &error)) {
> +        mm_dbg ("Unexpected PORTCFG reply '%s'. Could not parse
> it.", reply);
>          goto out;
> +    }
>  
>      if (!mm_get_uint_from_match_info (match_info, 2,
> &portcfg_current)) {
>          mm_dbg ("telit: unrecognized #PORTCFG <active> value");
> @@ -201,6 +203,7 @@ cache_port_mode (MMDevice *device,
>          break;
>      default:
>          /* portcfg value not supported */
> +        mm_err ("Invalid portcfg value from reply '%s'", reply);
>          goto out;
>      }
>      ret = TRUE;
> @@ -222,29 +225,40 @@ getportcfg_ready (MMPortSerialAt *port,
>  {
>      const gchar *response;
>      GError *error = NULL;
> +    MMDevice *device;
>  
> +    device = mm_port_probe_peek_device (ctx->probe);
>      response = mm_port_serial_at_command_finish (port, res, &error);
> -    if (error) {
> -        mm_dbg ("telit: couldn't get port mode: '%s'",
> -                error->message);
>  
> -        /* If ERROR or COMMAND NOT SUPPORT occur then do not retry
> the
> -         * command.
> -         */
> -        if (g_error_matches (error,
> -                             MM_MOBILE_EQUIPMENT_ERROR,
> -                             MM_MOBILE_EQUIPMENT_ERROR_UNKNOWN))
> -            ctx->getportcfg_done = TRUE;
> +    if (error) {
> +        mm_dbg ("telit: couldn't get port mode: '%s' (retries left
> %d)",
> +                error->message,
> +                ctx->getportcfg_retries);
> +
> +        if (ctx->getportcfg_retries == 0) {
> +            mm_dbg ("telit: getportcfg last retry failed. Try and
> setting default port config.");
> +            if (g_object_get_data (G_OBJECT (device),
> TAG_GETPORTCFG_SUPPORTED) == NULL) {
> +                mm_dbg ("telit: Defaulting to 00 and 06 for modem
> and aux");
> +                g_object_set_data (G_OBJECT (device),
> TAG_TELIT_MODEM_PORT, "00");
> +                g_object_set_data (G_OBJECT (device),
> TAG_TELIT_AUX_PORT, "06");
> +                g_object_set_data (G_OBJECT (device),
> TAG_GETPORTCFG_SUPPORTED, GUINT_TO_POINTER (TRUE));
> +                ctx->getportcfg_done = TRUE;
> +            }
> +        }
>      } else {
> -        MMDevice *device;
> -        device = mm_port_probe_peek_device (ctx->probe);
> -
>          /* Results are cached in the parent device object */
>          if (g_object_get_data (G_OBJECT (device),
> TAG_GETPORTCFG_SUPPORTED) == NULL) {
>              mm_dbg ("telit: retrieving port mode layout");
>              if (cache_port_mode (device, response)) {
>                  g_object_set_data (G_OBJECT (device),
> TAG_GETPORTCFG_SUPPORTED, GUINT_TO_POINTER (TRUE));
>                  ctx->getportcfg_done = TRUE;
> +            } else {
> +                mm_dbg ("telit: Error in cache port mode");
> +                mm_dbg ("telit: Defaulting to 00 and 06 for modem
> and aux");
> +                g_object_set_data (G_OBJECT (device),
> TAG_TELIT_MODEM_PORT, "00");
> +                g_object_set_data (G_OBJECT (device),
> TAG_TELIT_AUX_PORT, "06");
> +                g_object_set_data (G_OBJECT (device),
> TAG_GETPORTCFG_SUPPORTED, GUINT_TO_POINTER (TRUE));
> +                ctx->getportcfg_done = TRUE;
>              }
>          }
>  
> @@ -334,7 +348,7 @@ telit_custom_init (MMPortProbe *probe,
>      ctx->port = g_object_ref (port);
>      ctx->cancellable = cancellable ? g_object_ref (cancellable) :
> NULL;
>      ctx->getportcfg_done = FALSE;
> -    ctx->getportcfg_retries = 3;
> +    ctx->getportcfg_retries = 5;
>  
>      /* If the device is tagged for supporting #PORTCFG do the custom
> init */
>      if (g_udev_device_get_property_as_boolean (udevDevice,
> "ID_MM_TELIT_PORTS_TAGGED")) {