diff mbox

ath10k: ensure pdev sta kickout threshold is set.

Message ID 1472843202-12428-1-git-send-email-greearb@candelatech.com (mailing list archive)
State Rejected
Delegated to: Kalle Valo
Headers show

Commit Message

Ben Greear Sept. 2, 2016, 7:06 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

The station kickout threshold is a pdev value, not per vdev,
so it should be set all the time, not just when vdev is an
AP.  This should fix setting the station kickout threshold
when using ibss interfaces.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 27 ++++++++++++++++++---------
 drivers/net/wireless/ath/ath10k/mac.h |  2 ++
 2 files changed, 20 insertions(+), 9 deletions(-)

Comments

Rajkumar Manoharan Sept. 6, 2016, 7:39 a.m. UTC | #1 
[...]

> +int ath10k_mac_set_pdev_kickout(struct ath10k *ar)
> +{
> +       u32 param = ar->wmi.pdev_param->sta_kickout_th;
> +       int rv;
> +
> +       rv = ath10k_wmi_pdev_set_param(ar, param,
> +                                      ar->sta_xretry_kickout_thresh);
> +       if (rv) {
> +               ath10k_warn(ar, "failed to set sta kickout threshold to %d: %d\n",
> +                           ar->sta_xretry_kickout_thresh, rv);
> +       }
> +       return rv;
> +}
>
Ben,

I plan to get rid of setting station kickout threshold from host. Each firmware revision (i.e qca988x, qca99x0, ipq4019)  follows different logic based on hw capability for station kickout and follows different default paramters. So configuring common threshold will affect firmware logic. Better to get rid of these configuration from host driver and let firmware to work with default parameters.

Also I could not find out sta_xretry_kickout_thresh definition in upstream driver. Have you posted any changes for the same?

-Rajkumar
Ben Greear Sept. 6, 2016, 1:12 p.m. UTC | #2 
On 09/06/2016 12:39 AM, Manoharan, Rajkumar wrote:
> [...]
>
>> +int ath10k_mac_set_pdev_kickout(struct ath10k *ar)
>> +{
>> +       u32 param = ar->wmi.pdev_param->sta_kickout_th;
>> +       int rv;
>> +
>> +       rv = ath10k_wmi_pdev_set_param(ar, param,
>> +                                      ar->sta_xretry_kickout_thresh);
>> +       if (rv) {
>> +               ath10k_warn(ar, "failed to set sta kickout threshold to %d: %d\n",
>> +                           ar->sta_xretry_kickout_thresh, rv);
>> +       }
>> +       return rv;
>> +}
>>
> Ben,
>
> I plan to get rid of setting station kickout threshold from host. Each firmware revision (i.e qca988x, qca99x0, ipq4019)  follows different logic based on hw capability for station kickout and follows different default paramters. So configuring common threshold will affect firmware logic. Better to get rid of these configuration from host driver and let firmware to work with default parameters.
>
> Also I could not find out sta_xretry_kickout_thresh definition in upstream driver. Have you posted any changes for the same?

Hmm, maybe that last bit is something I added in another patch.  Probably my
patch to enable firmware config on a per radio basis (fwcfg in my tree).  Those patches are
larger and probably will never make it upstream.

I need a way to configure this kickout, since firmware is kicking out stations when it should
not.

Instead of removing the capability, you should instead make it configurable
through debugfs or something like my fwcfg patches, and/or disable the kickout entirely.
Since mac80211 can deal with kicking out stations already, the stuff in the firmware just makes
things less stable in poor RF environments and/or with stations with flaky power-save and
off-channel roaming.

Thanks,
Ben
Rajkumar Manoharan Sept. 6, 2016, 5:16 p.m. UTC | #3 
On 2016-09-06 18:42, Ben Greear wrote:
> On 09/06/2016 12:39 AM, Manoharan, Rajkumar wrote:
>> [...]
>> 
>>> +int ath10k_mac_set_pdev_kickout(struct ath10k *ar)
>>> +{
>>> +       u32 param = ar->wmi.pdev_param->sta_kickout_th;
>>> +       int rv;
>>> +
>>> +       rv = ath10k_wmi_pdev_set_param(ar, param,
>>> +                                      
>>> ar->sta_xretry_kickout_thresh);
>>> +       if (rv) {
>>> +               ath10k_warn(ar, "failed to set sta kickout threshold 
>>> to %d: %d\n",
>>> +                           ar->sta_xretry_kickout_thresh, rv);
>>> +       }
>>> +       return rv;
>>> +}
>>> 
>> Ben,
>> 
>> I plan to get rid of setting station kickout threshold from host. Each 
>> firmware revision (i.e qca988x, qca99x0, ipq4019)  follows different 
>> logic based on hw capability for station kickout and follows different 
>> default paramters. So configuring common threshold will affect 
>> firmware logic. Better to get rid of these configuration from host 
>> driver and let firmware to work with default parameters.
>> 
>> Also I could not find out sta_xretry_kickout_thresh definition in 
>> upstream driver. Have you posted any changes for the same?
> 
> Hmm, maybe that last bit is something I added in another patch.  
> Probably my
> patch to enable firmware config on a per radio basis (fwcfg in my
> tree).  Those patches are
> larger and probably will never make it upstream.
> 
> I need a way to configure this kickout, since firmware is kicking out
> stations when it should
> not.
> 

Could you please explain the scenario?

> Instead of removing the capability, you should instead make it 
> configurable
> through debugfs or something like my fwcfg patches, and/or disable the
> kickout entirely.
> Since mac80211 can deal with kicking out stations already, the stuff
> in the firmware just makes
> things less stable in poor RF environments and/or with stations with
> flaky power-save and
> off-channel roaming.
> 
I plan to get rid of hardcoded value and fix this through proper netlink 
interface instead of debugfs. While ago, I posted a change for 
configuring low ack threshold. It is still pending in my TODO list.

http://comments.gmane.org/gmane.linux.kernel.wireless.general/137645

-Rajkumar
Ben Greear Sept. 6, 2016, 5:26 p.m. UTC | #4 
On 09/06/2016 10:16 AM, Rajkumar Manoharan wrote:
> On 2016-09-06 18:42, Ben Greear wrote:
>> On 09/06/2016 12:39 AM, Manoharan, Rajkumar wrote:
>>> [...]
>>>
>>>> +int ath10k_mac_set_pdev_kickout(struct ath10k *ar)
>>>> +{
>>>> +       u32 param = ar->wmi.pdev_param->sta_kickout_th;
>>>> +       int rv;
>>>> +
>>>> +       rv = ath10k_wmi_pdev_set_param(ar, param,
>>>> +                                      ar->sta_xretry_kickout_thresh);
>>>> +       if (rv) {
>>>> +               ath10k_warn(ar, "failed to set sta kickout threshold to %d: %d\n",
>>>> +                           ar->sta_xretry_kickout_thresh, rv);
>>>> +       }
>>>> +       return rv;
>>>> +}
>>>>
>>> Ben,
>>>
>>> I plan to get rid of setting station kickout threshold from host. Each firmware revision (i.e qca988x, qca99x0, ipq4019)  follows different logic based on hw
>>> capability for station kickout and follows different default paramters. So configuring common threshold will affect firmware logic. Better to get rid of
>>> these configuration from host driver and let firmware to work with default parameters.
>>>
>>> Also I could not find out sta_xretry_kickout_thresh definition in upstream driver. Have you posted any changes for the same?
>>
>> Hmm, maybe that last bit is something I added in another patch.  Probably my
>> patch to enable firmware config on a per radio basis (fwcfg in my
>> tree).  Those patches are
>> larger and probably will never make it upstream.
>>
>> I need a way to configure this kickout, since firmware is kicking out
>> stations when it should
>> not.
>>
>
> Could you please explain the scenario?

A customer reported issues when having a bunch of android tablets connected to
a QCA9880 NIC running my 10.1 CT firmware in AP mode.

It is not clear exactly why the peers suddenly cannot ack packets,
but disabling the logic that kicks out stations due to low acks appears
to resolve the issue.

To turn the question around:  Is there any benefit to having the
firmware handle this keep-alive stuff?

I can see maybe it is useful in station only mode for power-saving reasons,
but not in AP mode.

>> Instead of removing the capability, you should instead make it configurable
>> through debugfs or something like my fwcfg patches, and/or disable the
>> kickout entirely.
>> Since mac80211 can deal with kicking out stations already, the stuff
>> in the firmware just makes
>> things less stable in poor RF environments and/or with stations with
>> flaky power-save and
>> off-channel roaming.
>>
> I plan to get rid of hardcoded value and fix this through proper netlink interface instead of debugfs. While ago, I posted a change for configuring low ack
> threshold. It is still pending in my TODO list.
>
> http://comments.gmane.org/gmane.linux.kernel.wireless.general/137645

That sounds nice.  I would hope that setting the value to 0 would
disable as much firmware keep-alive logic as possible.

Thanks,
Ben
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 39fcdad..4aa0736 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -744,21 +744,26 @@  static int ath10k_peer_create(struct ath10k *ar,
 	return 0;
 }
 
+int ath10k_mac_set_pdev_kickout(struct ath10k *ar)
+{
+	u32 param = ar->wmi.pdev_param->sta_kickout_th;
+	int rv;
+
+	rv = ath10k_wmi_pdev_set_param(ar, param,
+				       ar->sta_xretry_kickout_thresh);
+	if (rv) {
+		ath10k_warn(ar, "failed to set sta kickout threshold to %d: %d\n",
+			    ar->sta_xretry_kickout_thresh, rv);
+	}
+	return rv;
+}
+
 static int ath10k_mac_set_kickout(struct ath10k_vif *arvif)
 {
 	struct ath10k *ar = arvif->ar;
 	u32 param;
 	int ret;
 
-	param = ar->wmi.pdev_param->sta_kickout_th;
-	ret = ath10k_wmi_pdev_set_param(ar, param,
-					ar->sta_xretry_kickout_thresh);
-	if (ret) {
-		ath10k_warn(ar, "failed to set kickout threshold on vdev %i: %d\n",
-			    arvif->vdev_id, ret);
-		return ret;
-	}
-
 	param = ar->wmi.vdev_param->ap_keepalive_min_idle_inactive_time_secs;
 	ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, param,
 					ATH10K_KEEPALIVE_MIN_IDLE);
@@ -5427,6 +5432,10 @@  static int ath10k_add_interface(struct ieee80211_hw *hw,
 		arvif->peer_id = HTT_INVALID_PEERID;
 	}
 
+	ret = ath10k_mac_set_pdev_kickout(ar);
+	if (ret)
+		return ret;
+
 	if (arvif->vdev_type == WMI_VDEV_TYPE_AP) {
 		ret = ath10k_mac_set_kickout(arvif);
 		if (ret) {
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 5f027ec..1a34cab 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -113,4 +113,6 @@  static inline void ath10k_tx_h_seq_no(struct ieee80211_vif *vif,
 	}
 }
 
+int ath10k_mac_set_pdev_kickout(struct ath10k *ar);
+
 #endif /* _MAC_H_ */