Hi,
On Wed, Oct 28, 2020 at 10:01 AM Rakesh Pillai <[email protected]> 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 <[email protected]>
> ---
> 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
> -----Original Message-----
> From: Doug Anderson <[email protected]>
> Sent: Thursday, October 29, 2020 12:15 AM
> To: Rakesh Pillai <[email protected]>
> Cc: ath10k <[email protected]>; linux-wireless <linux-
> [email protected]>; LKML <[email protected]>; Abhishek
> Kumar <[email protected]>; Brian Norris <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> > ---
> > 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
Doug Anderson <[email protected]> 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/[email protected]/
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
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 <[email protected]> wrote:
>
> Doug Anderson <[email protected]> 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/[email protected]/
>
> 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