Re: [PATCH] broadband-modem-mbim: parse nw_error in register_state_set_ready only if MBIM_STATUS_FAILURE
On Thu, 2019-04-25 at 12:33 +0200, Aleksander Morgado wrote: > Hey! > > > Any update on the patch? :) > > In the meantime I got response from Telit that they won't fix the > > original problem which made me create this patch. > > > > Oh wow, I forgot about it, sorry... > I have pushed it to a new merge request in gitlab now: > https://gitlab.freedesktop.org/mobile-broadband/ModemManager/merge_requests/97 > > @Dan Williams @Ben Chan any comments on this? Looks good to me, I merged it. Dan ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH] broadband-modem-mbim: parse nw_error in register_state_set_ready only if MBIM_STATUS_FAILURE
Hey! > > Any update on the patch? :) > In the meantime I got response from Telit that they won't fix the original > problem which made me create this patch. > Oh wow, I forgot about it, sorry... I have pushed it to a new merge request in gitlab now: https://gitlab.freedesktop.org/mobile-broadband/ModemManager/merge_requests/97 @Dan Williams @Ben Chan any comments on this? -- Aleksander https://aleksander.es ___ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH] broadband-modem-mbim: parse nw_error in register_state_set_ready only if MBIM_STATUS_FAILURE
Hi Aleksander,
Any update on the patch? :)
In the meantime I got response from Telit that they won't fix the original
problem which made me create this patch.
With kind regards,
Lech
W dniu 21.03.2019 o 13:14, Lech Perczak pisze:
> 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.
>
> 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.
>
> Fixes: 854c371c8aa9 ("broadband-modem-mbim: implement 3GPP registration
> request")
> Signed-off-by: Lech Perczak
> ---
> 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;
>
> response = mbim_device_command_finish (device, res, );
> if (response &&
> -mbim_message_response_get_result (response,
> MBIM_MESSAGE_TYPE_COMMAND_DONE, ) &&
> -mbim_message_register_state_response_parse (
> -response,
> -_error,
> -NULL, /* _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, ) &&
> +g_error_matches (error, MBIM_STATUS_ERROR,
> MBIM_STATUS_ERROR_FAILURE)) {
> +g_clear_error ();
> +if (mbim_message_register_state_response_parse (
> +response,
> +_error,
> +NULL, /* _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 = mm_mobile_equipment_error_from_mbim_nw_error (nw_error);
> }
>
--
Pozdrawiam/With kind regards,
Lech Perczak
Sr. Software Engineer
Camlin Technologies Poland Limited Sp. z o.o.
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
Re: [PATCH] broadband-modem-mbim: parse nw_error in register_state_set_ready only if MBIM_STATUS_FAILURE
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
> 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
>> ---
>> 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, );
>> if (response &&
>> -mbim_message_response_get_result (response,
>> MBIM_MESSAGE_TYPE_COMMAND_DONE, ) &&
>> -mbim_message_register_state_response_parse (
>> -response,
>> -_error,
>> -NULL, /* _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, ) &&
> 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 ();
>> +if (mbim_message_register_state_response_parse (
>> +response,
>> +_error,
>> +NULL, /* _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 = 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
Re: [PATCH] broadband-modem-mbim: parse nw_error in register_state_set_ready only if MBIM_STATUS_FAILURE
Hey!
See comments below.
On Thu, Mar 21, 2019 at 1:14 PM Lech Perczak
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
> ---
> 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, );
> if (response &&
> -mbim_message_response_get_result (response,
> MBIM_MESSAGE_TYPE_COMMAND_DONE, ) &&
> -mbim_message_register_state_response_parse (
> -response,
> -_error,
> -NULL, /* _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, ) &&
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 ();
> +if (mbim_message_register_state_response_parse (
> +response,
> +_error,
> +NULL, /* _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 = 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
--
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
[PATCH] broadband-modem-mbim: parse nw_error in register_state_set_ready only if MBIM_STATUS_FAILURE
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.
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.
Fixes: 854c371c8aa9 ("broadband-modem-mbim: implement 3GPP registration
request")
Signed-off-by: Lech Perczak
---
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;
response = mbim_device_command_finish (device, res, );
if (response &&
-mbim_message_response_get_result (response,
MBIM_MESSAGE_TYPE_COMMAND_DONE, ) &&
-mbim_message_register_state_response_parse (
-response,
-_error,
-NULL, /* _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, ) &&
+g_error_matches (error, MBIM_STATUS_ERROR, MBIM_STATUS_ERROR_FAILURE))
{
+g_clear_error ();
+if (mbim_message_register_state_response_parse (
+response,
+_error,
+NULL, /* _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 = 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
