[PATCH v2 13/31] timer: Remove meaningless .data/.function assignments

[Kees Cook]

Several timer users needlessly reset their .function/.data fields during
their timer callback, but nothing else changes them. Some users do not
use their .data field at all. Each instance is removed here.

Cc: Krzysztof Halasa 
Cc: Aditya Shankar 
Cc: Ganesh Krishna 
Cc: Greg Kroah-Hartman 
Cc: Jens Axboe 
Cc: net...@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: de...@driverdev.osuosl.org
Signed-off-by: Kees Cook 
Acked-by: Greg Kroah-Hartman  # for staging
Acked-by: Krzysztof Halasa  # for wan/hdlc*
Acked-by: Jens Axboe  # for amiflop
---
 drivers/block/amiflop.c   | 3 +--
 drivers/net/wan/hdlc_cisco.c  | 2 --
 drivers/net/wan/hdlc_fr.c | 2 --
 drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 4 +---
 4 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/block/amiflop.c b/drivers/block/amiflop.c
index c4b1cba27178..6680d75bc857 100644
--- a/drivers/block/amiflop.c
+++ b/drivers/block/amiflop.c
@@ -323,7 +323,7 @@ static void fd_deselect (int drive)
 
 }
 
-static void motor_on_callback(unsigned long nr)
+static void motor_on_callback(unsigned long ignored)
 {
if (!(ciaa.pra & DSKRDY) || --on_attempts == 0) {
complete_all(_on_completion);
@@ -344,7 +344,6 @@ static int fd_motor_on(int nr)
fd_select(nr);
 
reinit_completion(_on_completion);
-   motor_on_timer.data = nr;
mod_timer(_on_timer, jiffies + HZ/2);
 
on_attempts = 10;
diff --git a/drivers/net/wan/hdlc_cisco.c b/drivers/net/wan/hdlc_cisco.c
index c696d42f4502..6c98d85f2773 100644
--- a/drivers/net/wan/hdlc_cisco.c
+++ b/drivers/net/wan/hdlc_cisco.c
@@ -276,8 +276,6 @@ static void cisco_timer(unsigned long arg)
spin_unlock(>lock);
 
st->timer.expires = jiffies + st->settings.interval * HZ;
-   st->timer.function = cisco_timer;
-   st->timer.data = arg;
add_timer(>timer);
 }
 
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index de42faca076a..7da2424c28a4 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -644,8 +644,6 @@ static void fr_timer(unsigned long arg)
state(hdlc)->settings.t391 * HZ;
}
 
-   state(hdlc)->timer.function = fr_timer;
-   state(hdlc)->timer.data = arg;
add_timer((hdlc)->timer);
 }
 
diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c 
b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
index ac5aaafa461c..60f088babf27 100644
--- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
+++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
@@ -266,7 +266,7 @@ static void update_scan_time(void)
last_scanned_shadow[i].time_scan = jiffies;
 }
 
-static void remove_network_from_shadow(unsigned long arg)
+static void remove_network_from_shadow(unsigned long unused)
 {
unsigned long now = jiffies;
int i, j;
@@ -287,7 +287,6 @@ static void remove_network_from_shadow(unsigned long arg)
}
 
if (last_scanned_cnt != 0) {
-   hAgingTimer.data = arg;
mod_timer(, jiffies + msecs_to_jiffies(AGING_TIME));
}
 }
@@ -304,7 +303,6 @@ static int is_network_in_shadow(struct network_info 
*pstrNetworkInfo,
int i;
 
if (last_scanned_cnt == 0) {
-   hAgingTimer.data = (unsigned long)user_void;
mod_timer(, jiffies + msecs_to_jiffies(AGING_TIME));
state = -1;
} else {
-- 
2.7.4

Re: rtl8821ae keep alive not set, connection lost

[James Cameron]

On Wed, Sep 20, 2017 at 04:48:23PM -0500, Larry Finger wrote:
> On 09/20/2017 04:36 AM, James Cameron wrote:
> >When the problem occurs, register 0x350 bit 25 is set, for which a
> >comment in _rtl8821ae_check_pcie_dma_hang says means there is an RX
> >hang.
> >
> >So perhaps driver should call _rtl8821ae_check_pcie_dma_hang
> >and _rtl8821ae_reset_pcie_interface_dma.
> >
> >Any ideas where to do this?
> 
> Thanks for the extended debugging.
> 
> I was able to repeat your findings. With the 8-bit read of
> REG_DBI_RDATA, I got poor connection stability. Reverting that part
> made it stable again. For that reason, I pushed the partial
> reversion of commit 40b368af4b75 ("rtlwifi: Fix alignment issues").

That's great you were able to reproduce, thanks!

> Where did you detect that bit 25 of register 0x350 was set?

In _rtl8821ae_check_pcie_dma_hang on link up.

REG_DBI_FLAG (0x350 bits 16-31) is observed as;

- 0x on entry to function after warm boot,

- 0x0400 on exit from function; debug bit 23 is set by the function,

- 0x0400 on entry to function on link up when the problem has not
  happened,

- 0x0600 on entry to function on link up when the problem has
  happened.

But I don't know if 0x0600 is useful to detect earlier, or if it is
only a symptom of link down while device active.  Either way, if it
truly does signal an RX hang or firmware RX queue full, it's useful.

My "-q9" and "-qa" test kernels dump REG_DBI_CTRL and REG_DBI_FLAG.

"-q9" is with 8-bit read of REG_DBI_RDATA.

"-qa" is with 16-bit read of REG_DBI_DATA.

My "-qa" test kernel;
http://dev.laptop.org/~quozl/y/1dunwN.txt (git diff v4.13)
http://dev.laptop.org/~quozl/z/1dubX7.txt (dmesg)

REG_DBI_CTRL+3 used by _rtl8821ae_check_pcie_dma_hang is effectively
REG_DBI_FLAG+1 (0x353).

REG_DBI_CTRL is REG_DBI_ADDR; a duplicate register definition.

I'm still pondering a few more theories;

- change write_readback, it is true now, and the while()/udelay in
  _rtl8821ae_dbi_read seems a waste, it never executes,

- clearing REG_DBI_CTRL write enable bits at the end of
  _rtl8821ae_dbi_write,

- switching to 32-bit access as used by rtl8192de.

And a giggle from reviewing the code,
_rtl8821ae_wowlan_initialize_adapter says "Patch Pcie Rx DMA hang
after S3/S4 several times.  The root cause has not be found."
... I've learned that root causes that aren't found tend to cause
further problems later.  ;-)

Given this, my gut feel is firmware or silicon problem; RX DMA ceases,
the driver does not detect it, the connection is lost.

-- 
James Cameron
http://quozl.netrek.org/

Re: rtl8821ae keep alive not set, connection lost

[Larry Finger]

On 09/20/2017 04:36 AM, James Cameron wrote:

When the problem occurs, register 0x350 bit 25 is set, for which a
comment in _rtl8821ae_check_pcie_dma_hang says means there is an RX
hang.

So perhaps driver should call _rtl8821ae_check_pcie_dma_hang
and _rtl8821ae_reset_pcie_interface_dma.

Any ideas where to do this?


Thanks for the extended debugging.

I was able to repeat your findings. With the 8-bit read of REG_DBI_RDATA, I got 
poor connection stability. Reverting that part made it stable again. For that 
reason, I pushed the partial reversion of commit 40b368af4b75 ("rtlwifi: Fix 
alignment issues").


Where did you detect that bit 25 of register 0x350 was set?

Larry

[PATCH] rtlwifi: rtl8821ae: Fix connection lost problem

[Larry Finger]

In commit 40b368af4b75 ("rtlwifi: Fix alignment issues"), the read
of REG_DBI_READ was changed from 16 to 8 bits. For unknown reasonsi
this change results in reduced stability for the wireless connection.
This regression was located using bisection.

Fixes: 40b368af4b75 ("rtlwifi: Fix alignment issues")
Reported-and-tested-by: James Cameron 
Signed-off-by: Larry Finger 
Cc: Stable  # 4.11+
Cc: Ping-Ke Shih 
---
 drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 3571ce4bd276..ac2ce86de506 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -1122,7 +1122,7 @@ static u8 _rtl8821ae_dbi_read(struct rtl_priv *rtlpriv, 
u16 addr)
}
if (0 == tmp) {
read_addr = REG_DBI_RDATA + addr % 4;
-   ret = rtl_read_byte(rtlpriv, read_addr);
+   ret = rtl_read_word(rtlpriv, read_addr);
}
return ret;
 }
-- 
2.12.3

Re: usb/net/p54: trying to register non-static key in p54_unregister_leds

[Johannes Berg]

On Wed, 2017-09-20 at 21:27 +0200, Christian Lamparter wrote:

> It seems this is caused as a result of:
> -> lock_map_acquire(>lockdep_map);
>   lock_map_release(>lockdep_map);
> 
> in flush_work() [0]

Agree.

> This was added by:
> 
>   commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
>   Author: Stephen Boyd 
>   Date:   Fri Apr 20 17:28:50 2012 -0700
> 
>   workqueue: Catch more locking problems with flush_work()

Yes, but that doesn't matter.

> Looking at the Stephen's patch, it's clear that it was made
> with "static DECLARE_WORK(work, my_work)" in mind. However
> p54's led_work is "per-device", hence it is stored in the
> devices context p54_common, which is dynamically allocated.
> So, maybe revert Stephen's patch?

I disagree - as the lockdep warning says:

> > INFO: trying to register non-static key.
> > the code is fine but needs lockdep annotation.
> > turning off the locking correctness validator.

What it needs is to actually correctly go through initializing the work
at least once.

Without more information, I can't really say what's going on, but I
assume that something is failing and p54_unregister_leds() is getting
invoked without p54_init_leds() having been invoked, so essentially
it's trying to flush a work that was never initialized?

INIT_DELAYED_WORK() does, after all, initialize the lockdep map
properly via __INIT_WORK().

johannes

AP6335 with mainline kernel

[Vanessa Ayumi Maegima]

Hi,

I am trying to enable Wifi on imx7d-pico using mainline kernel. imx7d-pico
has an AP6335 chip.

I am facing some issues related to the nvram file. I am using the firmware
provided by Buildroot (brcmfmac4339-sdio.bin). I get the following error:

[8.630380] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (100):
clkctl 0x50

I have tried to use the firmware and nvram provided by TechNexion but I get
the same error.

Is there anyone that could enable Wifi on AP6335 using kernel mainline?
What nvram file was used?

I am able to use Wifi on the board if I use the firmware, nvram file and kernel
provided by TechNexion. They use a 4.1 kernel from NXP with the bcmdhd
driver.

So I know that the hardware is functional.

Any suggestions as how to get it working with a 4.13 and brcmfmac driver is
appreciated.

Thanks!

Regards,
Vanessa

Re: usb/net/p54: trying to register non-static key in p54_unregister_leds

[Christian Lamparter]

On Wednesday, September 20, 2017 8:37:08 PM CEST Andrey Konovalov wrote:
> Hi!
> 
> I've got the following report while fuzzing the kernel with syzkaller.
> 
> On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18).
> 
> INFO: trying to register non-static key.
> the code is fine but needs lockdep annotation.
> turning off the locking correctness validator.
> CPU: 1 PID: 1404 Comm: kworker/1:1 Not tainted
> 4.14.0-rc1-42251-gebb2c2437d80-dirty #205
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: usb_hub_wq hub_event
> Call Trace:
>  __dump_stack lib/dump_stack.c:16
>  dump_stack+0x292/0x395 lib/dump_stack.c:52
>  register_lock_class+0x6c4/0x1a00 kernel/locking/lockdep.c:769
>  __lock_acquire+0x27e/0x4550 kernel/locking/lockdep.c:3385
>  lock_acquire+0x259/0x620 kernel/locking/lockdep.c:4002
>  flush_work+0xf0/0x8c0 kernel/workqueue.c:2886
>  __cancel_work_timer+0x51d/0x870 kernel/workqueue.c:2961
>  cancel_delayed_work_sync+0x1f/0x30 kernel/workqueue.c:3081
>  p54_unregister_leds+0x6c/0xc0 drivers/net/wireless/intersil/p54/led.c:160
>  p54_unregister_common+0x3d/0xb0 drivers/net/wireless/intersil/p54/main.c:856
>  p54u_disconnect+0x86/0x120 drivers/net/wireless/intersil/p54/p54usb.c:1073
>  usb_unbind_interface+0x21c/0xa90 drivers/usb/core/driver.c:423
>  __device_release_driver drivers/base/dd.c:861
>  device_release_driver_internal+0x4f4/0x5c0 drivers/base/dd.c:893
>  device_release_driver+0x1e/0x30 drivers/base/dd.c:918
>  bus_remove_device+0x2f4/0x4b0 drivers/base/bus.c:565
>  device_del+0x5c4/0xab0 drivers/base/core.c:1985
>  usb_disable_device+0x1e9/0x680 drivers/usb/core/message.c:1170
>  usb_disconnect+0x260/0x7a0 drivers/usb/core/hub.c:2124
>  hub_port_connect drivers/usb/core/hub.c:4754
>  hub_port_connect_change drivers/usb/core/hub.c:5009
>  port_event drivers/usb/core/hub.c:5115
>  hub_event+0x1318/0x3740 drivers/usb/core/hub.c:5195
>  process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
>  process_scheduled_works kernel/workqueue.c:2179
>  worker_thread+0xb2b/0x1850 kernel/workqueue.c:2255
>  kthread+0x3a1/0x470 kernel/kthread.c:231
>  ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
> 

It seems this is caused as a result of:
-> lock_map_acquire(>lockdep_map);
lock_map_release(>lockdep_map);

in flush_work() [0]

This was added by:

commit 0976dfc1d0cd80a4e9dfaf87bd8744612bde475a
Author: Stephen Boyd 
Date:   Fri Apr 20 17:28:50 2012 -0700

workqueue: Catch more locking problems with flush_work()

Looking at the Stephen's patch, it's clear that it was made
with "static DECLARE_WORK(work, my_work)" in mind. However
p54's led_work is "per-device", hence it is stored in the
devices context p54_common, which is dynamically allocated.
So, maybe revert Stephen's patch?

[0] 

usb/net/p54: trying to register non-static key in p54_unregister_leds

[Andrey Konovalov]

Hi!

I've got the following report while fuzzing the kernel with syzkaller.

On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18).

INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
CPU: 1 PID: 1404 Comm: kworker/1:1 Not tainted
4.14.0-rc1-42251-gebb2c2437d80-dirty #205
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
 __dump_stack lib/dump_stack.c:16
 dump_stack+0x292/0x395 lib/dump_stack.c:52
 register_lock_class+0x6c4/0x1a00 kernel/locking/lockdep.c:769
 __lock_acquire+0x27e/0x4550 kernel/locking/lockdep.c:3385
 lock_acquire+0x259/0x620 kernel/locking/lockdep.c:4002
 flush_work+0xf0/0x8c0 kernel/workqueue.c:2886
 __cancel_work_timer+0x51d/0x870 kernel/workqueue.c:2961
 cancel_delayed_work_sync+0x1f/0x30 kernel/workqueue.c:3081
 p54_unregister_leds+0x6c/0xc0 drivers/net/wireless/intersil/p54/led.c:160
 p54_unregister_common+0x3d/0xb0 drivers/net/wireless/intersil/p54/main.c:856
 p54u_disconnect+0x86/0x120 drivers/net/wireless/intersil/p54/p54usb.c:1073
 usb_unbind_interface+0x21c/0xa90 drivers/usb/core/driver.c:423
 __device_release_driver drivers/base/dd.c:861
 device_release_driver_internal+0x4f4/0x5c0 drivers/base/dd.c:893
 device_release_driver+0x1e/0x30 drivers/base/dd.c:918
 bus_remove_device+0x2f4/0x4b0 drivers/base/bus.c:565
 device_del+0x5c4/0xab0 drivers/base/core.c:1985
 usb_disable_device+0x1e9/0x680 drivers/usb/core/message.c:1170
 usb_disconnect+0x260/0x7a0 drivers/usb/core/hub.c:2124
 hub_port_connect drivers/usb/core/hub.c:4754
 hub_port_connect_change drivers/usb/core/hub.c:5009
 port_event drivers/usb/core/hub.c:5115
 hub_event+0x1318/0x3740 drivers/usb/core/hub.c:5195
 process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
 process_scheduled_works kernel/workqueue.c:2179
 worker_thread+0xb2b/0x1850 kernel/workqueue.c:2255
 kthread+0x3a1/0x470 kernel/kthread.c:231
 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431

Re: mac80211: avoid allocating TXQs that won't be used

[Johannes Berg]

On Wed, 2017-09-20 at 17:08 +0100, Colin Ian King wrote:
> Johannes,
> 
> Static analysis with CoverityScan on linux-next today detected a null
> pointer dereference issue on commit:
> 
> From 0fc4b3403d215ecd3c05505ec1f0028a227ed319 Mon Sep 17 00:00:00
> 2001
> From: Johannes Berg 
> Date: Thu, 22 Jun 2017 12:20:29 +0200
> Subject: [PATCH] mac80211: avoid allocating TXQs that won't be used
> 
> Issue: sdata is null when the sdata is dereferenced by:
> 
>    sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
>    sdata->vif.type != NL80211_IFTYPE_MONITOR)
> 
> note that sdata is assigned a non-null much later with the statement
> sdata = netdev_priv(ndev).

Yeah, umm, that should be checking just 'type'. Thanks, will fix.

johannes

re: mac80211: avoid allocating TXQs that won't be used

[Colin Ian King]

Johannes,

Static analysis with CoverityScan on linux-next today detected a null
pointer dereference issue on commit:

>From 0fc4b3403d215ecd3c05505ec1f0028a227ed319 Mon Sep 17 00:00:00 2001
From: Johannes Berg 
Date: Thu, 22 Jun 2017 12:20:29 +0200
Subject: [PATCH] mac80211: avoid allocating TXQs that won't be used

Issue: sdata is null when the sdata is dereferenced by:

   sdata->vif.type != NL80211_IFTYPE_AP_VLAN &&
   sdata->vif.type != NL80211_IFTYPE_MONITOR)

note that sdata is assigned a non-null much later with the statement
sdata = netdev_priv(ndev).

Detected by CoverityScan CID#1456974 ("Explicit null dereferenced")

Colin

Re: [V2, 1/9] qtnfmac: qlink: convert channel width from bitfiled to simple enum

[Sergey Matyukevich]

Hello Kalle,

> BTW, I got a mail undelivered message. Not sure why smtp.codeaurora.org
> could not find the host but reporting it here just in case:
> 
> : unable to look up host
> quantenna-com.mail.protection.outlook.com: Name or service not known
> 
> : unable to look up host
> quantenna-com.mail.protection.outlook.com: Name or service not known
> 
> : unable to look up host
> quantenna-com.mail.protection.outlook.com: Name or service not known

Thanks for letting us know. I see those emails im my box, but probably
because of mailing list subscription. I will check with colleagues
if they see your replies as well.

Regards,
Sergey

Re: [2/2] mwifiex: use get_random_mask_addr() helper

[Kalle Valo]

Ganapathi Bhat  wrote:

> Avoid calculating random MAC address in driver. Instead make
> use of 'get_random_mask_addr()' function.
> 
> Signed-off-by: Ganapathi Bhat 
> Reviewed-by: Brian Norris 

Patch applied to wireless-drivers-next.git, thanks.

e9a3846afaa4 mwifiex: use get_random_mask_addr() helper

-- 
https://patchwork.kernel.org/patch/9955631/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: mwifiex: avoid storing random_mac in private

[Kalle Valo]

Ganapathi Bhat  wrote:

> Application will keep track of whether MAC address randomization
> is enabled or not during scan. But at present driver is storing
> 'random_mac' in mwifiex_private which implies even after scan is
> done driver has some reference to the earlier 'scan request'. To
> avoid this, make use of 'mac_addr' variable in 'scan_request' to
> store 'random_mac'. This structure will be freed by cfg80211 once
> scan is done.
> 
> Signed-off-by: Ganapathi Bhat 
> Reviewed-by: Brian Norris 

Patch applied to wireless-drivers-next.git, thanks.

e251a882c0ba mwifiex: avoid storing random_mac in private

-- 
https://patchwork.kernel.org/patch/9955573/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: mwifiex: make const arrays static to shink object code size

[Kalle Valo]

Colin Ian King  wrote:

> From: Colin Ian King 
> 
> Don't populate const arrays on the stack, instead make them static
> Makes the object code smaller by nearly 300 bytes:
> 
> Before:
>text  data bss dec hex filename
>   69260 16149 576   85985   14fe1 cfg80211.o
> 
> After:
>text  data bss dec hex filename
>   68385 16725 576   85686   14eb6 cfg80211.o
> 
> Signed-off-by: Colin Ian King 

Patch applied to wireless-drivers-next.git, thanks.

d157bcfaf854 mwifiex: make const arrays static to shink object code size

-- 
https://patchwork.kernel.org/patch/9954375/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: mwifiex: remove unnecessary call to memset

[Kalle Valo]

Himanshu Jha  wrote:

> call to memset to assign 0 value immediately after allocating
> memory with kzalloc is unnecesaary as kzalloc allocates the memory
> filled with 0 value.
> 
> Semantic patch used to resolve this issue:
> 
> @@
> expression e,e2; constant c;
> statement S;
> @@
> 
>   e = kzalloc(e2, c);
>   if(e == NULL) S
> - memset(e, 0, e2);
> 
> Signed-off-by: Himanshu Jha 

Patch applied to wireless-drivers-next.git, thanks.

85dafc129196 mwifiex: remove unnecessary call to memset

-- 
https://patchwork.kernel.org/patch/9947331/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: mwifiex: check for mfg_mode in add_virtual_intf

[Kalle Valo]

Ganapathi Bhat  wrote:

> If driver is loaded with 'mfg_mode' enabled, then the sending
> commands are not allowed. So, skip sending commands, to firmware
> in mwifiex_add_virtual_intf if 'mfg_mode' is enabled.
> 
> Fixes: 7311ea850079 ("mwifiex: fix AP start problem for newly added 
> interface")
> Signed-off-by: Ganapathi Bhat 
> Reviewed-by: Brian Norris 

Patch applied to wireless-drivers-next.git, thanks.

26177d7f3969 mwifiex: check for mfg_mode in add_virtual_intf

-- 
https://patchwork.kernel.org/patch/9942827/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: mwifiex: notify cfg80211 about scan abort

[Kalle Valo]

Ganapathi Bhat  wrote:

> Driver sends a series of scan commands to firmware to serve a
> user scan request. If an intermediate scan command fails, driver
> aborts the scan but it is not being informed to cfg80211. This
> will cause issues in applications performing periodic scans.
> Fix this by informing scan abort.
> 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 

Patch applied to wireless-drivers-next.git, thanks.

31726ff20190 mwifiex: notify cfg80211 about scan abort

-- 
https://patchwork.kernel.org/patch/9930689/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: rtlwifi: rtl8192ee: Fix memory leak when loading firmware

[Kalle Valo]

Larry Finger  wrote:

> In routine rtl92ee_set_fw_rsvdpagepkt(), the driver allocates an skb, but
> never calls rtl_cmd_send_packet(), which will free the buffer. All other
> rtlwifi drivers perform this operation correctly.
> 
> This problem has been in the driver since it was included in the kernel.
> Fortunately, each firmware load only leaks 4 buffers, which likely
> explains why it has not previously been detected.
> 
> Cc: Stable  # 3.18+
> Signed-off-by: Larry Finger 

Patch applied to wireless-drivers-next.git, thanks.

519ce2f933fa rtlwifi: rtl8192ee: Fix memory leak when loading firmware

-- 
https://patchwork.kernel.org/patch/9953677/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: rtlwifi: btcoexist: 23b 1ant: fix duplicated code for different branches

[Kalle Valo]

Larry Finger  wrote:

> A typo led to this issue, which was detected with the help of Coccinelle.
> 
> In addition to fixing the error, the code is refactored to eliminate an
> if statement.
> 
> Addresses-Coverity-ID: 1226788
> 
> Reported-by: Gustavo A. R. Silva 
> Signed-off-by: Larry Finger 
> Cc: Ping-Ke Shih 
> Cc: Yan-Hsuan Chuang 
> Cc: Birming Chiu 
> Cc: Shaofu 
> Cc: Steven Ting 

Patch applied to wireless-drivers-next.git, thanks.

0f61953dd0f5 rtlwifi: btcoexist: 23b 1ant: fix duplicated code for different 
branches

-- 
https://patchwork.kernel.org/patch/9932349/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [1/2] b43: fix unitialized reads of ret by initializing the array to zero

[Kalle Valo]

Colin Ian King  wrote:

> From: Colin Ian King 
> 
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values.  Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
> 
> Detected by CoverityScan CID#139652 ("Uninitialized scalar variable")
> 
> Signed-off-by: Colin Ian King 
> Reviewed-by: Michael Buesch 

2 patches applied to wireless-drivers-next.git, thanks.

e31fbe1034d9 b43: fix unitialized reads of ret by initializing the array to zero
e3ae1c772046 b43legacy: fix unitialized reads of ret by initializing the array 
to zero

-- 
https://patchwork.kernel.org/patch/9939435/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: rsi: fix a dereference on adapter before it has been null checked

[Kalle Valo]

Colin Ian King  wrote:

> From: Colin Ian King 
> 
> The assignment of dev is dereferencing adapter before adapter has
> been null checked, potentially leading to a null pointer dereference.
> Fix this by simply moving the assignment of dev to a later point
> after the sanity null check of adapter.
> 
> Detected by CoverityScan CID#1398383 ("Dereference before null check")
> 
> Fixes: dad0d04fa7ba ("rsi: Add RS9113 wireless driver")
> Signed-off-by: Colin Ian King 

Patch applied to wireless-drivers-next.git, thanks.

6508497cbdc7 rsi: fix a dereference on adapter before it has been null checked

-- 
https://patchwork.kernel.org/patch/9944509/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [1/8] rsi: add p2p support parameters to mac80211

[Kalle Valo]

Amitkumar Karwar  wrote:

> From: Prameela Rani Garnepudi 
> 
> This patch adds p2p supported parameters to mac80211 hw and
> wiphy structures during mac80211 registration.
> 
> Signed-off-by: Prameela Rani Garnepudi 
> Signed-off-by: Amitkumar Karwar 

8 patches applied to wireless-drivers-next.git, thanks.

421eedff1180 rsi: add p2p support parameters to mac80211
b8bd3a439f35 rsi: add/remove interface enhancements for p2p
df771911914a rsi: add support for p2p listen
4671c209ac46 rsi: handle peer connection and disconnection in p2p mode
eac4eed3224b rsi: tx and rx path enhancements for p2p mode
efe877aa0f40 rsi: disallow power save config when AP vap running
c7245c0975f1 rsi: aggregation changes for p2p mode
af75687286bf rsi: miscellaneous changes for p2p mode

-- 
https://patchwork.kernel.org/patch/9929129/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [v2,1/2] qtnfmac: lock access to h/w in tx path

[Kalle Valo]

Sergey Matyukevich  wrote:

> Fix tx path regression. Lock should be held when queuing packets
> to h/w fifos in order to properly handle configurations with
> multiple enabled interfaces.
> 
> Signed-off-by: Sergey Matyukevich 

2 patches applied to wireless-drivers.git, thanks.

20da2ec06bfa qtnfmac: lock access to h/w in tx path
a715b3a0efe7 qtnfmac: cancel scans on wireless interface changes

-- 
https://patchwork.kernel.org/patch/9956607/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [V2, 1/9] qtnfmac: qlink: convert channel width from bitfiled to simple enum

[Kalle Valo]

Kalle Valo  writes:

> Igor Mitsyanko  wrote:
>
>> From: Igor Mitsyanko 
>> 
>> Signed-off-by: Igor Mitsyanko 
>> Reviewed-by: Sergey Matyukevich 
>
> No empty commit logs, please. After the first patch I was about to write
> one myself but after seeing multiple of them I'm not going to do that.
>
> 9 patches set to Changes Requested.

BTW, I got a mail undelivered message. Not sure why smtp.codeaurora.org
could not find the host but reporting it here just in case:

: unable to look up host
quantenna-com.mail.protection.outlook.com: Name or service not known

: unable to look up host
quantenna-com.mail.protection.outlook.com: Name or service not known

: unable to look up host
quantenna-com.mail.protection.outlook.com: Name or service not known

-- 
Kalle Valo

Re: [V2, 1/9] qtnfmac: qlink: convert channel width from bitfiled to simple enum

[Kalle Valo]

Igor Mitsyanko  wrote:

> From: Igor Mitsyanko 
> 
> Signed-off-by: Igor Mitsyanko 
> Reviewed-by: Sergey Matyukevich 

No empty commit logs, please. After the first patch I was about to write
one myself but after seeing multiple of them I'm not going to do that.

9 patches set to Changes Requested.

9935431 [V2,1/9] qtnfmac: qlink: convert channel width from bitfiled to simple 
enum
9935437 [V2,2/9] qtnfmac: make "Channel change" event report full channel info
9935443 [V2,3/9] qtnfmac: retrieve current channel info from EP
9935445 [V2,4/9] qtnfmac: do not cache channel info from "connect" command
9935429 [V2,5/9] qtnfmac: let wifi card handle channel switch request to the 
same chan
9935441 [V2,6/9] qtnfmac: pass VIF info to SendChannel command
9935433 [V2,7/9] qtnfmac: do not cache CSA chandef info
9935439 [V2,8/9] qtnfmac: remove unused mac::status field
9935435 [V2,9/9] qtnfmac: do not report channel changes until wiphy is 
registered

-- 
https://patchwork.kernel.org/patch/9935431/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections

[Johannes Berg]

On Wed, 2017-09-20 at 15:11 +0300, Ville Syrjälä wrote:
> 
> > I guess since the outer pointer isn't protected, only the inner ...
> 
> I think just the fact that even the pointers in ieee80211_tx_data
> don't have the __rcu annotation makes it rather hard to see what is
> really rcu protected and what isn't. If every user of those pointers
> would have to do the rcu_dereference() things would be rather more
> explicit.

It wouldn't make sense though, because those users don't need to
provide the protection, and they don't need to make sure to use the
pointer in an RCU safe manner (access once etc.) since they're in code
that can't really go wrong... mostly.

> > Therefore, this patch is wrong.
> 
> OK, so the problem is in ath9k then.

I agree.

> > I actually think the same is true for ieee80211_tx_dequeue(), but 
[...]
> Well, I think this is as far as I want to dig into the matter. I can
> respin the patch once more with just tx_dequeue() fix in there, if
> you want (not sure you do if you think it's wrong as well). After
> that I'll leave it to someone who actually knows what they're doing
> with mac80211 ;)

:-)
I think we should rather document that RCU is required for that
function, I think for some usages it may be OK without but with keys
I'm pretty sure you'll need it.

johannes

Re: [PATCH 1/2] mwifiex: resubmit failed to submit RX URBs in main thread

[Kalle Valo]

Ganapathi Bhat  writes:

> From: James Cao 
>
> Current driver has 6 Rx data URBs. Once any packet received
> kernel calls our callback, in which the same URB will be
> resubmitted after Rx indication. In URB submission function a new
> skb will be allocated since the previous one is passed to upper
> layer (freed later). Since the skb is from a special pool (not
> regular memory), skb allocation may fail when kernel holds a lot
> of Rx packets on some low resource platforms.

The special pool being GFP_ATOMIC allocations or what?

> The URB will not be resubmitted in this no free skb case. If driver
> fails to resubmit all 6 URBs, Rx will stop. To cover this scenario
> check and resubmit Rx URBs in main thread.
>
> Signed-off-by: James Cao 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 

[...]

> @@ -278,6 +279,16 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
>   break;
>   }
>  
> + /* Try to resubmit RX URB if sunmission failed earlier */
> + if (!atomic_read(>rx_pending) &&
> + adapter->iface_type == MWIFIEX_USB) {
> + usb_card = adapter->card;
> + if (atomic_read(_card->rx_data_urb_pending) <
> + MWIFIEX_RX_DATA_URB &&
> + adapter->if_ops.submit_rem_rx_urbs)
> + adapter->if_ops.submit_rem_rx_urbs(adapter);
> + }

To me this just feels wrong. Normally the proceduce is to drop the frame
if allocations fail, not try to reallocate. I need more convincing that
this really is the right approach.

-- 
Kalle Valo

Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections

[Ville Syrjälä]

On Wed, Sep 20, 2017 at 12:39:24PM +0200, Johannes Berg wrote:
> On Wed, 2017-09-20 at 13:11 +0300, Ville Syrjala wrote:
> 
> > --- a/net/mac80211/tx.c
> > +++ b/net/mac80211/tx.c
> > @@ -1770,15 +1770,21 @@ bool ieee80211_tx_prepare_skb(struct ieee80211_hw 
> > *hw,
> >     struct ieee80211_tx_data tx;
> >     struct sk_buff *skb2;
> >  
> > -   if (ieee80211_tx_prepare(sdata, , NULL, skb) == TX_DROP)
> > +   rcu_read_lock();
> 
> The documentation says:
> 
> /**
>  * ieee80211_tx_prepare_skb - prepare an 802.11 skb for transmission
>  * @hw: pointer as obtained from ieee80211_alloc_hw()
>  * @vif: virtual interface
>  * @skb: frame to be sent from within the driver
>  * @band: the band to transmit on
>  * @sta: optional pointer to get the station to send the frame to
>  *
>  * Note: must be called under RCU lock
>  */
> 
> You can't even argue that it should be the function itself doing it,
> because the (admittedly optional) sta pointer would otherwise not have
> proper protection after you leave the function ... You can't pass out a
> sta pointer that's RCU protected.

Yeah, I suppose that would need rcu_handoff+some other mechanism to
make sure it stays around after that.

> 
> Side note: Perhaps some annotation should be there? not sure it's
> possible - would have to be something like
>   struct ieee80211_sta * __rcu *sta;
> 
> I guess since the outer pointer isn't protected, only the inner ...

I think just the fact that even the pointers in ieee80211_tx_data don't
have the __rcu annotation makes it rather hard to see what is really rcu
protected and what isn't. If every user of those pointers would have to
do the rcu_dereference() things would be rather more explicit.

> Therefore, this patch is wrong.

OK, so the problem is in ath9k then.

> I actually think the same is true for ieee80211_tx_dequeue(), but I'm
> less sure about it - the sta pointer there clearly is somehow safely
> passed in (even if it's w/o RCU, the driver can potentially make that
> safe), but the key pointer seems unsafe in this case (as well) if
> there's no outer RCU protection.

Well, I think this is as far as I want to dig into the matter. I can
respin the patch once more with just tx_dequeue() fix in there, if you
want (not sure you do if you think it's wrong as well). After that I'll
leave it to someone who actually knows what they're doing with mac80211 ;)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] mwifiex: print URB submit failure error after threshold attemtps

[Kalle Valo]

Brian Norris  writes:

> On Wed, Sep 20, 2017 at 07:30:29AM +0300, Kalle Valo wrote:
>> Brian Norris  writes:
>> 
>> > Hi Ganapathi,
>> >
>> > On Thu, Sep 14, 2017 at 02:14:24PM +, Ganapathi Bhat wrote:
>> >> > On Thu, 2017-08-31 at 01:21 +0530, Ganapathi Bhat wrote:
>
>> >> > Why not use a ratelimit?
>> >> Since this is for receive, the packets are from AP side and we cannot
>> >> lower the rate from AP. On some low performance systems this change
>> >> will be helpful.
>> >
>> > I think Joe was referring to things like printk_ratelimited() or
>> > dev_err_ratelimited(). Those automatically ratelimit prints for you,
>> > using a static counter. You'd just need to make a small warpper for
>> > mwifiex_dbg() using __ratelimit().
>> >
>> > Those sort of rate limits are significantly different than yours though.
>> > You were looking to avoid printing errors when there are only a few
>> > failures in a row, whereas the existing rate-limiting infrastructure
>> > looks to avoid printing errors if too many happen in a row. Those are
>> > different goals.
>> 
>> Are you saying that this patch is good to take? Or should Ganapathi
>> submit v2?
>
> If you're asking me...

Yeah, I was asking you because to me this patch looks like an ugly
workaround to a bug. And now that looked patch 1 more closely it feels
the same.

> All I was saying was that I don't think Joe's suggestion will help
> Ganapathi. I'd expect Ganapathi could confirm/deny that part. (Or Joe
> could correct me if my interpretation is wrong.)

Ok.

> I'm also not familiar with how we expect dev_alloc_skb() failures to be
> handled. If that's a common expected failure mode in low-memory
> situations (seems reasonable?) and the driver handles these failure just
> fine (completely unreviewed by me, so far; I suspect it doesn't do this
> completely correctly), then sure, being less noisy about it as done in
> this patch should be fine.

But this is a debug message so it should not bother normal users, right?
I think that having a threshold like this is just hiding problems and
not solving them. The real issue here is that dev_alloc_skb() is failing
and that's what should be solved, not to paper it over by limiting debug
messages. It just means that the real issue will be even more difficult
to detect in the future.

> IOW, I don't have concrete feedback for Ganapathi to address, but I'm
> not exactly "ack"ing it myself.

I'm not very confident about this patch either, it's not just making any
sense.

-- 
Kalle Valo

Re: [PATCH 2/2] mwifiex: use get_random_mask_addr() helper

[Kalle Valo]

Brian Norris  writes:

> Hi,
>
> On Tue, Sep 19, 2017 at 05:30:06PM +0300, Kalle Valo wrote:
>> Ganapathi Bhat  writes:
>> 
>> > Hi Kalle,
>> >> 
>> >> > Avoid calculating random MAC address in driver. Instead make use of
>> >> > 'get_random_mask_addr()' function.
>> >> >
>> >> > Signed-off-by: Ganapathi Bhat 
>> >> 
>> >> I don't see 1/2 anywhere. Did it get lost?
>> >
>> > Actually there is no 1/2. What I did is: 'git send-email'; CTRL + C
>
> It's dependent on this patch though, which kinda should be '1/2':
>
> [PATCH] mwifiex: avoid storing random_mac in private

Thanks for pointing out, I'll make sure that I commit these in correct
order.

>> > (to correct a typo); and then tried sending it again. I think that
>> > created some problem here. Kindly let me know how to proceed.
>> 
>> Ok. I'll wait for review comments and if all goes well I'll apply it in
>> few days.
>
> FWIW, this looks OK to me:
>
> Reviewed-by: Brian Norris 
>
> It's just a bit strange that we have to keep our own on-stack temporary
> buffer for this. Maybe this could use an in-place helper too? Or (if
> it's really legal for us to modify the cfg80211_scan_request in-place)
> why doesn't the upper-layer nl80211 code do the randomization for us?
> Many (all?) drivers I see implementing randomization have to do this
> anyway; they don't use request->mac_addr directly. (Or I suppose some
> firmware could implement the randomization on its own someday...but
> would we really trust it?)

Good questions and I was wondering the same when looking at this patch.
But I wasn't involved in the interface design so I don't really know the
background here.

I'm planning to apply this patch anyway, any improvements can be done as
a followup patch.

-- 
Kalle Valo

Re: [PATCH] mwifiex: add device specific ioctl handler

[Kalle Valo]

Xinming Hu  writes:

>> You could look at
>> nl80211 vendor commands, but that is also under scrutiny if common wifi
>> functionality is provided by it. I am pretty sure Kalle has his ideas about 
>> this :-)
>> 
>
> Yes, the new utility should use nl80211 interface.
> But we have lots of legacy utility based on ioctl, it will be helpful
> to reduce massive work on refactoring.

That's not an excuse to implement bad interfaces. It's not really that
hard to convert ioctl based tools to use nl80211 testmode command, I
even have myself done that.

-- 
Kalle Valo

Re: [PATCH] mwifiex: add device specific ioctl handler

[Kalle Valo]

Arend van Spriel  writes:

> On 9/20/2017 8:44 AM, Xinming Hu wrote:
>> From: Xinming Hu 
>>
>> Add net_dev ndo_do_ioctl handler, which could be used for
>> downloading host command by utility in manufactory test
>
> Not sure if we want ioctls in upstream wireless drivers.

Yeah, patches adding ioctls to wireless drivers are automatically
rejected.

> You could look at nl80211 vendor commands, but that is also under
> scrutiny if common wifi functionality is provided by it. I am pretty
> sure Kalle has his ideas about this :-)

For manufacturing tests we have nl80211 testmode command and event.
There are upstream drivers already using it so you even have examples.

-- 
Kalle Valo

Re: [PATCH v2 1/2] mac80211: Add rcu read side critical sections

[Johannes Berg]

On Wed, 2017-09-20 at 13:11 +0300, Ville Syrjala wrote:

> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1770,15 +1770,21 @@ bool ieee80211_tx_prepare_skb(struct ieee80211_hw *hw,
>   struct ieee80211_tx_data tx;
>   struct sk_buff *skb2;
>  
> - if (ieee80211_tx_prepare(sdata, , NULL, skb) == TX_DROP)
> + rcu_read_lock();

The documentation says:

/**
 * ieee80211_tx_prepare_skb - prepare an 802.11 skb for transmission
 * @hw: pointer as obtained from ieee80211_alloc_hw()
 * @vif: virtual interface
 * @skb: frame to be sent from within the driver
 * @band: the band to transmit on
 * @sta: optional pointer to get the station to send the frame to
 *
 * Note: must be called under RCU lock
 */

You can't even argue that it should be the function itself doing it,
because the (admittedly optional) sta pointer would otherwise not have
proper protection after you leave the function ... You can't pass out a
sta pointer that's RCU protected.

Side note: Perhaps some annotation should be there? not sure it's
possible - would have to be something like
struct ieee80211_sta * __rcu *sta;

I guess since the outer pointer isn't protected, only the inner ...


Therefore, this patch is wrong.

I actually think the same is true for ieee80211_tx_dequeue(), but I'm
less sure about it - the sta pointer there clearly is somehow safely
passed in (even if it's w/o RCU, the driver can potentially make that
safe), but the key pointer seems unsafe in this case (as well) if
there's no outer RCU protection.

johannes

[PATCH v2 1/2] mac80211: Add rcu read side critical sections

[Ville Syrjala]

From: Ville Syrjälä 

I got the following lockdep warning about the rcu_dereference()s in
ieee80211_tx_h_select_key(). After tracing all callers of
ieee80211_tx_h_select_key() I discovered that ieee80211_get_buffered_bc()
and ieee80211_build_data_template() had the rcu_read_lock/unlock() but
three other places did not. So I just blindly added them and made the
read side critical section extend as far as the lifetime of 'tx' which
is where we seem to be stuffing the rcu protected pointers. No real clue
whether this is correct or not.

[  854.573700] ../net/mac80211/tx.c:594 suspicious rcu_dereference_check() 
usage!
[  854.573704]
   other info that might help us debug this:

[  854.573707]
   rcu_scheduler_active = 2, debug_locks = 1
[  854.573712] 6 locks held by kworker/u2:0/2877:
[  854.573715]  #0:  ("%s"wiphy_name(local->hw.wiphy)){.+}, at: 
[] process_one_work+0x127/0x580
[  854.573742]  #1:  ((>work)){+.+.+.}, at: [] 
process_one_work+0x127/0x580
[  854.573758]  #2:  (>mtx){+.+.+.}, at: [] 
ieee80211_sta_work+0x23/0x1c70 [mac80211]
[  854.573902]  #3:  (>sta_mtx){+.+.+.}, at: [] 
__sta_info_flush+0x60/0x160 [mac80211]
[  854.573947]  #4:  (&(>axq_lock)->rlock){+.-...}, at: [] 
ath_tx_node_cleanup+0x5c/0x180 [ath9k]
[  854.573973]  #5:  (&(>lock)->rlock){+.-...}, at: [] 
ieee80211_tx_dequeue+0x24/0xa80 [mac80211]
[  854.574023]
   stack backtrace:
[  854.574028] CPU: 0 PID: 2877 Comm: kworker/u2:0 Not tainted 4.13.0-mgm-ovl+ 
#52
[  854.574032] Hardware name: FUJITSU SIEMENS LIFEBOOK S6120/FJNB16C, BIOS 
Version 1.26  05/10/2004
[  854.574070] Workqueue: phy0 ieee80211_iface_work [mac80211]
[  854.574076] Call Trace:
[  854.574086]  dump_stack+0x16/0x19
[  854.574092]  lockdep_rcu_suspicious+0xcb/0xf0
[  854.574131]  ieee80211_tx_h_select_key+0x1b5/0x500 [mac80211]
[  854.574171]  ieee80211_tx_dequeue+0x283/0xa80 [mac80211]
[  854.574181]  ath_tid_dequeue+0x84/0xf0 [ath9k]
[  854.574189]  ath_tx_node_cleanup+0xb8/0x180 [ath9k]
[  854.574199]  ath9k_sta_state+0x48/0xf0 [ath9k]
[  854.574207]  ? ath9k_del_ps_key.isra.19+0x60/0x60 [ath9k]
[  854.574240]  drv_sta_state+0xaf/0x8c0 [mac80211]
[  854.574275]  __sta_info_destroy_part2+0x10b/0x140 [mac80211]
[  854.574309]  __sta_info_flush+0xd5/0x160 [mac80211]
[  854.574349]  ieee80211_set_disassoc+0xd3/0x570 [mac80211]
[  854.574390]  ieee80211_sta_connection_lost+0x30/0x60 [mac80211]
[  854.574431]  ieee80211_sta_work+0x1ff/0x1c70 [mac80211]
[  854.574436]  ? mark_held_locks+0x62/0x90
[  854.574443]  ? _raw_spin_unlock_irqrestore+0x55/0x70
[  854.574447]  ? trace_hardirqs_on_caller+0x11c/0x1a0
[  854.574452]  ? trace_hardirqs_on+0xb/0x10
[  854.574459]  ? dev_mc_net_exit+0xe/0x20
[  854.574467]  ? skb_dequeue+0x48/0x70
[  854.574504]  ieee80211_iface_work+0x2d8/0x320 [mac80211]
[  854.574509]  process_one_work+0x1d1/0x580
[  854.574513]  ? process_one_work+0x127/0x580
[  854.574519]  worker_thread+0x31/0x380
[  854.574525]  kthread+0xd9/0x110
[  854.574529]  ? process_one_work+0x580/0x580
[  854.574534]  ? kthread_create_on_node+0x30/0x30
[  854.574540]  ret_from_fork+0x19/0x24

[  854.574548] =
[  854.574551] WARNING: suspicious RCU usage
[  854.574555] 4.13.0-mgm-ovl+ #52 Not tainted
[  854.574558] -
[  854.574561] ../net/mac80211/tx.c:608 suspicious rcu_dereference_check() 
usage!
[  854.574564]
   other info that might help us debug this:

[  854.574568]
   rcu_scheduler_active = 2, debug_locks = 1
[  854.574572] 6 locks held by kworker/u2:0/2877:
[  854.574574]  #0:  ("%s"wiphy_name(local->hw.wiphy)){.+}, at: 
[] process_one_work+0x127/0x580
[  854.574590]  #1:  ((>work)){+.+.+.}, at: [] 
process_one_work+0x127/0x580
[  854.574606]  #2:  (>mtx){+.+.+.}, at: [] 
ieee80211_sta_work+0x23/0x1c70 [mac80211]
[  854.574657]  #3:  (>sta_mtx){+.+.+.}, at: [] 
__sta_info_flush+0x60/0x160 [mac80211]
[  854.574702]  #4:  (&(>axq_lock)->rlock){+.-...}, at: [] 
ath_tx_node_cleanup+0x5c/0x180 [ath9k]
[  854.574721]  #5:  (&(>lock)->rlock){+.-...}, at: [] 
ieee80211_tx_dequeue+0x24/0xa80 [mac80211]
[  854.574771]
   stack backtrace:
[  854.574775] CPU: 0 PID: 2877 Comm: kworker/u2:0 Not tainted 4.13.0-mgm-ovl+ 
#52
[  854.574779] Hardware name: FUJITSU SIEMENS LIFEBOOK S6120/FJNB16C, BIOS 
Version 1.26  05/10/2004
[  854.574814] Workqueue: phy0 ieee80211_iface_work [mac80211]
[  854.574821] Call Trace:
[  854.574825]  dump_stack+0x16/0x19
[  854.574830]  lockdep_rcu_suspicious+0xcb/0xf0
[  854.574869]  ieee80211_tx_h_select_key+0x44e/0x500 [mac80211]
[  854.574908]  ieee80211_tx_dequeue+0x283/0xa80 [mac80211]
[  854.574919]  ath_tid_dequeue+0x84/0xf0 [ath9k]
[  854.574927]  ath_tx_node_cleanup+0xb8/0x180 [ath9k]
[  854.574936]  ath9k_sta_state+0x48/0xf0 [ath9k]
[  854.574945]  ? ath9k_del_ps_key.isra.19+0x60/0x60 [ath9k]
[  854.574978]  drv_sta_state+0xaf/0x8c0 [mac80211]
[  854.575012]  

Re: Re: [PATCH] mwifiex: add device specific ioctl handler

[Xinming Hu]

Hi,

> -Original Message-
> From: Arend van Spriel [mailto:arend.vanspr...@broadcom.com]
> Sent: 2017年9月20日 16:40
> To: Xinming Hu ; Linux Wireless
> 
> Cc: Kalle Valo ; Brian Norris
> ; Dmitry Torokhov ;
> raja...@google.com; Zhiyuan Yang ; Tim Song
> ; Cathy Luo ; Ganapathi Bhat
> ; Xinming Hu 
> Subject: [EXT] Re: [PATCH] mwifiex: add device specific ioctl handler
> 
> External Email
> 
> --
> On 9/20/2017 8:44 AM, Xinming Hu wrote:
> > From: Xinming Hu 
> >
> > Add net_dev ndo_do_ioctl handler, which could be used for downloading
> > host command by utility in manufactory test
> 
> Not sure if we want ioctls in upstream wireless drivers.

Yes, wext ioctl could be discard and replaced by well defined nl80211/cfg80211.
But per my understand, the net device ioctl will still works normally. Right ?

> You could look at
> nl80211 vendor commands, but that is also under scrutiny if common wifi
> functionality is provided by it. I am pretty sure Kalle has his ideas about 
> this :-)
> 

Yes, the new utility should use nl80211 interface.
But we have lots of legacy utility based on ioctl, it will be helpful to reduce 
massive work on refactoring.

> Regards,
> Arend

Re: rtl8821ae keep alive not set, connection lost

[James Cameron]

On Tue, Sep 19, 2017 at 07:42:04PM +1000, James Cameron wrote:
> On Thu, Sep 14, 2017 at 07:27:39PM +1000, James Cameron wrote:
> > On Wed, Sep 13, 2017 at 07:39:35PM -0500, Larry Finger wrote:
> > > On 09/13/2017 04:46 PM, James Cameron wrote:
> > > >
> > > >I'll give it some more testing and let you know, but it seems as
> > > >capable of keeping a connection as 4.13 plus my earlier revert.
> > > >
> > 
> > Testing went well; removing the call to enable ASPM was as good as
> > changing the DBI read back to 16-bit width.
> > 
> > > The change I sent earlier should be as good as reverting the change
> > > to write_byte in your reversion.
> > 
> > Yes, that would be the hope.
> > 
> > But with the 16-bit DBI read, the register REG_DBI_CTRL+0 is being
> > read as well, in the first read in _rtl8821ae_enable_aspm_back_door,
> > so perhaps reading that register has an unexpected side-effect.
> > 
> 
> I've ruled that out after testing for several days different kernels
> based on v4.13;
> 
> - add an rtl_read_byte of REG_DBI_CTRL+0 in rtl8821ae_hw_init just
>   after the call to enable_aspm; does not solve problem,
> 
> - add an rtl_read_byte of REG_DBI_CTRL+0 at the start of
>   _rtl8821ae_check_pcie_dma_hang; does not solve problem,

When the problem occurs, register 0x350 bit 25 is set, for which a
comment in _rtl8821ae_check_pcie_dma_hang says means there is an RX
hang.

So perhaps driver should call _rtl8821ae_check_pcie_dma_hang
and _rtl8821ae_reset_pcie_interface_dma.

Any ideas where to do this?

> [...]

-- 
James Cameron
http://quozl.netrek.org/

RE: [EXT] Re: [PATCH] mwifiex: add device specific ioctl handler

[Xinming Hu]

Hi,

> -Original Message-
> From: Souptick Joarder [mailto:jrdr.li...@gmail.com]
> Sent: 2017年9月20日 16:07
> To: Xinming Hu 
> Cc: Linux Wireless ; Kalle Valo
> ; Brian Norris ; Dmitry
> Torokhov ; raja...@google.com; Zhiyuan Yang
> ; Tim Song ; Cathy Luo
> ; Ganapathi Bhat ; Xinming Hu
> 
> Subject: [EXT] Re: [PATCH] mwifiex: add device specific ioctl handler
> 
> External Email
> 
> --
> On Wed, Sep 20, 2017 at 12:14 PM, Xinming Hu 
> wrote:
> > From: Xinming Hu 
> >
> > Add net_dev ndo_do_ioctl handler, which could be used for downloading
> > host command by utility in manufactory test
> >
> > Signed-off-by: Xinming Hu 
> > Signed-off-by: Cathy Luo 
> > Signed-off-by: Ganapathi Bhat 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 59
> +++
> >  drivers/net/wireless/marvell/mwifiex/main.c   | 23 +++
> >  drivers/net/wireless/marvell/mwifiex/main.h   |  7 +++-
> >  3 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > index 0edc5d6..86ee399 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> > @@ -839,6 +839,8 @@ int mwifiex_process_cmdresp(struct
> mwifiex_adapter *adapter)
> > hostcmd = adapter->curr_cmd->data_buf;
> > hostcmd->len = size;
> > memcpy(hostcmd->cmd, resp, size);
> > +   adapter->hostcmd_resp_data.len = size;
> > +   memcpy(adapter->hostcmd_resp_data.cmd,
> resp,
> > + size);
> > }
> > }
> > orig_cmdresp_no = le16_to_cpu(resp->command); @@ -1221,6
> > +1223,63 @@ int mwifiex_ret_802_11_hs_cfg(struct mwifiex_private
> > *priv,  }  EXPORT_SYMBOL_GPL(mwifiex_process_hs_config);
> >
> > +/* This function get hostcmd data from userspace and construct a cmd
> > + * to be download to FW.
> > + */
> > +int mwifiex_process_host_command(struct mwifiex_private *priv,
> > +struct ifreq *req) {
> > +   struct mwifiex_ds_misc_cmd *hostcmd_buf;
> > +   struct host_cmd_ds_command *cmd;
> > +   struct mwifiex_adapter *adapter = priv->adapter;
> > +   int ret;
> > +
> > +   hostcmd_buf = kzalloc(sizeof(*hostcmd_buf), GFP_KERNEL);
> 
> will it be sizeof(*hostcmd_buf) or sizeof( struct mwifiex_ds_misc_cmd *) ?

It should be sizeof(*hostcmd_buf), as we are trying to allocate a struct, not a 
pointer.

> 
> > +   if (!hostcmd_buf)
> > +   return -ENOMEM;
> > +
> > +   cmd = (void *)hostcmd_buf->cmd;
> > +
> > +   if (copy_from_user(cmd, req->ifr_data,
> > +  sizeof(cmd->command) + sizeof(cmd->size)))
> {
> > +   ret = -EFAULT;
> > +   goto done;
> > +   }
> > +
> > +   hostcmd_buf->len = le16_to_cpu(cmd->size);
> > +   if (hostcmd_buf->len > MWIFIEX_SIZE_OF_CMD_BUFFER) {
> > +   ret = -EINVAL;
> > +   goto done;
> > +   }
> > +
> > +   if (copy_from_user(cmd, req->ifr_data, hostcmd_buf->len)) {
> > +   ret = -EFAULT;
> > +   goto done;
> > +   }
> > +
> > +   if (mwifiex_send_cmd(priv, 0, 0, 0, hostcmd_buf, true)) {
> > +   dev_err(priv->adapter->dev, "Failed to process
> hostcmd\n");
> > +   ret = -EFAULT;
> > +   goto done;
> > +   }
> > +
> > +   if (adapter->hostcmd_resp_data.len > hostcmd_buf->len) {
> > +   ret = -EFBIG;
> > +   goto done;
> > +   }
> > +
> > +   if (copy_to_user(req->ifr_data, adapter->hostcmd_resp_data.cmd,
> > +adapter->hostcmd_resp_data.len)) {
> > +   ret = -EFAULT;
> > +   goto done;
> > +   }
> > +
> > +   ret = 0;
> > +done:
> > +   kfree(hostcmd_buf);
> > +   return ret;
> > +}
> > +
> >  /*
> >   * This function handles the command response of a sleep confirm
> command.
> >   *
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> > b/drivers/net/wireless/marvell/mwifiex/main.c
> > index ee40b73..3e7700f 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1265,12 +1265,35 @@ static struct net_device_stats
> *mwifiex_get_stats(struct net_device *dev)
> > return mwifiex_1d_to_wmm_queue[skb->priority];
> >  }
> >
> > +static int mwifiex_do_ioctl(struct net_device *dev, struct ifreq
> > +*req, int cmd) {
> > + 

Re: [PATCH] mwifiex: add device specific ioctl handler

[Arend van Spriel]

On 9/20/2017 8:44 AM, Xinming Hu wrote:

From: Xinming Hu 

Add net_dev ndo_do_ioctl handler, which could be used for
downloading host command by utility in manufactory test


Not sure if we want ioctls in upstream wireless drivers. You could look 
at nl80211 vendor commands, but that is also under scrutiny if common 
wifi functionality is provided by it. I am pretty sure Kalle has his 
ideas about this :-)


Regards,
Arend

Re: [PATCH] mwifiex: add device specific ioctl handler

[Souptick Joarder]

On Wed, Sep 20, 2017 at 12:14 PM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> Add net_dev ndo_do_ioctl handler, which could be used for
> downloading host command by utility in manufactory test
>
> Signed-off-by: Xinming Hu 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 59 
> +++
>  drivers/net/wireless/marvell/mwifiex/main.c   | 23 +++
>  drivers/net/wireless/marvell/mwifiex/main.h   |  7 +++-
>  3 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
> b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> index 0edc5d6..86ee399 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> @@ -839,6 +839,8 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter 
> *adapter)
> hostcmd = adapter->curr_cmd->data_buf;
> hostcmd->len = size;
> memcpy(hostcmd->cmd, resp, size);
> +   adapter->hostcmd_resp_data.len = size;
> +   memcpy(adapter->hostcmd_resp_data.cmd, resp, size);
> }
> }
> orig_cmdresp_no = le16_to_cpu(resp->command);
> @@ -1221,6 +1223,63 @@ int mwifiex_ret_802_11_hs_cfg(struct mwifiex_private 
> *priv,
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_process_hs_config);
>
> +/* This function get hostcmd data from userspace and construct a cmd
> + * to be download to FW.
> + */
> +int mwifiex_process_host_command(struct mwifiex_private *priv,
> +struct ifreq *req)
> +{
> +   struct mwifiex_ds_misc_cmd *hostcmd_buf;
> +   struct host_cmd_ds_command *cmd;
> +   struct mwifiex_adapter *adapter = priv->adapter;
> +   int ret;
> +
> +   hostcmd_buf = kzalloc(sizeof(*hostcmd_buf), GFP_KERNEL);

will it be sizeof(*hostcmd_buf) or sizeof( struct mwifiex_ds_misc_cmd *) ?

> +   if (!hostcmd_buf)
> +   return -ENOMEM;
> +
> +   cmd = (void *)hostcmd_buf->cmd;
> +
> +   if (copy_from_user(cmd, req->ifr_data,
> +  sizeof(cmd->command) + sizeof(cmd->size))) {
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   hostcmd_buf->len = le16_to_cpu(cmd->size);
> +   if (hostcmd_buf->len > MWIFIEX_SIZE_OF_CMD_BUFFER) {
> +   ret = -EINVAL;
> +   goto done;
> +   }
> +
> +   if (copy_from_user(cmd, req->ifr_data, hostcmd_buf->len)) {
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   if (mwifiex_send_cmd(priv, 0, 0, 0, hostcmd_buf, true)) {
> +   dev_err(priv->adapter->dev, "Failed to process hostcmd\n");
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   if (adapter->hostcmd_resp_data.len > hostcmd_buf->len) {
> +   ret = -EFBIG;
> +   goto done;
> +   }
> +
> +   if (copy_to_user(req->ifr_data, adapter->hostcmd_resp_data.cmd,
> +adapter->hostcmd_resp_data.len)) {
> +   ret = -EFAULT;
> +   goto done;
> +   }
> +
> +   ret = 0;
> +done:
> +   kfree(hostcmd_buf);
> +   return ret;
> +}
> +
>  /*
>   * This function handles the command response of a sleep confirm command.
>   *
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index ee40b73..3e7700f 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1265,12 +1265,35 @@ static struct net_device_stats 
> *mwifiex_get_stats(struct net_device *dev)
> return mwifiex_1d_to_wmm_queue[skb->priority];
>  }
>
> +static int mwifiex_do_ioctl(struct net_device *dev, struct ifreq *req, int 
> cmd)
> +{
> +   struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
> +   int ret;
> +
> +   if (!priv->adapter->mfg_mode)
> +   return -EINVAL;

ret can be used instead of returning -EINVAL.

> +
> +   mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
> +
> +   switch (cmd) {
> +   case MWIFIEX_HOSTCMD_IOCTL:
> +   ret = mwifiex_process_host_command(priv, req);
> +   break;
> +   default:
> +   ret = -EINVAL;
> +   break;
> +   }
> +
> +   return ret;
> +}
> +
>  /* Network device handlers */
>  static const struct net_device_ops mwifiex_netdev_ops = {
> .ndo_open = mwifiex_open,
> .ndo_stop = mwifiex_close,
> .ndo_start_xmit = mwifiex_hard_start_xmit,
> .ndo_set_mac_address = mwifiex_ndo_set_mac_address,
> +   .ndo_do_ioctl = mwifiex_do_ioctl,
> .ndo_validate_addr = eth_validate_addr,
> .ndo_tx_timeout = mwifiex_tx_timeout,
>  

[PATCH] mwifiex: add device specific ioctl handler

[Xinming Hu]

From: Xinming Hu 

Add net_dev ndo_do_ioctl handler, which could be used for
downloading host command by utility in manufactory test

Signed-off-by: Xinming Hu 
Signed-off-by: Cathy Luo 
Signed-off-by: Ganapathi Bhat 
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 59 +++
 drivers/net/wireless/marvell/mwifiex/main.c   | 23 +++
 drivers/net/wireless/marvell/mwifiex/main.h   |  7 +++-
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 0edc5d6..86ee399 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -839,6 +839,8 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
hostcmd = adapter->curr_cmd->data_buf;
hostcmd->len = size;
memcpy(hostcmd->cmd, resp, size);
+   adapter->hostcmd_resp_data.len = size;
+   memcpy(adapter->hostcmd_resp_data.cmd, resp, size);
}
}
orig_cmdresp_no = le16_to_cpu(resp->command);
@@ -1221,6 +1223,63 @@ int mwifiex_ret_802_11_hs_cfg(struct mwifiex_private 
*priv,
 }
 EXPORT_SYMBOL_GPL(mwifiex_process_hs_config);
 
+/* This function get hostcmd data from userspace and construct a cmd
+ * to be download to FW.
+ */
+int mwifiex_process_host_command(struct mwifiex_private *priv,
+struct ifreq *req)
+{
+   struct mwifiex_ds_misc_cmd *hostcmd_buf;
+   struct host_cmd_ds_command *cmd;
+   struct mwifiex_adapter *adapter = priv->adapter;
+   int ret;
+
+   hostcmd_buf = kzalloc(sizeof(*hostcmd_buf), GFP_KERNEL);
+   if (!hostcmd_buf)
+   return -ENOMEM;
+
+   cmd = (void *)hostcmd_buf->cmd;
+
+   if (copy_from_user(cmd, req->ifr_data,
+  sizeof(cmd->command) + sizeof(cmd->size))) {
+   ret = -EFAULT;
+   goto done;
+   }
+
+   hostcmd_buf->len = le16_to_cpu(cmd->size);
+   if (hostcmd_buf->len > MWIFIEX_SIZE_OF_CMD_BUFFER) {
+   ret = -EINVAL;
+   goto done;
+   }
+
+   if (copy_from_user(cmd, req->ifr_data, hostcmd_buf->len)) {
+   ret = -EFAULT;
+   goto done;
+   }
+
+   if (mwifiex_send_cmd(priv, 0, 0, 0, hostcmd_buf, true)) {
+   dev_err(priv->adapter->dev, "Failed to process hostcmd\n");
+   ret = -EFAULT;
+   goto done;
+   }
+
+   if (adapter->hostcmd_resp_data.len > hostcmd_buf->len) {
+   ret = -EFBIG;
+   goto done;
+   }
+
+   if (copy_to_user(req->ifr_data, adapter->hostcmd_resp_data.cmd,
+adapter->hostcmd_resp_data.len)) {
+   ret = -EFAULT;
+   goto done;
+   }
+
+   ret = 0;
+done:
+   kfree(hostcmd_buf);
+   return ret;
+}
+
 /*
  * This function handles the command response of a sleep confirm command.
  *
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index ee40b73..3e7700f 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1265,12 +1265,35 @@ static struct net_device_stats 
*mwifiex_get_stats(struct net_device *dev)
return mwifiex_1d_to_wmm_queue[skb->priority];
 }
 
+static int mwifiex_do_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
+{
+   struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
+   int ret;
+
+   if (!priv->adapter->mfg_mode)
+   return -EINVAL;
+
+   mwifiex_dbg(priv->adapter, "ioctl cmd = 0x%x\n", cmd);
+
+   switch (cmd) {
+   case MWIFIEX_HOSTCMD_IOCTL:
+   ret = mwifiex_process_host_command(priv, req);
+   break;
+   default:
+   ret = -EINVAL;
+   break;
+   }
+
+   return ret;
+}
+
 /* Network device handlers */
 static const struct net_device_ops mwifiex_netdev_ops = {
.ndo_open = mwifiex_open,
.ndo_stop = mwifiex_close,
.ndo_start_xmit = mwifiex_hard_start_xmit,
.ndo_set_mac_address = mwifiex_ndo_set_mac_address,
+   .ndo_do_ioctl = mwifiex_do_ioctl,
.ndo_validate_addr = eth_validate_addr,
.ndo_tx_timeout = mwifiex_tx_timeout,
.ndo_get_stats = mwifiex_get_stats,
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index a76bd79..4639f49 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -160,6 +160,9 @@ enum {
 /* Threshold for tx_timeout_cnt before we trigger a card reset */
 #define TX_TIMEOUT_THRESHOLD   6
 
+/* IOCTL number used for hostcmd process*/
+#define 

Re: [PATCH 2/2] mwifiex: print URB submit failure error after threshold attemtps

[Brian Norris]

Hi Kalle,

On Wed, Sep 20, 2017 at 07:30:29AM +0300, Kalle Valo wrote:
> Brian Norris  writes:
> 
> > Hi Ganapathi,
> >
> > On Thu, Sep 14, 2017 at 02:14:24PM +, Ganapathi Bhat wrote:
> >> > On Thu, 2017-08-31 at 01:21 +0530, Ganapathi Bhat wrote:

> >> > Why not use a ratelimit?
> >> Since this is for receive, the packets are from AP side and we cannot
> >> lower the rate from AP. On some low performance systems this change
> >> will be helpful.
> >
> > I think Joe was referring to things like printk_ratelimited() or
> > dev_err_ratelimited(). Those automatically ratelimit prints for you,
> > using a static counter. You'd just need to make a small warpper for
> > mwifiex_dbg() using __ratelimit().
> >
> > Those sort of rate limits are significantly different than yours though.
> > You were looking to avoid printing errors when there are only a few
> > failures in a row, whereas the existing rate-limiting infrastructure
> > looks to avoid printing errors if too many happen in a row. Those are
> > different goals.
> 
> Are you saying that this patch is good to take? Or should Ganapathi
> submit v2?

If you're asking me...

All I was saying was that I don't think Joe's suggestion will help
Ganapathi. I'd expect Ganapathi could confirm/deny that part. (Or Joe
could correct me if my interpretation is wrong.)

I'm also not familiar with how we expect dev_alloc_skb() failures to be
handled. If that's a common expected failure mode in low-memory
situations (seems reasonable?) and the driver handles these failure just
fine (completely unreviewed by me, so far; I suspect it doesn't do this
completely correctly), then sure, being less noisy about it as done in
this patch should be fine.

IOW, I don't have concrete feedback for Ganapathi to address, but I'm
not exactly "ack"ing it myself.

Brian