* [PATCH v2] ath10k: Fix the parsing error in service available event
@ 2020-10-28 17:01 Rakesh Pillai
2020-10-28 18:44 ` Doug Anderson
2020-11-06 7:21 ` Kalle Valo
0 siblings, 2 replies; 6+ messages in thread
From: Rakesh Pillai @ 2020-10-28 17:01 UTC (permalink / raw)
To: ath10k
Cc: kuabhs, briannorris, linux-wireless, linux-kernel, dianders,
Rakesh Pillai
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
Fixes: cea19a6ce8bf ("ath10k: add WMI_SERVICE_AVAILABLE_EVENT support")
Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
---
Changes from v1:
- Access service_map_ext only if this TLV was sent in service
available event.
---
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 4 +++-
drivers/net/wireless/ath/ath10k/wmi.c | 5 +++--
drivers/net/wireless/ath/ath10k/wmi.h | 1 +
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index 932266d..7b58341 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -1401,13 +1401,15 @@ static int ath10k_wmi_tlv_svc_avail_parse(struct ath10k *ar, u16 tag, u16 len,
switch (tag) {
case WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT:
+ arg->service_map_ext_valid = true;
arg->service_map_ext_len = *(__le32 *)ptr;
arg->service_map_ext = ptr + sizeof(__le32);
return 0;
default:
break;
}
- return -EPROTO;
+
+ return 0;
}
static int ath10k_wmi_tlv_op_pull_svc_avail(struct ath10k *ar,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 1fa7107..2e4b561 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -5751,8 +5751,9 @@ void ath10k_wmi_event_service_available(struct ath10k *ar, struct sk_buff *skb)
ret);
}
- ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
- __le32_to_cpu(arg.service_map_ext_len));
+ if (arg.service_map_ext_valid)
+ ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
+ __le32_to_cpu(arg.service_map_ext_len));
}
static int ath10k_wmi_event_temperature(struct ath10k *ar, struct sk_buff *skb)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 4898e19..66ecf09 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -6917,6 +6917,7 @@ struct wmi_svc_rdy_ev_arg {
};
struct wmi_svc_avail_ev_arg {
+ bool service_map_ext_valid;
__le32 service_map_ext_len;
const __le32 *service_map_ext;
};
--
2.7.4
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] ath10k: Fix the parsing error in service available event
2020-10-28 17:01 [PATCH v2] ath10k: Fix the parsing error in service available event Rakesh Pillai
@ 2020-10-28 18:44 ` Doug Anderson
2020-10-29 5:29 ` Rakesh Pillai
2020-11-06 7:25 ` Kalle Valo
2020-11-06 7:21 ` Kalle Valo
1 sibling, 2 replies; 6+ messages in thread
From: Doug Anderson @ 2020-10-28 18:44 UTC (permalink / raw)
To: Rakesh Pillai; +Cc: Abhishek Kumar, Brian Norris, linux-wireless, LKML, ath10k
Hi,
On Wed, Oct 28, 2020 at 10:01 AM Rakesh Pillai <pillair@codeaurora.org> 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
>
> Fixes: cea19a6ce8bf ("ath10k: add WMI_SERVICE_AVAILABLE_EVENT support")
> Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
> ---
> Changes from v1:
> - Access service_map_ext only if this TLV was sent in service
> available event.
> ---
> drivers/net/wireless/ath/ath10k/wmi-tlv.c | 4 +++-
> drivers/net/wireless/ath/ath10k/wmi.c | 5 +++--
> drivers/net/wireless/ath/ath10k/wmi.h | 1 +
> 3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> index 932266d..7b58341 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> @@ -1401,13 +1401,15 @@ static int ath10k_wmi_tlv_svc_avail_parse(struct ath10k *ar, u16 tag, u16 len,
>
> switch (tag) {
> case WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT:
> + arg->service_map_ext_valid = true;
> arg->service_map_ext_len = *(__le32 *)ptr;
> arg->service_map_ext = ptr + sizeof(__le32);
> return 0;
> default:
> break;
> }
> - return -EPROTO;
> +
> + return 0;
I notice your v2 now returns 0 for _all_ unknown tags. v1 just had a
special case for "WMI_TLV_TAG_FIRST_ARRAY_ENUM". I don't have enough
experience with this driver to know which is better, but this change
wasn't mentioned in the changes from v1. I guess you had a change of
heart and decided this way was better?
> }
>
> static int ath10k_wmi_tlv_op_pull_svc_avail(struct ath10k *ar,
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 1fa7107..2e4b561 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -5751,8 +5751,9 @@ void ath10k_wmi_event_service_available(struct ath10k *ar, struct sk_buff *skb)
> ret);
> }
>
> - ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
> - __le32_to_cpu(arg.service_map_ext_len));
> + if (arg.service_map_ext_valid)
> + ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
> + __le32_to_cpu(arg.service_map_ext_len));
Your new patch still requires the caller to init the
"service_map_ext_valid" to false before calling, but I guess there's
not a whole lot more we can do because we might be parsing more than
one tag. It does seem nice that at least we now have a validity bit
instead of just relying on a non-zero length to be valid.
It might be nice to have a comment saying that it's up to us to init
"arg.service_map_ext_valid" to false before calling
ath10k_wmi_pull_svc_avail(), but I won't insist. Maybe that's obvious
to everyone but me...
-Doug
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH v2] ath10k: Fix the parsing error in service available event
2020-10-28 18:44 ` Doug Anderson
@ 2020-10-29 5:29 ` Rakesh Pillai
2020-11-06 7:25 ` Kalle Valo
1 sibling, 0 replies; 6+ messages in thread
From: Rakesh Pillai @ 2020-10-29 5:29 UTC (permalink / raw)
To: 'Doug Anderson'
Cc: 'Abhishek Kumar', 'Brian Norris',
'linux-wireless', 'LKML', 'ath10k'
> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Thursday, October 29, 2020 12:15 AM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: ath10k <ath10k@lists.infradead.org>; linux-wireless <linux-
> wireless@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Abhishek
> Kumar <kuabhs@chromium.org>; Brian Norris <briannorris@chromium.org>
> Subject: Re: [PATCH v2] ath10k: Fix the parsing error in service available
> event
>
> Hi,
>
> On Wed, Oct 28, 2020 at 10:01 AM Rakesh Pillai <pillair@codeaurora.org>
> 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
> >
> > Fixes: cea19a6ce8bf ("ath10k: add WMI_SERVICE_AVAILABLE_EVENT
> support")
> > Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>
> > ---
> > Changes from v1:
> > - Access service_map_ext only if this TLV was sent in service
> > available event.
> > ---
> > drivers/net/wireless/ath/ath10k/wmi-tlv.c | 4 +++-
> > drivers/net/wireless/ath/ath10k/wmi.c | 5 +++--
> > drivers/net/wireless/ath/ath10k/wmi.h | 1 +
> > 3 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> > index 932266d..7b58341 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> > @@ -1401,13 +1401,15 @@ static int
> ath10k_wmi_tlv_svc_avail_parse(struct ath10k *ar, u16 tag, u16 len,
> >
> > switch (tag) {
> > case WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT:
> > + arg->service_map_ext_valid = true;
> > arg->service_map_ext_len = *(__le32 *)ptr;
> > arg->service_map_ext = ptr + sizeof(__le32);
> > return 0;
> > default:
> > break;
> > }
> > - return -EPROTO;
> > +
> > + return 0;
>
> I notice your v2 now returns 0 for _all_ unknown tags. v1 just had a
> special case for "WMI_TLV_TAG_FIRST_ARRAY_ENUM". I don't have
> enough
> experience with this driver to know which is better, but this change
> wasn't mentioned in the changes from v1. I guess you had a change of
> heart and decided this way was better?
Earlier patchset which added a case for " WMI_TLV_TAG_FIRST_ARRAY_ENUM", still returned error if there is any other TLV except for the two cases handled.
This leaves the possibility of an error when a new TLV gets added to this service_available message.
Since we are using the "valid" flag to indicate the validity of a particular tag, we need not return failure in any case. This makes it scalable (and maintains backward compatibility), in case extra TLVs are added to this message in future.
>
>
> > }
> >
> > static int ath10k_wmi_tlv_op_pull_svc_avail(struct ath10k *ar,
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
> b/drivers/net/wireless/ath/ath10k/wmi.c
> > index 1fa7107..2e4b561 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > @@ -5751,8 +5751,9 @@ void ath10k_wmi_event_service_available(struct
> ath10k *ar, struct sk_buff *skb)
> > ret);
> > }
> >
> > - ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar-
> >wmi.svc_map,
> > - __le32_to_cpu(arg.service_map_ext_len));
> > + if (arg.service_map_ext_valid)
> > + ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar-
> >wmi.svc_map,
> > + __le32_to_cpu(arg.service_map_ext_len));
>
> Your new patch still requires the caller to init the
> "service_map_ext_valid" to false before calling, but I guess there's
> not a whole lot more we can do because we might be parsing more than
> one tag. It does seem nice that at least we now have a validity bit
> instead of just relying on a non-zero length to be valid.
>
> It might be nice to have a comment saying that it's up to us to init
> "arg.service_map_ext_valid" to false before calling
> ath10k_wmi_pull_svc_avail(), but I won't insist. Maybe that's obvious
> to everyone but me...
I will wait for a couple of days, if there are any other comments, to post a v3 addressing all of them together.
This approach of having a argument initialized to parse TLVs is used for many other wmi events as well.
>
>
> -Doug
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] ath10k: Fix the parsing error in service available event
2020-10-28 18:44 ` Doug Anderson
2020-10-29 5:29 ` Rakesh Pillai
@ 2020-11-06 7:25 ` Kalle Valo
2020-11-13 22:45 ` Abhishek Kumar
1 sibling, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2020-11-06 7:25 UTC (permalink / raw)
To: Doug Anderson
Cc: Abhishek Kumar, Brian Norris, linux-wireless, LKML, ath10k,
Rakesh Pillai
Doug Anderson <dianders@chromium.org> writes:
>> static int ath10k_wmi_tlv_op_pull_svc_avail(struct ath10k *ar,
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>> index 1fa7107..2e4b561 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -5751,8 +5751,9 @@ void ath10k_wmi_event_service_available(struct ath10k *ar, struct sk_buff *skb)
>> ret);
>> }
>>
>> - ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
>> - __le32_to_cpu(arg.service_map_ext_len));
>> + if (arg.service_map_ext_valid)
>> + ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
>> + __le32_to_cpu(arg.service_map_ext_len));
>
> Your new patch still requires the caller to init the
> "service_map_ext_valid" to false before calling, but I guess there's
> not a whole lot more we can do because we might be parsing more than
> one tag. It does seem nice that at least we now have a validity bit
> instead of just relying on a non-zero length to be valid.
>
> It might be nice to have a comment saying that it's up to us to init
> "arg.service_map_ext_valid" to false before calling
> ath10k_wmi_pull_svc_avail(), but I won't insist. Maybe that's obvious
> to everyone but me...
It's not obvious to me either. Please add that comment.
BTW, for some reason Doug's response didn't get to patchwork:
https://patchwork.kernel.org/project/linux-wireless/patch/1603904469-598-1-git-send-email-pillair@codeaurora.org/
Though I do see it in linux-wireless, so most likely this was a
temporary glitch in patchwork. But it's just worrisome as nowadays I
only check the comments in patchwork before I apply the patch.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ath10k: Fix the parsing error in service available event
2020-11-06 7:25 ` Kalle Valo
@ 2020-11-13 22:45 ` Abhishek Kumar
0 siblings, 0 replies; 6+ messages in thread
From: Abhishek Kumar @ 2020-11-13 22:45 UTC (permalink / raw)
To: Kalle Valo
Cc: Brian Norris, linux-wireless, Doug Anderson, ath10k, LKML, Rakesh Pillai
Hi All,
The V2 patch now has good comments and probably spinning off a new V3
might be a good idea. Here are a few comments to the discussion.
In response to Doug's comment
> case WMI_TLV_TAG_FIRST_ARRAY_ENUM:
> arg->service_map_ext_len = 0;
> arg->service_map_ext = NULL;
> return 0;
Since the TLV messages are parsed iteratively for each tag, if
WMI_TLV_TAG_FIRST_ARRAY_ENUM this comes as the last TLV tag then this
might cause the map_len to be zero even if there is a valid tag like
WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT , so having a "valid" flag
seems to be a better and scalable approach.
> > The TLV TAG " WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT" is the first
> > and a mandatory TLV in the service available event. The subsequent
> > TLVs are optional ones and may or may not be present (based on FW
> > versions).
>
> From ath10k point of view never trust what the firmware sends you. Even
> if WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT is a mandatory TLV it
> might be missing for whatever reasons. The same is with buffer lengths
> etc and always confirm what you are receiving from the firmware.
>
Looks like the length for each tag is already being validated in
ath10k_wmi_tlv_iter() and would return error if the length does not
match against the wmi policy., so I think the tlv message validation
is already being done. Kalle, Is the expectation here is to do
anything additional ?
-Abhishek
On Thu, Nov 5, 2020 at 11:25 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Doug Anderson <dianders@chromium.org> writes:
>
> >> static int ath10k_wmi_tlv_op_pull_svc_avail(struct ath10k *ar,
> >> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> >> index 1fa7107..2e4b561 100644
> >> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> >> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> >> @@ -5751,8 +5751,9 @@ void ath10k_wmi_event_service_available(struct ath10k *ar, struct sk_buff *skb)
> >> ret);
> >> }
> >>
> >> - ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
> >> - __le32_to_cpu(arg.service_map_ext_len));
> >> + if (arg.service_map_ext_valid)
> >> + ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map,
> >> + __le32_to_cpu(arg.service_map_ext_len));
> >
> > Your new patch still requires the caller to init the
> > "service_map_ext_valid" to false before calling, but I guess there's
> > not a whole lot more we can do because we might be parsing more than
> > one tag. It does seem nice that at least we now have a validity bit
> > instead of just relying on a non-zero length to be valid.
> >
> > It might be nice to have a comment saying that it's up to us to init
> > "arg.service_map_ext_valid" to false before calling
> > ath10k_wmi_pull_svc_avail(), but I won't insist. Maybe that's obvious
> > to everyone but me...
>
> It's not obvious to me either. Please add that comment.
>
> BTW, for some reason Doug's response didn't get to patchwork:
>
> https://patchwork.kernel.org/project/linux-wireless/patch/1603904469-598-1-git-send-email-pillair@codeaurora.org/
>
> Though I do see it in linux-wireless, so most likely this was a
> temporary glitch in patchwork. But it's just worrisome as nowadays I
> only check the comments in patchwork before I apply the patch.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ath10k: Fix the parsing error in service available event
2020-10-28 17:01 [PATCH v2] ath10k: Fix the parsing error in service available event Rakesh Pillai
2020-10-28 18:44 ` Doug Anderson
@ 2020-11-06 7:21 ` Kalle Valo
1 sibling, 0 replies; 6+ messages in thread
From: Kalle Valo @ 2020-11-06 7:21 UTC (permalink / raw)
To: Rakesh Pillai
Cc: kuabhs, briannorris, linux-wireless, linux-kernel, ath10k, dianders
Rakesh Pillai <pillair@codeaurora.org> writes:
> 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
Firmware version missing.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-13 22:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-28 17:01 [PATCH v2] ath10k: Fix the parsing error in service available event Rakesh Pillai
2020-10-28 18:44 ` Doug Anderson
2020-10-29 5:29 ` Rakesh Pillai
2020-11-06 7:25 ` Kalle Valo
2020-11-13 22:45 ` Abhishek Kumar
2020-11-06 7:21 ` Kalle Valo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).