Re: [PATCH] ath10k: Fix the parsing error in service available event

[Doug Anderson]

Hi,

On Tue, Oct 27, 2020 at 8:20 AM Rakesh Pillai <pillair@xxxxxxxxxxxxxx> wrote:
>
> The wmi service available event has been
> extended to contain extra 128 bit for new services
> to be indicated by firmware.
>
> Currently the presence of any optional TLVs in
> the wmi service available event leads to a parsing
> error with the below error message:
> ath10k_snoc 18800000.wifi: failed to parse svc_avail tlv: -71
>
> The wmi service available event parsing should
> not return error for the newly added optional TLV.
> Fix this parsing for service available event message.
>
> Tested-on: WCN3990 hw1.0 SNOC
>
> Signed-off-by: Rakesh Pillai <pillair@xxxxxxxxxxxxxx>
> ---
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> index 932266d..3b49e29 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> @@ -1404,9 +1404,12 @@ static int ath10k_wmi_tlv_svc_avail_parse(struct ath10k *ar, u16 tag, u16 len,
>                 arg->service_map_ext_len = *(__le32 *)ptr;
>                 arg->service_map_ext = ptr + sizeof(__le32);
>                 return 0;
> +       case WMI_TLV_TAG_FIRST_ARRAY_ENUM:
> +               return 0;

This is at least slightly worrying to me.  If I were calling this
function, I'd expect that if I didn't get back an error that at least
"arg->service_map_ext_len" was filled in.  Seems like you should do:

case WMI_TLV_TAG_FIRST_ARRAY_ENUM:
  arg->service_map_ext_len = 0;
  arg->service_map_ext = NULL;
  return 0;

...and maybe add a comment about why you're doing that?

At the moment things are working OK because
ath10k_wmi_event_service_available() happens to init the structure to
0 before calling with:

  struct wmi_svc_avail_ev_arg arg = {};

....but it doesn't seem like a great idea to rely on that.

That all being said, I'm just a drive-by reviewer and if everyone else
likes it the way it is, feel free to ignore my comments.

-Doug