Hi Aleksander,
Thank you for such a swift response!
I will post updated patch shortly, after testing updated changes locally.
W dniu 21.03.2019 o 13:42, Aleksander Morgado pisze:
> Hey!
>
> See comments below.
>
> On Thu, Mar 21, 2019 at 1:14 PM Lech Perczak
> <l.perc...@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.perc...@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, /* ®ister_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, /* ®ister_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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
>
>
--
Pozdrawiam/With kind regards,
Lech Perczak
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
