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
