[PATCH] broadband-modem-mbim: parse nw_error in register_state_set_ready only if MBIM_STATUS_FAILURE

[Aleksander Morgado]

Hey!

See comments below.

On Thu, Mar 21, 2019 at 1:14 PM Lech Perczak
<l.perczak at camlintechnologies.com> wrote:
>
> Some modems (Namely: Telit LE910 V2) report nonzero NwError code,
> outside of 3GPP TS 24.008 - in "register-state set command-done" response,
> while status code equals MBIM_STATUS_ERROR_NONE.
> In such cases network is operational.
> According to MBIM specification 1.0 table 10.5.9.8 "Status codes",
> NwError shall be nonzero only if Status Code equals MBIM_STATUS_FAILURE,
> and client shall parse NwError only in such cases.
> Also, MBIM specification does not explicitly state that 'NwError == 0' equals
> no error, rather than that it is unknown error, hence raise an error
> unconditionally if MBIM status code is MBIM_STATUS_FAILURE.
>
> Therefore, check NwError IFF MBIM response status code equals
> MBIM_STATUS_FAILURE.
>

Understood, makes sense.

> While at that, ensure that nw_error is initialized if parsing MBIM message
> fails for any reason, which could also break registration process and
> desynchronize ModemManager's internal state, preventing connection until
> MM or modem restart.
>

That's not a bug, because if
mbim_message_register_state_response_parse() returns TRUE, we're sure
nw_error has been initialized. This assumption is expected in all
message parsing methods.

> Fixes: 854c371c8aa9 ("broadband-modem-mbim: implement 3GPP registration request")
> Signed-off-by: Lech Perczak <l.perczak at camlintechnologies.com>
> ---
>  src/mm-broadband-modem-mbim.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/src/mm-broadband-modem-mbim.c b/src/mm-broadband-modem-mbim.c
> index fa62485c01e6..3da5b6944718 100644
> --- a/src/mm-broadband-modem-mbim.c
> +++ b/src/mm-broadband-modem-mbim.c
> @@ -3944,24 +3944,25 @@ register_state_set_ready (MbimDevice *device,
>  {
>      MbimMessage *response;
>      GError *error = NULL;
> -    MbimNwError nw_error;
> +    MbimNwError nw_error = MBIM_NW_ERROR_UNKNOWN;
>

As said, the above not needed really.

>      response = mbim_device_command_finish (device, res, &error);
>      if (response &&
> -        mbim_message_response_get_result (response, MBIM_MESSAGE_TYPE_COMMAND_DONE, &error) &&
> -        mbim_message_register_state_response_parse (
> -            response,
> -            &nw_error,
> -            NULL, /* &register_state */
> -            NULL, /* register_mode */
> -            NULL, /* available_data_classes */
> -            NULL, /* current_cellular_class */
> -            NULL, /* provider_id */
> -            NULL, /* provider_name */
> -            NULL, /* roaming_text */
> -            NULL, /* registration_flag */
> -            NULL)) {
> -        if (nw_error)
> +        !mbim_message_response_get_result (response, MBIM_MESSAGE_TYPE_COMMAND_DONE, &error) &&

Could you please add a comment here, explaining why we're trying to
parse the response only when it's reporting a FAILURE? I understand
the logic, but given it's a completely the opposite of what's usually
done, it makes sense to explain it in a comment.

> +        g_error_matches (error, MBIM_STATUS_ERROR, MBIM_STATUS_ERROR_FAILURE)) {

And given that you have now an additional context here inside the
if(), you could declare the MbimNwError nw_error variable here.

> +        g_clear_error (&error);
> +        if (mbim_message_register_state_response_parse (
> +                response,
> +                &nw_error,
> +                NULL, /* &register_state */
> +                NULL, /* register_mode */
> +                NULL, /* available_data_classes */
> +                NULL, /* current_cellular_class */
> +                NULL, /* provider_id */
> +                NULL, /* provider_name */
> +                NULL, /* roaming_text */
> +                NULL, /* registration_flag */
> +                &error))
>              error = mm_mobile_equipment_error_from_mbim_nw_error (nw_error);
>      }
>
> --
> 2.7.4
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel



-- 
Aleksander
https://aleksander.es