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

[Aleksander Morgado]

On 04/09/13 06:06, Ben Chan wrote:
> From: Prathmesh Prabhu <pprabhu at chromium.org>
> 
> The Huawei MU736 modem sometimes responds to the ^NDISSTATQRY? query with a
> '+CME ERROR: 100' error. This patch works around the issue by ignoring a few
> of these error responses in a connect / disconnect attempt. The overall timeout
> for the connect/disconnect operation is not affected by this change.
> ---
>  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..505b354 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,14 @@ 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: %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;
>      }
>  
>      /* Connected in IPv4? */
> -    if (ipv4_available && ipv4_connected) {
> +    if (response && ipv4_available && ipv4_connected) {


I still think that checking for a valid 'response' here is not really
needed. There will never be a case where ip4_available and
ipv4_connected are both TRUE but response isn't.


>          /* Success! */
>          ctx->step++;
>          connect_3gpp_context_step (ctx);
> @@ -316,6 +311,18 @@ connect_3gpp_context_step (Connect3gppContext *ctx)
>              return;
>          }
>  
> +        /* Give up if too many unexpected responses to NIDSSTATQRY are encountered. */
> +        if (ctx->failed_ndisstatqry_count > 3) {


And now just wondering; is 3 a safe value? Why not 10 or so? NDISSTATQRY
should really never fail, so why not play safer and avoid the firmware
issue with a bigger value?


> +            /* 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++;
>          mm_base_modem_at_command_full (ctx->modem,
> @@ -413,6 +420,7 @@ typedef struct {
>      GSimpleAsyncResult *result;
>      Disconnect3gppContextStep step;
>      guint check_count;
> +    guint failed_ndisstatqry_count;
>  } Disconnect3gppContext;
>  
>  static void
> @@ -480,20 +488,14 @@ 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: %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;
>      }
>  
>      /* Disconnected IPv4? */
> -    if (ipv4_available && !ipv4_connected) {
> +    if (response && ipv4_available && !ipv4_connected) {


Same here; there will never be a case where ipv4_available is TRUE and
response is NULL, so no need to check for response being not NULL.


>          /* Success! */
>          ctx->step++;
>          disconnect_3gpp_context_step (ctx);
> @@ -569,6 +571,18 @@ disconnect_3gpp_context_step (Disconnect3gppContext *ctx)
>              return;
>          }
>  
> +        /* 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++;
>          mm_base_modem_at_command_full (ctx->modem,
> 


-- 
Aleksander