[PATCH 2/2] dell: speed probing time up and reduce udev dependency

[Aleksander Morgado]

Hey Carlo!

On 11/05/17 17:05, Carlo Lobrano wrote:
> Currently Dell plugin implements a retry logic of three commands
> (AT+GMI, AT+CGMI, ATI1I2I3) to detect modem's vendor string.
> Moreover, since Telit modems always reply to the first command, to speed
> the probing time up, those modem are tagged with an Udev rule so that we
> can avoid sending the other two commands at all.
> 
> However, the retry logic is in case a port needs some time to reply, so
> it makes sense to apply it only to the first command. Then if the port still
> does not respond with any kind of reply, that probably means that it is not
> AT capable and we can skip the other AT commands as well.
> 
> Then, this patch:
> - sets a maximum number of timeouts for AT+GMI to 3. After this
>   timeouts, the port is considered not AT-capable.
> - sets AT+CGMI and ATI1I2I3 to be sent only once.
> - removes Dell udev rule for tagging Telit Modems.
> ---
> 
> Updated the patch according to Aleksander code review.

Thanks for the update, I believe the logic is good, but see some
additional comments below. Also, any chance you can send debug logs with
the whole process once the patch updated?

> 
> ---
>  plugins/dell/77-mm-dell-port-types.rules |  4 ----
>  plugins/dell/mm-plugin-dell.c            | 26 ++++++++++++++++----------
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/plugins/dell/77-mm-dell-port-types.rules b/plugins/dell/77-mm-dell-port-types.rules
> index bbb59b9..206fb6e 100644
> --- a/plugins/dell/77-mm-dell-port-types.rules
> +++ b/plugins/dell/77-mm-dell-port-types.rules
> @@ -8,9 +8,5 @@ GOTO="mm_dell_port_types_end"
>  LABEL="mm_dell_vendorcheck"
>  SUBSYSTEMS=="usb", ATTRS{bInterfaceNumber}=="?*", ENV{.MM_USBIFNUM}="$attr{bInterfaceNumber}"
>  
> -# DW5580 is a Dell-branded Telit modem
> -# tag is needed here for dynamic port recognition
> -ATTRS{idVendor}=="413c", ATTRS{idProduct}=="81ba", ENV{ID_MM_TELIT_PORTS_TAGGED}="1"
> -
>  GOTO="mm_dell_port_types_end"
>  LABEL="mm_dell_port_types_end"
> diff --git a/plugins/dell/mm-plugin-dell.c b/plugins/dell/mm-plugin-dell.c
> index f9dfd1a..f982cfb 100644
> --- a/plugins/dell/mm-plugin-dell.c
> +++ b/plugins/dell/mm-plugin-dell.c
> @@ -45,6 +45,8 @@
>  #include "mm-broadband-modem-mbim.h"
>  #endif
>  
> +#define MAX_PORT_PROBE_TIMEOUTS 3
> +
>  G_DEFINE_TYPE (MMPluginDell, mm_plugin_dell, MM_TYPE_PLUGIN)
>  
>  MM_PLUGIN_DEFINE_MAJOR_VERSION
> @@ -70,6 +72,7 @@ typedef struct {
>      guint gmi_retries;
>      guint cgmi_retries;
>      guint ati_retries;
> +    guint timeouts;
>  } CustomInitContext;
>  
>  static void
> @@ -140,6 +143,8 @@ static void custom_init_step (CustomInitContext *ctx);
>  static void
>  custom_init_step_next_command (CustomInitContext *ctx)
>  {
> +    ctx->timeouts = 0;
> +
>      if (ctx->gmi_retries > 0)
>          ctx->gmi_retries = 0;
>      else if (ctx->cgmi_retries > 0)
> @@ -169,6 +174,7 @@ response_ready (MMPortSerialAt *port,
>              return;
>          }
>          /* Directly retry same command on timeout */
> +        ctx->timeouts++;
>          custom_init_step (ctx);
>          g_error_free (error);
>          return;
> @@ -269,6 +275,12 @@ custom_init_step (CustomInitContext *ctx)
>      }
>  #endif
>  
> +    if (ctx->timeouts >= MAX_PORT_PROBE_TIMEOUTS) {
> +        mm_dbg ("(Dell) couldn't flip secondary port to AT in (%s): too many timeouts",
> +                mm_port_get_device (MM_PORT (ctx->port)));

Could you also do this at this point?

  mm_port_probe_set_result_at (ctx->probe, FALSE);

This will store the "AT probing result" in the probe itself and will not
try any other AT command afterwards. Without this previous line I
believe you would still be seeing plain "AT" commands sent to the port
afterwards as the port wasn't flagged as not being AT capable. Could you
maybe confirm this?

Also, the "couldn't flip..." message is wrong. It should say something
like "(Dell) couldn't detect real manufacturer in (%s): too many timeouts".


> +        goto out;
> +    }
> +
>      if (ctx->gmi_retries > 0) {
>          ctx->gmi_retries--;
>          mm_port_serial_at_command (ctx->port,
> @@ -309,9 +321,10 @@ custom_init_step (CustomInitContext *ctx)
>          return;
>      }
>  
> -    /* Finish custom_init */
>      mm_dbg ("(Dell) couldn't flip secondary port to AT in (%s): all retries consumed",
>              mm_port_get_device (MM_PORT (ctx->port)));

Same here with the message; I know this was not part of your patch, but
could you also fix the message there? :)

> +out:
> +    /* Finish custom_init */
>      g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
>      custom_init_context_complete_and_free (ctx);
>  }
> @@ -337,15 +350,8 @@ dell_custom_init (MMPortProbe *probe,
>      ctx->port = g_object_ref (port);
>      ctx->cancellable = cancellable ? g_object_ref (cancellable) : NULL;
>      ctx->gmi_retries = 3;
> -    ctx->cgmi_retries = 3;
> -    ctx->ati_retries = 3;
> -
> -    /* Dell-branded Telit modems always answer to +GMI
> -     * Avoid +CGMI and ATI sending for minimizing port probing time */
> -    if (mm_kernel_device_get_global_property_as_boolean (port_device, "ID_MM_TELIT_PORTS_TAGGED")) {
> -        ctx->cgmi_retries = 0;
> -        ctx->ati_retries = 0;
> -    }
> +    ctx->cgmi_retries = 1;
> +    ctx->ati_retries = 1;
>  
>      custom_init_step (ctx);
>  }
> 


-- 
Aleksander
https://aleksander.es