[PATCH] huawei: retry connect/disconnect attempt upon NDISSTATQRY failures

[Aleksander Morgado]

On 28/08/13 20:59, Prathmesh Prabhu wrote:
> This is a workaround for a bug in the modem firmware that causes the NDISSTATQRY
> response to be an error response intermittently. With this patch, the connect /
> disconnect attempt ignores a few of these error responses. Note that the overall
> time-out for the connect/disconnect is not affected by this change.


See some comments below.

But I'm mostly interested in what the exact issues are. What does
NDISSTATQRY reply that the parser doesn't like? Which is the exact
response string? Shouldn't we just improve the parser to handle those cases?



> ---
>  plugins/huawei/mm-broadband-bearer-huawei.c | 54 ++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/plugins/huawei/mm-broadband-bearer-huawei.c b/plugins/huawei/mm-broadband-bearer-huawei.c
> index 2e1b0b0..75d3566 100644
> --- a/plugins/huawei/mm-broadband-bearer-huawei.c
> +++ b/plugins/huawei/mm-broadband-bearer-huawei.c
> @@ -57,6 +57,7 @@ typedef struct {
>      GSimpleAsyncResult *result;
>      Connect3gppContextStep step;
>      guint check_count;
> +    guint failed_ndisstatqry_count;
>  } Connect3gppContext;
>  
>  static void
> @@ -130,20 +131,15 @@ connect_ndisstatqry_check_ready (MMBaseModem *modem,
>                                                 &ipv6_available,
>                                                 &ipv6_connected,
>                                                 &error)) {
> -        mm_dbg ("Modem doesn't properly support ^NDISSTATQRY command: %s", error->message);
> +        ctx->failed_ndisstatqry_count++;
> +        mm_dbg ("Unexpected response to ^NDISSTATQRY command: %s (Attempts so far: %d)",


"failed_ndisstatqry_count" is a guint, so should be printed with "%u".


> +                error->message, ctx->failed_ndisstatqry_count);
>          g_error_free (error);
> -
> -        ctx->self->priv->connect_pending = NULL;
> -        g_simple_async_result_set_error (ctx->result,
> -                                         MM_MOBILE_EQUIPMENT_ERROR,
> -                                         MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED,
> -                                         "Connection attempt not supported");
> -        connect_3gpp_context_complete_and_free (ctx);
> -        return;
> +        response = NULL;  /* Set to NULL to skip steps below. */


I don't think this response=NULL is needed. The parser method will only
set 'ip4_available' and/or 'ip4_connected' if it returned TRUE; so if it
returns FALSE these will always be FALSE, and therefore the next steps
will already be skipped.


>      }
>  
>      /* Connected in IPv4? */
> -    if (ipv4_available && ipv4_connected) {
> +    if (response && ipv4_available && ipv4_connected) {
>          /* Success! */
>          ctx->step++;
>          connect_3gpp_context_step (ctx);
> @@ -315,6 +311,17 @@ connect_3gpp_context_step (Connect3gppContext *ctx)
>              connect_3gpp_context_complete_and_free (ctx);
>              return;
>          }

Add a whiteline here.

> +        /* Give up if too many unexpected responses to NIDSSTATQRY are encountered. */
> +        if (ctx->failed_ndisstatqry_count > 3) {
> +            /* Clear context */
> +            ctx->self->priv->connect_pending = NULL;
> +            g_simple_async_result_set_error (ctx->result,
> +                                             MM_MOBILE_EQUIPMENT_ERROR,
> +                                             MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED,
> +                                             "Connection attempt not supported.");
> +            connect_3gpp_context_complete_and_free (ctx);
> +            return;
> +        }
>  
>          /* Check if connected */
>          ctx->check_count++;
> @@ -413,6 +420,7 @@ typedef struct {
>      GSimpleAsyncResult *result;
>      Disconnect3gppContextStep step;
>      guint check_count;
> +    guint failed_ndisstatqry_count;
>  } Disconnect3gppContext;
>  
>  static void
> @@ -480,20 +488,15 @@ disconnect_ndisstatqry_check_ready (MMBaseModem *modem,
>                                                 &ipv6_available,
>                                                 &ipv6_connected,
>                                                 &error)) {
> -        mm_dbg ("Modem doesn't properly support ^NDISSTATQRY command: %s", error->message);
> +        ctx->failed_ndisstatqry_count++;
> +        mm_dbg ("Unexpected response to ^NDISSTATQRY command: %s (Attempts so far: %d)",


"failed_ndisstatqry_count" is a guint, so should be printed with "%u".


> +                error->message, ctx->failed_ndisstatqry_count);
>          g_error_free (error);
> -
> -        ctx->self->priv->disconnect_pending = NULL;
> -        g_simple_async_result_set_error (ctx->result,
> -                                         MM_MOBILE_EQUIPMENT_ERROR,
> -                                         MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED,
> -                                         "Disconnection attempt not supported");
> -        disconnect_3gpp_context_complete_and_free (ctx);
> -        return;
> +        response = NULL;  /* Set to NULL to skip steps below. */


Same here; I don't think this response=NULL is needed.


>      }
>  
>      /* Disconnected IPv4? */
> -    if (ipv4_available && !ipv4_connected) {
> +    if (response && ipv4_available && !ipv4_connected) {
>          /* Success! */
>          ctx->step++;
>          disconnect_3gpp_context_step (ctx);
> @@ -568,6 +571,17 @@ disconnect_3gpp_context_step (Disconnect3gppContext *ctx)
>              disconnect_3gpp_context_complete_and_free (ctx);
>              return;
>          }

Add a whiteline here.

> +        /* Give up if too many unexpected responses to NIDSSTATQRY are encountered. */
> +        if (ctx->failed_ndisstatqry_count > 3) {
> +            /* Clear context */
> +            ctx->self->priv->disconnect_pending = NULL;
> +            g_simple_async_result_set_error (ctx->result,
> +                                             MM_MOBILE_EQUIPMENT_ERROR,
> +                                             MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED,
> +                                             "Disconnection attempt not supported.");
> +            disconnect_3gpp_context_complete_and_free (ctx);
> +            return;
> +        }
>  
>          /* Check if disconnected */
>          ctx->check_count++;
> 


-- 
Aleksander