[PATCH] net: wireless: search and hold bss in cfg80211_connect_done

[Abhishek Kumar]

If BSS instance is not provided in __cfg80211_connect_result then
a get bss is performed. This can return NULL if the BSS for the
given SSID is expired due to delayed scheduling of connect result event
in rdev->event_work. This can cause WARN_ON(!cr->bss) in
__cfg80211_connect_result to be triggered and cause cascading
failures. To mitigate this, initiate a get bss call in
cfg80211_connect_done itself and hold it to ensure that the BSS
instance does not get expired.

Signed-off-by: Abhishek Kumar 
---

 net/wireless/sme.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 07756ca5e3b5..52f65991accd 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -724,15 +724,8 @@ void __cfg80211_connect_result(struct net_device *dev,
}
 #endif
 
-   if (!cr->bss && (cr->status == WLAN_STATUS_SUCCESS)) {
+   if (cr->status == WLAN_STATUS_SUCCESS)
WARN_ON_ONCE(!wiphy_to_rdev(wdev->wiphy)->ops->connect);
-   cr->bss = cfg80211_get_bss(wdev->wiphy, NULL, cr->bssid,
-  wdev->ssid, wdev->ssid_len,
-  wdev->conn_bss_type,
-  IEEE80211_PRIVACY_ANY);
-   if (cr->bss)
-   cfg80211_hold_bss(bss_from_pub(cr->bss));
-   }
 
if (wdev->current_bss) {
cfg80211_unhold_bss(wdev->current_bss);
@@ -882,6 +875,18 @@ void cfg80211_connect_done(struct net_device *dev,
ev->cr.fils.update_erp_next_seq_num = 
params->fils.update_erp_next_seq_num;
if (params->fils.update_erp_next_seq_num)
ev->cr.fils.erp_next_seq_num = params->fils.erp_next_seq_num;
+
+   /* Acquire and hold the bss if bss is not provided in argument.
+* This ensures that the BSS does not get expired if the schedule
+* of the rdev->event_work gets delayed.
+*/
+   if (!params->bss && params->bssid)
+   params->bss = cfg80211_get_bss(wdev->wiphy, NULL,
+  params->bssid,
+  wdev->ssid, wdev->ssid_len,
+  wdev->conn_bss_type,
+  IEEE80211_PRIVACY_ANY);
+
if (params->bss)
cfg80211_hold_bss(bss_from_pub(params->bss));
ev->cr.bss = params->bss;
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH v3] ath10k: skip the wait for completion to recovery in shutdown path

[Abhishek Kumar]

This patch seems to address the comments on v2. Overall this patch LGTM.

Reviewed-by: Abhishek Kumar 

On Tue, Feb 23, 2021 at 6:29 AM Youghandhar Chintala
 wrote:
>
> Currently in the shutdown callback we wait for recovery to complete
> before freeing up the resources. This results in additional two seconds
> delay during the shutdown and thereby increase the shutdown time.
>
> As an attempt to take less time during shutdown, remove the wait for
> recovery completion in the shutdown callback and added an API to freeing
> the reosurces in which they were common for shutdown and removing
> the module.
>
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
>
> Signed-off-by: Youghandhar Chintala 
> Change-Id: I65bc27b5adae1fedc7f7b367ef13aafbd01f8c0c
> ---
> Changes from v2:
> -Corrected commit text and added common API for freeing the
>  resources for shutdown and unloading the module
> ---
>  drivers/net/wireless/ath/ath10k/snoc.c | 29 ++
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c 
> b/drivers/net/wireless/ath/ath10k/snoc.c
> index 84666f72bdfa..70b3f2bd1c81 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1781,17 +1781,11 @@ static int ath10k_snoc_probe(struct platform_device 
> *pdev)
> return ret;
>  }
>
> -static int ath10k_snoc_remove(struct platform_device *pdev)
> +static int ath10k_snoc_free_resources(struct ath10k *ar)
>  {
> -   struct ath10k *ar = platform_get_drvdata(pdev);
> struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
>
> -   ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc remove\n");
> -
> -   reinit_completion(&ar->driver_recovery);
> -
> -   if (test_bit(ATH10K_SNOC_FLAG_RECOVERY, &ar_snoc->flags))
> -   wait_for_completion_timeout(&ar->driver_recovery, 3 * HZ);
> +   ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc free resources\n");
>
> set_bit(ATH10K_SNOC_FLAG_UNREGISTERING, &ar_snoc->flags);
>
> @@ -1805,12 +1799,29 @@ static int ath10k_snoc_remove(struct platform_device 
> *pdev)
> return 0;
>  }
>
> +static int ath10k_snoc_remove(struct platform_device *pdev)
> +{
> +   struct ath10k *ar = platform_get_drvdata(pdev);
> +   struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
> +
> +   ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc remove\n");
> +
> +   reinit_completion(&ar->driver_recovery);
> +
> +   if (test_bit(ATH10K_SNOC_FLAG_RECOVERY, &ar_snoc->flags))
> +   wait_for_completion_timeout(&ar->driver_recovery, 3 * HZ);
> +
> +   ath10k_snoc_free_resources(ar);
> +
> +   return 0;
> +}
> +
>  static void ath10k_snoc_shutdown(struct platform_device *pdev)
>  {
> struct ath10k *ar = platform_get_drvdata(pdev);
>
> ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc shutdown\n");
> -   ath10k_snoc_remove(pdev);
> +   ath10k_snoc_free_resources(ar);
>  }
>
>  static struct platform_driver ath10k_snoc_driver = {
> --
> 2.29.0
>

Re: [PATCH 1/3] cfg80211: Add wiphy flag to trigger STA disconnect after hardware restart

[Abhishek Kumar]

LGTM! If no one has any objections, I would be happy to see this
considered for upstream.

Reviewed-by: Abhishek Kumar 

Thanks
Abhishek

On Tue, Dec 15, 2020 at 9:30 AM Youghandhar Chintala
 wrote:
>
> Many wifi drivers (e.g. ath10k using qualcomm wifi chipsets)
> support silent target hardware restart/recovery. Out of these
> drivers which support target hw restart, certain chipsets
> have the wifi mac sequence number addition for transmitted
> frames done by the firmware. For such chipsets, a silent
> target hardware restart breaks the continuity of the wifi
> mac sequence number, since the wifi mac sequence number
> restarts from 0 after the restart, which in-turn leads
> to the peer access point dropping all the frames from device
> until it receives the frame with the mac sequence which was
> expected by the AP.
>
> Add a wiphy flag for the driver to indicate that it needs a
> trigger for STA disconnect after hardware restart.
>
> Tested on ath10k using WCN3990, QCA6174.
>
> Signed-off-by: Youghandhar Chintala 
> ---
>  include/net/cfg80211.h | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index ab249ca..7fba6f6 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4311,6 +4311,9 @@ struct cfg80211_ops {
>   * @WIPHY_FLAG_HAS_STATIC_WEP: The device supports static WEP key 
> installation
>   * before connection.
>   * @WIPHY_FLAG_SUPPORTS_EXT_KEK_KCK: The device supports bigger kek and kck 
> keys
> + * @WIPHY_FLAG_STA_DISCONNECT_ON_HW_RESTART: The device needs a trigger to
> + * disconnect STA after target hardware restart. This flag should be
> + * exposed by drivers which support target recovery.
>   */
>  enum wiphy_flags {
> WIPHY_FLAG_SUPPORTS_EXT_KEK_KCK = BIT(0),
> @@ -4337,6 +4340,7 @@ enum wiphy_flags {
> WIPHY_FLAG_SUPPORTS_5_10_MHZ= BIT(22),
> WIPHY_FLAG_HAS_CHANNEL_SWITCH   = BIT(23),
> WIPHY_FLAG_HAS_STATIC_WEP   = BIT(24),
> +   WIPHY_FLAG_STA_DISCONNECT_ON_HW_RESTART = BIT(25),
>  };
>
>  /**
> --
> 2.7.4
>

Re: [PATCH 3/3] ath10k: Set wiphy flag to trigger sta disconnect on hardware restart

[Abhishek Kumar]

> I'm not sure what you mean. But if you are saying that we should move
> ath10k_hw_params_list entirely to firmware then that is a huge task as
> we would need to make changes in every firmware branch, and there are so
> many different branches that I have lost count. And due to backwards
> compatibility we still need to have ath10k_hw_params_list in ath10k for
> few years.
>
 Apologies for late reply on this thread. Yes you got me right( to
move ath10k_hw_params_list entirely to firmware ). I wanted to trigger
an idea and know what other people's views are, or atleast what are
the challenges. As you said the task is much bigger with so many
firmware branches. As long as you feel it is scalable for coming
years, we should be good.

As far as the patch is concerned, it LGTM.

Reviewed-by: Abhishek Kumar 

Thanks
Abhishek

Re: [PATCH 2/3] mac80211: Add support to trigger sta disconnect on hardware restart

[Abhishek Kumar]

Since using DELBA frame to APs to re-establish BA session has a
dependency on APs and also some APs may not honour the DELBA frame. I
am fine with having the disconnect/reconnect solution. The change
looks good to me.

Reviewed-by: Abhishek Kumar 

Thanks
Abhishek

On Thu, Jan 28, 2021 at 12:08 AM  wrote:
>
> On 2020-12-15 23:10, Felix Fietkau wrote:
> > On 2020-12-15 18:23, Youghandhar Chintala wrote:
> >> Currently in case of target hardware restart, we just reconfig and
> >> re-enable the security keys and enable the network queues to start
> >> data traffic back from where it was interrupted.
> >>
> >> Many ath10k wifi chipsets have sequence numbers for the data
> >> packets assigned by firmware and the mac sequence number will
> >> restart from zero after target hardware restart leading to mismatch
> >> in the sequence number expected by the remote peer vs the sequence
> >> number of the frame sent by the target firmware.
> >>
> >> This mismatch in sequence number will cause out-of-order packets
> >> on the remote peer and all the frames sent by the device are dropped
> >> until we reach the sequence number which was sent before we restarted
> >> the target hardware
> >>
> >> In order to fix this, we trigger a sta disconnect, for the targets
> >> which expose this corresponding wiphy flag, in case of target hw
> >> restart. After this there will be a fresh connection and thereby
> >> avoiding the dropping of frames by remote peer.
> >>
> >> The right fix would be to pull the entire data path into the host
> >> which is not feasible or would need lots of complex changes and
> >> will still be inefficient.
> > How about simply tracking which tids have aggregation enabled and send
> > DELBA frames for those after the restart?
> > It would mean less disruption for affected stations and less ugly hacks
> > in the stack for unreliable hardware.
> >
> > - Felix
>
> Hi Felix,
>
> We did try to send an ADDBA frame to the AP once the SSR happened. The
> AP ack’ed the frame and the new BA session with renewed sequence number
> was established. But still, the AP did not respond to the ping requests
> with the new sequence number. It did not respond until one of the two
> happened.
> 1.  The sequence number was more than the sequence number that DUT had
> used before SSR happened
> 2.  DUT disconnected and then reconnected.
> The other option is to send a DELBA frame to the AP and make the AP also
> force to establish the BA session from its side. This we feel can have
> some interoperability issues as some of the AP’s may not honour the
> DELBA frame and will continue to use the earlier BA session that it had
> established. Given that re-negotiating the BA session is prone to IOT
> issues, we feel that it would be good to go with the
> Disconnect/Reconnect solution which is foolproof and will work in all
> scenarios.
>
> Regards,
> Youghandhar

Re: [PATCH 3/3] ath10k: Set wiphy flag to trigger sta disconnect on hardware restart

[Abhishek Kumar]

On Tue, Dec 15, 2020 at 9:24 AM Youghandhar Chintala
 wrote:
>
> From: Rakesh Pillai 
>
> Currently after the hardware restart triggered from the driver,
> the station interface connection remains intact, since a disconnect
> trigger is not sent to userspace. This can lead to a problem in
> hardwares where the wifi mac sequence is added by the firmware.
>
> After the firmware restart, during subsytem recovery, the firmware
> restarts its wifi mac sequence number. Hence AP to which our device
> is connected will receive frames with a  wifi mac sequence number jump
> to the past, thereby resulting in the AP dropping all these frames,
> until the frame arrives with a wifi mac sequence number which AP was
> expecting.
>
> To avoid such frame drops, its better to trigger a station disconnect
> upon the  hardware restart. Indicate this support via a WIPHY flag
> to mac80211, if the hardware params flag mentions the support to
> add wifi mac sequence numbers for TX frames in the firmware.
>
> All the other hardwares, except WCN3990, are not affected by this
> change, since the hardware params flag is not set for any hardware
> except for WCN3990
>
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00110-QCARMSWP-1
> Tested-on: QCA6174 hw3.2 SDIO WLAN.RMH.4.4.1-00048
>
> Signed-off-by: Rakesh Pillai 
> Signed-off-by: Youghandhar Chintala 
> ---
>  drivers/net/wireless/ath/ath10k/core.c | 15 +++
>  drivers/net/wireless/ath/ath10k/hw.h   |  3 +++
>  drivers/net/wireless/ath/ath10k/mac.c  |  3 +++
>  3 files changed, 21 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c 
> b/drivers/net/wireless/ath/ath10k/core.c
> index 796107b..4155f94 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -90,6 +90,7 @@ static const struct ath10k_hw_params 
> ath10k_hw_params_list[] = {
> .hw_filter_reset_required = true,
> .fw_diag_ce_download = false,
> .tx_stats_over_pktlog = true,
> +   .tx_mac_seq_by_fw = false,
Probably orthogonal to this patch, there is a static array maintained
for different hardware configs and the structure members like
"tx_mac_seq_by_fw" are initialized. This does not seem to be scalable
and probably these parameters can be auto populated based on FW
capabilities and so we don't have to maintain the static array.
Thoughts?

-Abhishek

[PATCH v3] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.

The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'

Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Signed-off-by: Abhishek Kumar 
---

Changes in v3:
- Resurreted Patch V1 because as per discussion we do need two
fallback board names and refactored ath10k_core_create_board_name.

 drivers/net/wireless/ath/ath10k/core.c | 41 +++---
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index d73ad60b571c..09d0fdfa6693 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1349,7 +1349,8 @@ static int ath10k_core_search_bd(struct ath10k *ar,
 
 static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
  const char *boardname,
- const char *fallback_boardname,
+ const char *fallback_boardname1,
+ const char *fallback_boardname2,
  const char *filename)
 {
size_t len, magic_len;
@@ -1398,8 +1399,11 @@ static int ath10k_core_fetch_board_data_api_n(struct 
ath10k *ar,
ret = ath10k_core_search_bd(ar, boardname, data, len);
 
/* if we didn't find it and have a fallback name, try that */
-   if (ret == -ENOENT && fallback_boardname)
-   ret = ath10k_core_search_bd(ar, fallback_boardname, data, len);
+   if (ret == -ENOENT && fallback_boardname1)
+   ret = ath10k_core_search_bd(ar, fallback_boardname1, data, len);
+
+   if (ret == -ENOENT && fallback_boardname2)
+   ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len);
 
if (ret == -ENOENT) {
ath10k_err(ar,
@@ -1419,7 +1423,8 @@ static int ath10k_core_fetch_board_data_api_n(struct 
ath10k *ar,
 }
 
 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-size_t name_len, bool with_variant)
+size_t name_len, bool with_variant,
+bool with_chip_id)
 {
/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
@@ -1438,7 +1443,7 @@ static int ath10k_core_create_board_name(struct ath10k 
*ar, char *name,
}
 
if (ar->id.qmi_ids_valid) {
-   if (with_variant && ar->id.bdf_ext[0] != '\0')
+   if (with_chip_id)
scnprintf(name, name_len,
  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
  ath10k_bus_str(ar->hif.bus),
@@ -1482,21 +1487,34 @@ static int ath10k_core_create_eboard_name(struct ath10k 
*ar, char *name,
 
 int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
 {
-   char boardname[100], fallback_boardname[100];
+   char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
int ret;
 
if (bd_ie_type == ATH10K_BD_IE_BOARD) {
+   /* With variant and chip id */
ret = ath10k_core_create_board_name(ar, boardname,
-   sizeof(boardname), true);
+   sizeof(boardname), true,
+   true);
if (ret) {
ath10k_err(ar, "failed to create board name: %d", ret);
return ret;
}
 
-   ret = ath10k_core_create_board_name(ar, fallback_boardname,
-   sizeof(boardname), false);
+   /* Without variant and only chip-id */
+   ret = ath10k_core_create_board_name(ar, fallback_boardname1,
+   sizeof(boardname), false,
+   true);
+   if (ret) {
+   ath10k_err(ar, "failed to create 1st fallback board 
name: %d", ret);
+   return ret;
+   }
+
+   /* Without variant and without chip-id */
+   ret = ath10k_core_create_board_name(ar, fallback_boardname2,
+   sizeof(boardname), false,
+   false);
 

Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

Hi,

> > (no changes since v1)
>
> I think you need to work on the method you're using to generate your
> patches.  There are most definitely changes since v1.  You described
> them in your cover letter (which you don't really need for a singleton
> patch) instead of here.
I agree, this was not intentional, I will fix this in the upcoming patches.

On Thu, Dec 3, 2020 at 7:34 AM Doug Anderson  wrote:
>
> Hi,
>
> On Thu, Dec 3, 2020 at 3:33 AM Rakesh Pillai  wrote:
> >
> > > What I'm trying to say is this.  Imagine that:
> > >
> > > a) the device tree has the "variant" property.
> > >
> > > b) the BRD file has two entries, one for "board-id" (1) and one for
> > > "board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
> > > + variant" (3).
> > >
> > > With your suggestion we'll see the "variant" property in the device
> > > tree.  That means we'll search for (1) and (3).  (3) isn't there, so
> > > we'll pick (1).  ...but we really should have picked (2), right?
> >
> > Do we expect board-2.bin to not be populated with the bdf with variant 
> > field (if its necessary ?)
>
> The whole fact that there is a fallback to begin with implies that
> there can be a mismatch between the board-2.bin and the device tree
> file.  Once we accept that there can be a mismatch, it seems good to
> try all 3 fallbacks in order.
>
> > Seems fine for me, if we have 2 fallback names if that is needed.
> OK, sounds good.  So hopefully Abhishek can post a v3 based on what's
> in  and you can confirm you're good with
> it there?
I agree, with this patch there can be mismatch between what's provided
in the Board file and what required board name we are generating, so
three calls are needed. So in a sense, we want to keep the V1 patch
with fix to reuse the same BDF.

I am making V3 changes and will address and push that out.

Thanks
Abhishek

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

[Abhishek Kumar]

Hi,

The patch LGTM there is small nit, though.
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -5751,8 +5751,13 @@ 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));
> +   /*
> +* Initialization of "arg.service_map_ext_valid" to ZERO is necessary
> +* for the below logic to work.
> +*/

Nit: The comment will throw a checkpatch warning.  "networking block
comments don't use an empty /* line, use /* Comment..."

-Abhishek

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

[Abhishek Kumar]

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  wrote:
>
> Doug Anderson  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-pill...@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

[PATCH v2 0/1] This patch address comments on patch v1

[Abhishek Kumar]

This patch extends ath10k_core_create_board_name function to support chip id
based BDF selection and not add provision for fallback_boardname2, thus
introducing lesser lines of code.

(no changes since v1)

Abhishek Kumar (1):
  ath10k: add option for chip-id based BDF selection

 drivers/net/wireless/ath/ath10k/core.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
2.29.2.222.g5d2a92d10f8-goog

[PATCH v2 1/1] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.

The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'

Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Signed-off-by: Abhishek Kumar 
---

(no changes since v1)

 drivers/net/wireless/ath/ath10k/core.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index d73ad60b571c..fa9e676b26d9 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1419,12 +1419,13 @@ static int ath10k_core_fetch_board_data_api_n(struct 
ath10k *ar,
 }
 
 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-size_t name_len, bool with_variant)
+size_t name_len,
+bool with_additional_params)
 {
/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
 
-   if (with_variant && ar->id.bdf_ext[0] != '\0')
+   if (with_additional_params && ar->id.bdf_ext[0] != '\0')
scnprintf(variant, sizeof(variant), ",variant=%s",
  ar->id.bdf_ext);
 
@@ -1438,12 +1439,17 @@ static int ath10k_core_create_board_name(struct ath10k 
*ar, char *name,
}
 
if (ar->id.qmi_ids_valid) {
-   if (with_variant && ar->id.bdf_ext[0] != '\0')
+   if (with_additional_params && ar->id.bdf_ext[0] != '\0')
scnprintf(name, name_len,
  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
  ath10k_bus_str(ar->hif.bus),
  ar->id.qmi_board_id, ar->id.qmi_chip_id,
  variant);
+   else if (with_additional_params)
+   scnprintf(name, name_len,
+ "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
+ ath10k_bus_str(ar->hif.bus),
+ ar->id.qmi_board_id, ar->id.qmi_chip_id);
else
scnprintf(name, name_len,
  "bus=%s,qmi-board-id=%x",
-- 
2.29.2.222.g5d2a92d10f8-goog

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

Apologies for the delay, was busy so could not work on V2 . I have
started working on V2 patch. Will upload by today/tomorrow.

Abhishek


On Thu, Nov 5, 2020 at 11:11 PM Kalle Valo  wrote:
>
> Abhishek Kumar  wrote:
>
> > In some devices difference in chip-id should be enough to pick
> > the right BDF. Add another support for chip-id based BDF selection.
> > With this new option, ath10k supports 2 fallback options.
> >
> > The board name with chip-id as option looks as follows
> > board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
> >
> > Signed-off-by: Abhishek Kumar 
> > Reviewed-by: Douglas Anderson 
> > Tested-by: Douglas Anderson 
> > Tested-by: Abhishek Kumar 
>
> There were few checkpatch warnings which I fixed:
>
> $ ath10k-check
> drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open 
> parenthesis
> drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 
> columns
> drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 
> columns
>
> The first one was also what Doug commented. I also added Tested-on tags,
> thanks for those. The updated patch is in pending branch (soon).
>
> But is this patch ok to take now? I didn't quite get the conclusion of the
> discussion.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/patch/2020102506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

Additionally tested on ath10k based non-QMI platform

Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Tested-by: Abhishek Kumar 

-Abhishek

[PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.

The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'

Signed-off-by: Abhishek Kumar 
---

 drivers/net/wireless/ath/ath10k/core.c | 45 +++---
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index a3f15cc89a10..767bb9d6a197 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1343,7 +1343,8 @@ static int ath10k_core_search_bd(struct ath10k *ar,
 
 static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
  const char *boardname,
- const char *fallback_boardname,
+ const char *fallback_boardname1,
+ const char *fallback_boardname2,
  const char *filename)
 {
size_t len, magic_len;
@@ -1392,8 +1393,11 @@ static int ath10k_core_fetch_board_data_api_n(struct 
ath10k *ar,
ret = ath10k_core_search_bd(ar, boardname, data, len);
 
/* if we didn't find it and have a fallback name, try that */
-   if (ret == -ENOENT && fallback_boardname)
-   ret = ath10k_core_search_bd(ar, fallback_boardname, data, len);
+   if (ret == -ENOENT && fallback_boardname1)
+   ret = ath10k_core_search_bd(ar, fallback_boardname1, data, len);
+
+   if (ret == -ENOENT && fallback_boardname2)
+   ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len);
 
if (ret == -ENOENT) {
ath10k_err(ar,
@@ -1413,7 +1417,8 @@ static int ath10k_core_fetch_board_data_api_n(struct 
ath10k *ar,
 }
 
 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-size_t name_len, bool with_variant)
+size_t name_len, bool with_variant,
+bool with_chip_id)
 {
/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
@@ -1432,12 +1437,17 @@ static int ath10k_core_create_board_name(struct ath10k 
*ar, char *name,
}
 
if (ar->id.qmi_ids_valid) {
-   if (with_variant && ar->id.bdf_ext[0] != '\0')
+   if (with_variant && ar->id.bdf_ext[0] != '\0' && with_chip_id)
scnprintf(name, name_len,
  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
  ath10k_bus_str(ar->hif.bus),
  ar->id.qmi_board_id, ar->id.qmi_chip_id,
  variant);
+   else if (with_chip_id)
+   scnprintf(name, name_len,
+ "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
+ ath10k_bus_str(ar->hif.bus),
+ ar->id.qmi_board_id, ar->id.qmi_chip_id);
else
scnprintf(name, name_len,
  "bus=%s,qmi-board-id=%x",
@@ -1476,21 +1486,33 @@ static int ath10k_core_create_eboard_name(struct ath10k 
*ar, char *name,
 
 int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
 {
-   char boardname[100], fallback_boardname[100];
+   char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
int ret;
 
if (bd_ie_type == ATH10K_BD_IE_BOARD) {
+   /* With variant and chip id */
ret = ath10k_core_create_board_name(ar, boardname,
-   sizeof(boardname), true);
+   sizeof(boardname), true, true);
if (ret) {
ath10k_err(ar, "failed to create board name: %d", ret);
return ret;
}
 
-   ret = ath10k_core_create_board_name(ar, fallback_boardname,
-   sizeof(boardname), false);
+   /* Without variant and only chip-id */
+   ret = ath10k_core_create_board_name(ar, fallback_boardname1,
+   sizeof(boardname), false,
+   true);
+   if (ret) {
+   ath10k_err(ar, "failed to create 1st fallback board 
name: %d", r