Re: [PATCH] ath10k: Disable 4addr source port learning in 10.4 FW by default
thank you very much. waiting for this fix since a long time. i also reported this issue years ago with no response Sebastian Am 09.11.2018 um 07:11 schrieb Sathishkumar Muruganandam: One such visible functional impact is when GTK rekey frame from ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH] ath10k: Disable 4addr source port learning in 10.4 FW by default
Currently in 10.4 FW, all the received 4addr frames are processed for
source port learning which is enabled by default. This learning can't
be disabled by default in FW since it breaks backward compatibility.
Since ath10k uses mac80211 based 4addr mode, source port learning done
in 10.4 FW is redundant and also causes issues when 3addr frames are
transmitted/received for a 4addr station.
One such visible functional impact is when GTK rekey frame from
hostapd based AP to 4addr STA is dropped in AP's 10.4 FW. This is since
GTK rekey EAPOL frame is 3addr frame on AP interface and STA enabled
with 4addr is already allowed for receiving 3addr EAPOL frames.
Source port learning implementation in 10.4 FW drops this 3addr GTK rekey
frame in AP destinated for 4addr STA causing disassociation and
re-association for every GTK rekey session. GTK rekey issue is not seen
when learning is disabled in FW.
To prevent such issues without breaking backward compatibility, FW
advertises new service bit making the source port learning configurable and
this learning is being currently disabled during ath10k vdev creation.
* Tested HW: QCA9984
* Tested FW: 10.4-3.6.0.1-3
Signed-off-by: Sathishkumar Muruganandam
---
drivers/net/wireless/ath/ath10k/mac.c | 11 +++
drivers/net/wireless/ath/ath10k/wmi.c | 2 ++
drivers/net/wireless/ath/ath10k/wmi.h | 21 -
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c
b/drivers/net/wireless/ath/ath10k/mac.c
index c5130fa264eb..aac4d842d504 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -5154,6 +5154,17 @@ static int ath10k_add_interface(struct ieee80211_hw *hw,
goto err;
}
+ if (test_bit(WMI_SERVICE_VDEV_DISABLE_4_ADDR_SRC_LRN_SUPPORT,
+ar->wmi.svc_map)) {
+ vdev_param = ar->wmi.vdev_param->disable_4addr_src_lrn;
+ ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id, vdev_param,
+
WMI_VDEV_DISABLE_4_ADDR_SRC_LRN);
+ if (ret && ret != -EOPNOTSUPP) {
+ ath10k_warn(ar, "failed to disable 4addr src lrn vdev
%i: %d\n",
+ arvif->vdev_id, ret);
+ }
+ }
+
ar->free_vdev_map &= ~(1LL << arvif->vdev_id);
spin_lock_bh(>data_lock);
list_add(>list, >arvifs);
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
b/drivers/net/wireless/ath/ath10k/wmi.c
index 659513bf4ddc..8757ae297084 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -825,6 +825,7 @@ static struct wmi_vdev_param_map wmi_vdev_param_map = {
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .disable_4addr_src_lrn = WMI_VDEV_PARAM_UNSUPPORTED,
};
/* 10.X WMI VDEV param map */
@@ -900,6 +901,7 @@ static struct wmi_vdev_param_map wmi_10x_vdev_param_map = {
.meru_vc = WMI_VDEV_PARAM_UNSUPPORTED,
.rx_decap_type = WMI_VDEV_PARAM_UNSUPPORTED,
.bw_nss_ratemask = WMI_VDEV_PARAM_UNSUPPORTED,
+ .disable_4addr_src_lrn = WMI_VDEV_PARAM_UNSUPPORTED,
};
static struct wmi_vdev_param_map wmi_10_2_4_vdev_param_map = {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h
b/drivers/net/wireless/ath/ath10k/wmi.h
index 58e33ab9e0e9..3e17be938420 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -205,6 +205,7 @@ enum wmi_service {
WMI_SERVICE_SPOOF_MAC_SUPPORT,
WMI_SERVICE_TX_DATA_ACK_RSSI,
WMI_SERVICE_VDEV_DIFFERENT_BEACON_INTERVAL_SUPPORT,
+ WMI_SERVICE_VDEV_DISABLE_4_ADDR_SRC_LRN_SUPPORT,
/* keep last */
WMI_SERVICE_MAX,
@@ -359,6 +360,9 @@ enum wmi_10_4_service {
WMI_10_4_SERVICE_PEER_TID_CONFIGS_SUPPORT,
WMI_10_4_SERVICE_VDEV_BCN_RATE_CONTROL,
WMI_10_4_SERVICE_VDEV_DIFFERENT_BEACON_INTERVAL_SUPPORT,
+ WMI_10_4_SERVICE_HTT_ASSERT_TRIGGER_SUPPORT,
+ WMI_10_4_SERVICE_VDEV_FILTER_NEIGHBOR_RX_PACKETS,
+ WMI_10_4_SERVICE_VDEV_DISABLE_4_ADDR_SRC_LRN_SUPPORT,
};
static inline char *wmi_service_name(int service_id)
@@ -786,6 +790,8 @@ static inline void wmi_10_4_svc_map(const __le32 *in,
unsigned long *out,
WMI_SERVICE_TX_DATA_ACK_RSSI, len);
SVCMAP(WMI_10_4_SERVICE_VDEV_DIFFERENT_BEACON_INTERVAL_SUPPORT,
WMI_SERVICE_VDEV_DIFFERENT_BEACON_INTERVAL_SUPPORT, len);
+ SVCMAP(WMI_10_4_SERVICE_VDEV_DISABLE_4_ADDR_SRC_LRN_SUPPORT,
+ WMI_SERVICE_VDEV_DISABLE_4_ADDR_SRC_LRN_SUPPORT, len);
}
#undef SVCMAP
@@ -5065,6 +5071,7 @@ struct wmi_vdev_param_map {
u32 bw_nss_ratemask;
u32 inc_tsf;
u32 dec_tsf;
+ u32 disable_4addr_src_lrn;
};
#define WMI_VDEV_PARAM_UNSUPPORTED 0
@@
Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"
On 2018-11-08 09:30, Brian Norris wrote: On Wed, Nov 7, 2018 at 8:32 PM Govind Singh wrote: On 2018-11-08 03:00, Rajkumar Manoharan wrote: > > The change "ath10k: add quiet mode support for QCA6174/QCA9377" was > merged even > before full WCN3990 device support was added in ath10k. How come it > could be regression > for WCN3990. I know both are sharing same WMI-TLV interface but > reverting this > will break QCA6174/QCA9377. no? I don't see how the revert would "break" QCA6174 -- QCA6174 worked just fine without this feature and should continue to do so. I meant that the revert commit remove quiet mode support from QCA6174 & QCA9377. This regression is found while we switched from 4.18 + WCN3990 back-ports to 4.19. ^^ What Govind said. WCN3990 support has been trickling in over a few releases, and it doesn't seem kosher to allow people to submit regressions in the midst of that. Nobody prefers regression :). WCN3990 support was still in progress, at the time the commit got merged into upstream. My point is that we can't expect the community to validate the changes against in-progress platform. IMO, we should use (WMI_SERVICE_THERMAL_MGMT | WMI_SERVICE_THERM_THROT ) service bitmap check and call ath10k_thermal_set_throttling only if fw supports THERMAL THROTTLE feature. But we need to ensure all available ath10k fw's are reporting this service. And the above notes from Govind highlight this -- if the feature was not protected by the appropriate service flags, then we can't be sure that you didn't break a bunch of other firmware releases out there. Linux should not break for everyone just because you spun a firmware release. That is true. Any new features or interface changes in firmware will be advertised by feature bit. But the quiet param was available in firmware since first release. Of course, I'll leave it up to Kalle as to how he wants to mediate this. And if you come up with a solid patch soon that can fix this without dropping the feature, then so be it. Govind is working on to handle this properly either by instantiating new WMI-TLV table for WCN or by adding conditional check in exiting path. -Rajkumar ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 1/4] New netlink command for TID specific configuration
Hello Tamizh,
Co-Developed-by: Tamizh Chelvam
Signed-off-by: Vasanthakumar Thiagarajan
Signed-off-by: Tamizh chelvam
---
include/net/cfg80211.h | 14 +++
include/uapi/linux/nl80211.h | 69 +
net/wireless/nl80211.c | 86
++
net/wireless/rdev-ops.h | 15
net/wireless/trace.h | 27 +
5 files changed, 211 insertions(+)
...
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d744388..d386ad7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
...
+static int nl80211_set_tid_config(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev = info->user_ptr[0];
+ struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
+ struct nlattr *tid;
+ struct net_device *dev = info->user_ptr[1];
+ const char *peer = NULL;
+ u8 tid_no;
+ int ret = -EINVAL, retry_short = -1, retry_long = -1;
+
+ tid = info->attrs[NL80211_ATTR_TID_CONFIG];
+ if (!tid)
+ return -EINVAL;
+
+ ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
+ nl80211_attr_tid_policy, info->extack);
+ if (ret)
+ return ret;
+
+ if (!attrs[NL80211_ATTR_TID])
+ return -EINVAL;
+
+ if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
+ retry_short =
nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
+ if (!retry_short ||
+ retry_short > rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ }
+
+ if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
+ retry_long =
nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
+ if (!retry_long ||
+ retry_long > rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ }
+
+ tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
+ if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
+ return -EINVAL;
+
+ if (info->attrs[NL80211_ATTR_MAC])
+ peer = nla_data(info->attrs[NL80211_ATTR_MAC]);
+
+ if (nla_get_flag(attrs[NL80211_ATTR_TID_RETRY_CONFIG])) {
Do we really need this additional flag to indicate retry data ?
Maybe we can simply check retry attrs or even retry data, e.g.:
Yes, because this implementation gives provision to set default retry
count for TID traffic type for a station.
Since we use single NL command for all TID configurations, this flag
will be useful to notify the driver about
retry TID configuration change.
if (attrs[NL80211_ATTR_TID_RETRY_LONG] ||
attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
...
if ((retry_short > 0) || (retry_long > 0)) {
...
+ if (!wiphy_ext_feature_isset(
+ >wiphy,
+
NL80211_EXT_FEATURE_PER_TID_RETRY_CONFIG))
+ return -EOPNOTSUPP;
+
+ if (peer && !wiphy_ext_feature_isset(
+ >wiphy,
+
NL80211_EXT_FEATURE_PER_STA_RETRY_CONFIG))
+ return -EOPNOTSUPP;
+
+ if (!rdev->ops->set_data_retry_count ||
+ !rdev->wiphy.max_data_retry_count)
+ return -EOPNOTSUPP;
+
+ ret = rdev_set_data_retry_count(rdev, dev, peer,
tid_no,
+ retry_short,
retry_long);
+ }
+
+ return ret;
+}
Regards,
Sergey
Thanks,
Tamizh.
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"
On Wed, Nov 7, 2018 at 8:32 PM Govind Singh wrote: > On 2018-11-08 03:00, Rajkumar Manoharan wrote: > > On 2018-11-07 10:56, Brian Norris wrote: > >> This reverts commit cfb353c0dc058bc1619cc226d3cbbda1f360bdd3. > >> > >> WCN3990 firmware does not yet implement this feature, and so it > >> crashes > >> like this: > >> > >> fatal error received: err_qdi.c:456:EX:wlan_process:1:WLAN > >> RT:207a:PC=b001b4f0 > >> > >> This feature can be re-implemented with a proper service bitmap or > >> other > >> feature-discovery mechanism in the future. But it should not break > >> working boards. > >> > > Brian, > > > > The change "ath10k: add quiet mode support for QCA6174/QCA9377" was > > merged even > > before full WCN3990 device support was added in ath10k. How come it > > could be regression > > for WCN3990. I know both are sharing same WMI-TLV interface but > > reverting this > > will break QCA6174/QCA9377. no? I don't see how the revert would "break" QCA6174 -- QCA6174 worked just fine without this feature and should continue to do so. > This regression is found while we switched from 4.18 + WCN3990 > back-ports to 4.19. ^^ What Govind said. WCN3990 support has been trickling in over a few releases, and it doesn't seem kosher to allow people to submit regressions in the midst of that. > > I would prefer to handle this within WMI callback or upper layer. > > > > IMO, we should use (WMI_SERVICE_THERMAL_MGMT | WMI_SERVICE_THERM_THROT ) > service bitmap check and call > ath10k_thermal_set_throttling only if fw supports THERMAL THROTTLE > feature. But we need to ensure all > available ath10k fw's are reporting this service. And the above notes from Govind highlight this -- if the feature was not protected by the appropriate service flags, then we can't be sure that you didn't break a bunch of other firmware releases out there. Linux should not break for everyone just because you spun a firmware release. Of course, I'll leave it up to Kalle as to how he wants to mediate this. And if you come up with a solid patch soon that can fix this without dropping the feature, then so be it. Brian ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 1/4] New netlink command for TID specific configuration
On 2018-11-06 15:46, Sergey Matyukevich wrote:
Hello Tamizh,
Co-Developed-by: Tamizh Chelvam
Signed-off-by: Vasanthakumar Thiagarajan
Signed-off-by: Tamizh chelvam
---
include/net/cfg80211.h | 14 +++
include/uapi/linux/nl80211.h | 69 +
net/wireless/nl80211.c | 86
++
net/wireless/rdev-ops.h | 15
net/wireless/trace.h | 27 +
5 files changed, 211 insertions(+)
...
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5801d76..dd024da 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
...
/**
@@ -4035,6 +4044,9 @@ struct wiphy_iftype_ext_capab {
* @txq_limit: configuration of internal TX queue frame limit
* @txq_memory_limit: configuration internal TX queue memory limit
* @txq_quantum: configuration of internal TX queue scheduler quantum
+ *
+ * @max_data_retry_count: Maximum limit can be configured as retry
count
+ * for a TID.
*/
struct wiphy {
/* assign these fields before you register the wiphy */
@@ -4171,6 +4183,8 @@ struct wiphy {
u32 txq_memory_limit;
u32 txq_quantum;
+ u8 max_data_retry_count;
+
char priv[0] __aligned(NETDEV_ALIGN);
};
Could you please clarify why do you define max_data_retry_count instead
of
making use of existing wiphy params: retry_short (dot11ShortRetryLimit)
and retry_long (dot11LongRetryLimit) ?
max_data_retry_count added to validate the max limit for the retry count
supported by the driver.
existing wiphy parames: retry_short and retry_long can be modified
through user command.
So, I've added this param for validation purpose. Correct me If I'm
wrong.
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d744388..d386ad7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
...
+static int nl80211_set_tid_config(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct cfg80211_registered_device *rdev = info->user_ptr[0];
+ struct nlattr *attrs[NL80211_ATTR_TID_MAX + 1];
+ struct nlattr *tid;
+ struct net_device *dev = info->user_ptr[1];
+ const char *peer = NULL;
+ u8 tid_no;
+ int ret = -EINVAL, retry_short = -1, retry_long = -1;
+
+ tid = info->attrs[NL80211_ATTR_TID_CONFIG];
+ if (!tid)
+ return -EINVAL;
+
+ ret = nla_parse_nested(attrs, NL80211_ATTR_TID_MAX, tid,
+ nl80211_attr_tid_policy, info->extack);
+ if (ret)
+ return ret;
+
+ if (!attrs[NL80211_ATTR_TID])
+ return -EINVAL;
+
+ if (attrs[NL80211_ATTR_TID_RETRY_SHORT]) {
+ retry_short =
nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_SHORT]);
+ if (!retry_short ||
+ retry_short > rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ }
+
+ if (attrs[NL80211_ATTR_TID_RETRY_LONG]) {
+ retry_long =
nla_get_u8(attrs[NL80211_ATTR_TID_RETRY_LONG]);
+ if (!retry_long ||
+ retry_long > rdev->wiphy.max_data_retry_count)
+ return -EINVAL;
+ }
+
+ tid_no = nla_get_u8(attrs[NL80211_ATTR_TID]);
+ if (tid_no >= IEEE80211_FIRST_TSPEC_TSID)
+ return -EINVAL;
Not that important, but this tid_no check can be placed after
attrs[NL80211_ATTR_TID].
BTW, some special tid_no value (e.g. (u8)-1) could be used to notify
driver
that retry settings should be applied for all the TIDs. IIUC the only
required change would be to modify this tid_no sanity check.
Sure, we can use that.
Regards,
Sergey
Thanks,
Tamizh.
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 0/4] cfg80211/mac80211: Add support for TID specific configuration
On 11/07/2018 04:55 PM, Igor Mitsyanko wrote: On 10/22/2018 10:55 AM, Tamizh chelvam wrote: Add infrastructure for per TID aggregation/retry count configurations such as retry count and AMPDU aggregation control(disable/enable). In some scenario reducing the number of retry count for a specific data traffic can reduce the latency by proceeding with the next packet instead of retrying the same packet more time. This will be useful where the next packet can resume the operation without an issue. Here added NL80211_CMD_SET_TID_CONFIG to support this operation by accepting retry count and AMPDU aggregation control. This command can accept STA mac addreess to make the configuration station specific rather than applying to all the connected stations to the netdev. It's not immediately clear how to make use of these settings, here are several comments: 1. What max retry count limit should actually be applied to? Retries decisions are in a rate adaptation domain, it should know how many retries should be done on each rate, single "max retry" value will not suffice. For example, it can retry twice on MCS9, twice on MCS7, three times on MCS5 or something like that. I'm not familiar with what ATH10k is doing, 4th patch defines ATH10K_MAX_RETRY_COUNT=30, what does it actually mean? It's unlikely "do 30 retries on the same rate". Does retry limit setting interacts with rate adaptation somehow in ath10k? Maybe it makes sense to extend max retry value specification to make it possible to define per-rate? I'm not sure how much flexibility we want here, something like retry value per MCS:BW:SGI? For ath10k, my understanding is that each time it (re)sends a packet, it will query FW rate-ctrl and choose the optimal rate. It doesn't pay much attention to whether a specific frame is retried or not, other than to maybe enable RTS/CTS, but lots of retries will bump the rate-ctrl down to a lower rate. There are no per-rate retry counter logic, but I think there is per-tid control, though currently it might not be wired up to the driver. 2. AMPDU/AMSDU - the way it is, it is also relevant to rate in Tx direction only, correct? We keep advertised capabilities intact and peer has all rights to send AMPDUs/AMSDUs of whatever size that was advertised. Additionally, posted patches do not do anything with established blockack agreement. 3 With above being said, perhaps it would make sense for this new interface to indicate explicitly that it's related to Tx rate? That can be controlled per-TID, per-node or globally, depending on device capabilities. Some other settings that may be useful are fixed MCS, MCS limit, SGI on/off, bandwidth, maybe even provide rate retry rules. I think there should be a way to configure the advertised capabilities, and also a way to configure the settings actually used for transmit. This is what we use for test-related use cases, but maybe there is not a great deal of general use for this type of thing. For general use, the 'transmit' settings are probably more useful. I do know that several ath10k users are forcing it back to /n mode which works around some bugs in their mesh setup. You can already set a fixed transmit rate or set the MCS rates allowed to be used (my supplicant, ath10k-ct driver/firmware is needed to take full advantage of this for ath10k). In upstream kernels, this will not much affect the advertised capabilities. I also have patches that allow setting the advertised rates and capabilities, so you can force a station to advertise only a/n rates even though it and peer have /AC capability. Those patches are not upstream, though if opinions are changed, I'd be happy to repost and try to get them upstream. Thanks, Ben I don't see how it can be used in real product, unless there is an external rate adaptation logic of some kind. But definitely it will be useful for testing, and can be used for WFA certification. -- Ben Greear Candela Technologies Inc http://www.candelatech.com ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 3/6] mac80211: Add airtime accounting and scheduling to TXQs
Rajkumar Manoharan writes: > On 2018-11-07 06:53, Toke Høiland-Jørgensen wrote: >> Rajkumar Manoharan writes: >> >>> Meanwhile we did some more experiments with both modes. The experiment >>> was done in open environment and fixed rate and UDP traffic ran for 60 >>> seconds. >>> >>> Seems like push mode not honoring the configured weight. Always the >>> airtime was almost same whereas in pull-mode airtime is changing based >>> on configured weight. Hence I would like to know your results. >> >> Right, so I verified that the current version of the patch set still >> works with ath9k. However, the ath10k card I have doesn't seem to >> support peer stats, so I can't test ath10k. >> >> $ lspci | grep Qualcomm >> 03:00.0 Network controller: Qualcomm Atheros QCA986x/988x 802.11ac >> Wireless Network Adapter >> >> $ cat /sys/kernel/debug/ieee80211/phy1/ath10k/chip_id >> 0x043202ff >> >> $ cat /sys/kernel/debug/ieee80211/phy1/ath10k/wmi_services | grep PEER >> WMI_SERVICE_PEER_CACHING - >> WMI_SERVICE_PEER_STATS - >> > > Oops... Yeah 988x firmware (10.2.4) does not have peer stats support. > >> >> Is there a way to force-enable airtime support, is this a hardware >> issue? >> > Unfortunately not. There is one more pending change that handles > airtime report from HTT tx-compl. Again it depends firmware support. > These experiments are taken with this f/w interface. Will post the > change. Right, thought so. Too bad. Guess you are doing all the ath10k testing, then ;) >>> sta1sta2sta3sta4 >>> pull-mode 8s(205us) 18s(3.2ms) 8s(205us) 14s(410us) >>> 12s(256us) 12s(256us) 13s(256us) 12s(256us) >>> 14s(4ms)13s(4ms)14s(4ms)13s(4ms) >>> >>> push-mode 15s(205us) 12s(3.2ms) 16s(205us) 12s(410us) >>> 15s(256us) 12s(256us) 16s(256us) 12s(256us) >>> 14s(4ms)13s(4ms)16s(4ms)12s(4ms) >> >> Right, so the pull-mode results are encouraging! *Something* is >> happening, at least, even though the aggregate airtime doesn't quite >> match the ratios of the configured weights. >> >> Are you running the UDP generator on the AP itself, or on a separate >> device, BTW? If it's on the AP, the userspace socket can get throttled, >> which will interfere with results, so it's better to have it on a >> separate device (connected via ethernet). >> > Traffic b/w wired client (via ethernet) and wireless clients. Cool. >> As for push-mode, could this be because of bad buffer management? I.e., >> because there's a lag between the time airtime is registered, and the >> time that airtime usage is reported, the driver just pushes a whole >> bunch of packets to the firmware when it gets the chance, which >> prevents >> the scheduler from throttling properly. This could also explain the >> better, but not quite perfect, results in pull mode, assuming that pull >> mode results in better firmware buffer management which reduces, but >> doesn't quite remove, the lag. >> > Hmm... I agree that lag in reporting airtime may cause more data push > to hw. Right now both tx and tx-compl are serialized by > active_txq_lock. So once lock acquired by tx path, it will download > all frames. i.e it is even true for ath9k driver. Hence I am wondering > how it is working only with ath9k. If enough packets are dequeued at once that the TXQ empties and is not put back on the scheduling loop, the next_txq() loop is just going to loop through the remaining TXQs and immediately increase their quantum. In ath9k, there's a maximum of two outstanding aggregates below the TXQ level, but I think you mentioned that ath10k can queue thousands in firmware? Your patch removes the 'max 16 packets at a time' check before the call to ath10k_mac_tx_push_txq(); try adding that back and see if it helps? >> If this is indeed the reason, the queue limit patches should hopefully >> be a solution. So guess we need to get those working as well :) >> > I would prefer to baseline the basic infra into upstream first and do > enhancement on top of that. Sure, I'm fine with merging this first and building on top. > I request you to revisit maintaining per driver default. Otherwise > there would be performance impact with 256us. :( Hmm, how about we make it a driver-specified multiplier instead (which could be 4, 8 or 16 for ath10k)? That way it would still work even if userspace changes the weights. It would affect the minimum possible granularity, but that is probably acceptable; and we would be free to change the mechanism later, without affecting the userspace API. -Toke ___ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 3/4] mac80211: Add api to support configuring TID specific configuration
On 2018-11-06 16:03, Sergey Matyukevich wrote:
Signed-off-by: Tamizh chelvam
---
include/net/mac80211.h| 40 +
net/mac80211/cfg.c| 71
+
net/mac80211/driver-ops.h | 16 ++
net/mac80211/trace.h | 34 ++
4 files changed, 161 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index b6cc3e33..7fa7e25 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1478,6 +1478,35 @@ struct ieee80211_channel_switch {
u8 count;
};
+/*
+ * enum ieee80211_tid_conf_change - TID change configuration
notification flags
+ *
+ * These flags are used with the set_tid_conf() callback
+ * to indicate which TID configuration parameter changed.
+ *
+ * @TID_RETRY_CONF_CHANGED: retry configuration changed.
+ * @TID_AGGR_CONF_CHANGED: Aggregation config changed for the TID.
+ */
+enum ieee80211_tid_conf_change {
+ TID_RETRY_CONF_CHANGED = BIT(0),
+ TID_AGGR_CONF_CHANGED = BIT(1),
+};
Following your approach, AMSDU support can be added in addition to
AMPDU. So I would suggest to replace AGGR by AMPDU right away.
+
+/*
+ * struct ieee80211_tid_conf - holds the tid configiuration data
+ * The information provided in the structure is required for the
driver
+ * to configure TID specific configuration.
+ * @tid: TID number
+ * @retry_short: retry count value
+ * @retry_long: retry count value
+ * @aggr: enable/disable aggregation
+ */
+struct ieee80211_tid_conf {
+ u8 tid;
+ int retry_short;
+ int retry_long;
+ bool aggr;
+};
ditto: aggr -> ampdu
Sure. I'll update in the next version.
Thanks,
Tamizh.
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
Re: [PATCH 2/4] nl80211: Add netlink attribute for AMPDU aggregation enable/disable
Hi Sergey,
Hello Tamizh,
Signed-off-by: Tamizh chelvam
---
include/net/cfg80211.h |6 ++
include/uapi/linux/nl80211.h | 21 +
net/wireless/nl80211.c | 17 +
net/wireless/rdev-ops.h | 15 +++
net/wireless/trace.h | 23 +++
5 files changed, 82 insertions(+)
...
diff --git a/include/uapi/linux/nl80211.h
b/include/uapi/linux/nl80211.h
index 9dfcf0a6..7ba0fb7 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4449,6 +4449,20 @@ enum nl80211_ps_state {
* the max value should be advertised by the driver through
* max_data_retry_count. when this attribute is not present, the
driver
* would use the default configuration.
+ * @NL80211_ATTR_TID_AMPDU_AGGR_CTRL: Enable/Disable aggregation for
the TID
+ * specified in %%NL80211_ATTR_TID. Its type is u8, if the peer
MAC address
+ * is passed in %NL80211_ATTR_MAC, the aggregation configuration
is applied
+ * to the data frame for the tid to that connected station.
+ * Station specific aggregation configuration is valid only for
STA's
+ * current connection. i.e. the configuration will be reset to
default when
+ * the station connects back after disconnection/roaming.
+ * when user-space does not include %NL80211_ATTR_MAC, this
configuration
+ * should be treated as per-netdev configuration. This
configuration will
+ * be cleared when the interface goes down and on the
disconnection from a
+ * BSS. Driver supporting this feature should advertise
+ * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL and supporting per
station
typo: should be NL80211_EXT_FEATURE_PER_TID_AMPDU_AGGR_CTRL
Sure, I'll update in the next version.
+ * aggregation configuration should advertise
+ * NL80211_EXT_FEATURE_PER_STA_AMPDU_AGGR_CTRL.
*/
Regards,
Sergey
Thanks,
Tamizh.
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
[PATCH] ath10k: remove set but not used variable 'num_tdls_vifs'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/wireless/ath/ath10k/mac.c: In function 'ath10k_sta_state':
drivers/net/wireless/ath/ath10k/mac.c:6238:7: warning:
variable 'num_tdls_vifs' set but not used [-Wunused-but-set-variable]
'num_tdls_vifs' not used any more after
9a993cc1ea95 ("ath10k: fix the logic of limiting tdls peer counts")
Also, remove the single called function ath10k_mac_tdls_vifs_count
and ath10k_mac_tdls_vifs_count_iter.
Signed-off-by: YueHaibing
---
drivers/net/wireless/ath/ath10k/mac.c | 26 --
1 file changed, 26 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c
b/drivers/net/wireless/ath/ath10k/mac.c
index a1c2801..6006f7a 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -5754,30 +5754,6 @@ static int ath10k_mac_tdls_vif_stations_count(struct
ieee80211_hw *hw,
return data.num_tdls_stations;
}
-static void ath10k_mac_tdls_vifs_count_iter(void *data, u8 *mac,
- struct ieee80211_vif *vif)
-{
- struct ath10k_vif *arvif = (void *)vif->drv_priv;
- int *num_tdls_vifs = data;
-
- if (vif->type != NL80211_IFTYPE_STATION)
- return;
-
- if (ath10k_mac_tdls_vif_stations_count(arvif->ar->hw, vif) > 0)
- (*num_tdls_vifs)++;
-}
-
-static int ath10k_mac_tdls_vifs_count(struct ieee80211_hw *hw)
-{
- int num_tdls_vifs = 0;
-
- ieee80211_iterate_active_interfaces_atomic(hw,
- IEEE80211_IFACE_ITER_NORMAL,
-
ath10k_mac_tdls_vifs_count_iter,
- _tdls_vifs);
- return num_tdls_vifs;
-}
-
static int ath10k_hw_scan(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_scan_request *hw_req)
@@ -6285,7 +6261,6 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
*/
enum wmi_peer_type peer_type = WMI_PEER_TYPE_DEFAULT;
u32 num_tdls_stations;
- u32 num_tdls_vifs;
ath10k_dbg(ar, ATH10K_DBG_MAC,
"mac vdev %d peer create %pM (new sta) sta %d / %d
peer %d / %d\n",
@@ -6301,7 +6276,6 @@ static int ath10k_sta_state(struct ieee80211_hw *hw,
}
num_tdls_stations = ath10k_mac_tdls_vif_stations_count(hw, vif);
- num_tdls_vifs = ath10k_mac_tdls_vifs_count(hw);
if (sta->tdls) {
if (num_tdls_stations >= ar->max_num_tdls_vdevs) {
___
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
