[PATCH] ath10k: fix reporting channel survey data

[Ashok Raj Nagarajan]

When user requests for survey dump data, driver is providing wrong survey
information. This information we sent is the survey data that we have
collected during previous user request.

This issue occurs because we request survey dump for wrong channel. With
this change, we correctly display the correct and current survey
information to userspace.

Fixes: fa7937e3d5c2 ("ath10k: update bss channel survey information")
Signed-off-by: Ashok Raj Nagarajan 
---
 drivers/net/wireless/ath/ath10k/mac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index 0bbd0a0..1fa3c17 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -6538,7 +6538,7 @@ static int ath10k_get_survey(struct ieee80211_hw *hw, int 
idx,
goto exit;
}
 
-   ath10k_mac_update_bss_chan_survey(ar, survey->channel);
+   ath10k_mac_update_bss_chan_survey(ar, >channels[idx]);
 
spin_lock_bh(>data_lock);
memcpy(survey, ar_survey, sizeof(*survey));
-- 
1.9.1

Re: [PATCH v5] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Jason Andryuk]

On Thu, Sep 1, 2016 at 12:03 PM, Toke Høiland-Jørgensen  wrote:
> @@ -1481,33 +1506,57 @@ struct sk_buff *ieee80211_tx_dequeue(struct 
> ieee80211_hw *hw,
>  {
> struct ieee80211_local *local = hw_to_local(hw);
> struct txq_info *txqi = container_of(txq, struct txq_info, txq);
> -   struct ieee80211_hdr *hdr;
> struct sk_buff *skb = NULL;
> struct fq *fq = >fq;
> struct fq_tin *tin = >tin;
> +   struct ieee80211_tx_info *info;
>
> spin_lock_bh(>lock);
>
> if (test_bit(IEEE80211_TXQ_STOP, >flags))
> goto out;
>
> +begin:
> skb = fq_tin_dequeue(fq, tin, fq_tin_dequeue_func);
> if (!skb)
> goto out;
>
> ieee80211_set_skb_vif(skb, txqi);
>
> -   hdr = (struct ieee80211_hdr *)skb->data;
> -   if (txq->sta && ieee80211_is_data_qos(hdr->frame_control)) {
> +   info = IEEE80211_SKB_CB(skb);
> +   if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
> struct sta_info *sta = container_of(txq->sta, struct sta_info,
> sta);
> -   struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> +   struct ieee80211_fast_tx *fast_tx;
>
> -   hdr->seq_ctrl = ieee80211_tx_next_seq(sta, txq->tid);
> -   if (test_bit(IEEE80211_TXQ_AMPDU, >flags))
> -   info->flags |= IEEE80211_TX_CTL_AMPDU;
> -   else
> -   info->flags &= ~IEEE80211_TX_CTL_AMPDU;
> +   fast_tx = rcu_dereference(sta->fast_tx);
> +   if (WARN_ON(!fast_tx)) {
> +   /* lost the fast_tx pointer while the packet was 
> queued */
> +   ieee80211_free_txskb(hw, skb);
> +   goto begin;
> +   }
> +   ieee80211_xmit_fast_finish(sta->sdata, sta, fast_tx, skb, 
> false);
> +   } else {
> +   struct ieee80211_tx_data tx = { };
> +
> +   __skb_queue_head_init();
> +   tx.local = local;
> +   if (txq->sta) {
> +   struct sta_info *sta = container_of(txq->sta,
> +   struct sta_info,
> +   sta);

sta is unneeded give the assignment below?

Regards,
Jason

> +   tx.sta = container_of(txq->sta, struct sta_info, sta);
> +   tx.sdata = sta->sdata;
> +   } else {
> +   tx.sdata = vif_to_sdata(info->control.vif);
> +   }
> +
> +   __skb_queue_tail(, skb);
> +
> +   if (invoke_tx_handlers_late())
> +   goto begin;
> +
> +   __skb_unlink(skb, );
> }

Re: Ath10k probe response error related to mac80211 commit.

[Johannes Berg]

> If someone has any idea of why this patch might trigger it, please
> let me know.
> I'll keep digging in the meantime...
> 
>  Revert "mac80211: don't advertise NL80211_FEATURE_FULL_AP_CLIENT_STATE"
> 

With a sufficiently recent hostapd/wpa_supplicant, the patch will cause
a station entry to be added to the firmware before sending the
authentication frame.

Why, of all frames, probe response frames should be corrupted I don't
know - I could imagine auth/assoc replies being treated differently
since they are now with a station entry rather than without.

> This only breaks AP mode (station mode works fine).

It also has no impact on anything but AP mode, as even indicated by the
name of the flag :)


Anyway, I was pretty sure this was safe and it does help other drivers
to have the full state, but I guess you can make the driver opt out of
the flag again (just unset it before register_hw).

johannes

[PATCH] ath9k: bring back direction setting in ath9k_{start_stop}

[Giedrius Statkevičius]

A regression was introduced in commit id 79d4db1214a ("ath9k: cleanup
led_pin initial") that broken the WLAN status led on my laptop with
AR9287 after suspending and resuming.

Steps to reproduce:
* Suspend (laptop)
* Resume (laptop)
* Observe that the WLAN led no longer turns ON/OFF depending on the
  status and is always red

Even though for my case it only needs to be set to OUT in ath9k_start
but for consistency bring back the IN direction setting as well.

Cc: Miaoqing Pan 
Cc: Kalle Valo 
Cc: 
Signed-off-by: Giedrius Statkevičius 
---
This patch should be applied to all 4.7 and later kernels

Another user complaining about probably the same problem:
https://bugzilla.kernel.org/show_bug.cgi?id=151711

 drivers/net/wireless/ath/ath9k/main.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c 
b/drivers/net/wireless/ath/ath9k/main.c
index 8b63988..121dc05 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -718,9 +718,12 @@ static int ath9k_start(struct ieee80211_hw *hw)
if (!ath_complete_reset(sc, false))
ah->reset_power_on = false;
 
-   if (ah->led_pin >= 0)
+   if (ah->led_pin >= 0) {
ath9k_hw_set_gpio(ah, ah->led_pin,
  (ah->config.led_active_high) ? 1 : 0);
+   ath9k_hw_gpio_request_out(ah, ah->led_pin, NULL,
+ AR_GPIO_OUTPUT_MUX_AS_OUTPUT);
+   }
 
/*
 * Reset key cache to sane defaults (all entries cleared) instead of
@@ -864,9 +867,11 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 
spin_lock_bh(>sc_pcu_lock);
 
-   if (ah->led_pin >= 0)
+   if (ah->led_pin >= 0) {
ath9k_hw_set_gpio(ah, ah->led_pin,
  (ah->config.led_active_high) ? 0 : 1);
+   ath9k_hw_gpio_request_in(ah, ah->led_pin, NULL);
+   }
 
ath_prepare_reset(sc);
 
-- 
2.9.3

Re: [PATCH v5] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Johannes Berg]

> To avoid having to deal with fragmentation on dequeue, the split is
> set to be after the fragmentation handler. This means that some
> reordering of TX handlers is necessary, and some handlers had to be
> made aware of fragmentation due to this reordering.

Come to think of it, that's actually counterproductive.

If a fragment is dropped, or even just if fragments are reordered, the
receiver will not be able to defragment the frame, and will thus drop
it. Therefore, it's all-or-nothing, and we shouldn't transmit any
fragment if we drop/reorder one (*).

So ... I think you'll just have to deal with fragmentation on the
codel/fq/whatever queues and keep fragments together, or do
fragmentation afterwards.

johannes


(*) also, couldn't this mean that we send something completely stupid
like

seq=1,frag=0
seq=2,frag=0
seq=2,frag=1
seq=2,frag=1

if reordering happened?

Re: Ath10k probe response error related to mac80211 commit.

[Ben Greear]

On 09/01/2016 11:53 AM, Johannes Berg wrote:

On Thu, 2016-09-01 at 11:23 -0700, Ben Greear wrote:


Could easily be that others are corrupted too, but since probe resp
is bad, the association will not proceed.


makes sense.


Heh, I spent 4 days tracking this down, so I wanted to be precise in
my bug report :)


Ahrg, ouch. Sorry about that. I really didn't think the flag would
cause any issues for anyone.


It took so much time because for whatever reason git bisect couldn't
find the issue, and I spent a day thinking it was in ath10k driver and
poking hard at that.

Anyway, no worries...it happens, and could easily just be a bug in
my firmware modifications that are somehow triggered by this.


The result I see is that there is an extra 10 bytes at the end of the
frame on air.  But, it looks like the exact same pkt is sent to the
firmware both with and without this patch.  Maybe the firmware is
using the wrong tid or something like that due to how the station is
created differently with this patch.


That makes no sense though, unless this only happens on say the second
station that connects? Until the first station sends an authentication
frame, that patch really should have no impact whatsoever.


'makes no sense' is the ath10k motto.

I have verified that it is not an obvious difference in the ath10k driver
though...copying 4.5 ath10k driver onto 4.4 works fine, and copying 4.4
ath10k driver into 4.5 is broken.

I don't think any stations connect, but I didn't look precisely for that
yet.  I know the stations I'm trying to connect will not.

Thanks,
Ben


--
Ben Greear 
Candela Technologies Inc  http://www.candelatech.com

Re: Ath10k probe response error related to mac80211 commit.

[Johannes Berg]

On Thu, 2016-09-01 at 11:23 -0700, Ben Greear wrote:
> 
> Could easily be that others are corrupted too, but since probe resp
> is bad, the association will not proceed.

makes sense.

> Heh, I spent 4 days tracking this down, so I wanted to be precise in
> my bug report :)

Ahrg, ouch. Sorry about that. I really didn't think the flag would
cause any issues for anyone.

> The result I see is that there is an extra 10 bytes at the end of the
> frame on air.  But, it looks like the exact same pkt is sent to the
> firmware both with and without this patch.  Maybe the firmware is
> using the wrong tid or something like that due to how the station is
> created differently with this patch.

That makes no sense though, unless this only happens on say the second
station that connects? Until the first station sends an authentication
frame, that patch really should have no impact whatsoever.

johannes

Re: [PATCH v5] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Johannes Berg]

On Thu, 2016-09-01 at 20:30 +0200, Toke Høiland-Jørgensen wrote:

> > seq=1,frag=0
> > seq=2,frag=0
> > seq=2,frag=1
> > seq=2,frag=1
> > 
> > if reordering happened?
> 
> (assuming the last line was supposed to read 'seq=1,frag=1')

I did actually mean seq=2,frag=1, since the seqno assignment happened
after fragmentation in your patch, and after codel reordering, and
would not change the seqno until it encountered a frag=0 packet.

Or maybe that was only with the previous version of the patch.

> When does fragmentation happen anyway? Is it safe to assume there's
> no aggregation when it does?
> 

Yes, fragmented packets are not allowed to be aggregated.

johannes

Re: [PATCH v5] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> To avoid having to deal with fragmentation on dequeue, the split is
>> set to be after the fragmentation handler. This means that some
>> reordering of TX handlers is necessary, and some handlers had to be
>> made aware of fragmentation due to this reordering.
>
> Come to think of it, that's actually counterproductive.
>
> If a fragment is dropped, or even just if fragments are reordered, the
> receiver will not be able to defragment the frame, and will thus drop
> it. Therefore, it's all-or-nothing, and we shouldn't transmit any
> fragment if we drop/reorder one (*).
>
> So ... I think you'll just have to deal with fragmentation on the
> codel/fq/whatever queues and keep fragments together, or do
> fragmentation afterwards.

Hmm, guess that makes sense. Bugger. Will think about how to do that.

>
> johannes
>
> (*) also, couldn't this mean that we send something completely stupid
> like
>
> seq=1,frag=0
> seq=2,frag=0
> seq=2,frag=1
> seq=2,frag=1
>
> if reordering happened?

(assuming the last line was supposed to read 'seq=1,frag=1')

Yes, that could happen, in principle (it depends on the fragments' size
in relation to the FQ quantum).


When does fragmentation happen anyway? Is it safe to assume there's no
aggregation when it does?

-Toke

Re: Ath10k probe response error related to mac80211 commit.

[Ben Greear]

On 09/01/2016 11:53 AM, Johannes Berg wrote:

On Thu, 2016-09-01 at 11:23 -0700, Ben Greear wrote:


Could easily be that others are corrupted too, but since probe resp
is bad, the association will not proceed.


makes sense.


Heh, I spent 4 days tracking this down, so I wanted to be precise in
my bug report :)


Ahrg, ouch. Sorry about that. I really didn't think the flag would
cause any issues for anyone.


The result I see is that there is an extra 10 bytes at the end of the
frame on air.  But, it looks like the exact same pkt is sent to the
firmware both with and without this patch.  Maybe the firmware is
using the wrong tid or something like that due to how the station is
created differently with this patch.


That makes no sense though, unless this only happens on say the second
station that connects? Until the first station sends an authentication
frame, that patch really should have no impact whatsoever.


Ok, I found the problem.

In the 10.1 firmware (at least), it will force the TID to be NON-QOS-TID
if the peer object does not have the qos_enabled flag set.  This is probably
a work-around for some other thing lost in antiquity.

When using my firmware that puts mgt frames over HTT, the TID for mgt
frames was being over-ridden and set to non-qos TID.  Due to other
hackery and work-arounds, mgt frames cannot be sent on the non-qos
TID because they will be put on-air 10 bytes too long.  They must be
sent on the mgt-tid or non-pause tid.

I am guessing that somehow this mac80211 change creates a peer early
that does not have the qos logic enabled, and so that is why I suddenly
started hitting this bug.

Probably stock firmware is OK since it uses a second tx path for management,
and I guess that by whatever time it starts sending non-mgt frames the
qos-enabled logic is set properly.

I can fix this in my firmware by making it not over-ride the TID in
this case.

Thanks,
Ben

--
Ben Greear 
Candela Technologies Inc  http://www.candelatech.com

Re: Ath10k probe response error related to mac80211 commit.

[Ben Greear]

On 09/01/2016 11:01 AM, Johannes Berg wrote:



If someone has any idea of why this patch might trigger it, please
let me know.
I'll keep digging in the meantime...

 Revert "mac80211: don't advertise NL80211_FEATURE_FULL_AP_CLIENT_STATE"



With a sufficiently recent hostapd/wpa_supplicant, the patch will cause
a station entry to be added to the firmware before sending the
authentication frame.

Why, of all frames, probe response frames should be corrupted I don't
know - I could imagine auth/assoc replies being treated differently
since they are now with a station entry rather than without.


Could easily be that others are corrupted too, but since probe resp is bad,
the association will not proceed.




This only breaks AP mode (station mode works fine).


It also has no impact on anything but AP mode, as even indicated by the
name of the flag :)


Heh, I spent 4 days tracking this down, so I wanted to be precise in
my bug report :)


Anyway, I was pretty sure this was safe and it does help other drivers
to have the full state, but I guess you can make the driver opt out of
the flag again (just unset it before register_hw).


The result I see is that there is an extra 10 bytes at the end of the frame on
air.  But, it looks like the exact same pkt is sent to the firmware both with
and without this patch.  Maybe the firmware is using the wrong tid or something
like that due to how the station is created differently with this patch.

Since this only happens (as far as I know) with my modified firmware, then
I will try to fix it there.  Or, possibly I can change ath10k driver to flip 
this
mac80211 flag when loading my firmware variant if firmware cannot be easily 
fixed.

Thanks for the info,
--Ben



johannes




--
Ben Greear 
Candela Technologies Inc  http://www.candelatech.com

RE: [PATCH] mwifiex: handle edmac vendor command

[Amitkumar Karwar]

Hi Kalle,

> From: Kalle Valo [mailto:kv...@codeaurora.org]
> Sent: Thursday, September 01, 2016 8:23 PM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org
> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> 
> Amitkumar Karwar  writes:
> 
> >> From: Kalle Valo [mailto:kv...@codeaurora.org]
> >> Sent: Tuesday, May 24, 2016 1:53 AM
> >> To: Amitkumar Karwar
> >> Cc: linux-wireless@vger.kernel.org
> >> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
> >>
> >> Amitkumar Karwar  writes:
> >>
> >> > Hi Kalle,
> >> >
> >> >> From: Amitkumar Karwar [mailto:akar...@marvell.com]
> >> >> Sent: Friday, April 29, 2016 9:28 PM
> >> >> To: linux-wireless@vger.kernel.org
> >> >> Cc: Jeff CF Chen; Amitkumar Karwar
> >> >> Subject: [PATCH] mwifiex: handle edmac vendor command
> >> >>
> >> >> From: chunfan chen 
> >> >>
> >> >> Userspace can configure edmac values through a custom vendor
> command.
> >> >> They will be used by hardware for adaptivity.
> >> >>
> >> >> Signed-off-by: chunfan chen 
> >> >> Signed-off-by: Amitkumar Karwar 
> >>
> >> [deleted over 200 lines of unnecessary quotes]
> >>
> >> > This patch seems to have deferred. We basically want a way to
> >> > download a vendor specific configuration to our firmware. Do you
> >> > have any suggestions on how can achieve this in better way?
> >> >
> >> > I can see below iw command suits our requirement.
> >> >
> >> > iw dev  vendor send   
> >> >
> >> > Please guide.
> >>
> >> It was deferred because use of the nl80211 vendor interface (which I
> >> don't think belong to upstream drivers). I'll take a look at this
> >> patch in detail after the merge window.
> >>
> >
> > Did you get a chance to check this patch?
> > Please let me know if you have any suggestions.
> 
> Sorry for the delay. I have been thinking about vendor commands quite a
> lot and I don't think they belong to upstream drivers. For regular use
> cases (used by normal users) we have nl80211, for developer and testing
> purposes we have debugfs and for manufacturing type of tests we have
> nl80211 testmode interface. The focus of development should be adding
> new functionality to nl80211 (and other generic interfaces), not to
> driver specific vendor commands.
> 
> I know brcm80211 and ti's wlcore have few vendor commands but I'm hoping
> to remove them sometime soon.
> 
> Thoughts?
> 

Thanks for your reply.

There is something called energy detect mode. Chip can detect non-WiFi radio 
signal also and monitor it for specified time before transmitting frames.
As per ETSI specification, enabling this mode is mandatory for some countries 
for certain frequencies.

The purpose of this patch is to configure the chip for working in this mode.
I can see cfg80211 has a framework to handle vendor specific commands and 
events. I think having vendor commands would be helpful to support vendor 
specific functionalities which can't be generalized.

I suppose debugfs interface is only for collecting stats and info which can be 
used for debugging purpose.

Regards,
Amitkumar

Ath10k probe response error related to mac80211 commit.

[Ben Greear]

I have a variant of the ath10k 10.1 firmware that puts all frames, including 
management,
over the normal htt packet transport.  This has worked fine for many kernels, 
but
for reasons that currently escape me, the patch below breaks this firmware.

On air, I see corrupted probe responses, seems the end of the frames are 
corrupted.

This only breaks AP mode (station mode works fine).

There may be some other similar breakage somewhere, since git bisect could not 
find this
error and I had to bisect it by hand.

If someone has any idea of why this patch might trigger it, please let me know.
I'll keep digging in the meantime...

Author: Johannes Berg   2015-11-26 07:26:14
Committer: Johannes Berg   2015-12-04 05:43:32
Tags: ben-bad-athia-6
Parent: bda95eb1d1581cfd79e9717ebda4b7ccd2265351 (cfg80211: handle add_station 
auth/assoc flag quirks)
Child:  86c7ec9eb154020797c39e1cc7dafa92da02f603 (mac80211: properly free skb 
when r-o-c for TX fails)
Branches: master, remotes/origin/master
Follows: ben-good-athia-7
Precedes: ben-bad-5-athai

Revert "mac80211: don't advertise NL80211_FEATURE_FULL_AP_CLIENT_STATE"

This reverts commit 45bb780a2147b9995f3d288c44ecb87ca8a330e2,
the previous two patches fixed the functionality.

Signed-off-by: Johannes Berg 

- net/mac80211/main.c -
index 175ffcf..858f6b1 100644
@@ -541,7 +541,8 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t 
priv_data_len,
   NL80211_FEATURE_HT_IBSS |
   NL80211_FEATURE_VIF_TXPOWER |
   NL80211_FEATURE_MAC_ON_CREATE |
-  NL80211_FEATURE_USERSPACE_MPM;
+  NL80211_FEATURE_USERSPACE_MPM |
+  NL80211_FEATURE_FULL_AP_CLIENT_STATE;

if (!ops->hw_scan)
wiphy->features |= NL80211_FEATURE_LOW_PRIORITY_SCAN |

Thanks,
Ben

--
Ben Greear 
Candela Technologies Inc  http://www.candelatech.com

Re: linux-4.8-rcX: ath9k traceback

[Kalle Valo]

Mimi Zohar  writes:

> There weren't any problems on linux-4.7 kernels.  I'm getting the
> following traceback on linux-4.8-rc1/-rc4 kernels.  Let me know if you
> need any additional information.
>
> [   64.006529] WARNING: CPU: 3 PID: 94 at 
> drivers/net/wireless/ath/ath9k/beacon.c:642 ath9k_beacon_config+0x12c/0x140 
> [ath9k]

This should fix it:

ath9k: fix client mode beacon configuration

https://git.kernel.org/cgit/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=05860bed491b114a9f2d7a4f6e09fb02c0b69056

-- 
Kalle Valo

[PATCH] ath10k: Remove unnecessary error code assignment

[Mohammed Shafi Shajakhan]

From: Mohammed Shafi Shajakhan 

The error assigned does not seems to be used anywhere,
fixes nothing just a small cleanup

Signed-off-by: Mohammed Shafi Shajakhan 
---
 drivers/net/wireless/ath/ath10k/wmi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c 
b/drivers/net/wireless/ath/ath10k/wmi.c
index eb2831c..3117a71 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -3544,7 +3544,6 @@ void ath10k_wmi_event_host_swba(struct ath10k *ar, struct 
sk_buff *skb)
ath10k_warn(ar, "failed to map beacon: %d\n",
ret);
dev_kfree_skb_any(bcn);
-   ret = -EIO;
goto skip;
}
 
-- 
1.9.1

[PATCH v5] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Toke Høiland-Jørgensen]

The TXQ intermediate queues can cause packet reordering when more than
one flow is active to a single station. Since some of the wifi-specific
packet handling (notably sequence number and encryption handling) is
sensitive to re-ordering, things break if they are applied before the
TXQ.

This splits up the TX handlers and fast_xmit logic into two parts: An
early part and a late part. The former is applied before TXQ enqueue,
and the latter after dequeue. The non-TXQ path just applies both parts
at once.

To avoid having to deal with fragmentation on dequeue, the split is set
to be after the fragmentation handler. This means that some reordering
of TX handlers is necessary, and some handlers had to be made aware of
fragmentation due to this reordering.

This approach avoids having to scatter special cases for when TXQ is
enabled, at the cost of making the fast_xmit and TX handler code
slightly more complex.

Signed-off-by: Toke Høiland-Jørgensen 
---
Changes since v4:
- Keep fragnum assignment in fragmentation handler and fix endianness
  issues in seqno handler.
- Assume xmit_fast_finish can't fail in dequeue handler (and warn if
  fast_tx handle disappears).
- Move TKIP MIC and key selection handlers back before fragmentation
  handler. Turns out the MIC doesn't actually depend on a global
  sequence number, so it can be before the intermediate queueing step.
  The only cost of this is running the key selection handler twice in
  some cases.
- Improve readability of the composite invoke_tx_handlers() function.


 include/net/mac80211.h |   2 +
 net/mac80211/tx.c  | 266 +++--
 2 files changed, 214 insertions(+), 54 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cca510a..9a6a3e9 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -715,6 +715,7 @@ enum mac80211_tx_info_flags {
  * frame (PS-Poll or uAPSD).
  * @IEEE80211_TX_CTRL_RATE_INJECT: This frame is injected with rate information
  * @IEEE80211_TX_CTRL_AMSDU: This frame is an A-MSDU frame
+ * @IEEE80211_TX_CTRL_FAST_XMIT: This frame is going through the fast_xmit path
  *
  * These flags are used in tx_info->control.flags.
  */
@@ -723,6 +724,7 @@ enum mac80211_tx_control_flags {
IEEE80211_TX_CTRL_PS_RESPONSE   = BIT(1),
IEEE80211_TX_CTRL_RATE_INJECT   = BIT(2),
IEEE80211_TX_CTRL_AMSDU = BIT(3),
+   IEEE80211_TX_CTRL_FAST_XMIT = BIT(4),
 };
 
 /*
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1d0746d..f7373c2 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -38,6 +38,12 @@
 #include "wme.h"
 #include "rate.h"
 
+static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx);
+static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
+  struct sta_info *sta,
+  struct ieee80211_fast_tx *fast_tx,
+  struct sk_buff *skb, bool xmit);
+
 /* misc utils */
 
 static inline void ieee80211_tx_stats(struct net_device *dev, u32 len)
@@ -585,20 +591,27 @@ static ieee80211_tx_result debug_noinline
 ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
 {
struct ieee80211_key *key;
-   struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
-   struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
+   struct ieee80211_tx_info *info;
+   struct ieee80211_hdr *hdr;
+   struct sk_buff *skb = tx->skb;
+
+   if (!skb)
+   skb = skb_peek(>skbs);
+
+   info = IEEE80211_SKB_CB(skb);
+   hdr = (struct ieee80211_hdr *)skb->data;
 
if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT))
tx->key = NULL;
else if (tx->sta &&
 (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
tx->key = key;
-   else if (ieee80211_is_group_privacy_action(tx->skb) &&
+   else if (ieee80211_is_group_privacy_action(skb) &&
(key = rcu_dereference(tx->sdata->default_multicast_key)))
tx->key = key;
else if (ieee80211_is_mgmt(hdr->frame_control) &&
 is_multicast_ether_addr(hdr->addr1) &&
-ieee80211_is_robust_mgmt_frame(tx->skb) &&
+ieee80211_is_robust_mgmt_frame(skb) &&
 (key = rcu_dereference(tx->sdata->default_mgmt_key)))
tx->key = key;
else if (is_multicast_ether_addr(hdr->addr1) &&
@@ -628,8 +641,8 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
case WLAN_CIPHER_SUITE_GCMP_256:
if (!ieee80211_is_data_present(hdr->frame_control) &&
!ieee80211_use_mfp(hdr->frame_control, tx->sta,
-  tx->skb) &&
-   !ieee80211_is_group_privacy_action(tx->skb))
+  

Re: [PATCH] drivers: staging: rtl823au: hal: Remove pointless test

[Greg KH]

On Wed, Aug 31, 2016 at 09:32:56PM +0200, Matthias Beyer wrote:
> Pinging here as nobody responded yet.
> 
> Maybe this was overlooked.

Nope, it was only a week, staging patches are at the bottom of my queue,
please give me time to get to them, I process them usually ever few
weeks...

thanks,

greg k-h

Re: [PATCH 1/6] rtl8723au: remove declaration of unimplemented functions

[Greg Kroah-Hartman]

On Sat, Aug 27, 2016 at 02:40:44PM +0200, Luca Ceresoli wrote:
> Signed-off-by: Luca Ceresoli 
> Cc: Larry Finger 
> Cc: Jes Sorensen 
> Cc: Greg Kroah-Hartman 
> Cc: linux-wireless@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
>  drivers/staging/rtl8723au/include/recv_osdep.h | 3 ---
>  1 file changed, 3 deletions(-)

I can't take patches without any changelog text, sorry.  Please fix that
up for all 6 of these patches and resend.

thanks,

greg k-h

Re: [PATCH] mwifiex: handle edmac vendor command

[Kalle Valo]

Amitkumar Karwar  writes:

>> From: Kalle Valo [mailto:kv...@codeaurora.org]
>> Sent: Tuesday, May 24, 2016 1:53 AM
>> To: Amitkumar Karwar
>> Cc: linux-wireless@vger.kernel.org
>> Subject: Re: [PATCH] mwifiex: handle edmac vendor command
>> 
>> Amitkumar Karwar  writes:
>> 
>> > Hi Kalle,
>> >
>> >> From: Amitkumar Karwar [mailto:akar...@marvell.com]
>> >> Sent: Friday, April 29, 2016 9:28 PM
>> >> To: linux-wireless@vger.kernel.org
>> >> Cc: Jeff CF Chen; Amitkumar Karwar
>> >> Subject: [PATCH] mwifiex: handle edmac vendor command
>> >>
>> >> From: chunfan chen 
>> >>
>> >> Userspace can configure edmac values through a custom vendor command.
>> >> They will be used by hardware for adaptivity.
>> >>
>> >> Signed-off-by: chunfan chen 
>> >> Signed-off-by: Amitkumar Karwar 
>> 
>> [deleted over 200 lines of unnecessary quotes]
>> 
>> > This patch seems to have deferred. We basically want a way to download
>> > a vendor specific configuration to our firmware. Do you have any
>> > suggestions on how can achieve this in better way?
>> >
>> > I can see below iw command suits our requirement.
>> >
>> > iw dev  vendor send   
>> >
>> > Please guide.
>> 
>> It was deferred because use of the nl80211 vendor interface (which I
>> don't think belong to upstream drivers). I'll take a look at this patch
>> in detail after the merge window.
>> 
>
> Did you get a chance to check this patch?
> Please let me know if you have any suggestions.

Sorry for the delay. I have been thinking about vendor commands quite a
lot and I don't think they belong to upstream drivers. For regular use
cases (used by normal users) we have nl80211, for developer and testing
purposes we have debugfs and for manufacturing type of tests we have
nl80211 testmode interface. The focus of development should be adding
new functionality to nl80211 (and other generic interfaces), not to
driver specific vendor commands.

I know brcm80211 and ti's wlcore have few vendor commands but I'm hoping
to remove them sometime soon.

Thoughts?

-- 
Kalle Valo

[PATCH v3 2/2] wcn36xx: Implement firmware assisted scan

[Bjorn Andersson]

Using the software based channel scan mechanism from mac80211 keeps us
offline for 10-15 second, we should instead issue a start_scan/end_scan
on each channel reducing this time.

Signed-off-by: Bjorn Andersson 
---

Changes since v2:
- None

 drivers/net/wireless/ath/wcn36xx/main.c| 64 +-
 drivers/net/wireless/ath/wcn36xx/smd.c |  8 ++--
 drivers/net/wireless/ath/wcn36xx/smd.h |  4 +-
 drivers/net/wireless/ath/wcn36xx/txrx.c| 19 ++---
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h |  9 +
 5 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c 
b/drivers/net/wireless/ath/wcn36xx/main.c
index 3c2522b07c90..96a9584edcbb 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -568,23 +568,59 @@ out:
return ret;
 }
 
-static void wcn36xx_sw_scan_start(struct ieee80211_hw *hw,
- struct ieee80211_vif *vif,
- const u8 *mac_addr)
+static void wcn36xx_hw_scan_worker(struct work_struct *work)
 {
-   struct wcn36xx *wcn = hw->priv;
+   struct wcn36xx *wcn = container_of(work, struct wcn36xx, scan_work);
+   struct cfg80211_scan_request *req = wcn->scan_req;
+   u8 channels[WCN36XX_HAL_PNO_MAX_NETW_CHANNELS_EX];
+   struct cfg80211_scan_info scan_info = {};
+   int i;
+
+   wcn36xx_dbg(WCN36XX_DBG_MAC, "mac80211 scan %d channels worker\n", 
req->n_channels);
+
+   for (i = 0; i < req->n_channels; i++)
+   channels[i] = req->channels[i]->hw_value;
+
+   wcn36xx_smd_update_scan_params(wcn, channels, req->n_channels);
 
wcn36xx_smd_init_scan(wcn, HAL_SYS_MODE_SCAN);
-   wcn36xx_smd_start_scan(wcn);
+   for (i = 0; i < req->n_channels; i++) {
+   wcn->scan_freq = req->channels[i]->center_freq;
+   wcn->scan_band = req->channels[i]->band;
+
+   wcn36xx_smd_start_scan(wcn, req->channels[i]->hw_value);
+   msleep(30);
+   wcn36xx_smd_end_scan(wcn, req->channels[i]->hw_value);
+
+   wcn->scan_freq = 0;
+   }
+   wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN);
+
+   scan_info.aborted = false;
+   ieee80211_scan_completed(wcn->hw, _info);
+
+   mutex_lock(>scan_lock);
+   wcn->scan_req = NULL;
+   mutex_unlock(>scan_lock);
 }
 
-static void wcn36xx_sw_scan_complete(struct ieee80211_hw *hw,
-struct ieee80211_vif *vif)
+static int wcn36xx_hw_scan(struct ieee80211_hw *hw,
+  struct ieee80211_vif *vif,
+  struct ieee80211_scan_request *hw_req)
 {
struct wcn36xx *wcn = hw->priv;
 
-   wcn36xx_smd_end_scan(wcn);
-   wcn36xx_smd_finish_scan(wcn, HAL_SYS_MODE_SCAN);
+   mutex_lock(>scan_lock);
+   if (wcn->scan_req) {
+   mutex_unlock(>scan_lock);
+   return -EBUSY;
+   }
+   wcn->scan_req = _req->req;
+   mutex_unlock(>scan_lock);
+
+   schedule_work(>scan_work);
+
+   return 0;
 }
 
 static void wcn36xx_update_allowed_rates(struct ieee80211_sta *sta,
@@ -997,8 +1033,7 @@ static const struct ieee80211_ops wcn36xx_ops = {
.configure_filter   = wcn36xx_configure_filter,
.tx = wcn36xx_tx,
.set_key= wcn36xx_set_key,
-   .sw_scan_start  = wcn36xx_sw_scan_start,
-   .sw_scan_complete   = wcn36xx_sw_scan_complete,
+   .hw_scan= wcn36xx_hw_scan,
.bss_info_changed   = wcn36xx_bss_info_changed,
.set_rts_threshold  = wcn36xx_set_rts_threshold,
.sta_add= wcn36xx_sta_add,
@@ -1023,6 +1058,7 @@ static int wcn36xx_init_ieee80211(struct wcn36xx *wcn)
ieee80211_hw_set(wcn->hw, SUPPORTS_PS);
ieee80211_hw_set(wcn->hw, SIGNAL_DBM);
ieee80211_hw_set(wcn->hw, HAS_RATE_CONTROL);
+   ieee80211_hw_set(wcn->hw, SINGLE_SCAN_ON_ALL_BANDS);
 
wcn->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
BIT(NL80211_IFTYPE_AP) |
@@ -1032,6 +1068,9 @@ static int wcn36xx_init_ieee80211(struct wcn36xx *wcn)
wcn->hw->wiphy->bands[NL80211_BAND_2GHZ] = _band_2ghz;
wcn->hw->wiphy->bands[NL80211_BAND_5GHZ] = _band_5ghz;
 
+   wcn->hw->wiphy->max_scan_ssids = WCN36XX_MAX_SCAN_SSIDS;
+   wcn->hw->wiphy->max_scan_ie_len = WCN36XX_MAX_SCAN_IE_LEN;
+
wcn->hw->wiphy->cipher_suites = cipher_suites;
wcn->hw->wiphy->n_cipher_suites = ARRAY_SIZE(cipher_suites);
 
@@ -1152,6 +1191,9 @@ static int wcn36xx_probe(struct platform_device *pdev)
wcn->hw = hw;
wcn->dev = >dev;
mutex_init(>hal_mutex);
+   mutex_init(>scan_lock);
+
+   INIT_WORK(>scan_work, wcn36xx_hw_scan_worker);
 
wcn->smd_channel = qcom_wcnss_open_channel(wcnss, "WLAN_CTRL", 

[PATCH v3 1/2] wcn36xx: Transition driver to SMD client

[Bjorn Andersson]

The wcn36xx wifi driver follows the life cycle of the WLAN_CTRL SMD
channel, as such it should be a SMD client. This patch makes this
transition, now that we have the necessary frameworks available.

Signed-off-by: Bjorn Andersson 
---

Changes since v2:
- Correct the call to the new ieee80211_scan_completed()

 drivers/net/wireless/ath/wcn36xx/Kconfig   |  2 +-
 drivers/net/wireless/ath/wcn36xx/dxe.c | 16 +++---
 drivers/net/wireless/ath/wcn36xx/main.c| 79 --
 drivers/net/wireless/ath/wcn36xx/smd.c | 29 +--
 drivers/net/wireless/ath/wcn36xx/smd.h |  5 ++
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 21 +++-
 6 files changed, 86 insertions(+), 66 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/Kconfig 
b/drivers/net/wireless/ath/wcn36xx/Kconfig
index 591ebaea8265..394fe5b77c90 100644
--- a/drivers/net/wireless/ath/wcn36xx/Kconfig
+++ b/drivers/net/wireless/ath/wcn36xx/Kconfig
@@ -1,6 +1,6 @@
 config WCN36XX
tristate "Qualcomm Atheros WCN3660/3680 support"
-   depends on MAC80211 && HAS_DMA
+   depends on MAC80211 && HAS_DMA && QCOM_SMD
---help---
  This module adds support for wireless adapters based on
  Qualcomm Atheros WCN3660 and WCN3680 mobile chipsets.
diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c 
b/drivers/net/wireless/ath/wcn36xx/dxe.c
index 231fd022f0f5..87dfdaf9044c 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -23,6 +23,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include 
+#include 
 #include "wcn36xx.h"
 #include "txrx.h"
 
@@ -151,9 +152,12 @@ int wcn36xx_dxe_alloc_ctl_blks(struct wcn36xx *wcn)
goto out_err;
 
/* Initialize SMSM state  Clear TX Enable RING EMPTY STATE */
-   ret = wcn->ctrl_ops->smsm_change_state(
-   WCN36XX_SMSM_WLAN_TX_ENABLE,
-   WCN36XX_SMSM_WLAN_TX_RINGS_EMPTY);
+   ret = qcom_smem_state_update_bits(wcn->tx_enable_state,
+ WCN36XX_SMSM_WLAN_TX_ENABLE |
+ WCN36XX_SMSM_WLAN_TX_RINGS_EMPTY,
+ WCN36XX_SMSM_WLAN_TX_RINGS_EMPTY);
+   if (ret)
+   goto out_err;
 
return 0;
 
@@ -678,9 +682,9 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
 * notify chip about new frame through SMSM bus.
 */
if (is_low &&  vif_priv->pw_state == WCN36XX_BMPS) {
-   wcn->ctrl_ops->smsm_change_state(
- 0,
- WCN36XX_SMSM_WLAN_TX_ENABLE);
+   qcom_smem_state_update_bits(wcn->tx_rings_empty_state,
+   WCN36XX_SMSM_WLAN_TX_ENABLE,
+   WCN36XX_SMSM_WLAN_TX_ENABLE);
} else {
/* indicate End Of Packet and generate interrupt on descriptor
 * done.
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c 
b/drivers/net/wireless/ath/wcn36xx/main.c
index e1d59da2ad20..3c2522b07c90 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -21,6 +21,10 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
 #include "wcn36xx.h"
 
 unsigned int wcn36xx_dbg_mask;
@@ -1058,8 +1062,7 @@ static int wcn36xx_platform_get_resources(struct wcn36xx 
*wcn,
int ret;
 
/* Set TX IRQ */
-   res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
-  "wcnss_wlantx_irq");
+   res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "tx");
if (!res) {
wcn36xx_err("failed to get tx_irq\n");
return -ENOENT;
@@ -1067,14 +1070,29 @@ static int wcn36xx_platform_get_resources(struct 
wcn36xx *wcn,
wcn->tx_irq = res->start;
 
/* Set RX IRQ */
-   res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
-  "wcnss_wlanrx_irq");
+   res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "rx");
if (!res) {
wcn36xx_err("failed to get rx_irq\n");
return -ENOENT;
}
wcn->rx_irq = res->start;
 
+   /* Acquire SMSM tx enable handle */
+   wcn->tx_enable_state = qcom_smem_state_get(>dev,
+   "tx-enable", >tx_enable_state_bit);
+   if (IS_ERR(wcn->tx_enable_state)) {
+   wcn36xx_err("failed to get tx-enable state\n");
+   return PTR_ERR(wcn->tx_enable_state);
+   }
+
+   /* Acquire SMSM tx rings empty handle */
+   wcn->tx_rings_empty_state = qcom_smem_state_get(>dev,
+   "tx-rings-empty", >tx_rings_empty_state_bit);
+   if (IS_ERR(wcn->tx_rings_empty_state)) {
+   wcn36xx_err("failed to get tx-rings-empty state\n");
+ 

Re: pull-request: iwlwifi-next 2016-08-30-2

[Kalle Valo]

Luca Coelho  writes:

> This is a v2 of my pull request with the potential below array bounds
> access, reported by kbuild bot, fixed.
>
>> Another pull request, this time intended for 4.9.  I have a lot of
>> patches to send, but I'll send smaller batches this time. :)
>> 
>> These patches contain mostly preparation for new HW, but also some
>> improvements in the dynamic queue allocation code and the
>> implementation to support GMAC.
>
> Let me know if everything's fine (or not). :)
>
> Luca.
>
>
> The following changes since commit 60747ef4d173c2747bf7f0377fb22846cb422195:
>
>   Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net (2016-08-18 
> 01:17:32 -0400)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-next.git 
> tags/iwlwifi-next-for-kalle-2016-08-30-2

Pulled, thanks.

-- 
Kalle Valo

Re: pull-request: iwlwifi 2016-08-29

[Kalle Valo]

Luca Coelho  writes:

> Here are 4 patches intended for 4.8.  They are all very small and low
> risk, but fix some actual issues.  More details in the tag description.
>
> Let me know if everything's fine (or not). :)
>
> Luca.
>
>
> The following changes since commit bb87f02b7e4ccdb614a83cbf840524de81e9b321:
>
>   Merge ath-current from ath.git (2016-08-29 21:39:04 +0300)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes.git 
> tags/iwlwifi-for-kalle-2016-08-29

Pulled, thanks.

-- 
Kalle Valo

Re: [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> Yeah, was going to do that anyway. But since I'm touching the code
>> anyway, this might be an opportunity to avoid constructs like this:
>> 
>> if (!invoke_tx_handlers(tx))
>>   /* continue sending the packet */
>> 
>> Most other succeed/fail functions seem to be of type bool, so it
>> would help consistency as well. Unless there is some particular
>> reason why this function happens to be using 0 to indicate success?
>> 
>
> It's just convention in the kernel, really.
>
> IMHO if a function has a bool return value it should be have a more
> expressive name that indicates better what's going on, like e.g.
>
> bool ieee80211_is_radar_required(...);
>
> but of course that's not always done.

Well, it's applied somewhat inconsistently across mac80211, it seems
(e.g. ieee80211_tx() and ieee80211_tx_prepare_skb() are bool, while
invoke_tx_handlers() and ieee80211_skb_resize() are int). But okay,
don't have that strong an opinion about the colour of this particular
bikeshed so I'll keep it the way it is ;)

-Toke

Re: [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Johannes Berg]

> Yeah, was going to do that anyway. But since I'm touching the code
> anyway, this might be an opportunity to avoid constructs like this:
> 
> if (!invoke_tx_handlers(tx))
>   /* continue sending the packet */
> 
> Most other succeed/fail functions seem to be of type bool, so it
> would help consistency as well. Unless there is some particular
> reason why this function happens to be using 0 to indicate success?
> 

It's just convention in the kernel, really.

IMHO if a function has a bool return value it should be have a more
expressive name that indicates better what's going on, like e.g.

bool ieee80211_is_radar_required(...);

but of course that's not always done.

johannes

Re: [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> > They have three possible values ... :)
>> 
>> Ah, no, not the handlers themselves. Meant the invoke_tx_handlers()
>> function (or all three of them after my patch; hence the plural). To
>> avoid the "0 means true" confusion you alluded to :)
>> 
>
> Ah. Actually, even I got confused and thought the return value *was*
> the same as the handler.
>
> I think it doesn't matter to be tricky, gcc is probably going to (have
> to) generate exactly the same code like when you explicitly put an if
> statement in there, it seems?

Yeah, was going to do that anyway. But since I'm touching the code
anyway, this might be an opportunity to avoid constructs like this:

if (!invoke_tx_handlers(tx))
  /* continue sending the packet */

Most other succeed/fail functions seem to be of type bool, so it would
help consistency as well. Unless there is some particular reason why
this function happens to be using 0 to indicate success?

-Toke

Re: [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Johannes Berg]

> > They have three possible values ... :)
> 
> Ah, no, not the handlers themselves. Meant the invoke_tx_handlers()
> function (or all three of them after my patch; hence the plural). To
> avoid the "0 means true" confusion you alluded to :)
> 

Ah. Actually, even I got confused and thought the return value *was*
the same as the handler.

I think it doesn't matter to be tricky, gcc is probably going to (have
to) generate exactly the same code like when you explicitly put an if
statement in there, it seems?

johannes

Re: [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> > > +static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
>> > > +{
>> > > +return invoke_tx_handlers_early(tx) ||
>> > > invoke_tx_handlers_late(tx);
>> > > +}
>> > 
>> > Ugh, please, no, don't be tricky where it's not necessary. Now
>> > every
>> > person reading this has to first look up the return type, and then
>> > the
>> > return value, and make sure they understand that success is
>> > actually
>> > the value 0 ... that's way too much to ask.
>> 
>> Noted. Any objections to turning these into bool return types?
>
> They have three possible values ... :)

Ah, no, not the handlers themselves. Meant the invoke_tx_handlers()
function (or all three of them after my patch; hence the plural). To
avoid the "0 means true" confusion you alluded to :)

-Toke

Re: [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Johannes Berg]

> > > +static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
> > > +{
> > > + return invoke_tx_handlers_early(tx) ||
> > > invoke_tx_handlers_late(tx);
> > > +}
> > 
> > Ugh, please, no, don't be tricky where it's not necessary. Now
> > every
> > person reading this has to first look up the return type, and then
> > the
> > return value, and make sure they understand that success is
> > actually
> > the value 0 ... that's way too much to ask.
> 
> Noted. Any objections to turning these into bool return types?

They have three possible values ... :)

johannes

Re: [PATCH v4] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> +static int invoke_tx_handlers(struct ieee80211_tx_data *tx)
>> +{
>> +return invoke_tx_handlers_early(tx) ||
>> invoke_tx_handlers_late(tx);
>> +}
>
> Ugh, please, no, don't be tricky where it's not necessary. Now every
> person reading this has to first look up the return type, and then the
> return value, and make sure they understand that success is actually
> the value 0 ... that's way too much to ask.

Noted. Any objections to turning these into bool return types?


I'll go through and fix your other comments and send a new version.
Thanks for the feedback :)

-Toke