[ath9k-devel] [PATCH 0/2] ath10k: monitor mode fixes

[ath9k-devel] [PATCH 0/2] ath10k: monitor mode fixes

[Michal Kazior]

This cleans up some warnings during runtime.

Michal Kazior (2):
  ath10k: avoid null dereference on vif
  ath10k: don't advertise we want monitor vif

 drivers/net/wireless/ath/ath10k/mac.c |   10 +++++++---
 drivers/net/wireless/ath/ath10k/mac.h |    3 +++
 2 files changed, 10 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/2] ath10k: avoid null dereference on vif

[Michal Kazior]

The vif isn't really guaranteed to be non-NULL.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c |    9 +++++++--
 drivers/net/wireless/ath/ath10k/mac.h |    3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 6291935..6fbbb24 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1202,11 +1202,13 @@ static void ath10k_tx_h_update_wep_key(struct sk_buff *skb)
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_vif *vif = info->control.vif;
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
-	struct ath10k *ar = arvif->ar;
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
 	struct ieee80211_key_conf *key = info->control.hw_key;
 	int ret;
 
+	if (!vif)
+		return;
+
 	/* TODO AP mode should be implemented */
 	if (vif->type != NL80211_IFTYPE_STATION)
 		return;
@@ -1226,7 +1228,7 @@ static void ath10k_tx_h_update_wep_key(struct sk_buff *skb)
 
 	ath10k_dbg(ATH10K_DBG_MAC, "new wep keyidx will be %d\n", key->keyidx);
 
-	ret = ath10k_wmi_vdev_set_param(ar, arvif->vdev_id,
+	ret = ath10k_wmi_vdev_set_param(arvif->ar, arvif->vdev_id,
 					WMI_VDEV_PARAM_DEF_KEYID,
 					key->keyidx);
 	if (ret) {
@@ -1244,6 +1246,9 @@ static void ath10k_tx_h_add_p2p_noa_ie(struct ath10k *ar, struct sk_buff *skb)
 	struct ieee80211_vif *vif = info->control.vif;
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 
+	if (!vif)
+		return;
+
 	/* This is case only for P2P_GO */
 	if (arvif->vdev_type != WMI_VDEV_TYPE_AP ||
 	    arvif->vdev_subtype != WMI_VDEV_SUBTYPE_P2P_GO)
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 272ba74..077b798 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -42,6 +42,9 @@ static inline void ath10k_tx_h_seq_no(struct sk_buff *skb)
 	struct ieee80211_vif *vif = info->control.vif;
 	struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
 
+	if (!vif)
+		return;
+
 	if (info->flags  & IEEE80211_TX_CTL_ASSIGN_SEQ) {
 		if (arvif->tx_seq_no == 0)
 			arvif->tx_seq_no = 0x1000;
-- 
1.7.9.5

[ath9k-devel] [PATCH 2/2] ath10k: don't advertise we want monitor vif

[Michal Kazior]

We were triggering warnings in our code and
mac80211. We could fix the monitor state machine
in ath10k but it doesn't make much sense. We don't
benefit from having the monitor vif create in the
first place so just drop it.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/mac.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 6fbbb24..15e3779 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2683,7 +2683,6 @@ int ath10k_mac_register(struct ath10k *ar)
 			IEEE80211_HW_REPORTS_TX_ACK_STATUS |
 			IEEE80211_HW_HAS_RATE_CONTROL |
 			IEEE80211_HW_SUPPORTS_STATIC_SMPS |
-			IEEE80211_HW_WANT_MONITOR_VIF |
 			IEEE80211_HW_AP_LINK_PS;
 
 	if (ar->ht_cap_info & WMI_HT_CAP_DYNAMIC_SMPS)
-- 
1.7.9.5

[ath9k-devel] [PATCH 2/2] ath10k: don't advertise we want monitor vif

[Sujith Manoharan]

Michal Kazior wrote:
> We were triggering warnings in our code and
> mac80211. We could fix the monitor state machine
> in ath10k but it doesn't make much sense. We don't
> benefit from having the monitor vif create in the
> first place so just drop it.

What warnings ?

I think this flag is required, mac80211 uses it to notify
the driver to create an explicit monitor interface.

For example:

* Create a monitor interface

* Add a station interface
  (Now, mac80211 deletes the first monitor interface in the driver,
   but it is maintained internally in the stack).

* Delete the station interface.
  (mac80211 recreates the first monitor interface now, because
   the WANT_MONITOR_FLAG is set).

Sujith

[ath9k-devel] [PATCH 2/2] ath10k: don't advertise we want monitor vif

[Michal Kazior]

On 24/04/13 13:36, Sujith Manoharan wrote:
> Michal Kazior wrote:
>> We were triggering warnings in our code and
>> mac80211. We could fix the monitor state machine
>> in ath10k but it doesn't make much sense. We don't
>> benefit from having the monitor vif create in the
>> first place so just drop it.
> 
> What warnings ?

Commands:

; ifconfig wlan1 up
; iw wlan1 connect -w dd-wrt-open
wlan1 (phy #20): connected to 68:7f:74:0a:ca:80
; iw wlan1 interface add mon type monitor
; ifconfig mon up
; ifconfig wlan1 down


Dmesg:

[ 1989.477975] ath10k: Only one monitor interface allowed
[ 1989.478016] ath10k: ath10k_htc_notify_tx_completion: ep 2 skb ffff880220e480c0
[ 1989.481481] ------------[ cut here ]------------
[ 1989.486963] WARNING: at net/mac80211/iface.c:386 ieee80211_add_virtual_monitor+0xe4/0x154 [mac80211]()
[ 1989.489749] Hardware name: Latitude E6420
[ 1989.494853] Modules linked in: ath10k_pci(O) ath10k_core(O) ath3k ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack xt_comment xt_tcpudp iptable_filter ip_tables x_tables bnep rfcomm ext2 btusb bluetooth snd_hda_codec_hdmi arc4 snd_hda_codec_idt snd_hda_intel snd_hda_codec snd_hwdep acpi_cpufreq iwldvm snd_pcm_oss snd_mixer_oss coretemp mac80211 joydev snd_pcm kvm_intel snd_seq_midi iwlwifi ath snd_rawmidi kvm cfg80211 snd_seq_midi_event snd_seq ppdev ehci_pci dell_wmi snd_timer snd_seq_device snd sparse_keymap ehci_hcd microcode soundcore usbcore psmouse serio_raw rfkill dell_laptop dcdbas usb_common lpc_ich mfd_core snd_page_alloc parport_pc evdev mperf battery ac processor lp parport loop ext4 mbcache jbd2 crc16 sha256_generic dm_crypt dm_mod sr_mod sd_mod crc_t10dif cdrom i915 drm_kms_helper drm i2c_algo_bit crc32c_intel ghash_clmulni_intel aesni_intel ablk_helper cryptd lrw aes_x86_64 xts gf128mul ahci libahci sdhci_pci e1000e wmi libata sdhci ptp button i2c
_core mmc_core scsi_mod thermal video pps_core thermal_sys [last unloaded: ath10k_core]
[ 1989.529489] Pid: 13722, comm: ifconfig Tainted: G        W  O 3.9.0-rc6-wl-ath10k+ #27
[ 1989.535284] Call Trace:
[ 1989.540865]  [<ffffffff810439f4>] warn_slowpath_common+0x7e/0x98
[ 1989.546691]  [<ffffffff81043a23>] warn_slowpath_null+0x15/0x17
[ 1989.552393]  [<ffffffffa066f364>] ieee80211_add_virtual_monitor+0xe4/0x154 [mac80211]
[ 1989.558234]  [<ffffffffa066fa70>] ieee80211_do_stop+0x545/0x554 [mac80211]
[ 1989.564011]  [<ffffffffa066fa94>] ieee80211_stop+0x15/0x19 [mac80211]
[ 1989.569579]  [<ffffffff8130e4a7>] __dev_close_many+0x97/0xbf
[ 1989.575240]  [<ffffffff8130e512>] __dev_close+0x43/0x62
[ 1989.580956]  [<ffffffff81313d6b>] __dev_change_flags+0xa6/0x129
[ 1989.586554]  [<ffffffff81313e5d>] dev_change_flags+0x19/0x4f
[ 1989.592177]  [<ffffffff8136e3ab>] devinet_ioctl+0x28d/0x58b
[ 1989.597712]  [<ffffffff8136eda0>] inet_ioctl+0x87/0xa2
[ 1989.603262]  [<ffffffff812fc18c>] sock_do_ioctl+0x22/0x41
[ 1989.608740]  [<ffffffff812fc619>] sock_ioctl+0x20e/0x21c
[ 1989.614138]  [<ffffffff8113bb77>] do_vfs_ioctl+0x465/0x4a6
[ 1989.619686]  [<ffffffff81143c55>] ? fcheck_files+0xa9/0xe7
[ 1989.625088]  [<ffffffff8113bc11>] sys_ioctl+0x59/0x80
[ 1989.630644]  [<ffffffff81206cae>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 1989.636089]  [<ffffffff813f4269>] system_call_fastpath+0x16/0x1b
[ 1989.641483] ---[ end trace 549c97fd542fd00f ]---


> I think this flag is required, mac80211 uses it to notify
> the driver to create an explicit monitor interface.

Depending on how we want to make it work.

The flag is only useful if we want to know if there are monitor interfaces active only.

Right now we can create the monitor vdev in two ways:
 * config() callback and CONF_CHANGE_MONITOR
 * add_interface() callback with TYPE_MONITOR

Really depends on what we want to support.

I just tested the following:

; iw wlan1 interface add mon type monitor
; ifconfig mon up
; tcpdump -ni mon &
; ifconfig wlan1 up
; iw wlan1 connect -w dd-wrt-open
wlan1 (phy #20): connected to 68:7f:74:0a:ca:80

Right after association FW dumps.

However if I first associate and then start monitor - no dumps.

FW dump also happens if I try to do packet injection in
real monitor mode.

Considering how "stable" monitor mode is perhaps we should start
monitor vdev only when there's the monitor vif present? (right now
we can start it without the vdev). If so then we drop this patch.
And we probably also should forbid packet injection, at least until
FW gets this sorted out.


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [PATCH 2/2] ath10k: don't advertise we want monitor vif

[Sujith Manoharan]

Michal Kazior wrote:
> [ 1989.477975] ath10k: Only one monitor interface allowed
> [ 1989.478016] ath10k: ath10k_htc_notify_tx_completion: ep 2 skb ffff880220e480c0
> [ 1989.481481] ------------[ cut here ]------------
> [ 1989.486963] WARNING: at net/mac80211/iface.c:386 ieee80211_add_virtual_monitor+0xe4/0x154 [mac80211]()

This is because of the limitation we enforce in the driver - that
only one monitor interface can be allowed. This should really be fixed
in the firmware since the limitation is quite artificial.

> ; iw wlan1 interface add mon type monitor
> ; ifconfig mon up
> ; tcpdump -ni mon &
> ; ifconfig wlan1 up
> ; iw wlan1 connect -w dd-wrt-open
> wlan1 (phy #20): connected to 68:7f:74:0a:ca:80
> 
> Right after association FW dumps.

I don't get a crash FW crash when I do this.
(Using a test firmware with the QoS fix).

> However if I first associate and then start monitor - no dumps.

Yes, no crash in this case too.

> FW dump also happens if I try to do packet injection in
> real monitor mode.

I see this crash, but let's not workaround it. I'll take a look
and see what's wrong.

I am not even sure if packet injection is supported currently with
the firmware.

> Considering how "stable" monitor mode is perhaps we should start
> monitor vdev only when there's the monitor vif present? (right now
> we can start it without the vdev). If so then we drop this patch.
> And we probably also should forbid packet injection, at least until
> FW gets this sorted out.

Well, the requirement is that we *need* a VDEV for monitor mode and
this is precisely this flag is for. So I don't quite see how removing it
fixes anything ?

Sujith

[ath9k-devel] [PATCH 2/2] ath10k: don't advertise we want monitor vif

[Kalle Valo]

Hi,

Sujith Manoharan <sujith@msujith.org> writes:

> Michal Kazior wrote:
>
>> Considering how "stable" monitor mode is perhaps we should start
>> monitor vdev only when there's the monitor vif present? (right now
>> we can start it without the vdev). If so then we drop this patch.
>> And we probably also should forbid packet injection, at least until
>> FW gets this sorted out.
>
> Well, the requirement is that we *need* a VDEV for monitor mode and
> this is precisely this flag is for. So I don't quite see how removing it
> fixes anything ?

So what's the conclusion? What should we do with this patchset?

-- 
Kalle Valo