Re: [EXT] Re: [PATCH] mwifiex: fix nested rtnl locking on BG_SCAN_STOPPED

[Brian Norris]

Hi Ganapathi,

For some reason, I'm daft enough to reply to this ancient thread. It
appears as if you probably have not resolved this issue yet though, so
I figured I could lend some advice.

On Wed, Apr 25, 2018 at 1:06 AM Ganapathi Bhat  wrote:
> > Ganapathi Bhat  writes:
> > > --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> > > @@ -848,7 +848,10 @@ int mwifiex_process_sta_event(struct
> > > mwifiex_private *priv)
> > >
> > > case EVENT_BG_SCAN_STOPPED:
> > > dev_dbg(adapter->dev, "event: BGS_STOPPED\n");
> > > -   cfg80211_sched_scan_stopped(priv->wdev.wiphy, 0);
> > > +   if (rtnl_is_locked())
> > > +   cfg80211_sched_scan_stopped_rtnl(priv-
> > >wdev.wiphy, 0);
> > > +   else
> > > +   cfg80211_sched_scan_stopped(priv->wdev.wiphy,
> > 0);
> >
> > IMHO checking if a lock is taking is rather ugly and an indication there's a
> > problem with the locking. Instead making an ugly workaround like this I
> > would rather investigate who is holding the rtnl and solve that.

Agreed, this is not good. You are now bound to hit ASSERT_RTNL()
warnings occasionally, since you might see rtnl locked here, but a
split second later, when you're running in
__cfg80211_stop_sched_scan(), it could be released by some other
thread.

> There can be applications trying to access driver(via cfg80211), holding 
> rtnl_lock.
> For example(in our case):
> 1. "iw dev"  was called, when BG_SCAN was active.
> 2. NL80211_CMD_GET_INTERFACE requires rtnl_lock to be hold(specified in 
> internal_flags)
> 3. cfg80211 will  hold rtnl_lock before calling "get_tx_power"(in pre_doit).
> 4. mwifiex will download RF_TX_PWR command to firmware
> 5. firmware will send BG_SCAN_STOPPED event(since BG_SCAN was active).
> 6. mwifiex will call "cfg80211_sched_scan_stopped" causing nested rtnl 
> locking.

IIUC, it's not exactly a nested lock, but a lock inversion issue.

#1
NL80211_CMD_GET_INTERFACE thread is holding rtnl lock and later
waiting on one of its HostCmd_CMD_* to complete

In the meantime:
#2
a EVENT_BG_SCAN_STOPPED is queued, and it grabs the rtnl lock

Because events are served on the same thread as commands, you get #1
waiting on #2, which is waiting on the lock held in #1. i.e., ABBA.

The way to resolve this is to either move event processing to a
different thread than command processing (that looks it would be very
difficult to do correctly, given the ossified structure of your
driver; but that might be a wise move in the long term)...
...or else maybe you could defer this specific piece of work to its
own thread. That might require yet another workqueue?

Anyway, the key point is that you really don't want to hold rtnl_lock
in your main workqueue, or in any other way that might block the rest
of the operation of your driver.

Brian

[PATCH] iw: dump 'rx bitrate' in link stats

[Brian Norris]

We include it in 'station dump' but not 'link'.

Signed-off-by: Brian Norris 
---
 link.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/link.c b/link.c
index 0a323920ff2f..1ed7f631a121 100644
--- a/link.c
+++ b/link.c
@@ -121,6 +121,7 @@ static int print_link_sta(struct nl_msg *msg, void *arg)
[NL80211_STA_INFO_RX_PACKETS] = { .type = NLA_U32 },
[NL80211_STA_INFO_TX_PACKETS] = { .type = NLA_U32 },
[NL80211_STA_INFO_SIGNAL] = { .type = NLA_U8 },
+   [NL80211_STA_INFO_RX_BITRATE] = { .type = NLA_NESTED },
[NL80211_STA_INFO_TX_BITRATE] = { .type = NLA_NESTED },
[NL80211_STA_INFO_LLID] = { .type = NLA_U16 },
[NL80211_STA_INFO_PLID] = { .type = NLA_U16 },
@@ -160,6 +161,12 @@ static int print_link_sta(struct nl_msg *msg, void *arg)
printf("\tsignal: %d dBm\n",
(int8_t)nla_get_u8(sinfo[NL80211_STA_INFO_SIGNAL]));
 
+   if (sinfo[NL80211_STA_INFO_RX_BITRATE]) {
+   char buf[100];
+
+   parse_bitrate(sinfo[NL80211_STA_INFO_RX_BITRATE], buf, 
sizeof(buf));
+   printf("\trx bitrate: %s\n", buf);
+   }
if (sinfo[NL80211_STA_INFO_TX_BITRATE]) {
char buf[100];
 
-- 
2.20.0.rc0.387.gc7a69e6b6c-goog

[PATCH] iw: add FEATURE support for scan randomization

[Brian Norris]

Signed-off-by: Brian Norris 
---
 info.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/info.c b/info.c
index d0577e32552e..f1a25daa63da 100644
--- a/info.c
+++ b/info.c
@@ -609,6 +609,12 @@ broken_combination:
printf("\tDevice supports configuring vdev MAC-addr on 
create.\n");
if (features & NL80211_FEATURE_TDLS_CHANNEL_SWITCH)
printf("\tDevice supports TDLS channel switching\n");
+   if (features & NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR)
+   printf("\tDevice supports randomizing MAC-addr in 
scans.\n");
+   if (features & NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR)
+   printf("\tDevice supports randomizing MAC-addr in sched 
scans.\n");
+   if (features & NL80211_FEATURE_ND_RANDOM_MAC_ADDR)
+   printf("\tDevice supports randomizing MAC-addr in 
net-detect scans.\n");
}
 
if (tb_msg[NL80211_ATTR_TDLS_SUPPORT])
-- 
2.19.1.930.g4563a0d9d0-goog

[PATCH iw] iw: fix enum warnings

[Brian Norris]

clang warns about the misuse of enums:

reg.c:246:26: warning: implicit conversion from enumeration type 'enum 
command_identify_by' to different enumeration type 'enum id_input' 
[-Wenum-conversion]
err = handle_cmd(state, CIB_NONE, 2, dump_args);
  ~~^~~~
info.c:645:26: warning: implicit conversion from enumeration type 'enum 
command_identify_by' to different enumeration type 'enum id_input' 
[-Wenum-conversion]
err = handle_cmd(state, CIB_NONE, 2, feat_args);
  ~~^~~~

Signed-off-by: Brian Norris 
---
 info.c | 2 +-
 reg.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/info.c b/info.c
index fbf3ee095f9c..d0577e32552e 100644
--- a/info.c
+++ b/info.c
@@ -718,7 +718,7 @@ static int handle_info(struct nl80211_state *state,
char *feat_args[] = { "features", "-q" };
int err;
 
-   err = handle_cmd(state, CIB_NONE, 2, feat_args);
+   err = handle_cmd(state, II_NONE, 2, feat_args);
if (!err && nl80211_has_split_wiphy) {
nla_put_flag(msg, NL80211_ATTR_SPLIT_WIPHY_DUMP);
nlmsg_hdr(msg)->nlmsg_flags |= NLM_F_DUMP;
diff --git a/reg.c b/reg.c
index cadff3884c04..a2368df39009 100644
--- a/reg.c
+++ b/reg.c
@@ -243,7 +243,7 @@ static int handle_reg_get(struct nl80211_state *state,
char *dump_args[] = { "reg", "dump" };
int err;
 
-   err = handle_cmd(state, CIB_NONE, 2, dump_args);
+   err = handle_cmd(state, II_NONE, 2, dump_args);
/*
 * dump might fail since it's not supported on older kernels,
 * in that case the handler is still registered already
-- 
2.19.0.605.g01d371f741-goog

Re: Promiscuous (not monitor) Mode support for ath10k

[Brian Norris]

On Mon, Aug 06, 2018 at 06:08:46PM -0400, Tomasz Kalbarczyk wrote:
> Hello,

Hi!

> I have a QCA6174 and I'm running tcpdump on the wireless interface to
> listen for packets. I can receive packets addressed directly to the
> device, but not any other packets on the network.
> 
> Does anyone know the status of promiscuous mode support on ath10k?
> Note that this is different from monitor mode / supported on a larger
> subset of hardware. Dmesg merely indicates that the device has
> "entered promiscuous mode" when tcpdump is initiated.
> 
> Here is a link to similar discussion regarding the ath9k a few years
> back: https://marc.info/?l=linux-wireless=135936563626791=2

This might be relevant:

commit df1404650ccbfeb76a84f301f22316be0d00a864
Author: Johannes Berg 
Date:   Wed Apr 22 14:40:58 2015 +0200

mac80211: remove support for IFF_PROMISC

This support is essentially useless as typically networks are encrypted,
frames will be filtered by hardware, and rate scaling will be done with
the intended recipient in mind. For real monitoring of the network, the
monitor mode support should be used instead.

Removing it removes a lot of corner cases.

Regards,
Brian

Re: How to let devcoredump know data has been read?

[Brian Norris]

Hi,

On Tue, Jun 05, 2018 at 03:27:28PM -0700, Ben Greear wrote:
> I have been testing ath10k on 4.16, which uses the devcoredump API
> to notify about dumps.
> 
> I am able to see the binary crash dump at /sys/class/devcoredump/devcd2/data,
> for instance, but if I do another crash quickly, I get no new uevent sent
> and no new crash.
> 
> I see there is a 5 minute timer on the coredump data, but it also seems to 
> indicate
> that if I read the crash, the data should be cleared and ready to be
> recreated again?  How do you notify the system that the crash data has
> been read?
> 
> I tried 'cat' on the data file, but that did not seem to clear anything.

Try *writing* to it?

https://elixir.bootlin.com/linux/v4.16/source/drivers/base/devcoredump.c#L91

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=833c95456a70826d1384883b73fd23aff24d366f


The dumped data will be readable in sysfs in the virtual device's
data file under /sys/class/devcoredump/devcd*/. Writing to it will
free the data and remove the device, as does a 5-minute timeout.


e.g.:

# ls -l /sys/class/devcoredump/devcd1
lrwxrwxrwx. 1 root root 0 Jun  5 15:49 /sys/class/devcoredump/devcd1 -> 
../../devices/virtual/devcoredump/devcd1
# echo 1 > /sys/class/devcoredump/devcd1/data
# ls -l /sys/class/devcoredump/devcd1
ls: cannot access '/sys/class/devcoredump/devcd1': No such file or directory

Unfortunately, devcoredump is a bit lacking in documentation. Arend was
writing a bit for the new trigger mechanism at least.

Brian

Re: [PATCH 2/2] mwifiex: handle race during mwifiex_usb_disconnect

[Brian Norris]

Hi Ganapathi,

On Fri, Jun 01, 2018 at 04:11:20PM +0530, Ganapathi Bhat wrote:
> Race condition is observed during rmmod of mwifiex_usb:
> 
> 1. The rmmod thread will call mwifiex_usb_disconnect(), download
>SHUTDOWN command and do wait_event_interruptible_timeout(),
>waiting for response.
> 
> 2. The main thread will handle the response and will do a
>wake_up_interruptible(), unblocking rmmod thread.
> 
> 3. On getting unblocked, rmmod thread  will make rx_cmd.urb = NULL in
>mwifiex_usb_free().
> 
> 4. The main thread will try to resubmit rx_cmd.urb in
>mwifiex_usb_submit_rx_urb(), which is NULL.
> 
> To fix this, move mwifiex_usb_free() from mwifiex_usb_disconnect
> to mwifiex_unregister_dev(). Function mwifiex_unregister_dev() is
> called after flushing the command and RX work queues.
> 
> Signed-off-by: Brian Norris 

^^ I'm not sure if that line is quite accurate. While I nearly spelled
out what the patch would look like, you wrote it.

Anyway, patch seems good to me, assuming it tests out OK for you:

Reviewed-by: Brian Norris 

and if Kalle hasn't applied this yet, an alternative to Signed-off-by:

Suggested-by: Brian Norris 

> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c 
> b/drivers/net/wireless/marvell/mwifiex/usb.c
> index bc475b8..88f4c89 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -644,8 +644,6 @@ static void mwifiex_usb_disconnect(struct usb_interface 
> *intf)
>MWIFIEX_FUNC_SHUTDOWN);
>   }
>  
> - mwifiex_usb_free(card);
> -
>   mwifiex_dbg(adapter, FATAL,
>   "%s: removing card\n", __func__);
>   mwifiex_remove_card(adapter);
> @@ -1353,6 +1351,8 @@ static void mwifiex_unregister_dev(struct 
> mwifiex_adapter *adapter)
>  {
>   struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
>  
> + mwifiex_usb_free(card);
> +
>   mwifiex_usb_cleanup_tx_aggr(adapter);
>  
>   card->adapter = NULL;
> -- 
> 1.9.1
> 

Re: [PATCH] mwifiex: handle race during mwifiex_usb_disconnect

[Brian Norris]

Hi Ganapathi,

On Thu, May 24, 2018 at 07:18:27PM +0530, Ganapathi Bhat wrote:
> Race condition is observed during rmmod of mwifiex_usb:
> 
> 1. The rmmod thread will call mwifiex_usb_disconnect(), download
>SHUTDOWN command and do wait_event_interruptible_timeout(),
>waiting for response.
> 
> 2. The main thread will handle the response and will do a
>wake_up_interruptible(), unblocking rmmod thread.
> 
> 3. On getting unblocked, rmmod thread  will make rx_cmd.urb = NULL in
>mwifiex_usb_free().
> 
> 4. The main thread will try to resubmit rx_cmd.urb in
>mwifiex_usb_submit_rx_urb(), which is NULL.
> 
> To fix, wait for main thread to complete before calling
> mwifiex_usb_free().
> 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/usb.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c 
> b/drivers/net/wireless/marvell/mwifiex/usb.c
> index bc475b8..6e3cf98 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -644,6 +644,9 @@ static void mwifiex_usb_disconnect(struct usb_interface 
> *intf)
>MWIFIEX_FUNC_SHUTDOWN);
>   }
>  
> + if (adapter->workqueue)
> + flush_workqueue(adapter->workqueue);

This seems like a bad fix. I'm fairly sure there's another race in here
somewhere, and at a minimum, this is fragile code.

Instead, can't you just move the mwifiex_usb_free() into a .cleanup_if()
or .unregister_dev() callback? That's what your other drivers (PCIe and
SDIO) use to clean up old buffers and stop bus activity; those are
called after the appropriate synchronization points; and I'm pretty sure
I've already audited those to be more or less safe.

Brian

> +
>   mwifiex_usb_free(card);
>  
>   mwifiex_dbg(adapter, FATAL,
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] ath10k: support MAC address randomization in scan

[Brian Norris]

On Tue, Apr 17, 2018 at 2:49 PM, Arend van Spriel
<arend.vanspr...@broadcom.com> wrote:
> On 4/17/2018 6:07 PM, Brian Norris wrote:
>> On Tue, Apr 17, 2018 at 10:22:13AM +0200, Arend van Spriel wrote:
>>> I believe checking command support is not really recommended. Instead, you
>>> better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero (since kernel
>>> 4.12 that is).
>>
>>
>> Why not? Command support checking is what wpa_supplicant is doing.
>
>
> That's not really a good argument. A couple (or more) years ago 
> wpa_supplicant was not doing nl80211 but wext and some other using driver 
> private ioctls, but that did not make it the best approach.

I see what you're saying (though your comparison doesn't seem that
fair either; private ioctls are nothing like a well-defined nl80211
support list), and I'm totally good on looking at the new flag
eventually. But you still haven't answered my question ("why not?").
Is there a problem with the "supported commands" list?

> The START_SCHED_SCAN command is indeed still provided to user-space:

And as I see it, it probably needs to be for essentially forever. Or
at least a significant amount of time after wpa_supplicant stops
relying on it. (Hint: it's still using it today, with no reference to
NL80211_ATTR_SCHED_SCAN_MAX_REQS.) There's a reason the kernel has ABI
guarantees. I suspect you only get a chance to rewrite the world (WEXT
-> nl80211) a few times in the life of kernel ABIs.

Brian

Re: [PATCH 2/2] ath10k: support MAC address randomization in scan

[Brian Norris]

On Tue, Apr 17, 2018 at 10:22:13AM +0200, Arend van Spriel wrote:
> On 4/17/2018 2:28 AM, Brian Norris wrote:
> > It looks like the status quo for looking for SCHED_SCAN support is to
> > check if NL80211_CMD_START_SCHED_SCAN shows up in the command support
> > list. (IOW, that's what wpa_supplicant does.) We'll probably need to
> > imitate that.
> 
> I believe checking command support is not really recommended. Instead, you
> better check NL80211_ATTR_SCHED_SCAN_MAX_REQS being non-zero (since kernel
> 4.12 that is).

Why not? Command support checking is what wpa_supplicant is doing.

I noticed NL80211_ATTR_SCHED_SCAN_MAX_REQS, but unfortunately, we have
to support older kernels. It looks like randomization was added in
v3.19, and as you point out, that's only available in v4.12.

I welcome other alternatives if you have them to offer.

Brian

Re: [PATCH 2/2] ath10k: support MAC address randomization in scan

[Brian Norris]

Hi,

On Mon, Apr 16, 2018 at 02:32:47PM +0300, Kalle Valo wrote:
> cjhu...@codeaurora.org writes:
> > On 2018-04-14 05:13, Arend van Spriel wrote:
> >> On 4/13/2018 1:28 PM, Kalle Valo wrote:
> >>> cjhu...@codeaurora.org writes:
> >>>
> >> +  if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) {
> >> +  ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr);
> >> +  if (ret) {
> >> +  ath10k_err(ar, "failed to set prob req oui: 
> >> %i\n", ret);
> >> +  goto err_dfs_detector_exit;
> >> +  }
> >> +
> >> +  ar->hw->wiphy->features |=
> >> +  NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR;
> >
> > Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too?
> 
>  I'll add this flag too.
> >>>
> >>> Are you going to send v2 or what's the plan?
> >>
> >> Maybe a stupid question, but does ath10k support scheduled scan?

Not a stupid question. Sorry for not asking that first.

> AFAICS ath10k does not support it (sched_scan_start() op).

Right, that seems to be the case.

> > The reason is AVL test case needs this flag to enable random mac
> > address scan. Maybe Brian Can explain why this flag is necessary.

I never actually claimed you *needed* this flag; I just wondered how you
claimed to pass our test when you did not support this flag, since our
network manager currently checks for both
NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR and
NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR before enabling MAC
randomization in scans. But that's no matter to worry about here.

> If ath10k does not support scheduled scan what's the point? Shouldn't
> the test case then be it fixed instead of making hacks in ath10k?

Indeed. We're trying to work that out right now.

It looks like the status quo for looking for SCHED_SCAN support is to
check if NL80211_CMD_START_SCHED_SCAN shows up in the command support
list. (IOW, that's what wpa_supplicant does.) We'll probably need to
imitate that.

Brian

Re: [PATCH 2/2] ath10k: support MAC address randomization in scan

[Brian Norris]

Hi Carl,

On Fri, Mar 30, 2018 at 11:14:00AM +0800, Carl Huang wrote:
> The ath10k reports the random_mac_addr capability to upper layer
> based on the service bit firmware reported. Driver sets the
> spoofed flag in scan_ctrl_flag to firmware if upper layer has
> enabled this feature in scan request.
> 
> Test with QCA6174 hw3.0 and firmware-6.bin_WLAN.RM.4.4.1-00102-QCARMSWP-1,
> but QCA9377 is also affected.
> 
> Signed-off-by: Carl Huang 
> ---
>  drivers/net/wireless/ath/ath10k/mac.c | 17 +
>  drivers/net/wireless/ath/ath10k/wmi-ops.h | 22 ++
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c | 25 +
>  drivers/net/wireless/ath/ath10k/wmi-tlv.h | 11 +++
>  drivers/net/wireless/ath/ath10k/wmi.c |  5 +
>  drivers/net/wireless/ath/ath10k/wmi.h |  9 +
>  6 files changed, 89 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
> b/drivers/net/wireless/ath/ath10k/mac.c
> index ebb3f1b..c5cd5e5 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -5699,6 +5699,12 @@ static int ath10k_hw_scan(struct ieee80211_hw *hw,
>   arg.scan_ctrl_flags |= WMI_SCAN_FLAG_PASSIVE;
>   }
>  
> + if (req->flags & NL80211_SCAN_FLAG_RANDOM_ADDR) {
> + arg.scan_ctrl_flags |=  WMI_SCAN_ADD_SPOOFED_MAC_IN_PROBE_REQ;
> + ether_addr_copy(arg.mac_addr.addr, req->mac_addr);
> + ether_addr_copy(arg.mac_mask.addr, req->mac_addr_mask);
> + }
> +
>   if (req->n_channels) {
>   arg.n_channels = req->n_channels;
>   for (i = 0; i < arg.n_channels; i++)
> @@ -8397,6 +8403,17 @@ int ath10k_mac_register(struct ath10k *ar)
>   goto err_dfs_detector_exit;
>   }
>  
> + if (test_bit(WMI_SERVICE_SPOOF_MAC_SUPPORT, ar->wmi.svc_map)) {
> + ret = ath10k_wmi_scan_prob_req_oui(ar, ar->mac_addr);
> + if (ret) {
> + ath10k_err(ar, "failed to set prob req oui: %i\n", ret);
> + goto err_dfs_detector_exit;
> + }
> +
> + ar->hw->wiphy->features |=
> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR;

Do you support NL80211_FEATURE_SCHED_SCAN_RANDOM_MAC_ADDR too?

> + }
> +


Brian

Re: [PATCH] ath10k: Convert wow pattern from 802.3 to 802.11

[Brian Norris]

   memcpy(new_hdr_pattern->addr3, old_hdr_pattern->h_source, ETH_ALEN);
> + memcpy(new_hdr_mask->addr3, old_hdr_mask->h_source, ETH_ALEN);
> +
> + /* Copy logic link type */
> + memcpy(_rfc_pattern->snap_type,
> +_hdr_pattern->h_proto,
> +sizeof(old_hdr_pattern->h_proto));
> + memcpy(_rfc_mask->snap_type,
> +_hdr_mask->h_proto,
> +sizeof(old_hdr_mask->h_proto));
> +
> + /* Caculate new pkt_offset */
> + if (old->pkt_offset < ETH_ALEN)
> + new->pkt_offset = old->pkt_offset +
> + offsetof(struct ieee80211_hdr_3addr, addr1);
> + else if (old->pkt_offset < offsetof(struct ethhdr, h_proto))
> + new->pkt_offset = old->pkt_offset +
> + offsetof(struct ieee80211_hdr_3addr, addr3) -
> + offsetof(struct ethhdr, h_source);
> + else
> + new->pkt_offset = old->pkt_offset + hdr_len + rfc_len - 
> ETH_HLEN;
> +
> + /* Caculate new hdr end offset */
> + if (total_len > ETH_HLEN)
> + hdr_80211_end_offset = hdr_len + rfc_len;
> + else if (total_len > offsetof(struct ethhdr, h_proto))
> + hdr_80211_end_offset = hdr_len + rfc_len + total_len - ETH_HLEN;
> + else if (total_len > ETH_ALEN)
> + hdr_80211_end_offset = total_len - ETH_ALEN +
> + offsetof(struct ieee80211_hdr_3addr, addr3);
> + else
> + hdr_80211_end_offset = total_len +
> + offsetof(struct ieee80211_hdr_3addr, addr1);
> +
> + new->pattern_len = hdr_80211_end_offset - new->pkt_offset;
> +
> + memcpy((u8 *)new->pattern,
> +hdr_80211_pattern + new->pkt_offset,
> +new->pattern_len);
> + memcpy((u8 *)new->mask,
> +hdr_80211_bit_mask + new->pkt_offset,
> +new->pattern_len);
> +
> + if (total_len > ETH_HLEN) {
> + /* Copy frame body */
> + memcpy((u8 *)new->pattern + new->pattern_len,
> +(void *)old->pattern + ETH_HLEN - old->pkt_offset,
> +total_len - ETH_HLEN);
> + memcpy((u8 *)new->mask + new->pattern_len,
> +(void *)old->mask + ETH_HLEN - old->pkt_offset,
> +total_len - ETH_HLEN);
> +
> + new->pattern_len += total_len - ETH_HLEN;
> + }
> +}
> +
>  static int ath10k_vif_wow_set_wakeups(struct ath10k_vif *arvif,
>     struct cfg80211_wowlan *wowlan)
>  {
> @@ -116,22 +209,39 @@ static int ath10k_vif_wow_set_wakeups(struct ath10k_vif 
> *arvif,
>  
>   for (i = 0; i < wowlan->n_patterns; i++) {
>   u8 bitmask[WOW_MAX_PATTERN_SIZE] = {};
> + u8 ath_pattern[WOW_MAX_PATTERN_SIZE] = {};
> + u8 ath_bitmask[WOW_MAX_PATTERN_SIZE] = {};

So now we've got 3 * 148 = 444 bytes on the stack just for this? Seems
like it's getting a little large for kernel code, but maybe not too bad.
I guess we can go with this until somebody runs into a real problem...

Otherwise, seems to work for me:

Tested-by: Brian Norris <briannor...@chromium.org>
Reviewed-by: Brian Norris <briannor...@chromium.org>

> + struct cfg80211_pkt_pattern new_pattern = {};
> + struct cfg80211_pkt_pattern old_pattern = patterns[i];
>   int j;
> -
> + new_pattern.pattern = ath_pattern;
> + new_pattern.mask = ath_bitmask;
>   if (patterns[i].pattern_len > WOW_MAX_PATTERN_SIZE)
>   continue;
> -
>   /* convert bytemask to bitmask */
>   for (j = 0; j < patterns[i].pattern_len; j++)
>   if (patterns[i].mask[j / 8] & BIT(j % 8))
>   bitmask[j] = 0xff;
> + old_pattern.mask = bitmask;
> + new_pattern = old_pattern;
> +
> + if (ar->wmi.rx_decap_mode == ATH10K_HW_TXRX_NATIVE_WIFI) {
> + if (patterns[i].pkt_offset < ETH_HLEN)
> + ath10k_wow_convert_8023_to_80211(_pattern,
> +  _pattern);
> + else
> + new_pattern.pkt_offset += WOW_HDR_LEN - 
> ETH_HLEN;
> + }
> +
> + if (WARN_ON(new_pattern.pattern_len > WOW_MAX_PATTERN_SIZE))
> + return -EINVAL;
>  
>   ret = ath10k_wmi_wow_add_pattern(ar, arvif->vdev_id,
> 

Re: [PATCH v2] ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait

[Brian Norris]

+ Felix, who had feedback on the last version

On Mon, Mar 05, 2018 at 02:44:02PM +0800, Carl Huang wrote:
> The skb may be freed in tx completion context before
> trace_ath10k_wmi_cmd is called. This can be easily captured when
> KASAN(Kernel Address Sanitizer) is enabled. The fix is to move
> trace_ath10k_wmi_cmd before the send operation. As the ret has no
> meaning in trace_ath10k_wmi_cmd then, so remove this parameter too.
> 
> Signed-off-by: Carl Huang <cjhu...@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath10k/trace.h | 12 
>  drivers/net/wireless/ath/ath10k/wmi.c   |  2 +-
>  2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/trace.h 
> b/drivers/net/wireless/ath/ath10k/trace.h
> index e40edce..7d2fac3 100644
> --- a/drivers/net/wireless/ath/ath10k/trace.h
> +++ b/drivers/net/wireless/ath/ath10k/trace.h
> @@ -152,10 +152,9 @@ TRACE_EVENT(ath10k_log_dbg_dump,
>  );
>  
>  TRACE_EVENT(ath10k_wmi_cmd,
> - TP_PROTO(struct ath10k *ar, int id, const void *buf, size_t buf_len,
> -  int ret),
> + TP_PROTO(struct ath10k *ar, int id, const void *buf, size_t buf_len),
>  
> - TP_ARGS(ar, id, buf, buf_len, ret),
> + TP_ARGS(ar, id, buf, buf_len),
>  
>   TP_STRUCT__entry(
>   __string(device, dev_name(ar->dev))
> @@ -163,7 +162,6 @@ TRACE_EVENT(ath10k_wmi_cmd,
>   __field(unsigned int, id)
>   __field(size_t, buf_len)
>   __dynamic_array(u8, buf, buf_len)
> - __field(int, ret)
>   ),
>  
>   TP_fast_assign(
> @@ -171,17 +169,15 @@ TRACE_EVENT(ath10k_wmi_cmd,
>   __assign_str(driver, dev_driver_string(ar->dev));
>   __entry->id = id;
>   __entry->buf_len = buf_len;
> - __entry->ret = ret;
>   memcpy(__get_dynamic_array(buf), buf, buf_len);
>   ),
>  
>   TP_printk(
> - "%s %s id %d len %zu ret %d",
> + "%s %s id %d len %zu",
>   __get_str(driver),
>   __get_str(device),
>   __entry->id,
> - __entry->buf_len,
> - __entry->ret
> + __entry->buf_len
>   )
>  );
>  
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c 
> b/drivers/net/wireless/ath/ath10k/wmi.c
> index 58dc218..fc9f50d 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -1742,8 +1742,8 @@ int ath10k_wmi_cmd_send_nowait(struct ath10k *ar, 
> struct sk_buff *skb,
>   cmd_hdr->cmd_id = __cpu_to_le32(cmd);
>  
>   memset(skb_cb, 0, sizeof(*skb_cb));
> + trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len);
>   ret = ath10k_htc_send(>htc, ar->wmi.eid, skb);
> - trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len, ret);
>  
>   if (ret)
>   goto err_pull;

Tested-by: Brian Norris <briannor...@chromium.org>
Reviewed-by: Brian Norris <briannor...@chromium.org>

Re: [PATCH] ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait

[Brian Norris]

On Sun, Feb 11, 2018 at 10:56:45AM +0800, Carl Huang wrote:
> The skb may be freed in tx completion context before
> trace_ath10k_wmi_cmd is called. This can be easily captured
> when KASAN(Kernel Address Sanitizer) is enabled. The fix is
> to add a reference count to the skb and release it after
> trace_ath10k_wmi_cmd is called.
> 
> Signed-off-by: Carl Huang <cjhu...@codeaurora.org>
> ---
>  drivers/net/wireless/ath/ath10k/wmi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c 
> b/drivers/net/wireless/ath/ath10k/wmi.c
> index 58dc218..e63aedb 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -1,6 +1,7 @@
>  /*
>   * Copyright (c) 2005-2011 Atheros Communications Inc.
>   * Copyright (c) 2011-2017 Qualcomm Atheros, Inc.
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.

I agree with Felix that this seems excessive for a 2-line bugfix. But
I'm not a lawyer or a maintainer, and I have no power here :)

(On the practical side: it's annoying that the copyright line has to be
the one to provide conflicts when backporting.)

>   *
>   * Permission to use, copy, modify, and/or distribute this software for any
>   * purpose with or without fee is hereby granted, provided that the above
> @@ -1742,8 +1743,10 @@ int ath10k_wmi_cmd_send_nowait(struct ath10k *ar, 
> struct sk_buff *skb,
>   cmd_hdr->cmd_id = __cpu_to_le32(cmd);
>  
>   memset(skb_cb, 0, sizeof(*skb_cb));
> + skb_get(skb);
>   ret = ath10k_htc_send(>htc, ar->wmi.eid, skb);
>   trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len, ret);
> + dev_kfree_skb(skb);

Tested-by: Brian Norris <briannor...@chromium.org>

It does feel like capturing before the command is sent might make more
sense. Otherwise, you may be concurrently dumping the buffer and DMA'ing
to a device. If you really need to trace the return code, you could do
that separately.

Brian

>  
>   if (ret)
>   goto err_pull;

Re: [PATCH 2/3] mwifiex: support sysfs initiated device coredump

[Brian Norris]

Hi,

On Fri, Feb 23, 2018 at 2:51 AM, Johannes Berg
 wrote:
> On Fri, 2018-02-23 at 11:39 +0100, Arend van Spriel wrote:
>
> > > > Well, that depends on the eye of the beholder I guess. From user-space
> > > > perspective it is asynchronous regardless. A write access to the 
> > > > coredump
> > > > sysfs file eventually results in a uevent when the devcoredump entry is
> > > > created, ie. after driver has made a dev_coredump API call. Whether the
> > > > driver does that synchronously or asynchronously is irrelevant as far as
> > > > user-space is concerned.
> > >
> > > Is it really? The driver infrastructure seems to guarantee that the
> > > entirety of a driver's ->coredump() will complete before returning from
> > > the write. So it might be reasonable for some user to assume (based on
> > > implementation details, e.g., of brcmfmac) that the devcoredump will be
> > > ready by the time the write() syscall returns, absent documentation that
> > > says otherwise. But then, that's not how mwifiex works right now, so
> > > they might be surprised if they switch drivers.
>
> I can see how you might want to have that kind of behaviour, but you'd
> have to jump through some hoops to see if the coredump you saw is
> actually the right one - you probably want an asynchronous coredump
> "collector" and then wait for it to show up (with some reasonable
> timeout) on the actual filesystem, not on sysfs?
>
> Otherwise you have to trawl sysfs for the right coredump I guess, which
> too is possible.

It's not that I want that interface. It's that I want the *lack* of
such an interface to be guaranteed in the documentation. When the
questions like "where? when?" are not answered in the doc, users are
totally allowed to speculate ;) Perhaps the "where" can be deferred to
other documentation (which should probably exist someday), but the
"when" should be listed as "eventually; or not at all; listen for a
uevent."

> > > > You are right. Clearly I did not reach the end my learning curve here. I
> > > > assumed referring to the existing dev_coredump facility was sufficient, 
> > > > but
> > > > maybe it is worth a patch to be more explicit and mention the uevent
> > > > behavior. Also dev_coredump facility may be disabled upon which the 
> > > > trigger
> > > > will have no effect in sysfs. In the kernel the data passed by the 
> > > > driver is
> > > > simply freed by dev_coredump facility.
> > >
> > > Is there any other documentation for the coredump feature? I don't
> > > really see much.
> >
> > Any other than the code itself you mean? I am not sure. Maybe Johannes
> > knows.
>
> There isn't really, it originally was really simple, but then somebody
> (Kees perhaps?) requested a way to turn it off forever for security or
> privacy concerns and it became more complicated.

Then I don't think when adding a new sysfs ABI, we should be deferring
to "existing dev_coredump facility [documentation]" (which doesn't
exist). And just a few words about the user-facing interface would be
nice for the documentation. There previously wasn't any official way
to trigger a dump from userspace -- only from random debugfs files, I
think, or from unspecified device failures.

> > > static ssize_t coredump_store(struct device *dev, struct device_attribute 
> > > *attr,
> > > const char *buf, size_t count)
> > > {
> > > device_lock(dev);
> > > if (dev->driver->coredump)
> > > dev->driver->coredump(dev);
> > > device_unlock(dev);
> > >
> > > return count;
> > > }
> > > static DEVICE_ATTR_WO(coredump);
> > >
> > > Is that a bug or a feature?
> >
> > Yeah. Let's call it a bug. Just not sure what to go for. Return the
> > error or change coredump callback to void return type.
>
> I'm not sure it matters all that much - the underlying devcoredump
> calls all have no return value (void), and given the above complexities
> with the ability to turn off devcoredumping entirely you cannot rely on
> this return value to tell you if a dump was created or not, at least
> not without much more infrastructure work.

Then perhaps it makes sense to remove the return code before you
create users of it.

Brian

Re: [PATCH 2/3] mwifiex: support sysfs initiated device coredump

[Brian Norris]

Hi Arend,

On Thu, Feb 22, 2018 at 01:17:56PM +0100, Arend van Spriel wrote:
> On 2/21/2018 11:59 PM, Brian Norris wrote:
> > On Wed, Feb 21, 2018 at 11:50:19AM +0100, Arend van Spriel wrote:
> > > Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops")
> > > it is possible to initiate a device coredump from user-space. This
> > > patch adds support for it adding the .coredump() driver callback.
> > > As there is no longer a need to initiate it through debugfs remove
> > > that code.
> > > 
> > > Signed-off-by: Arend van Spriel <arend.vanspr...@broadcom.com>
> > > ---
> > >   drivers/net/wireless/marvell/mwifiex/debugfs.c | 31 
> > > +-
> > >   drivers/net/wireless/marvell/mwifiex/pcie.c| 19 ++--
> > >   drivers/net/wireless/marvell/mwifiex/sdio.c| 13 +++
> > >   drivers/net/wireless/marvell/mwifiex/usb.c | 14 
> > >   4 files changed, 45 insertions(+), 32 deletions(-)
> > 
> > The documentation doesn't really say [1], but is the coredump supposed
> > to happen synchronously? Because the mwifiex implementation is
> > asynchronous, whereas it looks like the brcmfmac one is synchronous.
> 
> Well, that depends on the eye of the beholder I guess. From user-space
> perspective it is asynchronous regardless. A write access to the coredump
> sysfs file eventually results in a uevent when the devcoredump entry is
> created, ie. after driver has made a dev_coredump API call. Whether the
> driver does that synchronously or asynchronously is irrelevant as far as
> user-space is concerned.

Is it really? The driver infrastructure seems to guarantee that the
entirety of a driver's ->coredump() will complete before returning from
the write. So it might be reasonable for some user to assume (based on
implementation details, e.g., of brcmfmac) that the devcoredump will be
ready by the time the write() syscall returns, absent documentation that
says otherwise. But then, that's not how mwifiex works right now, so
they might be surprised if they switch drivers.

Anyway, *I'm* already personally used to these dumps being asynchronous,
and writing tooling to listen for the uevent instead. But that doesn't
mean everyone will be.

Also, due to the differences in async/sync, mwifiex doesn't really
provide you much chance for error handling, because most errors would be
asynchronous. So brcmfmac's "coredump" has more chance for user programs
to error-check than mwifiex's (due to the asynchronous nature) [1].

BTW, I push on this mostly because this is migrating from a debugfs
feature (that is easy to hand-wave off as not really providing a
consistent/stable API, etc., etc.) to a documented sysfs feature. If it
were left to rot in debugfs, I probably wouldn't be as bothered ;)

> > Brian
> > 
> > [1] In fact, the ABI documentation really just describes kernel
> > internals, rather than documenting any user-facing details, from what I
> > can tell.
> 
> You are right. Clearly I did not reach the end my learning curve here. I
> assumed referring to the existing dev_coredump facility was sufficient, but
> maybe it is worth a patch to be more explicit and mention the uevent
> behavior. Also dev_coredump facility may be disabled upon which the trigger
> will have no effect in sysfs. In the kernel the data passed by the driver is
> simply freed by dev_coredump facility.

Is there any other documentation for the coredump feature? I don't
really see much.

Brian

[1] Oh wait, but I see that while ->coredump() has an integer return
code...the caller ignores it:

static ssize_t coredump_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
device_lock(dev);
if (dev->driver->coredump)
dev->driver->coredump(dev);
device_unlock(dev);

return count;
}
static DEVICE_ATTR_WO(coredump);

Is that a bug or a feature?

Re: [PATCH 2/3] mwifiex: support sysfs initiated device coredump

[Brian Norris]

On Wed, Feb 21, 2018 at 11:50:19AM +0100, Arend van Spriel wrote:
> Since commit 3c47d19ff4dc ("drivers: base: add coredump driver ops")
> it is possible to initiate a device coredump from user-space. This
> patch adds support for it adding the .coredump() driver callback.
> As there is no longer a need to initiate it through debugfs remove
> that code.
> 
> Signed-off-by: Arend van Spriel 
> ---
>  drivers/net/wireless/marvell/mwifiex/debugfs.c | 31 
> +-
>  drivers/net/wireless/marvell/mwifiex/pcie.c| 19 ++--
>  drivers/net/wireless/marvell/mwifiex/sdio.c| 13 +++
>  drivers/net/wireless/marvell/mwifiex/usb.c | 14 
>  4 files changed, 45 insertions(+), 32 deletions(-)

The documentation doesn't really say [1], but is the coredump supposed
to happen synchronously? Because the mwifiex implementation is
asynchronous, whereas it looks like the brcmfmac one is synchronous.

Brian

[1] In fact, the ABI documentation really just describes kernel
internals, rather than documenting any user-facing details, from what I
can tell.

Re: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX stall

[Brian Norris]

Hi,

On Sun, Jan 28, 2018 at 11:19 PM, Ganapathi Bhat <gb...@marvell.com> wrote:
>> From: Ganapathi Bhat
>> > From: Brian Norris [mailto:briannor...@chromium.org]
>> > On Thu, Jan 25, 2018 at 09:59:04AM +, Ganapathi Bhat wrote:
>> > > > I can't find any commit with id c7dbdcb2a4e1, is it correct?
>> > > Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent
>> > > in this
>> > series.
>> >
>> > What? Why would you introduce a bug and only fix it in the next patch?
>> With the first patch the original issue took considerably longer time to
>> recreate. Also it followed a different path to get recreated(shared in commit
>> message).
>> > Does that mean you should just combine the two?
>> Let us know if that is fine to merge them. We can modify the commit
>> message accordingly.
>> > Or reverse the order, if patch 2 doesn't cause problems on its own?
>> Patch 2 has a dependency on patch 1.
> One correction. There is no commit dependency between patch 1 and 2(they can 
> be applied in any order). But the result would be same. We need both fixes. 
> Let us know if we can combine them.

I haven't closely looked at patch 2 yet. My only statement was that it
makes no sense to have 2 commits, with the second one labeled as
"Fixing" the first one. From your descriptions, it sounds like patch 2
should actually come first, but I'm not really sure. I only looked far
enough to say that I didn't like patch 1 as-is :)

Brian

Re: [PATCH 1/2] mwifiex: schedule rx_work on RX interrupt for USB

[Brian Norris]

On Mon, Jan 22, 2018 at 08:34:56PM +0530, Ganapathi Bhat wrote:
> From: Shrenik Shikhare 
> 
> There is race for data_received flag between main thread and
> RX data interrupt(mwifiex_usb_rx_complete()):
> 1. USB received an RX data interrupt, set data_received flag
> 2. main thread checks data_received, if set queues rx_work

Stop right there.

There is a flag, data_received, and as you say, you are setting it one
thread, and reading it in another thread (and later clearing it; step
#5). Where is the locking? There is none. Therefore, you have a data
race.

You are not resolving any locking problems here, so you're not really
solving the entire problem.

Brian

> 3. rx worker thread independently start processing rx_data_q
> 4. rx work exits (once rx_data_q is empty)
> 5. main thread resets the data_received flag(after #2)
> 6. Now at the corner case there will be high RX data interrupts
> between #4 and #5
> 7. Driver stops submitting URBs to firmware, once rx_pending
> exceeds HIGH_RX_PENDING
> 8. The flag data_received(cleared in #5) will remain unset since
> there will be no interrupts from firmware to set it(after #7)
> 
> Above scenario causes RX stall in driver, which will finally
> result in command/TX timeouts in firmware.
> 
> As a fix, queue rx_work directly in mwifiex_usb_rx_complete()
> callback, instead in the main thread. This removes dependency
> of RX processing on data_received flag.
> 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 7 ---
>  drivers/net/wireless/marvell/mwifiex/main.h | 1 +
>  drivers/net/wireless/marvell/mwifiex/usb.c  | 2 ++
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index 12e7399..6e6e1a7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -171,7 +171,7 @@ void mwifiex_queue_main_work(struct mwifiex_adapter 
> *adapter)
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_queue_main_work);
>  
> -static void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
> +void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter)
>  {
>   unsigned long flags;
>  
> @@ -183,6 +183,7 @@ static void mwifiex_queue_rx_work(struct mwifiex_adapter 
> *adapter)
>   queue_work(adapter->rx_workqueue, >rx_work);
>   }
>  }
> +EXPORT_SYMBOL_GPL(mwifiex_queue_rx_work);
>  
>  static int mwifiex_process_rx(struct mwifiex_adapter *adapter)
>  {
> @@ -283,10 +284,10 @@ int mwifiex_main_process(struct mwifiex_adapter 
> *adapter)
>   mwifiex_process_hs_config(adapter);
>   if (adapter->if_ops.process_int_status)
>   adapter->if_ops.process_int_status(adapter);
> + if (adapter->rx_work_enabled && adapter->data_received)
> + mwifiex_queue_rx_work(adapter);
>   }
>  
> - if (adapter->rx_work_enabled && adapter->data_received)
> - mwifiex_queue_rx_work(adapter);
>  
>   /* Need to wake up the card ? */
>   if ((adapter->ps_state == PS_STATE_SLEEP) &&
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index 6b5539b..66ba95c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1667,6 +1667,7 @@ u8 mwifiex_adjust_data_rate(struct mwifiex_private 
> *priv,
>  void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter);
>  void *mwifiex_alloc_dma_align_buf(int rx_len, gfp_t flags);
>  void mwifiex_queue_main_work(struct mwifiex_adapter *adapter);
> +void mwifiex_queue_rx_work(struct mwifiex_adapter *adapter);
>  int mwifiex_get_wakeup_reason(struct mwifiex_private *priv, u16 action,
> int cmd_type,
> struct mwifiex_ds_wakeup_reason *wakeup_reason);
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c 
> b/drivers/net/wireless/marvell/mwifiex/usb.c
> index 4bc2448..d20fda1 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -144,6 +144,8 @@ static int mwifiex_usb_recv(struct mwifiex_adapter 
> *adapter,
>   skb_queue_tail(>rx_data_q, skb);
>   adapter->data_received = true;
>   atomic_inc(>rx_pending);
> + if (adapter->rx_work_enabled)
> + mwifiex_queue_rx_work(adapter);
>   break;
>   default:
>   mwifiex_dbg(adapter, ERROR,
> -- 
> 1.9.1
> 

Re: [EXT] Re: [2/2] mwifiex: use more_rx_task_flag to avoid USB RX stall

[Brian Norris]

On Thu, Jan 25, 2018 at 09:59:04AM +, Ganapathi Bhat wrote:
> > I can't find any commit with id c7dbdcb2a4e1, is it correct?
> Correct. Actually the commit id c7dbdcb2a4e1 is the PATCH [1/2] sent in this 
> series.

What? Why would you introduce a bug and only fix it in the next patch?
Does that mean you should just combine the two? Or reverse the order, if
patch 2 doesn't cause problems on its own?

Brian

Re: [1/2] mwifiex: schedule rx_work on RX interrupt for USB

[Brian Norris]

On Thu, Jan 25, 2018 at 07:10:52AM +, Kalle Valo wrote:
> Ganapathi Bhat  wrote:
> 
> > From: Shrenik Shikhare 
> > 
> > There is race for data_received flag between main thread and
> > RX data interrupt(mwifiex_usb_rx_complete()):
> > 1. USB received an RX data interrupt, set data_received flag
> > 2. main thread checks data_received, if set queues rx_work
> > 3. rx worker thread independently start processing rx_data_q
> > 4. rx work exits (once rx_data_q is empty)
> > 5. main thread resets the data_received flag(after #2)
> > 6. Now at the corner case there will be high RX data interrupts
> > between #4 and #5
> > 7. Driver stops submitting URBs to firmware, once rx_pending
> > exceeds HIGH_RX_PENDING
> > 8. The flag data_received(cleared in #5) will remain unset since
> > there will be no interrupts from firmware to set it(after #7)
> > 
> > Above scenario causes RX stall in driver, which will finally
> > result in command/TX timeouts in firmware.
> > 
> > As a fix, queue rx_work directly in mwifiex_usb_rx_complete()
> > callback, instead in the main thread. This removes dependency
> > of RX processing on data_received flag.
> > 
> > Signed-off-by: Cathy Luo 
> > Signed-off-by: Ganapathi Bhat 
> 
> Brian, did you have a chance to review these two?

Not really. I don't generally make a lot of time to review the USB
driver unless it's really screwing around with the main driver, since I
don't use the USB driver. But I'll try to give it a few glances.

[PATCH 1/2] Revert "mwifiex: cancel pcie/sdio work in remove/shutdown handler"

[Brian Norris]

This reverts commit b713bbf1471b56b572ce26bd02b81a85c2b007f4.

The "fix" in question does not actually fix all related problems, and it
also introduces new deadlock possibilities. Since commit b014e96d1abb
("PCI: Protect pci_error_handlers->reset_notify() usage with
device_lock()"), the race in question is actually resolved (PCIe reset
cannot happen at the same time as remove()). Instead, this "fix" just
introduces a deadlock where mwifiex_pcie_card_reset_work() is waiting on
device_lock, which is held by PCIe device remove(), which is waiting
on...mwifiex_pcie_card_reset_work().

The proper thing to do is just to fix the deadlock. Patch for this will
come separately.

Cc: Signed-off-by: Xinming Hu <h...@marvell.com>
Signed-off-by: Brian Norris <briannor...@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 2 --
 drivers/net/wireless/marvell/mwifiex/sdio.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 23209c5cab05..f666cb2ea7e0 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -310,8 +310,6 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
}
 
-   cancel_work_sync(>work);
-
mwifiex_remove_card(adapter);
 }
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 248858723753..a82880132af4 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -399,8 +399,6 @@ mwifiex_sdio_remove(struct sdio_func *func)
mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
}
 
-   cancel_work_sync(>work);
-
mwifiex_remove_card(adapter);
 }
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 2/2] mwifiex: resolve reset vs. remove()/shutdown() deadlocks

[Brian Norris]

Commit b014e96d1abb ("PCI: Protect pci_error_handlers->reset_notify()
usage with device_lock()") resolves races between driver reset and
removal, but it introduces some new deadlock problems. If we see a
timeout while we've already started suspending, removing, or shutting
down the driver, we might see:

(a) a worker thread, running mwifiex_pcie_work() ->
mwifiex_pcie_card_reset_work() -> pci_reset_function()
(b) a removal thread, running mwifiex_pcie_remove() ->
mwifiex_free_adapter() -> mwifiex_unregister() ->
mwifiex_cleanup_pcie() -> cancel_work_sync(>work)

Unfortunately, mwifiex_pcie_remove() already holds the device lock that
pci_reset_function() is now requesting, and so we see a deadlock.

It's necessary to cancel and synchronize our outstanding work before
tearing down the driver, so we can't have this work wait indefinitely
for the lock.

It's reasonable to only "try" to reset here, since this will mostly
happen for cases where it's already difficult to reset the firmware
anyway (e.g., while we're suspending or powering off the system). And if
reset *really* needs to happen, we can always try again later.

Fixes: b014e96d1abb ("PCI: Protect pci_error_handlers->reset_notify() usage 
with device_lock()")
Cc: <sta...@vger.kernel.org>
Cc: Xinming Hu <h...@marvell.com>
Signed-off-by: Brian Norris <briannor...@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index f666cb2ea7e0..97a6199692ab 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2786,7 +2786,10 @@ static void mwifiex_pcie_card_reset_work(struct 
mwifiex_adapter *adapter)
 {
struct pcie_service_card *card = adapter->card;
 
-   pci_reset_function(card->dev);
+   /* We can't afford to wait here; remove() might be waiting on us. If we
+* can't grab the device lock, maybe we'll get another chance later.
+*/
+   pci_try_reset_function(card->dev);
 }
 
 static void mwifiex_pcie_work(struct work_struct *work)
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: Re: Re: [PATCH] mwifiex: cancel pcie/sdio work in remove/shutdown handler

[Brian Norris]

On Thu, Jan 11, 2018 at 06:25:09PM -0800, Brian Norris wrote:
> Anyway, I'll do my own testing and then submit my patch properly.

OK, so I definitely confirmed: if your patch does anything, it
introduces a new deadlock possibility. Just trigger a Wifi timeout or
reset from within remove(), and you'll see the work event get stuck in
pci_reset_function(), while remove() gets stuck at cancel_work_sync().

I also confirmed that my patch resolves this problem.

I'll send the revert + my patch now.

Brian

Re: Re: Re: [PATCH] mwifiex: cancel pcie/sdio work in remove/shutdown handler

[Brian Norris]

Hi Simon,

On Wed, Jan 10, 2018 at 12:30:35PM +, Xinming Hu wrote:
> > -Original Message-
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > On Tue, Jan 09, 2018 at 11:42:21AM +, Xinming Hu wrote:
> > > I am not sure whether there is any mutual exclusion protect between
> > pcie_reset and pcie_remove in pcie core.
> > > But it looks a different race.
> > > We still need this fix, right?
> > 
> > Good point. Previously, there wasn't any such exclusion, and that's why 
> > races
> > like the above were even more likely. But as of 4.13, now there
> > *is* exclusion. See commit b014e96d1abb ("PCI: Protect
> > pci_error_handlers->reset_notify() usage with device_lock()"). That 
> > incidentally
> > means that you're creating a deadlock with this patch! [1]
> > 
> > If we start a timeout/reset sequence in mwifiex_init_shutdown_fw() (called
> > from remove()), then you'll eventually have pci_reset_function() waiting on 
> > the
> > device lock, but mwifiex_pcie_remove() will be holding the device lock 
> > already,
> > and now (with your patch), remove() will be waiting on the worker thread to
> > finish pci_reset_function()...deadlock!
> > 
> > I actually think that the above patch (adding device_lock()) resolves most 
> > of the
> > race but introduces a possible deadlock. I believe an easy solution is just 
> > to
> > switch to pci_try_reset_function() instead? That will just abort a reset 
> > attempt
> > if we're in the middle of removing the device. Problem solved? Diff 
> > appended,
> > but I'll send out a real version if that looks right. Can you test your 
> > original
> > problem with the above commit from Christopher, as well as the appended
> > diff?
> > 
> 
> Since I don't have the original customer platform, which is 4.1 based.

You could suggest your customer try the aforementioned commit + my
patch. It will likely get them a more reliable result in the end.

> Just run the stress test on my 4.14, with commands:
> while true; do rmmod mwifiex;insmod $mwd/mwifiex.ko; sleep 1; insmod 
> $mwd/mwifiex_pcie.ko; sleep 1; rmmod mwifiex_pcie & echo "1" > 
> /sys/kernel/debug/mwifiex/mlan0/reset;done &

That's actually not much of a good test, since the mutual exclusion of
reset and remove will mean that 'rmmod' and 'echo 1 > ... /reset' won't
really be doing anything significant in parallel.

What you'd really need to do is fake a timeout from within remove().
e.g., you could do

adapter->if_ops.card_reset(adapter);

plus some sleep (to let the work event get started) followed by the
cancel_work_sync() of your patch.

> 
> Till now, I didn't hit deadlock with/without below diff, though it
> looks finally will be reached, according to the above explanation.  I
> guess, maybe rmmod operation is slowly then sysfs operation.

Per my above explanation, you won't hit the deadlock like that.

Anyway, I'll do my own testing and then submit my patch properly.

Brian

Re: Re: [PATCH] mwifiex: cancel pcie/sdio work in remove/shutdown handler

[Brian Norris]

+ Christopher

Hi Simon and Kalle,

On Tue, Jan 09, 2018 at 11:42:21AM +, Xinming Hu wrote:
> Hi,
> 
> > -Original Message-
> > From: Kalle Valo [mailto:kv...@codeaurora.org]
> > Sent: 2018年1月9日 15:40
> > To: Brian Norris <briannor...@chromium.org>
> > Cc: Xinming Hu <h...@marvell.com>; Linux Wireless
> > <linux-wireless@vger.kernel.org>; Dmitry Torokhov <d...@google.com>;
> > raja...@google.com; Zhiyuan Yang <yan...@marvell.com>; Tim Song
> > <song...@marvell.com>; Cathy Luo <c...@marvell.com>; James Cao
> > <j...@marvell.com>; Ganapathi Bhat <gb...@marvell.com>; Ellie Reeves
> > <ellierev...@gmail.com>
> > Subject: [EXT] Re: [PATCH] mwifiex: cancel pcie/sdio work in remove/shutdown
> > handler
> > 
> > External Email
> > 
> > --
> > Brian Norris <briannor...@chromium.org> writes:
> > 
> > >> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > >> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > >> @@ -310,6 +310,8 @@ static void mwifiex_pcie_remove(struct pci_dev
> > *pdev)
> > >>  mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
> > >>  }
> > >>
> > >> +cancel_work_sync(>work);
> > >> +
> > >
> > > Just FYI, this "fix" is not a real fix. It will likely paper over some
> > > of your bugs (where, e.g., the FW shutdown command times out in the
> > > previous couple of lines), but this highlights the fact that there are
> > > other races that could trigger the same behavior. You're not fixing
> > > those.
> > >
> > > For example, what if somebody initiates a scan or other nl80211
> > > command between the above line and mwifiex_remove_card()? That
> > command
> > > could potentially time out too.
> > >
> 
> The hardware status have been reset before downloading the last
> command(FUNC SHUTDOWN), in this way, follow commands  download will be
> ignored and warned.

Hmm, I suppose that's true. So the race I'm talking about probably can't
happen usually. What about in manufacturing mode or !FIRMWARE_READY_PCIE
though? Those cases don't shut down the firmware. Can we still have
outstanding timeouts in those cases?

Anyway, I still think there's a problem though, and this patch is just
going to make things worse. See below.

> > > The proper fix would be to institute some kind of mutual exclusion
> > > (locking) between mwifiex_shutdown_sw() and mwifiex_remove_card(), so
> > > that they can't occur at the same time.
> > >
> 
> I am not sure whether there is any mutual exclusion protect between 
> pcie_reset and pcie_remove in pcie core.
> But it looks a different race.
> We still need this fix, right?

Good point. Previously, there wasn't any such exclusion, and that's why
races like the above were even more likely. But as of 4.13, now there
*is* exclusion. See commit b014e96d1abb ("PCI: Protect
pci_error_handlers->reset_notify() usage with device_lock()"). That
incidentally means that you're creating a deadlock with this patch! [1]

If we start a timeout/reset sequence in mwifiex_init_shutdown_fw()
(called from remove()), then you'll eventually have pci_reset_function()
waiting on the device lock, but mwifiex_pcie_remove() will be holding
the device lock already, and now (with your patch), remove() will be
waiting on the worker thread to finish pci_reset_function()...deadlock!

I actually think that the above patch (adding device_lock()) resolves
most of the race but introduces a possible deadlock. I believe an easy
solution is just to switch to pci_try_reset_function() instead? That
will just abort a reset attempt if we're in the middle of removing the
device. Problem solved? Diff appended, but I'll send out a real version
if that looks right. Can you test your original problem with the above
commit from Christopher, as well as the appended diff?

> Regards,
> Simon
> > > Unfortunately, I only paid attention to this after Kalle already
> > > applied this patch. Personally, I'd prefer this patch not get applied,
> > > since it's a bad solution to an obvious problem, which instead leaves
> > > a subtle problem that perhaps no one will bother fixing.
> > 
> > I can revert it, that's not a problem. Can I use the text below as 
> > explanation for
> > the revert?
> > 
> > --
> > Brian Norris <briannor...@chromium.org> says:
> > 
> > Just FYI, 

Re: [PATCH] mwifiex: cancel pcie/sdio work in remove/shutdown handler

[Brian Norris]

Hi,

On Wed, Dec 13, 2017 at 07:27:53PM +0800, Xinming Hu wrote:
> The last command used to shutdown firmware might be timeout,
> and trigger firmware dump in asynchronous pcie/sdio work.
> 
> The remove/shutdown handler will continue free core data
> structure private/adapter, which might be dereferenced in
> pcie/sdio work, finally crash the kernel.
> 
> Sync and Cancel pcie/sdio work, could be a fix for above
> cornel case. In this way, the last command timeout could

s/cornel/corner/

> be handled properly.
> 
> Signed-off-by: Xinming Hu 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 2 ++
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index f666cb2..23209c5 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -310,6 +310,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>   mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
>   }
>  
> + cancel_work_sync(>work);
> +

Just FYI, this "fix" is not a real fix. It will likely paper over some
of your bugs (where, e.g., the FW shutdown command times out in the
previous couple of lines), but this highlights the fact that there are
other races that could trigger the same behavior. You're not fixing
those.

For example, what if somebody initiates a scan or other nl80211 command
between the above line and mwifiex_remove_card()? That command could
potentially time out too.

The proper fix would be to institute some kind of mutual exclusion
(locking) between mwifiex_shutdown_sw() and mwifiex_remove_card(), so
that they can't occur at the same time.

Unfortunately, I only paid attention to this after Kalle already applied
this patch. Personally, I'd prefer this patch not get applied, since
it's a bad solution to an obvious problem, which instead leaves a subtle
problem that perhaps no one will bother fixing.

Brian

>   mwifiex_remove_card(adapter);
>  }
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index a828801..2488587 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -399,6 +399,8 @@ static int mwifiex_check_winner_status(struct 
> mwifiex_adapter *adapter)
>   mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
>   }
>  
> + cancel_work_sync(>work);
> +
>   mwifiex_remove_card(adapter);
>  }
>  
> -- 
> 1.9.1
> 

Re: [PATCH for-4.15] wireless: create, don't append, to shipped-certs.c

[Brian Norris]

On Tue, Dec 19, 2017 at 09:11:30PM +0100, Johannes Berg wrote:
> I'm not really sure what that problem was - I think a combination of
> missing clean-files and this perhaps? Anyway, I applied another fix for
> this today, and it's on the way to Linus's tree, along with the missing
> clean-files and a conversion to non-binary files so that patches work
> better.

Ah, I didn't see that. I guess you just applied it a few hours ago, and
it didn't trickle into -next yet.

Thanks,
Brian

[PATCH for-4.15] wireless: create, don't append, to shipped-certs.c

[Brian Norris]

The current rule for generating the {shipped,extra}-certs.c source files
might create an invalid C source file, containing redefinitions of the
same variables:

  CC [M]  net/wireless/shipped-certs.o
net/wireless/shipped-certs.c:686:10: error: redefinition of 
'shipped_regdb_certs'
 const u8 shipped_regdb_certs[] = {
  ^
net/wireless/shipped-certs.c:2:10: note: previous definition of 
'shipped_regdb_certs' was here
 const u8 shipped_regdb_certs[] = {
  ^
net/wireless/shipped-certs.c:1368:14: error: redefinition of 
'shipped_regdb_certs_len'
 unsigned int shipped_regdb_certs_len = sizeof(shipped_regdb_certs);
  ^
net/wireless/shipped-certs.c:684:14: note: previous definition of 
'shipped_regdb_certs_len' was here
 unsigned int shipped_regdb_certs_len = sizeof(shipped_regdb_certs);
  ^

This can be easily triggered by forcing a rebuild of these files:

  $ touch net/wireless/certs/sforshee.x509
  $ make

In practice, this is seen often by having a separate source and build
directory, where the build artifacts remain but the source tree changes
(even if Seth's cert doesn't change, it might get created/removed when
checking out different source revisions).

I don't see why this rule should be an append; we're writing the file
all in one go.

Fixes: 90a53e4432b1 ("cfg80211: implement regdb signature checking")
Signed-off-by: Brian Norris <briannor...@chromium.org>
---
This is an error introduced in 4.15-rc1.

I've seen other errors reported by Paul and Damian (CC'd); I think Paul's
failure was fixed already, but Damian might have still been having problems
with not having a "clean" environment. Perhaps he was hitting this bug?

 net/wireless/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/wireless/Makefile b/net/wireless/Makefile
index d7d6cb00c47b..b662be3422e1 100644
--- a/net/wireless/Makefile
+++ b/net/wireless/Makefile
@@ -43,7 +43,7 @@ $(obj)/shipped-certs.c: $(wildcard 
$(srctree)/$(src)/certs/*.x509)
  echo "$$allf"; \
  echo '};'; \
  echo 'unsigned int shipped_regdb_certs_len = 
sizeof(shipped_regdb_certs);'; \
- ) >> $@)
+ ) > $@)
 
 $(obj)/extra-certs.c: $(CONFIG_CFG80211_EXTRA_REGDB_KEYDIR:"%"=%) \
  $(wildcard 
$(CONFIG_CFG80211_EXTRA_REGDB_KEYDIR:"%"=%)/*.x509)
@@ -66,4 +66,4 @@ $(obj)/extra-certs.c: 
$(CONFIG_CFG80211_EXTRA_REGDB_KEYDIR:"%"=%) \
  echo "$$allf"; \
  echo '};'; \
  echo 'unsigned int extra_regdb_certs_len = 
sizeof(extra_regdb_certs);'; \
- ) >> $@)
+ ) > $@)
-- 
2.15.1.504.g5279b80103-goog

Re: [PATCH v4 1/3] mwifiex: refactor device dump code to make it generic for usb interface

[Brian Norris]

Hi,

On Mon, Dec 04, 2017 at 08:18:42PM +0800, Xinming Hu wrote:
> This patch refactor current device dump code to make it generic
> for subsequent implementation on usb interface.

I still think you're making the spaghetti worse. I only have a few
specific suggestions for improving your spaghetti code at the moment,
but I'm still sure you could do better.

> Signed-off-by: Xinming Hu <h...@marvell.com>
> Signed-off-by: Cathy Luo <c...@marvell.com>
> Signed-off-by: Ganapathi Bhat <gb...@marvell.com>
> ---
> v2: Addressed below review comments from Brian Norris
> a) use new API timer_setup/from_timer.
> b) reset devdump_len during initization
> v4: Same as v2,v3
> ---
>  drivers/net/wireless/marvell/mwifiex/init.c |  1 +
>  drivers/net/wireless/marvell/mwifiex/main.c | 87 
> +++--
>  drivers/net/wireless/marvell/mwifiex/main.h | 11 +++-
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 13 +++--
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 14 +++--
>  5 files changed, 72 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index e1aa860..b0d3d68 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -314,6 +314,7 @@ static void mwifiex_init_adapter(struct mwifiex_adapter 
> *adapter)
>   adapter->iface_limit.p2p_intf = MWIFIEX_MAX_P2P_NUM;
>   adapter->active_scan_triggered = false;
>   timer_setup(>wakeup_timer, wakeup_timer_fn, 0);
> + adapter->devdump_len = 0;
>  }
>  
>  /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index a96bd7e..f7d0299 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1051,9 +1051,23 @@ void mwifiex_multi_chan_resync(struct mwifiex_adapter 
> *adapter)
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_multi_chan_resync);
>  
> -int mwifiex_drv_info_dump(struct mwifiex_adapter *adapter, void **drv_info)
> +void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
>  {
> - void *p;
> + /* Dump all the memory data into single file, a userspace script will
> +  * be used to split all the memory data to multiple files
> +  */
> + mwifiex_dbg(adapter, MSG,
> + "== mwifiex dump information to /sys/class/devcoredump 
> start\n");
> + dev_coredumpv(adapter->dev, adapter->devdump_data, adapter->devdump_len,
> +   GFP_KERNEL);

Seems like you should reset adapter->devdump_data and ->devdump_len
here, so you don't accidentally reuse it? (You're expecting
dev_coredumpv() to free the buffer, no?)

> + mwifiex_dbg(adapter, MSG,
> + "== mwifiex dump information to /sys/class/devcoredump 
> end\n");
> +}
> +EXPORT_SYMBOL_GPL(mwifiex_upload_device_dump);
> +
> +void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
> +{
> + char *p;
>   char drv_version[64];
>   struct usb_card_rec *cardp;
>   struct sdio_mmc_card *sdio_card;
> @@ -1061,17 +1075,12 @@ int mwifiex_drv_info_dump(struct mwifiex_adapter 
> *adapter, void **drv_info)
>   int i, idx;
>   struct netdev_queue *txq;
>   struct mwifiex_debug_info *debug_info;
> - void *drv_info_dump;
>  
>   mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump start===\n");
>  
> - /* memory allocate here should be free in mwifiex_upload_device_dump*/
> - drv_info_dump = vzalloc(MWIFIEX_DRV_INFO_SIZE_MAX);
> -
> - if (!drv_info_dump)
> - return 0;
> -
> - p = (char *)(drv_info_dump);
> + p = adapter->devdump_data;
> + strcpy(p, "Start dump driverinfo\n");
> + p += strlen("Start dump driverinfo\n");
>   p += sprintf(p, "driver_name = " "\"mwifiex\"\n");
>  
>   mwifiex_drv_get_driver_version(adapter, drv_version,
> @@ -1155,21 +1164,18 @@ int mwifiex_drv_info_dump(struct mwifiex_adapter 
> *adapter, void **drv_info)
>   kfree(debug_info);
>   }
>  
> + strcpy(p, "\nEnd dump\n");
> + p += strlen("\nEnd dump\n");
>   mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump end===\n");
> - *drv_info = drv_info_dump;
> - return p - drv_info_dump;
> + adapter->devdump_len = p - (char *)adapter->devdump_data;
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_drv_info_dump);
>  
> -void mwifiex_upload_device_dump(struct mwifi

Re: Re: Re: [PATCH 3/3] mwifiex: debugfs: trigger device dump for usb interface

[Brian Norris]

On Fri, Dec 01, 2017 at 08:36:01AM +, Xinming Hu wrote:
> Thanks for the suggestion, it sounds better than exporting
> mwifiex_send_cmd() directly.
> In addition to used as debugfs tirgger, the "defined point"
> if_ops.device_dump is only used in command timeout context.
> For sdio/pcie interface, register operation will be made to trigger
> firmware dump and get dump context under specific algorithm.
> For usb interface, however,  this is not needed, since firmware will
> automatically send dump event to host without any trigger, and what's
> more , host is also not able to issue command in the situation.
> 
> So per my understand,  here we only need provide a simple way to
> trigger , instead of a totally functional complete dump entry point.
> Suppose if we make the command trigger a part of if_ops->device_dump,
> then we also need add check for "MWIFIEX_USB" in mwifiex_cmd_tmo_func. 

Ah, I see. Your explanation makes some more sense then. It would be nice
if you could include some of this in
(a) the commit message
(b) the entry point in debugfs.c, where you trigger this

Something along the lines of "For command timeouts, USB firmware will
automatically emit firmware dump events, so we don't implement
device_dump(). For user-initiated dumps, we trigger it ourselves."

> it also looks inelegant, and what we did looks weird, they are
> (1) export a new  kernel symbol,  the wrapper of mwifiex_send_command
> (2) add usb if_ops->device_dump, it  send the command in mwifiex_usb, instead 
> of in mwifiex itself.
> (3) bypass above "if_ops->device_dump" in mwifiex_cmd_tmo_func, which is the 
> mainly user case.

No, I'm not sure that solution would be much better. Your existing
solution with additional comments is probably fine.

> I am not sure whether there is a better way on this, perhaps we need a
> trade-off on different solutions, please let us know if you have more
> suggestions.
> 
> Thanks & Regards,
> SImon

Brian

Re: Re: [PATCH 3/3] mwifiex: debugfs: trigger device dump for usb interface

[Brian Norris]

Hi Simon,

On Wed, Nov 15, 2017 at 11:31:05AM +, Xinming Hu wrote:
> > -Original Message-
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > 
> > On Mon, Aug 14, 2017 at 12:19:03PM +, Xinming Hu wrote:
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c
> > > b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> > > index 6f4239b..5d476de 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> > > @@ -168,10 +168,11 @@
> > >  {
> > >   struct mwifiex_private *priv = file->private_data;
> > >
> > > - if (!priv->adapter->if_ops.device_dump)
> > > - return -EIO;
> > > -
> > > - priv->adapter->if_ops.device_dump(priv->adapter);
> > > + if (priv->adapter->iface_type == MWIFIEX_USB)
> > > + mwifiex_send_cmd(priv, HostCmd_CMD_FW_DUMP_EVENT,
> > > +  HostCmd_ACT_GEN_SET, 0, NULL, true);
> > 
> > Why couldn't you just implement the device_dump() callback?
> 
> Currently mwifiex_send_cmd function is not exported to external modules, it 
> is designed for module mwifiex internal use.

If you really don't want to export mwifiex_send_cmd(), you can export
some other wrapper which does the logic for you. The point is that
there's a well defined point for determining how to dump the firmware
based on which interface you're using. You should use it.

> If we export mwifiex_send_cmd, and call it in mwifiex_usb. There will be a 
> loop,

So? There are all sorts of interdependencies between mwifiex.ko and
mwifiex_usb.ko (or in your words, "loops").

> .Device_dump (module mwifiex_usb)  -->  mwifiex_send_cmd(module mwifiex)  --> 
> .host_to_card (module mwifiex_usb)
> 
> This seems not an elegant design, right?

No less elegant than scattering:

if (!USB)
driver->this();
else
that();

all over the place.

> Regards,
> Simon
> > 
> > > + else
> > > + priv->adapter->if_ops.device_dump(priv->adapter);
> > >
> > >   return 0;
> > >  }

Brian

Re: [PATCH v2 3/3] mwifiex: debugfs: trigger device dump for usb interface

[Brian Norris]

On Mon, Nov 27, 2017 at 03:09:15PM +0800, Xinming Hu wrote:
> This patch extend device_dump debugfs function to make it
> works for usb interface.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Cathy Luo 
> ---
> v2: Same as v1

With the same problems as v1 :)

Re: [PATCH 3/3] mwifiex: cleanup rx_reorder_tbl_lock usage

[Brian Norris]

Hi,

I haven't reviewed this patch in its entirety, but I'm pretty sure
you're introducing a reliable AA deadlock. See below.

Are you sure you're actually testing these codepaths?

On Tue, Oct 31, 2017 at 03:12:47PM +0530, Ganapathi Bhat wrote:
> From: Karthik Ananthapadmanabha 
> 
> Fix the incorrect usage of rx_reorder_tbl_lock spinlock:
> 
> 1. We shouldn't access the fields of the elements returned by
> mwifiex_11n_get_rx_reorder_tbl() after we have released spin
> lock. To fix this, To fix this, aquire the spinlock before
> calling this function and release the lock only after processing
> the corresponding element is complete.
> 
> 2. Realsing the spinlock during iteration of the list and holding
> it back before next iteration is unsafe. Fix it by releasing the
> lock only after iteration of the list is complete.
> 
> Signed-off-by: Karthik Ananthapadmanabha 
> Signed-off-by: Ganapathi Bhat 
> ---
>  .../net/wireless/marvell/mwifiex/11n_rxreorder.c   | 34 
> +-
>  drivers/net/wireless/marvell/mwifiex/uap_txrx.c|  3 ++
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c 
> b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> index 4631bc2..b99ace8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> @@ -234,23 +234,19 @@ static int mwifiex_11n_dispatch_pkt(struct 
> mwifiex_private *priv, void *payload)
>  
>  /*
>   * This function returns the pointer to an entry in Rx reordering
> - * table which matches the given TA/TID pair.
> + * table which matches the given TA/TID pair. The caller must
> + * hold rx_reorder_tbl_lock, before calling this function.
>   */
>  struct mwifiex_rx_reorder_tbl *
>  mwifiex_11n_get_rx_reorder_tbl(struct mwifiex_private *priv, int tid, u8 *ta)
>  {
>   struct mwifiex_rx_reorder_tbl *tbl;
> - unsigned long flags;
>  
> - spin_lock_irqsave(>rx_reorder_tbl_lock, flags);
>   list_for_each_entry(tbl, >rx_reorder_tbl_ptr, list) {
>   if (!memcmp(tbl->ta, ta, ETH_ALEN) && tbl->tid == tid) {
> - spin_unlock_irqrestore(>rx_reorder_tbl_lock,
> -flags);
>   return tbl;
>   }
>   }
> - spin_unlock_irqrestore(>rx_reorder_tbl_lock, flags);
>  
>   return NULL;
>  }
> @@ -355,11 +351,14 @@ void mwifiex_11n_del_rx_reorder_tbl_by_ta(struct 
> mwifiex_private *priv, u8 *ta)
>* If we get a TID, ta pair which is already present dispatch all the
>* the packets and move the window size until the ssn
>*/
> + spin_lock_irqsave(>rx_reorder_tbl_lock, flags);
>   tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid, ta);
>   if (tbl) {
>   mwifiex_11n_dispatch_pkt_until_start_win(priv, tbl, seq_num);
> + spin_unlock_irqrestore(>rx_reorder_tbl_lock, flags);
>   return;
>   }
> + spin_unlock_irqrestore(>rx_reorder_tbl_lock, flags);
>   /* if !tbl then create one */
>   new_node = kzalloc(sizeof(struct mwifiex_rx_reorder_tbl), GFP_KERNEL);
>   if (!new_node)
> @@ -571,15 +570,19 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private 
> *priv,
>   u16 pkt_index;
>   bool init_window_shift = false;
>   int ret = 0;
> + unsigned long flags;
>  
> + spin_lock_irqsave(>rx_reorder_tbl_lock, flags);
>   tbl = mwifiex_11n_get_rx_reorder_tbl(priv, tid, ta);
>   if (!tbl) {
> + spin_unlock_irqrestore(>rx_reorder_tbl_lock, flags);
>   if (pkt_type != PKT_TYPE_BAR)
>   mwifiex_11n_dispatch_pkt(priv, payload);
>   return ret;
>   }
>  
>   if ((pkt_type == PKT_TYPE_AMSDU) && !tbl->amsdu) {
> + spin_unlock_irqrestore(>rx_reorder_tbl_lock, flags);
>   mwifiex_11n_dispatch_pkt(priv, payload);
>   return ret;
>   }
> @@ -666,6 +669,8 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private 
> *priv,
>   if (!tbl->timer_context.timer_is_set ||
>   prev_start_win != tbl->start_win)
>   mwifiex_11n_rxreorder_timer_restart(tbl);
> +
> + spin_unlock_irqrestore(>rx_reorder_tbl_lock, flags);
>   return ret;
>  }
>  
> @@ -683,6 +688,7 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private 
> *priv,
>   struct mwifiex_ra_list_tbl *ra_list;
>   u8 cleanup_rx_reorder_tbl;
>   int tid_down;
> + unsigned long flags;
>  
>   if (type == TYPE_DELBA_RECEIVE)
>   cleanup_rx_reorder_tbl = (initiator) ? true : false;
> @@ -693,14 +699,18 @@ int mwifiex_11n_rx_reorder_pkt(struct mwifiex_private 
> *priv,
>   peer_mac, tid, initiator);
>  
>   if (cleanup_rx_reorder_tbl) {
> + spin_lock_irqsave(>rx_reorder_tbl_lock, flags);
>   tbl = 

Re: [PATCH] ath10k: move pci suspend/resume functions

[Brian Norris]

On Thu, Nov 02, 2017 at 04:40:57PM +0100, Arnd Bergmann wrote:
> On Thu, Nov 2, 2017 at 4:23 PM, Kalle Valo  wrote:
> > Brian has already fixed this, please check that:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=ath-next=20665a9076d48e9abd9a2db13d307f58f7ef6647
> >
> > But apparently I forgot to merge ath-next to wireless-drivers-next, will
> > do that soon.
> 
> Yes, Brian's version is better. I considered the same, but wasn't sure
> it was safe.

Yes, it's safe. The code is still basically dead, since the mac80211 ops
get dropped with !CONFIG_PM (so no one calls the
*_hif_{suspend,resume}() functions), but at least we have fewer
#ifdef's.

Brian

Re: [PATCH 1/3] mwifiex: cleanup rx_pkt_lock usage in 11n_rxreorder.c

[Brian Norris]

Hi,

On Tue, Oct 31, 2017 at 03:12:45PM +0530, Ganapathi Bhat wrote:
> From: Karthik Ananthapadmanabha 
> 
> Fix the incorrect usage of rx_pkt_lock spinlock. Realsing the
> spinlock while the element is still in process is unsafe. The
> lock must be released only after the element processing is
> complete.
> 
> Signed-off-by: Karthik Ananthapadmanabha 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c 
> b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> index 1edcdda..0149c4a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> +++ b/drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c
> @@ -124,9 +124,10 @@ static int mwifiex_11n_dispatch_pkt(struct 
> mwifiex_private *priv, void *payload)
>   rx_tmp_ptr = tbl->rx_reorder_ptr[i];
>   tbl->rx_reorder_ptr[i] = NULL;
>   }
> - spin_unlock_irqrestore(>rx_pkt_lock, flags);

So, what is this lock protecting? Is it for protecting the packet
itself, or for protecting the ring buffer? As I read it, it's just the
latter, since once we've removed it from the ring buffer (and are about
to dispatch it up toward the networking layer), it's handled only by a
single context (or else, protected via some other common lock(s)).

If I'm correct above, then I think the current code here is actually
safe, since it holds the lock while removing from the ring buffer --
after it's removed from the ring, there's no way another thread can find
this payload, and so we can release the lock.

On a related note: I don't actually see the ring buffer *insertion*
being protected by this rx_pkt_lock, so is it really useful? See
mwifiex_11n_rx_reorder_pkt(). Perhaps the correct lock is actually
rx_reorder_tbl_lock()? Except that serves multiple purposes it seems...

Another thought: from what I can tell, all of these operations are
*only* performed on the main thread, so there's actually no concurrency
involved at all. So we also could probably drop this lock entirely...

I guess the real point is: can you explain better what you intend this
lock to do? Then we can review whether you're accomplishing your
intention, or whether we should improve the usage of this lock in some
other way, or perhaps even kill it entirely.

Thanks,
Brian

>   if (rx_tmp_ptr)
>   mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> +
> + spin_unlock_irqrestore(>rx_pkt_lock, flags);
>   }
>  
>   spin_lock_irqsave(>rx_pkt_lock, flags);
> @@ -167,8 +168,8 @@ static int mwifiex_11n_dispatch_pkt(struct 
> mwifiex_private *priv, void *payload)
>   }
>   rx_tmp_ptr = tbl->rx_reorder_ptr[i];
>   tbl->rx_reorder_ptr[i] = NULL;
> - spin_unlock_irqrestore(>rx_pkt_lock, flags);
>   mwifiex_11n_dispatch_pkt(priv, rx_tmp_ptr);
> + spin_unlock_irqrestore(>rx_pkt_lock, flags);
>   }
>  
>   spin_lock_irqsave(>rx_pkt_lock, flags);
> -- 
> 1.9.1
> 

Re: [EXT] Re: pull-request mwifiex-firmware 2017-10-30

[Brian Norris]

On Mon, Oct 30, 2017 at 12:02 PM, Ganapathi Bhat  wrote:
> Ok, prepared the follow up patch.  Let me also know if we can send a merge 
> request while previous one is pending?
> If that is not the case I will send it as soon as the current request is 
> merged.

That's not up to me. I'm sure the maintainer will let you know if they
need something, eventually. It's so trivial, I don't see why they
couldn't just pull it in without any more fuss.

Re: [EXT] Re: pull-request mwifiex-firmware 2017-10-30

[Brian Norris]

On Mon, Oct 30, 2017 at 06:47:58PM +, Ganapathi Bhat wrote:
> > --
> > On Mon, Oct 30, 2017 at 02:13:31PM +, Ganapathi Bhat wrote:

> > > --- a/WHENCE
> > > +++ b/WHENCE
> > > @@ -757,7 +757,7 @@ File: mrvl/pcieuart8997_combo_v4.bin
> > >  Version: 16.68.1.p70
> > >
> > >  File: mrvl/pcieusb8997_combo_v4.bin
> > > -Version: 16.68.1.p133
> > > +Version: 16.68.1.p40
> >
> > This says .p40, whereas the commit message (and the version tag within the
> > firmware) says p140. I think you're missing a "1".
>  I did miss this.  Will it make sense to trigger another pull request with 
> just the version change?

Sure, why not? Email is cheap, and so is git. Either rewrite the commit
or add another on top to fix it...

Re: pull-request mwifiex-firmware 2017-10-30

[Brian Norris]

On Mon, Oct 30, 2017 at 02:13:31PM +, Ganapathi Bhat wrote:
> The following changes since commit e0494e95192ac5329989f4d128cf95c417d618cc:
> 
>   linux-firmware: update Marvell PCIe-USB8997 firmware image (2017-08-01 
> 23:55:30 +0530)
> 
> are available in the git repository at:
> 
>   git://git.marvell.com/mwifiex-firmware.git
> 
> for you to fetch changes up to c5bed1294f6cf6bf6cbef612204f96361a3c2539:
> 
>   linux-firmware: update Marvell PCIe-USB8997 firmware image (2017-10-30 
> 19:23:05 +0530)

Hooray, your git server is working again! And FWIW:

Tested-by: Brian Norris <briannor...@chromium.org>

But quoting your patch...

> --- a/WHENCE
> +++ b/WHENCE
> @@ -757,7 +757,7 @@ File: mrvl/pcieuart8997_combo_v4.bin
>  Version: 16.68.1.p70
>  
>  File: mrvl/pcieusb8997_combo_v4.bin
> -Version: 16.68.1.p133
> +Version: 16.68.1.p40

This says .p40, whereas the commit message (and the version tag within the
firmware) says p140. I think you're missing a "1".

>  
>  File: mrvl/pcie8997_wlan_v4.bin
>  Version: 16.68.1.p97


Brian

> 
> 
> Ganapathi Bhat (1):
>   linux-firmware: update Marvell PCIe-USB8997 firmware image
> 
> WHENCE|   2 +-
> mrvl/pcieusb8997_combo_v4.bin | Bin 620800 -> 622532 bytes
> 2 files changed, 1 insertion(+), 1 deletion(-)

Re: Commit 0711d638 breaks mwifiex

[Brian Norris]

Hi,

I'm not sure I've followed all the problems here, but I wanted to point
some things out:

On Tue, Oct 17, 2017 at 12:48:18PM +0200, Johannes Berg wrote:
> On Tue, 2017-10-17 at 18:18 +0800, Jesse Sung wrote:
> 
> > > Does mwifiex treat this -EALREADY as *keeping* an old connection,
> > > or tearing it down entirely?
> > 
> > From the call trace:
> 
> Well, the call trace can't really answer that :-)
> Does mwifiex firmware stay connected?

IIUC, mwifiex hasn't told the firmware to do anything at this point --
the -EALREADY check is practically the first thing it does within
connect(). So it just quits the connect() request and tries to carry on
as usual. It will only do something different if the upper layers tell
it to do so afterward (e.g., calling disconnect()).

> > 139.451318: nl80211_get_valid_chan <-nl80211_connect
> > 139.451321: cfg80211_connect <-nl80211_connect
> > 139.451322: cfg80211_oper_and_ht_capa <-cfg80211_connect
> > 139.451323: mwifiex_cfg80211_connect <-cfg80211_connect
> > 139.451337: nl80211_post_doit <-genl_family_rcv_msg
> > 139.451423: nl80211_pre_doit <-genl_family_rcv_msg
> > 139.451425: nl80211_disconnect <-genl_family_rcv_msg
> > 139.451426: cfg80211_disconnect <-nl80211_disconnect
> > 139.451430: mwifiex_cfg80211_disconnect <-cfg80211_disconnect
> > 
> > mwifiex_cfg80211_disconnect() would be called after
> > mwifiex_cfg80211_connect(), though I'm not sure if it's triggered by
> > the error returned.
> 
> I think so - it's probably wpa_supplicant trying to get back to a well-
> known state (of being disconnected).

Yes, that's definitely what's happening. And it's explicitly called out
in the supplicant's nl80211 driver that this is intentional:

static int wpa_driver_nl80211_connect(
struct wpa_driver_nl80211_data *drv,
struct wpa_driver_associate_params *params)
{
...
ret = wpa_driver_nl80211_try_connect(drv, params);
if (ret == -EALREADY) {
/*
 * cfg80211 does not currently accept new connections if
 * we are already connected. As a workaround, force
 * disconnection and try again.
 */
wpa_printf(MSG_DEBUG, "nl80211: Explicitly "
   "disconnecting before reassociation "
   "attempt");
if (wpa_driver_nl80211_disconnect(
drv, WLAN_REASON_PREV_AUTH_NOT_VALID))
return -1;
ret = wpa_driver_nl80211_try_connect(drv, params);
}
return ret;
}

This is the main code path for supplicant commands like "Reattach",
which boil down to (for non SME drivers):

wpas_request_connection()
  ...
 -> wpa_supplicant_connect()
   -> wpa_supplicant_associate()
 -> wpas_start_assoc_cb()
   -> wpa_drv_associate()
 -> wpa_driver_nl80211_associate()
   -> wpa_driver_nl80211_connect()

Now for the part I'm not so familiar with: is this really the *expected*
flow for full-MAC drivers in reattach, reassociate, and roaming flows?
All of those seem to boil down to this same connect() (and fallback to
disconnect()+connect() if -EALREADY) flow.

But it doesn't seem like all full-MAC drivers do the same thing. Some
seem to just blaze ahead with a connect attempt (maybe some firmwares
automatically interpret this for us?) and never return -EALREADY at all.

Sorry if this is slightly off-topic, but I'm trying to understand what
the general expectations are here, based on my relatively narrow
experience with a few drivers.

Brian

[PATCH] ath10k: fix build errors with !CONFIG_PM

[Brian Norris]

Build errors have been reported with CONFIG_PM=n:

drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit
declaration of function 'ath10k_pci_suspend'
[-Werror=implicit-function-declaration]

drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit
declaration of function 'ath10k_pci_resume'
[-Werror=implicit-function-declaration]

These are caused by the combination of the following two commits:

6af1de2e4ec4 ("ath10k: mark PM functions as __maybe_unused")
96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported but
disabled")

Both build fine on their own.

But now that ath10k_pci_pm_{suspend,resume}() is compiled
unconditionally, we should also compile ath10k_pci_{suspend,resume}()
unconditionally.

And drop the #ifdef around ath10k_pci_hif_{suspend,resume}() too; they
are trivial (empty), so we're not saving much space by compiling them
out. And the alternatives would be to sprinkle more __maybe_unused, or
spread the #ifdef's further.

Build tested with the following combinations:
CONFIG_PM=y && CONFIG_PM_SLEEP=y
CONFIG_PM=y && CONFIG_PM_SLEEP=n
CONFIG_PM=n

Fixes: 96378bd2c6cd ("ath10k: fix core PCI suspend when WoWLAN is supported but 
disabled")
Fixes: 096ad2a15fd8 ("Merge branch 'ath-next'")
Signed-off-by: Brian Norris <briannor...@chromium.org>
---
 drivers/net/wireless/ath/ath10k/pci.c | 5 -
 1 file changed, 5 deletions(-)

On Thu, Oct 19, 2017 at 10:12:25AM -0700, Brian Norris wrote:
> The solution would seem to be either to kill the #ifdefs around
> ath10k_pci_{suspend,resume}() and friends (and use __maybe_unused
> instead, to further extend Arnd's patch), or else revert Arnd's stuff
> and go with CONFIG_PM_SLEEP everywhere, which would resolve the original
> warning (promoted to error) that Arnd was resolving.
> 
> I can send out one of these if you'd like.

Here you go :)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c 
b/drivers/net/wireless/ath/ath10k/pci.c
index b18a9b690df4..d790ea20b95d 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2577,8 +2577,6 @@ void ath10k_pci_hif_power_down(struct ath10k *ar)
 */
 }
 
-#ifdef CONFIG_PM
-
 static int ath10k_pci_hif_suspend(struct ath10k *ar)
 {
/* Nothing to do; the important stuff is in the driver suspend. */
@@ -2627,7 +2625,6 @@ static int ath10k_pci_resume(struct ath10k *ar)
 
return ret;
 }
-#endif
 
 static bool ath10k_pci_validate_cal(void *data, size_t size)
 {
@@ -2782,10 +2779,8 @@ static const struct ath10k_hif_ops ath10k_pci_hif_ops = {
.power_down = ath10k_pci_hif_power_down,
.read32 = ath10k_pci_read32,
.write32= ath10k_pci_write32,
-#ifdef CONFIG_PM
.suspend= ath10k_pci_hif_suspend,
.resume = ath10k_pci_hif_resume,
-#endif
.fetch_cal_eeprom   = ath10k_pci_hif_fetch_cal_eeprom,
 };
 
-- 
2.15.0.rc1.287.g2b38de12cc-goog

Re: ath10k: fix core PCI suspend when WoWLAN is supported but disabled

[Brian Norris]

+ Arnd

On Thu, Oct 19, 2017 at 02:32:45PM +, Kalle Valo wrote:
> Kalle Valo <kv...@qca.qualcomm.com> writes:
> 
> > Brian Norris <briannor...@chromium.org> wrote:
> >
> >> For devices where the FW supports WoWLAN but user-space has not
> >> configured it, we don't do any PCI-specific suspend/resume operations,
> >> because mac80211 doesn't call drv_suspend() when !wowlan. This has
> >> particularly bad effects for some platforms, because we don't stop the
> >> power-save timer, and if this timer goes off after the PCI controller
> >> has suspended the link, Bad Things will happen.
> >> 
> >> Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
> >> got some of this right, in that it understood there was a problem on
> >> non-WoWLAN firmware. But it forgot the $subject case.
> >> 
> >> Fix this by moving all the PCI driver suspend/resume logic exclusively
> >> into the driver PM hooks. This shouldn't affect WoWLAN support much
> >> (this just gets executed later on).
> >> 
> >> I would just as well kill the entirety of ath10k_hif_suspend(), as it's
> >> not even implemented on the USB or SDIO drivers. I expect that we don't
> >> need the callback, except to return "supported" (i.e., 0) or "not
> >> supported" (i.e., -EOPNOTSUPP).
> >> 
> >> Fixes: 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
> >> Fixes: 77258d409ce4 ("ath10k: enable pci soc powersaving")
> >> Signed-off-by: Brian Norris <briannor...@chromium.org>
> >> Cc: Ryan Hsu <ryan...@qti.qualcomm.com>
> >> Cc: Kalle Valo <kv...@qca.qualcomm.com>
> >> Cc: Michal Kazior <michal.kaz...@tieto.com>
> >> Signed-off-by: Kalle Valo <kv...@qca.qualcomm.com>
> >
> > Patch applied to ath-next branch of ath.git, thanks.
> >
> > 96378bd2c6cd ath10k: fix core PCI suspend when WoWLAN is supported but 
> > disabled
> 
> Kbuild found a build problem, I suspect it's caused by this patch:

Actually, it's the interaction of this patch and Arnd's patch:

6af1de2e4ec4 ath10k: mark PM functions as __maybe_unused

I see that's now in these branches:

  ath/ath-current
  ath/ath-qca
  ath/master
  ath/master-pending
  wireless-drivers-next/master
  wireless-drivers-next/pending

Whereas mine got applied to:

  ath/ath-next

So technically, the problem is in your merge here :)

096ad2a15fd8 Merge branch 'ath-next'

> drivers/net/wireless/ath/ath10k/pci.c:3416:8: error: implicit
> declaration of function 'ath10k_pci_suspend'
> [-Werror=implicit-function-declaration]
> 
> drivers/net/wireless/ath/ath10k/pci.c:3428:8: error: implicit
> declaration of function 'ath10k_pci_resume'
> [-Werror=implicit-function-declaration]
> 
> http://lists.infradead.org/pipermail/ath10k/2017-October/010269.html
> 
> The .config.gz there doesn't have CONFIG_PM set, maybe that's the
> problem?

Yes, indirectly that's also the problem.

The solution would seem to be either to kill the #ifdefs around
ath10k_pci_{suspend,resume}() and friends (and use __maybe_unused
instead, to further extend Arnd's patch), or else revert Arnd's stuff
and go with CONFIG_PM_SLEEP everywhere, which would resolve the original
warning (promoted to error) that Arnd was resolving.

I can send out one of these if you'd like.

Brian

Re: [PATCH] ath10k: fix core PCI suspend when WoWLAN is supported but disabled

[Brian Norris]

Ping? Any comments? I know there's more than one way to slice this
problem, but it's most definitely a problem...

Brian

On Tue, Sep 19, 2017 at 04:24:16PM -0700, Brian Norris wrote:
> For devices where the FW supports WoWLAN but user-space has not
> configured it, we don't do any PCI-specific suspend/resume operations,
> because mac80211 doesn't call drv_suspend() when !wowlan. This has
> particularly bad effects for some platforms, because we don't stop the
> power-save timer, and if this timer goes off after the PCI controller
> has suspended the link, Bad Things will happen.
> 
> Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
> got some of this right, in that it understood there was a problem on
> non-WoWLAN firmware. But it forgot the $subject case.
> 
> Fix this by moving all the PCI driver suspend/resume logic exclusively
> into the driver PM hooks. This shouldn't affect WoWLAN support much
> (this just gets executed later on).
> 
> I would just as well kill the entirety of ath10k_hif_suspend(), as it's
> not even implemented on the USB or SDIO drivers. I expect that we don't
> need the callback, except to return "supported" (i.e., 0) or "not
> supported" (i.e., -EOPNOTSUPP).
> 
> Fixes: 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
> Fixes: 77258d409ce4 ("ath10k: enable pci soc powersaving")
> Signed-off-by: Brian Norris <briannor...@chromium.org>
> Cc: Ryan Hsu <ryan...@qti.qualcomm.com>
> Cc: Kalle Valo <kv...@qca.qualcomm.com>
> Cc: Michal Kazior <michal.kaz...@tieto.com>
> ---
>  drivers/net/wireless/ath/ath10k/pci.c | 24 ++--
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c 
> b/drivers/net/wireless/ath/ath10k/pci.c
> index bc1633945a56..4655c944e3fd 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2580,6 +2580,12 @@ void ath10k_pci_hif_power_down(struct ath10k *ar)
>  #ifdef CONFIG_PM
>  
>  static int ath10k_pci_hif_suspend(struct ath10k *ar)
> +{
> + /* Nothing to do; the important stuff is in the driver suspend. */
> + return 0;
> +}
> +
> +static int ath10k_pci_suspend(struct ath10k *ar)
>  {
>   /* The grace timer can still be counting down and ar->ps_awake be true.
>* It is known that the device may be asleep after resuming regardless
> @@ -2592,6 +2598,12 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
>  }
>  
>  static int ath10k_pci_hif_resume(struct ath10k *ar)
> +{
> + /* Nothing to do; the important stuff is in the driver resume. */
> + return 0;
> +}
> +
> +static int ath10k_pci_resume(struct ath10k *ar)
>  {
>   struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>   struct pci_dev *pdev = ar_pci->pdev;
> @@ -3403,11 +3415,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
>   struct ath10k *ar = dev_get_drvdata(dev);
>   int ret;
>  
> - if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
> -  ar->running_fw->fw_file.fw_features))
> - return 0;
> -
> - ret = ath10k_hif_suspend(ar);
> + ret = ath10k_pci_suspend(ar);
>   if (ret)
>   ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
>  
> @@ -3419,11 +3427,7 @@ static int ath10k_pci_pm_resume(struct device *dev)
>   struct ath10k *ar = dev_get_drvdata(dev);
>   int ret;
>  
> - if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
> -  ar->running_fw->fw_file.fw_features))
> - return 0;
> -
> - ret = ath10k_hif_resume(ar);
> + ret = ath10k_pci_resume(ar);
>   if (ret)
>   ath10k_warn(ar, "failed to resume hif: %d\n", ret);
>  
> -- 
> 2.14.1.690.gbb1197296e-goog
> 

Re: [PATCH 2/3] rsi: sdio: Add WOWLAN support for S4 hibernate state

[Brian Norris]

Hi Amitkumar,

On Wed, Oct 11, 2017 at 12:24:17PM +0300, Kalle Valo wrote:
> Amitkumar Karwar  writes:
> > On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo  wrote:
> >> And even if that would be the right approach it needs to be properly
> >> described in the commit log, a vague sentence in the end of a commit log
> >> is not enough.
> >
> > Understood. I will add detailed description and send updated version
> > if the patch is fine.
> 
> Not sure if this is fine or not. I think what you do here is ugly but I
> guess it's better than nothing?

I don't see why you can't try to reuse the existing mac80211 reset;
seems like you'd need to factor out some pieces of rsi_reset_card() and
call that from the ieee80211_ops::start() method. Then, you just call
ieee80211_restart_hw() from the appropriate place, instead of
implementing your own tear-down/reset.

Brian

Re: [PATCH] mwifiex: Random MAC address during scanning

[Brian Norris]

On Fri, Sep 29, 2017 at 04:23:10PM +0530, Ganapathi Bhat wrote:
> From: Karthik Ananthapadmanabha 
> 
> Driver will advertise RANDOM_MAC support only if the device
> supports this feature.
> 
> Signed-off-by: Karthik Ananthapadmanabha 
> Signed-off-by: Ganapathi Bhat 

I'd just like to point out that this is a very bad commit subject:

"[PATCH] mwifiex: Random MAC address during scanning"

It's borderline wrong, really. "Random MAC address during scanning" is
already supported. This patch is just adding a feature-flag check for
it, since some firmwares in the wild don't support it. A more accurate
description would be something like:

"[PATCH] mwifiex: Add feature flag support for MAC randomization"

The patch is already applied, so I'd only worry about it for future
submissions (no need to resend).

Brian

Re: [PATCH] mwifiex: Use put_unaligned_le32

[Brian Norris]

On Thu, Oct 05, 2017 at 08:52:33PM +0530, Himanshu Jha wrote:
> There are various instances where a function used in file say for eg
> int func_align (void* a)
> is used and it is defined in align.h
> But many files don't *directly* include align.h and rather include
> any other header which includes align.h

I believe the general rule is that you should included headers for all
symbols you use, and not rely on implicit includes.

The modification to the general rule is that not all headers are
intended to be included directly, and in such cases there's likely a
parent header that is the more appropriate target.

In this case, the key is CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. It
seems that asm-generic/unaligned.h is set up to include different
headers, based on the expected architecture behavior.

I wonder if include/linux/unaligned/access_ok.h should have a safety
check (e.g., raise an #error if
!CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?).

> Is compiling the file the only way to check if apppropriate header is
> included or is there some other way to check for it.

I believe it's mostly manual. Implicit includes have been a problem for
anyone who refactors header files.

Brian

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 <briannor...@chromium.org> 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

[PATCH] ath10k: fix core PCI suspend when WoWLAN is supported but disabled

[Brian Norris]

For devices where the FW supports WoWLAN but user-space has not
configured it, we don't do any PCI-specific suspend/resume operations,
because mac80211 doesn't call drv_suspend() when !wowlan. This has
particularly bad effects for some platforms, because we don't stop the
power-save timer, and if this timer goes off after the PCI controller
has suspended the link, Bad Things will happen.

Commit 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
got some of this right, in that it understood there was a problem on
non-WoWLAN firmware. But it forgot the $subject case.

Fix this by moving all the PCI driver suspend/resume logic exclusively
into the driver PM hooks. This shouldn't affect WoWLAN support much
(this just gets executed later on).

I would just as well kill the entirety of ath10k_hif_suspend(), as it's
not even implemented on the USB or SDIO drivers. I expect that we don't
need the callback, except to return "supported" (i.e., 0) or "not
supported" (i.e., -EOPNOTSUPP).

Fixes: 32faa3f0ee50 ("ath10k: add the PCI PM core suspend/resume ops")
Fixes: 77258d409ce4 ("ath10k: enable pci soc powersaving")
Signed-off-by: Brian Norris <briannor...@chromium.org>
Cc: Ryan Hsu <ryan...@qti.qualcomm.com>
Cc: Kalle Valo <kv...@qca.qualcomm.com>
Cc: Michal Kazior <michal.kaz...@tieto.com>
---
 drivers/net/wireless/ath/ath10k/pci.c | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c 
b/drivers/net/wireless/ath/ath10k/pci.c
index bc1633945a56..4655c944e3fd 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2580,6 +2580,12 @@ void ath10k_pci_hif_power_down(struct ath10k *ar)
 #ifdef CONFIG_PM
 
 static int ath10k_pci_hif_suspend(struct ath10k *ar)
+{
+   /* Nothing to do; the important stuff is in the driver suspend. */
+   return 0;
+}
+
+static int ath10k_pci_suspend(struct ath10k *ar)
 {
/* The grace timer can still be counting down and ar->ps_awake be true.
 * It is known that the device may be asleep after resuming regardless
@@ -2592,6 +2598,12 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
 }
 
 static int ath10k_pci_hif_resume(struct ath10k *ar)
+{
+   /* Nothing to do; the important stuff is in the driver resume. */
+   return 0;
+}
+
+static int ath10k_pci_resume(struct ath10k *ar)
 {
struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
struct pci_dev *pdev = ar_pci->pdev;
@@ -3403,11 +3415,7 @@ static int ath10k_pci_pm_suspend(struct device *dev)
struct ath10k *ar = dev_get_drvdata(dev);
int ret;
 
-   if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
-ar->running_fw->fw_file.fw_features))
-   return 0;
-
-   ret = ath10k_hif_suspend(ar);
+   ret = ath10k_pci_suspend(ar);
if (ret)
ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
 
@@ -3419,11 +3427,7 @@ static int ath10k_pci_pm_resume(struct device *dev)
struct ath10k *ar = dev_get_drvdata(dev);
int ret;
 
-   if (test_bit(ATH10K_FW_FEATURE_WOWLAN_SUPPORT,
-ar->running_fw->fw_file.fw_features))
-   return 0;
-
-   ret = ath10k_hif_resume(ar);
+   ret = ath10k_pci_resume(ar);
if (ret)
ath10k_warn(ar, "failed to resume hif: %d\n", ret);
 
-- 
2.14.1.690.gbb1197296e-goog

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

[Brian Norris]

Hi,

On Tue, Sep 19, 2017 at 05:30:06PM +0300, Kalle Valo wrote:
> Ganapathi Bhat <gb...@marvell.com> writes:
> 
> > Hi Kalle,
> >> 
> >> > Avoid calculating random MAC address in driver. Instead make use of
> >> > 'get_random_mask_addr()' function.
> >> >
> >> > Signed-off-by: Ganapathi Bhat <gb...@marvell.com>
> >> 
> >> 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

> > (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 <briannor...@chromium.org>

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?)

Brian

Re: [PATCH] mwifiex: avoid storing random_mac in private

[Brian Norris]

On Mon, Sep 18, 2017 at 12:25:02PM +0530, 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 <gb...@marvell.com>

Reviewed-by: Brian Norris <briannor...@chromium.org>

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

[Brian Norris]

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:
> > > Current driver prints dev_alloc_skb failures everytime while
> > > submitting RX URBs. This failure might be frequent in some low
> > > resource platforms. So, wait for a threshold failure count before
> > > start priting the error. This change is a follow up for the 'commit
> > > 7b368e3d15c3
> > > ("mwifiex: resubmit failed to submit RX URBs in main thread")'
> > 
> > []
> > 
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c
> > > b/drivers/net/wireless/marvell/mwifiex/usb.c
> > []
> > > @@ -300,9 +300,16 @@ static int mwifiex_usb_submit_rx_urb(struct
> > urb_context *ctx, int size)
> > >   if (card->rx_cmd_ep != ctx->ep) {
> > >   ctx->skb = dev_alloc_skb(size);
> > >   if (!ctx->skb) {
> > > - mwifiex_dbg(adapter, ERROR,
> > > - "%s: dev_alloc_skb failed\n", __func__);
> > > + if (++card->rx_urb_failure_count >
> > > + MWIFIEX_RX_URB_FAILURE_THRESHOLD) {
> > > + mwifiex_dbg(adapter, ERROR,
> > > + "%s: dev_alloc_skb failed, failure
> > count = %u\n",
> > > + __func__,
> > > + card->rx_urb_failure_count);
> > > + }
> > >   return -ENOMEM;
> > 
> > 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.

Brian

Re: [PATCH] mwifiex: check for mfg_mode in add_virtual_intf

[Brian Norris]

Hi,

Could have used a 'v2' in the subject, but hopefully that doesn't bother
Kalle too much.

On Fri, Sep 08, 2017 at 02:02:43AM +0530, 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 <gb...@marvell.com>
> ---
> v2: addressed Brian's comments.
> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 32c5074..ad1ebd8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -2959,18 +2959,21 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct 
> wiphy *wiphy,
>   }
>  
>   mwifiex_init_priv_params(priv, dev);
> - mwifiex_set_mac_address(priv, dev);
>  
>   priv->netdev = dev;
>  
> - ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
> -HostCmd_ACT_GEN_SET, 0, NULL, true);
> - if (ret)
> - goto err_set_bss_mode;
> + if (!adapter->mfg_mode) {
> + mwifiex_set_mac_address(priv, dev);
>  
> - ret = mwifiex_sta_init_cmd(priv, false, false);
> - if (ret)
> - goto err_sta_init;
> + ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
> +HostCmd_ACT_GEN_SET, 0, NULL, true);
> + if (ret)
> + goto err_set_bss_mode;
> +
> + ret = mwifiex_sta_init_cmd(priv, false, false);
> + if (ret)
> + goto err_sta_init;
> + }

Seems better to me.

Reviewed-by: Brian Norris <briannor...@chromium.org>

>  
>   mwifiex_setup_ht_caps(>bands[NL80211_BAND_2GHZ]->ht_cap, priv);
>   if (adapter->is_hw_11ac_capable)
> -- 
> 1.9.1
> 

Re: [PATCH] mwifiex: check for mfg_mode in add_virtual_intf

[Brian Norris]

On Thu, Aug 31, 2017 at 12:16:58AM +0530, Ganapathi Bhat wrote:
> If driver is loaded with 'mfg_mode' enabled, then the sending
> commands are not allowed. So, when mwifiex_send_cmd fails in
> mwifiex_add_virtual_intf, driver must check for 'mfg_mode' before
> returning error.
> 
> Fixes: 7311ea850079 ("mwifiex: fix AP start problem for newly added 
> interface")
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index ffada17..1856205 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -2967,11 +2967,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct 
> wiphy *wiphy,
>  
>   ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
>  HostCmd_ACT_GEN_SET, 0, NULL, true);
> - if (ret)
> + if (ret && !adapter->mfg_mode)

It doesn't feel like ignoring errors is the best approach here,
especially when it's only a single command that we could easily just
skip.

So, why don't you just skip it, like this?

if (!adapter->mfg_mode) {
ret = mwifiex_send_cmd(...);
if (ret)
goto err_set_bss_mode;
}

>   goto err_set_bss_mode;
>  
>   ret = mwifiex_sta_init_cmd(priv, false, false);
> - if (ret)
> + if (ret && !adapter->mfg_mode)
>   goto err_sta_init;

Same here; I think it's safe to just completely skip
mwifiex_sta_init_cmd(), no?

Brian

>  
>   mwifiex_setup_ht_caps(>bands[NL80211_BAND_2GHZ]->ht_cap, priv);
> -- 
> 1.9.1
> 

Re: [PATCH 2/3] mwifiex: device dump support for usb interface

[Brian Norris]

Hi,

On Mon, Aug 14, 2017 at 12:19:02PM +, Xinming Hu wrote:
> From: Xinming Hu 
> 
> Firmware dump on usb interface is different with current
> sdio/pcie chipset, which is based on register operation.
> 
> When firmware hang on usb interface, context dump will be
> upload to host using 0x73 firmware debug event.
> 
> This patch store dump data from debug event and send to
> userspace using device coredump API.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/fw.h|  1 +
>  drivers/net/wireless/marvell/mwifiex/init.c  |  3 ++
>  drivers/net/wireless/marvell/mwifiex/main.h  |  2 ++
>  drivers/net/wireless/marvell/mwifiex/sta_event.c | 39 
> 
>  4 files changed, 45 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h 
> b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 9e75522..610a3ea 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -568,6 +568,7 @@ enum mwifiex_channel_flags {
>  #define EVENT_BG_SCAN_STOPPED   0x0065
>  #define EVENT_REMAIN_ON_CHAN_EXPIRED0x005f
>  #define EVENT_MULTI_CHAN_INFO   0x006a
> +#define EVENT_FW_DUMP_INFO   0x0073
>  #define EVENT_TX_STATUS_REPORT   0x0074
>  #define EVENT_BT_COEX_WLAN_PARA_CHANGE   0X0076
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
> b/drivers/net/wireless/marvell/mwifiex/init.c
> index e11919d..389d725 100644
> --- a/drivers/net/wireless/marvell/mwifiex/init.c
> +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> @@ -315,6 +315,8 @@ static void mwifiex_init_adapter(struct mwifiex_adapter 
> *adapter)
>   adapter->active_scan_triggered = false;
>   setup_timer(>wakeup_timer, wakeup_timer_fn,
>   (unsigned long)adapter);
> + setup_timer(>devdump_timer, mwifiex_upload_device_dump,
> + (unsigned long)adapter);
>  }
>  
>  /*
> @@ -397,6 +399,7 @@ static void mwifiex_invalidate_lists(struct 
> mwifiex_adapter *adapter)
>  mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
>  {
>   del_timer(>wakeup_timer);
> + del_timer_sync(>devdump_timer);
>   mwifiex_cancel_all_pending_cmd(adapter);
>   wake_up_interruptible(>cmd_wait_q.wait);
>   wake_up_interruptible(>hs_activate_wait_q);
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
> b/drivers/net/wireless/marvell/mwifiex/main.h
> index e308b8a..6b00294 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1038,6 +1038,7 @@ struct mwifiex_adapter {
>   /* Device dump data/length */
>   char *devdump_data;
>   int devdump_len;
> + struct timer_list devdump_timer;
>  };
>  
>  void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
> @@ -1680,6 +1681,7 @@ void mwifiex_process_multi_chan_event(struct 
> mwifiex_private *priv,
>  void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter);
>  int mwifiex_set_mac_address(struct mwifiex_private *priv,
>   struct net_device *dev);
> +void mwifiex_devdump_tmo_func(unsigned long function_context);
>  
>  #ifdef CONFIG_DEBUG_FS
>  void mwifiex_debugfs_init(void);
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c 
> b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> index 839df8a..bcf2fa2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> @@ -586,6 +586,40 @@ void mwifiex_bt_coex_wlan_param_update_event(struct 
> mwifiex_private *priv,
>   adapter->coex_rx_win_size);
>  }
>  
> +static void
> +mwifiex_fw_dump_info_event(struct mwifiex_private *priv,
> +struct sk_buff *event_skb)
> +{
> + struct mwifiex_adapter *adapter = priv->adapter;
> +
> + if (adapter->iface_type != MWIFIEX_USB) {
> + mwifiex_dbg(adapter, MSG,
> + "event is not on usb interface, ignore it\n");
> + return;
> + }
> +
> + if (!adapter->devdump_data) {
> + /* When receive the first event, allocate device dump
> +  * buffer, dump driver info.
> +  */
> + adapter->devdump_data = vzalloc(MWIFIEX_FW_DUMP_SIZE);

Does this ever get freed?

> + if (!adapter->devdump_data) {
> + mwifiex_dbg(adapter, ERROR,
> + "vzalloc devdump data failure!\n");
> + return;
> + }
> +
> + mwifiex_drv_info_dump(adapter);
> + }
> +
> + memmove(adapter->devdump_data + adapter->devdump_len,
> + adapter->event_body, event_skb->len - MWIFIEX_EVENT_HEADER_LEN);
> + adapter->devdump_len += (event_skb->len - 

Re: [PATCH 3/3] mwifiex: debugfs: trigger device dump for usb interface

[Brian Norris]

Hi,

On Mon, Aug 14, 2017 at 12:19:03PM +, Xinming Hu wrote:
> From: Xinming Hu 
> 
> This patch extend device_dump debugfs function to make it
> works for usb interface.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c  | 11 +++
>  drivers/net/wireless/marvell/mwifiex/debugfs.c |  9 +
>  drivers/net/wireless/marvell/mwifiex/fw.h  |  1 +
>  drivers/net/wireless/marvell/mwifiex/sta_cmd.c |  4 
>  4 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
> b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> index 0edc5d6..b16dd6a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
> @@ -290,13 +290,16 @@ static int mwifiex_dnld_cmd_to_fw(struct 
> mwifiex_private *priv,
>   adapter->dbg.last_cmd_act[adapter->dbg.last_cmd_index] =
>   get_unaligned_le16((u8 *)host_cmd + S_DS_GEN);
>  
> + /* Setup the timer after transmit command, except that specific
> +  * command might not have command response.
> +  */
> + if (cmd_code != HostCmd_CMD_FW_DUMP_EVENT)
> + mod_timer(>cmd_timer,
> +   jiffies + msecs_to_jiffies(MWIFIEX_TIMER_10S));
> +
>   /* Clear BSS_NO_BITS from HostCmd */
>   cmd_code &= HostCmd_CMD_ID_MASK;
>  
> - /* Setup the timer after transmit command */
> - mod_timer(>cmd_timer,
> -   jiffies + msecs_to_jiffies(MWIFIEX_TIMER_10S));
> -
>   return 0;
>  }
>  
> diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c 
> b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> index 6f4239b..5d476de 100644
> --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
> +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> @@ -168,10 +168,11 @@
>  {
>   struct mwifiex_private *priv = file->private_data;
>  
> - if (!priv->adapter->if_ops.device_dump)
> - return -EIO;
> -
> - priv->adapter->if_ops.device_dump(priv->adapter);
> + if (priv->adapter->iface_type == MWIFIEX_USB)
> + mwifiex_send_cmd(priv, HostCmd_CMD_FW_DUMP_EVENT,
> +  HostCmd_ACT_GEN_SET, 0, NULL, true);

Why couldn't you just implement the device_dump() callback?

> + else
> + priv->adapter->if_ops.device_dump(priv->adapter);
>  
>   return 0;
>  }
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h 
> b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 610a3ea..2d3a644 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -398,6 +398,7 @@ enum MWIFIEX_802_11_PRIVACY_FILTER {
>  #define HostCmd_CMD_TDLS_CONFIG   0x0100
>  #define HostCmd_CMD_MC_POLICY 0x0121
>  #define HostCmd_CMD_TDLS_OPER 0x0122
> +#define HostCmd_CMD_FW_DUMP_EVENT  0x0125
>  #define HostCmd_CMD_SDIO_SP_RX_AGGR_CFG   0x0223
>  #define HostCmd_CMD_CHAN_REGION_CFG0x0242
>  #define HostCmd_CMD_PACKET_AGGR_CTRL   0x0251
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c 
> b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> index fb09014..211e47d 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> @@ -2206,6 +2206,10 @@ int mwifiex_sta_prepare_cmd(struct mwifiex_private 
> *priv, uint16_t cmd_no,
>   case HostCmd_CMD_CHAN_REGION_CFG:
>   ret = mwifiex_cmd_chan_region_cfg(priv, cmd_ptr, cmd_action);
>   break;
> + case HostCmd_CMD_FW_DUMP_EVENT:
> + cmd_ptr->command = cpu_to_le16(cmd_no);
> + cmd_ptr->size = cpu_to_le16(S_DS_GEN);
> + break;
>   default:
>   mwifiex_dbg(priv->adapter, ERROR,
>   "PREP_CMD: unknown cmd- %#x\n", cmd_no);
> -- 
> 1.9.1
> 

Re: [PATCH 1/3] mwifiex: refactor device dump code to make it generic for usb interface

[Brian Norris]

On Mon, Aug 14, 2017 at 12:19:01PM +, Xinming Hu wrote:
> From: Xinming Hu 
> 
> This patch refactor current device dump code to make it generic
> for subsequent implementation on usb interface.
> 
> Signed-off-by: Xinming Hu 
> Signed-off-by: Cathy Luo 
> Signed-off-by: Ganapathi Bhat 
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c | 90 
> +++--
>  drivers/net/wireless/marvell/mwifiex/main.h | 11 +++-
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 13 +++--
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 14 +++--
>  4 files changed, 74 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index ee40b73..919d91a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1052,9 +1052,26 @@ void mwifiex_multi_chan_resync(struct mwifiex_adapter 
> *adapter)
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_multi_chan_resync);
>  
> -int mwifiex_drv_info_dump(struct mwifiex_adapter *adapter, void **drv_info)
> +void mwifiex_upload_device_dump(unsigned long function_context)

My eyes! It burns!

Most callers don't need the casting, so this looks terribly unsafe.
Please keep the casting to only where it's needed (i.e., in the
timer-related code).

Also, why can't the caller pass in the dump data/len values? You're
making PCIe and SDIO look even uglier, just because you can't figure out
how to wrap this nicely for USB.

You might do better to consider how to make this nicer by implementing
better driver callbacks, and doing most of the work in the core.
Overall, you shouldn't need so many exported symbols.

>  {
> - void *p;
> + struct mwifiex_adapter *adapter =
> + (struct mwifiex_adapter *)function_context;
> +
> + /* Dump all the memory data into single file, a userspace script will
> +  * be used to split all the memory data to multiple files
> +  */
> + mwifiex_dbg(adapter, MSG,
> + "== mwifiex dump information to /sys/class/devcoredump 
> start");
> + dev_coredumpv(adapter->dev, adapter->devdump_data, adapter->devdump_len,
> +   GFP_KERNEL);
> + mwifiex_dbg(adapter, MSG,
> + "== mwifiex dump information to /sys/class/devcoredump 
> end");
> +}
> +EXPORT_SYMBOL_GPL(mwifiex_upload_device_dump);
> +
> +void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
> +{
> + char *p;
>   char drv_version[64];
>   struct usb_card_rec *cardp;
>   struct sdio_mmc_card *sdio_card;
> @@ -1062,17 +1079,12 @@ int mwifiex_drv_info_dump(struct mwifiex_adapter 
> *adapter, void **drv_info)
>   int i, idx;
>   struct netdev_queue *txq;
>   struct mwifiex_debug_info *debug_info;
> - void *drv_info_dump;
>  
>   mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump start===\n");
>  
> - /* memory allocate here should be free in mwifiex_upload_device_dump*/
> - drv_info_dump = vzalloc(MWIFIEX_DRV_INFO_SIZE_MAX);
> -
> - if (!drv_info_dump)
> - return 0;
> -
> - p = (char *)(drv_info_dump);
> + p = adapter->devdump_data;
> + strcpy(p, "Start dump driverinfo\n");
> + p += strlen("Start dump driverinfo\n");
>   p += sprintf(p, "driver_name = " "\"mwifiex\"\n");
>  
>   mwifiex_drv_get_driver_version(adapter, drv_version,
> @@ -1156,21 +1168,18 @@ int mwifiex_drv_info_dump(struct mwifiex_adapter 
> *adapter, void **drv_info)
>   kfree(debug_info);
>   }
>  
> + strcpy(p, "\nEnd dump\n");
> + p += strlen("\nEnd dump\n");
>   mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump end===\n");
> - *drv_info = drv_info_dump;
> - return p - drv_info_dump;
> + adapter->devdump_len = p - adapter->devdump_data;
>  }
>  EXPORT_SYMBOL_GPL(mwifiex_drv_info_dump);
>  
> -void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter, void 
> *drv_info,
> - int drv_info_size)
> +void mwifiex_prepare_fw_dump_info(struct mwifiex_adapter *adapter)
>  {
> - u8 idx, *dump_data, *fw_dump_ptr;
> - u32 dump_len;
> -
> - dump_len = (strlen("Start dump driverinfo\n") +
> -drv_info_size +
> -strlen("\nEnd dump\n"));
> + u8 idx;
> + char *fw_dump_ptr;
> + u32 dump_len = 0;
>  
>   for (idx = 0; idx < adapter->num_mem_types; idx++) {
>   struct memory_type_mapping *entry =
> @@ -1185,24 +1194,24 @@ void mwifiex_upload_device_dump(struct 
> mwifiex_adapter *adapter, void *drv_info,
>   }
>   }
>  
> - dump_data = vzalloc(dump_len + 1);
> - if (!dump_data)
> - goto done;
> -
> - fw_dump_ptr = dump_data;
> + if (dump_len + 1 + adapter->devdump_len > 

Re: [EXT] Re: pull-request mwifiex-firmware 2017-08-01

[Brian Norris]

Hi Ganapathi,

On Wed, Aug 02, 2017 at 09:44:51AM +, Ganapathi Bhat wrote:
> Hi Brian,
> 
> > On Tue, Aug 01, 2017 at 06:35:40PM +, Ganapathi Bhat wrote:
> > > The following changes since commit
> > 11db1311bff6fed84d11b03fbfb142209c1239ab:

^^^ Note that even the above commit is not in linux-firmware.git. Your
pull requests haven't been making it through.

> > >   linux-firmware: update Marvell PCIe-USB8897-A2 firmware image
> > (2017-06-05 02:19:40 -0700)
> > >
> > > are available in the git repository at:
> > >
> > >   git://git.marvell.com/mwifiex-firmware.git
> > 
> > Is your server down? I can't get a response.
> No, it is running fine. I did a clone just now to confirm the same.

Still not working today:

   $ git pull
   fatal: unable to connect to git.marvell.com:
   git.marvell.com[0: 199.233.58.162]: errno=Connection timed out

   $ git remote -v
   origin   git://git.marvell.com/mwifiex-firmware.git (fetch)
   origin   git://git.marvell.com/mwifiex-firmware.git (push)

It also looks like this is not the first time:

Re: pull-request mwifiex-firmware 2017-06-05
http://marc.info/?l=linux-wireless=149754207831326=2

Your team's last pull request was never picked up because your server isn't
working. Maybe you only have a company-internal instance working now?

Brian

Re: pull-request mwifiex-firmware 2017-08-01

[Brian Norris]

On Tue, Aug 01, 2017 at 06:35:40PM +, Ganapathi Bhat wrote:
> The following changes since commit 11db1311bff6fed84d11b03fbfb142209c1239ab:
> 
>   linux-firmware: update Marvell PCIe-USB8897-A2 firmware image (2017-06-05 
> 02:19:40 -0700)
> 
> are available in the git repository at:
> 
>   git://git.marvell.com/mwifiex-firmware.git

Is your server down? I can't get a response.

Brian

> for you to fetch changes up to e0494e95192ac5329989f4d128cf95c417d618cc:
> 
>   linux-firmware: update Marvell PCIe-USB8997 firmware image (2017-08-01 
> 23:55:30 +0530)
> 
> 
> Ganapathi Bhat (1):
>   linux-firmware: update Marvell PCIe-USB8997 firmware image
> 
>  WHENCE|   2 +-
>  mrvl/pcieusb8997_combo_v4.bin | Bin 597136 -> 620800 bytes
>  2 files changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH 2/2] mwifiex: pcie: compatible with wifi-only image while extract wifi-part fw

[Brian Norris]

Hi,

Two nitpicks below:

On Mon, Jul 31, 2017 at 01:07:27PM +, Xinming Hu wrote:
> From: Xinming Hu <h...@marvell.com>
> 
> Sometimes, we might using wifi-only firmware with a combo firmware name,
> in this case, do not need to filter bluetooth part from header.
> 
> Signed-off-by: Xinming Hu <h...@marvell.com>
> Signed-off-by: Cathy Luo <c...@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 26 +++---
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 3da1eeb..dc4e054 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -1985,7 +1985,8 @@ static int mwifiex_pcie_event_complete(struct 
> mwifiex_adapter *adapter,
>   * (3) wifi image.
>   *
>   * This function bypass the header and bluetooth part, return
> - * the offset of tail wifi-only part.
> + * the offset of tail wifi-only part. if the image is already wifi-only,

Sentences start with capital letters (i.e., "If the image ...").

> + * that is start with CMD1, return 0.
>   */
>  
>  static int mwifiex_extract_wifi_fw(struct mwifiex_adapter *adapter,
> @@ -1993,7 +1994,7 @@ static int mwifiex_extract_wifi_fw(struct 
> mwifiex_adapter *adapter,
>   const struct mwifiex_fw_data *fwdata;
>   u32 offset = 0, data_len, dnld_cmd;
>   int ret = 0;
> - bool cmd7_before = false;
> + bool cmd7_before = false, first_cmd = false;
>  
>   while (1) {
>   /* Check for integer and buffer overflow */
> @@ -2014,20 +2015,29 @@ static int mwifiex_extract_wifi_fw(struct 
> mwifiex_adapter *adapter,
>  
>   switch (dnld_cmd) {
>   case MWIFIEX_FW_DNLD_CMD_1:
> - if (!cmd7_before) {
> - mwifiex_dbg(adapter, ERROR,
> - "no cmd7 before cmd1!\n");
> + if (offset + data_len < data_len) {
> + mwifiex_dbg(adapter, ERROR, "bad FW parse\n");
>   ret = -1;
>   goto done;
>   }
> - if (offset + data_len < data_len) {
> - mwifiex_dbg(adapter, ERROR, "bad FW parse\n");
> +
> + /* Image start with cmd1, already wifi-only firmware*/

You need a space before closing the comment. i.e.:

/* Image starts with cmd1; already wifi-only firmware */

Otherwise, I think both of these look fine:

Reviewed-by: Brian Norris <briannor...@chromium.org>

> + if (!first_cmd) {
> + mwifiex_dbg(adapter, MSG,
> + "input wifi-only firmware\n");
> + return 0;
> + }
> +
> + if (!cmd7_before) {
> + mwifiex_dbg(adapter, ERROR,
> + "no cmd7 before cmd1!\n");
>   ret = -1;
>   goto done;
>   }
>   offset += data_len;
>   break;
>   case MWIFIEX_FW_DNLD_CMD_5:
> + first_cmd = true;
>   /* Check for integer overflow */
>   if (offset + data_len < data_len) {
>   mwifiex_dbg(adapter, ERROR, "bad FW parse\n");
> @@ -2037,6 +2047,7 @@ static int mwifiex_extract_wifi_fw(struct 
> mwifiex_adapter *adapter,
>   offset += data_len;
>   break;
>   case MWIFIEX_FW_DNLD_CMD_6:
> + first_cmd = true;
>   /* Check for integer overflow */
>   if (offset + data_len < data_len) {
>   mwifiex_dbg(adapter, ERROR, "bad FW parse\n");
> @@ -2053,6 +2064,7 @@ static int mwifiex_extract_wifi_fw(struct 
> mwifiex_adapter *adapter,
>   }
>   goto done;
>   case MWIFIEX_FW_DNLD_CMD_7:
> + first_cmd = true;
>   cmd7_before = true;
>   break;
>   default:
> -- 
> 1.9.1
> 

[PATCH v2 15/20] mwifiex: pcie: unify MSI-X / non-MSI-X interrupt process

[Brian Norris]

After removing the interrupt loop in commit 5d5ddb5e0d9b ("mwifiex:
pcie: don't loop/retry interrupt status checks"), there is practically
zero difference between mwifiex_process_pcie_int() (which handled legacy
PCI interrupts and MSI interrupts) and mwifiex_process_msix_int() (which
handled MSI-X interrupts). Let's add the one relevant line to
mwifiex_process_pcie_int() and kill the copy-and-paste.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 68 ++---
 1 file changed, 3 insertions(+), 65 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 2f4da08f127c..c08ebb55a7e8 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2417,7 +2417,7 @@ static irqreturn_t mwifiex_pcie_interrupt(int irq, void 
*context)
  * In case of Rx packets received, the packets are uploaded from card to
  * host and processed accordingly.
  */
-static int mwifiex_process_pcie_int(struct mwifiex_adapter *adapter)
+static int mwifiex_process_int_status(struct mwifiex_adapter *adapter)
 {
int ret;
u32 pcie_ireg = 0;
@@ -2492,75 +2492,13 @@ static int mwifiex_process_pcie_int(struct 
mwifiex_adapter *adapter)
mwifiex_dbg(adapter, INTR,
"info: cmd_sent=%d data_sent=%d\n",
adapter->cmd_sent, adapter->data_sent);
-   if (!card->msi_enable && adapter->ps_state != PS_STATE_SLEEP)
+   if (!card->msi_enable && !card->msix_enable &&
+adapter->ps_state != PS_STATE_SLEEP)
mwifiex_pcie_enable_host_int(adapter);
 
return 0;
 }
 
-static int mwifiex_process_msix_int(struct mwifiex_adapter *adapter)
-{
-   int ret;
-   u32 pcie_ireg;
-   unsigned long flags;
-
-   spin_lock_irqsave(>int_lock, flags);
-   /* Clear out unused interrupts */
-   pcie_ireg = adapter->int_status;
-   adapter->int_status = 0;
-   spin_unlock_irqrestore(>int_lock, flags);
-
-   if (pcie_ireg & HOST_INTR_DNLD_DONE) {
-   mwifiex_dbg(adapter, INTR,
-   "info: TX DNLD Done\n");
-   ret = mwifiex_pcie_send_data_complete(adapter);
-   if (ret)
-   return ret;
-   }
-   if (pcie_ireg & HOST_INTR_UPLD_RDY) {
-   mwifiex_dbg(adapter, INTR,
-   "info: Rx DATA\n");
-   ret = mwifiex_pcie_process_recv_data(adapter);
-   if (ret)
-   return ret;
-   }
-   if (pcie_ireg & HOST_INTR_EVENT_RDY) {
-   mwifiex_dbg(adapter, INTR,
-   "info: Rx EVENT\n");
-   ret = mwifiex_pcie_process_event_ready(adapter);
-   if (ret)
-   return ret;
-   }
-
-   if (pcie_ireg & HOST_INTR_CMD_DONE) {
-   if (adapter->cmd_sent) {
-   mwifiex_dbg(adapter, INTR,
-   "info: CMD sent Interrupt\n");
-   adapter->cmd_sent = false;
-   }
-   /* Handle command response */
-   ret = mwifiex_pcie_process_cmd_complete(adapter);
-   if (ret)
-   return ret;
-   }
-
-   mwifiex_dbg(adapter, INTR,
-   "info: cmd_sent=%d data_sent=%d\n",
-   adapter->cmd_sent, adapter->data_sent);
-
-   return 0;
-}
-
-static int mwifiex_process_int_status(struct mwifiex_adapter *adapter)
-{
-   struct pcie_service_card *card = adapter->card;
-
-   if (card->msix_enable)
-   return mwifiex_process_msix_int(adapter);
-   else
-   return mwifiex_process_pcie_int(adapter);
-}
-
 /*
  * This function downloads data from driver to card.
  *
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 13/20] mwifiex: drop 'add_tail' param from mwifiex_insert_cmd_to_pending_q()

[Brian Norris]

It's always called with 'true' -- we only determine it 'false' locally
within this function. So drop the parameter.

Also, this should be 'bool' (since we use true/false), not 'u32'.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 5 +++--
 drivers/net/wireless/marvell/mwifiex/main.h   | 3 +--
 drivers/net/wireless/marvell/mwifiex/scan.c   | 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 6ff8e84b01e0..3f5e822673bf 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -664,7 +664,7 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 
cmd_no,
cmd_no == HostCmd_CMD_802_11_SCAN_EXT) {
mwifiex_queue_scan_cmd(priv, cmd_node);
} else {
-   mwifiex_insert_cmd_to_pending_q(adapter, cmd_node, true);
+   mwifiex_insert_cmd_to_pending_q(adapter, cmd_node);
queue_work(adapter->workqueue, >main_work);
if (cmd_node->wait_q_enabled)
ret = mwifiex_wait_queue_complete(adapter, cmd_node);
@@ -682,11 +682,12 @@ int mwifiex_send_cmd(struct mwifiex_private *priv, u16 
cmd_no,
  */
 void
 mwifiex_insert_cmd_to_pending_q(struct mwifiex_adapter *adapter,
-   struct cmd_ctrl_node *cmd_node, u32 add_tail)
+   struct cmd_ctrl_node *cmd_node)
 {
struct host_cmd_ds_command *host_cmd = NULL;
u16 command;
unsigned long flags;
+   bool add_tail = true;
 
host_cmd = (struct host_cmd_ds_command *) (cmd_node->cmd_skb->data);
if (!host_cmd) {
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index 2bee5cdf1fc8..909bd1ad3838 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1088,8 +1088,7 @@ void mwifiex_recycle_cmd_node(struct mwifiex_adapter 
*adapter,
  struct cmd_ctrl_node *cmd_node);
 
 void mwifiex_insert_cmd_to_pending_q(struct mwifiex_adapter *adapter,
-struct cmd_ctrl_node *cmd_node,
-u32 addtail);
+struct cmd_ctrl_node *cmd_node);
 
 int mwifiex_exec_next_cmd(struct mwifiex_adapter *adapter);
 int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter);
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
b/drivers/net/wireless/marvell/mwifiex/scan.c
index ae9630b49342..16eaeae74979 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1534,8 +1534,7 @@ int mwifiex_scan_networks(struct mwifiex_private *priv,
list_del(_node->list);
spin_unlock_irqrestore(>scan_pending_q_lock,
   flags);
-   mwifiex_insert_cmd_to_pending_q(adapter, cmd_node,
-   true);
+   mwifiex_insert_cmd_to_pending_q(adapter, cmd_node);
queue_work(adapter->workqueue, >main_work);
 
/* Perform internal scan synchronously */
@@ -2033,7 +2032,7 @@ static void mwifiex_check_next_scan_command(struct 
mwifiex_private *priv)
struct cmd_ctrl_node, list);
list_del(_node->list);
spin_unlock_irqrestore(>scan_pending_q_lock, flags);
-   mwifiex_insert_cmd_to_pending_q(adapter, cmd_node, true);
+   mwifiex_insert_cmd_to_pending_q(adapter, cmd_node);
}
 
return;
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 08/20] mwifiex: ensure "disable auto DS" struct is initialized

[Brian Norris]

The .idle_time field *should* be unused, but technically, we're allowing
unitialized stack garbage to pass all the way through to the firmware
host command. Let's zero it out instead.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c 
b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
index 42997e05d90f..43ecd621d1ef 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
@@ -654,9 +654,9 @@ int mwifiex_get_bss_info(struct mwifiex_private *priv,
  */
 int mwifiex_disable_auto_ds(struct mwifiex_private *priv)
 {
-   struct mwifiex_ds_auto_ds auto_ds;
-
-   auto_ds.auto_ds = DEEP_SLEEP_OFF;
+   struct mwifiex_ds_auto_ds auto_ds = {
+   .auto_ds = DEEP_SLEEP_OFF,
+   };
 
return mwifiex_send_cmd(priv, HostCmd_CMD_802_11_PS_MODE_ENH,
DIS_AUTO_PS, BITMAP_AUTO_DS, _ds, true);
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 14/20] mwifiex: pcie: remove unnecessary masks

[Brian Norris]

After removing the interrupt loop in commit 5d5ddb5e0d9b ("mwifiex:
pcie: don't loop/retry interrupt status checks"), we don't need to keep
track of the cleared interrupts (actually, we didn't need to do that
before, but we *really* don't need to now).

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 5c07edd4e094..2f4da08f127c 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2460,28 +2460,24 @@ static int mwifiex_process_pcie_int(struct 
mwifiex_adapter *adapter)
}
 
if (pcie_ireg & HOST_INTR_DNLD_DONE) {
-   pcie_ireg &= ~HOST_INTR_DNLD_DONE;
mwifiex_dbg(adapter, INTR, "info: TX DNLD Done\n");
ret = mwifiex_pcie_send_data_complete(adapter);
if (ret)
return ret;
}
if (pcie_ireg & HOST_INTR_UPLD_RDY) {
-   pcie_ireg &= ~HOST_INTR_UPLD_RDY;
mwifiex_dbg(adapter, INTR, "info: Rx DATA\n");
ret = mwifiex_pcie_process_recv_data(adapter);
if (ret)
return ret;
}
if (pcie_ireg & HOST_INTR_EVENT_RDY) {
-   pcie_ireg &= ~HOST_INTR_EVENT_RDY;
mwifiex_dbg(adapter, INTR, "info: Rx EVENT\n");
ret = mwifiex_pcie_process_event_ready(adapter);
if (ret)
return ret;
}
if (pcie_ireg & HOST_INTR_CMD_DONE) {
-   pcie_ireg &= ~HOST_INTR_CMD_DONE;
if (adapter->cmd_sent) {
mwifiex_dbg(adapter, INTR,
"info: CMD sent Interrupt\n");
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 07/20] mwifiex: fixup init_channel_scan_gap error case

[Brian Norris]

In reading through _mwifiex_fw_dpc(), I noticed that after we've
registered our wiphy, we still have error paths that don't free it back
up. Let's do that.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 77e491720664..0448dcc07139 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -588,7 +588,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, 
void *context)
if (mwifiex_init_channel_scan_gap(adapter)) {
mwifiex_dbg(adapter, ERROR,
"could not init channel stats table\n");
-   goto err_init_fw;
+   goto err_init_chan_scan;
}
 
if (driver_mode) {
@@ -636,6 +636,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, 
void *context)
 
 err_add_intf:
vfree(adapter->chan_stats);
+err_init_chan_scan:
wiphy_unregister(adapter->wiphy);
wiphy_free(adapter->wiphy);
 err_init_fw:
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 06/20] mwifiex: don't short-circuit netdev notifiers on interface deletion

[Brian Norris]

When we leave the delete interface function, there are still netdev
hooks that might try to process the device. We're short-circuiting some
of that by changing the interface type and clearing ieee80211_ptr. This
means we skip NETDEV_UNREGISTER_FINAL in cfg80211. Fortunately, that is
currently a no-op.

We don't need most of the cleanup here anyway:

 * the connection state will get (un)set as part of the disconnect
   process (which cfg80211 already initiates for us)
 * the interface type doesn't actually need to be cleared at all (it'll
   trigger a WARN_ON() in cfg80211 if we do)
 * the iee80211_ptr isn't really "ours" to clear anyway

So stop resetting those 3 things.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 06ad2d50f9b0..f86a8a69d060 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -3123,11 +3123,7 @@ int mwifiex_del_virtual_intf(struct wiphy *wiphy, struct 
wireless_dev *wdev)
priv->dfs_chan_sw_workqueue = NULL;
}
/* Clear the priv in adapter */
-   priv->netdev->ieee80211_ptr = NULL;
priv->netdev = NULL;
-   priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
-
-   priv->media_connected = false;
 
switch (priv->bss_mode) {
case NL80211_IFTYPE_UNSPECIFIED:
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 10/20] mwifiex: make mwifiex_free_cmd_buffer() return void

[Brian Norris]

It doesn't fail.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2; noticed when reworking driver
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 6 ++
 drivers/net/wireless/marvell/mwifiex/main.h   | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 8dad52886034..6ff8e84b01e0 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -427,7 +427,7 @@ int mwifiex_alloc_cmd_buffer(struct mwifiex_adapter 
*adapter)
  * The function calls the completion callback for all the command
  * buffers that still have response buffers associated with them.
  */
-int mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter)
+void mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter)
 {
struct cmd_ctrl_node *cmd_array;
u32 i;
@@ -436,7 +436,7 @@ int mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter)
if (!adapter->cmd_pool) {
mwifiex_dbg(adapter, FATAL,
"info: FREE_CMD_BUF: cmd_pool is null\n");
-   return 0;
+   return;
}
 
cmd_array = adapter->cmd_pool;
@@ -464,8 +464,6 @@ int mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter)
kfree(adapter->cmd_pool);
adapter->cmd_pool = NULL;
}
-
-   return 0;
 }
 
 /*
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index 62ce4e81f695..2bee5cdf1fc8 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1077,7 +1077,7 @@ int mwifiex_get_debug_info(struct mwifiex_private *,
   struct mwifiex_debug_info *);
 
 int mwifiex_alloc_cmd_buffer(struct mwifiex_adapter *adapter);
-int mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter);
+void mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter);
 void mwifiex_free_cmd_buffers(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter);
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 12/20] mwifiex: don't open-code ARRAY_SIZE()

[Brian Norris]

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/cfp.c | 4 +---
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 8 ++--
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 5 ++---
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfp.c 
b/drivers/net/wireless/marvell/mwifiex/cfp.c
index 6e2994308526..bfe84e55df77 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfp.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfp.c
@@ -180,11 +180,9 @@ static struct region_code_mapping region_code_mapping_t[] 
= {
 u8 *mwifiex_11d_code_2_region(u8 code)
 {
u8 i;
-   u8 size = sizeof(region_code_mapping_t)/
-   sizeof(struct region_code_mapping);
 
/* Look for code in mapping table */
-   for (i = 0; i < size; i++)
+   for (i = 0; i < ARRAY_SIZE(region_code_mapping_t); i++)
if (region_code_mapping_t[i].code == code)
return region_code_mapping_t[i].region;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c 
b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index 534d94a206a5..b71ad4de5e54 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -189,9 +189,7 @@ static int mwifiex_cmd_tx_rate_cfg(struct mwifiex_private 
*priv,
if (pbitmap_rates != NULL) {
rate_scope->hr_dsss_rate_bitmap = cpu_to_le16(pbitmap_rates[0]);
rate_scope->ofdm_rate_bitmap = cpu_to_le16(pbitmap_rates[1]);
-   for (i = 0;
-i < sizeof(rate_scope->ht_mcs_rate_bitmap) / sizeof(u16);
-i++)
+   for (i = 0; i < ARRAY_SIZE(rate_scope->ht_mcs_rate_bitmap); i++)
rate_scope->ht_mcs_rate_bitmap[i] =
cpu_to_le16(pbitmap_rates[2 + i]);
if (priv->adapter->fw_api_ver == MWIFIEX_FW_V15) {
@@ -206,9 +204,7 @@ static int mwifiex_cmd_tx_rate_cfg(struct mwifiex_private 
*priv,
cpu_to_le16(priv->bitmap_rates[0]);
rate_scope->ofdm_rate_bitmap =
cpu_to_le16(priv->bitmap_rates[1]);
-   for (i = 0;
-i < sizeof(rate_scope->ht_mcs_rate_bitmap) / sizeof(u16);
-i++)
+   for (i = 0; i < ARRAY_SIZE(rate_scope->ht_mcs_rate_bitmap); i++)
rate_scope->ht_mcs_rate_bitmap[i] =
cpu_to_le16(priv->bitmap_rates[2 + i]);
if (priv->adapter->fw_api_ver == MWIFIEX_FW_V15) {
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c 
b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
index 2945775e83c5..0fba5b10ef2d 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c
@@ -298,9 +298,8 @@ static int mwifiex_ret_tx_rate_cfg(struct mwifiex_private 
*priv,
priv->bitmap_rates[1] =
le16_to_cpu(rate_scope->ofdm_rate_bitmap);
for (i = 0;
-i <
-sizeof(rate_scope->ht_mcs_rate_bitmap) /
-sizeof(u16); i++)
+i < ARRAY_SIZE(rate_scope->ht_mcs_rate_bitmap);
+i++)
priv->bitmap_rates[2 + i] =
le16_to_cpu(rate_scope->
ht_mcs_rate_bitmap[i]);
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 09/20] mwifiex: fix misnomers in mwifiex_free_lock_list()

[Brian Norris]

Despite the name (and meticulous comments), this function frees no
memory and does not touch any locks. All it does is "delete" the list
heads -- which just means they'll be dangling, and we'll need to re-init
them if we use them again.

It seems like this code would work OK as a sort of canary for using the
list after we've torn everything down, so it's fine to keep the code;
let's just get the name and comments to match what's actually happening.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2; noticed when bugfixing/reworking other parts of this series
---
 drivers/net/wireless/marvell/mwifiex/init.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
b/drivers/net/wireless/marvell/mwifiex/init.c
index de96675e43d5..de974e8bb9c6 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -373,15 +373,13 @@ void mwifiex_stop_net_dev_queue(struct net_device *netdev,
 }
 
 /*
- *  This function releases the lock variables and frees the locks and
- *  associated locks.
+ * This function invalidates the list heads.
  */
-static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
+static void mwifiex_invalidate_lists(struct mwifiex_adapter *adapter)
 {
struct mwifiex_private *priv;
s32 i, j;
 
-   /* Free lists */
list_del(>cmd_free_q);
list_del(>cmd_pending_q);
list_del(>scan_pending_q);
@@ -422,8 +420,7 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
 
 void mwifiex_free_cmd_buffers(struct mwifiex_adapter *adapter)
 {
-   /* Free lock variables */
-   mwifiex_free_lock_list(adapter);
+   mwifiex_invalidate_lists(adapter);
 
/* Free command buffer */
mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 11/20] mwifiex: utilize netif_tx_{wake,stop}_all_queues()

[Brian Norris]

We're open-coding these. Just use the helpers.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/init.c | 20 ++--
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
b/drivers/net/wireless/marvell/mwifiex/init.c
index de974e8bb9c6..e11919db7818 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -337,17 +337,9 @@ void mwifiex_wake_up_net_dev_queue(struct net_device 
*netdev,
struct mwifiex_adapter *adapter)
 {
unsigned long dev_queue_flags;
-   unsigned int i;
 
spin_lock_irqsave(>queue_lock, dev_queue_flags);
-
-   for (i = 0; i < netdev->num_tx_queues; i++) {
-   struct netdev_queue *txq = netdev_get_tx_queue(netdev, i);
-
-   if (netif_tx_queue_stopped(txq))
-   netif_tx_wake_queue(txq);
-   }
-
+   netif_tx_wake_all_queues(netdev);
spin_unlock_irqrestore(>queue_lock, dev_queue_flags);
 }
 
@@ -358,17 +350,9 @@ void mwifiex_stop_net_dev_queue(struct net_device *netdev,
struct mwifiex_adapter *adapter)
 {
unsigned long dev_queue_flags;
-   unsigned int i;
 
spin_lock_irqsave(>queue_lock, dev_queue_flags);
-
-   for (i = 0; i < netdev->num_tx_queues; i++) {
-   struct netdev_queue *txq = netdev_get_tx_queue(netdev, i);
-
-   if (!netif_tx_queue_stopped(txq))
-   netif_tx_stop_queue(txq);
-   }
-
+   netif_tx_stop_all_queues(netdev);
spin_unlock_irqrestore(>queue_lock, dev_queue_flags);
 }
 
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 16/20] mwifiex: debugfs: allow card_reset() to cancel things

[Brian Norris]

The card_reset() implementation should be setting our state flags and
cancelling commands for us (i.e., in mwifiex_shutdown_drv()), so let's
not do it here.

Also, this debugfs file is useful for testing and debugging the reset
feature, so we shouldn't do extra preparatory steps here, as that might
cause different reset behavior, which could either cause new bugs or
paper over existing ones that this debug feature should otherwise help
us catch.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/debugfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c 
b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index f6f105a7d3ff..6f4239be609d 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -940,8 +940,6 @@ mwifiex_reset_write(struct file *file,
 
if (adapter->if_ops.card_reset) {
dev_info(adapter->dev, "Resetting per request\n");
-   adapter->hw_status = MWIFIEX_HW_STATUS_RESET;
-   mwifiex_cancel_all_pending_cmd(adapter);
adapter->if_ops.card_reset(adapter);
}
 
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 20/20] mwifiex: drop num CPU notice

[Brian Norris]

This print isn't very useful. It's also different between
mwifiex_add_card() and mwifiex_reinit_sw(), and I'd like to consolidate
them eventually.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 0448dcc07139..13fc7b6ed11d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1619,10 +1619,8 @@ mwifiex_add_card(void *card, struct completion *fw_done,
adapter->cmd_wait_q.status = 0;
adapter->scan_wait_q_woken = false;
 
-   if ((num_possible_cpus() > 1) || adapter->iface_type == MWIFIEX_USB) {
+   if ((num_possible_cpus() > 1) || adapter->iface_type == MWIFIEX_USB)
adapter->rx_work_enabled = true;
-   pr_notice("rx work enabled, cpus %d\n", num_possible_cpus());
-   }
 
adapter->workqueue =
alloc_workqueue("MWIFIEX_WORK_QUEUE",
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 17/20] mwifiex: pcie: disable device DMA before unmapping/freeing buffers

[Brian Norris]

In testing the mwifiex reset code path, I've noticed KASAN complaining
about some "overwritten poison values" in our RX buffer descriptors.
Because KASAN didn't notice this at the time of a CPU write, this seems
to suggest that the device is writing to this memory.

This makes a little sense, because when resetting, we don't necessarily
expect the device to be responsive, so we don't have a chance to disable
everything cleanly.

We can at least take the precaution of disabling DMA for the device
though, and in my testing that seems to clear up this particular issue.

This patch reorders the removal path so that we disable the device
*before* releasing our last PCIe buffers, and it clears/sets the bus
master feature from the PCI device when resetting.

Along the way, remove the insufficient (and confusing) error path in
mwifiex_pcie_up_dev() (it doesn't unwind things well enough, and it
doesn't propagate its errors upward anyway).

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c08ebb55a7e8..a1907e8e620f 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2958,15 +2958,17 @@ static void mwifiex_cleanup_pcie(struct mwifiex_adapter 
*adapter)
"Failed to write driver not-ready 
signature\n");
}
 
-   mwifiex_pcie_free_buffers(adapter);
-
if (pdev) {
+   pci_disable_device(pdev);
+
pci_iounmap(pdev, card->pci_mmap);
pci_iounmap(pdev, card->pci_mmap1);
pci_disable_device(pdev);
pci_release_region(pdev, 2);
pci_release_region(pdev, 0);
}
+
+   mwifiex_pcie_free_buffers(adapter);
 }
 
 static int mwifiex_pcie_request_irq(struct mwifiex_adapter *adapter)
@@ -3142,7 +3144,6 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter 
*adapter)
 static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter)
 {
struct pcie_service_card *card = adapter->card;
-   int ret;
struct pci_dev *pdev = card->dev;
 
/* tx_buf_size might be changed to 3584 by firmware during
@@ -3150,11 +3151,9 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter 
*adapter)
 */
adapter->tx_buf_size = card->pcie.tx_buf_size;
 
-   ret = mwifiex_pcie_alloc_buffers(adapter);
-   if (!ret)
-   return;
+   mwifiex_pcie_alloc_buffers(adapter);
 
-   pci_iounmap(pdev, card->pci_mmap1);
+   pci_set_master(pdev);
 }
 
 /* This function cleans up the PCI-E host memory space. */
@@ -3162,10 +3161,13 @@ static void mwifiex_pcie_down_dev(struct 
mwifiex_adapter *adapter)
 {
struct pcie_service_card *card = adapter->card;
const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
+   struct pci_dev *pdev = card->dev;
 
if (mwifiex_write_reg(adapter, reg->drv_rdy, 0x))
mwifiex_dbg(adapter, ERROR, "Failed to write driver not-ready 
signature\n");
 
+   pci_clear_master(pdev);
+
adapter->seq_num = 0;
 
mwifiex_pcie_free_buffers(adapter);
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 18/20] mwifiex: pcie: remove unnecessary 'pdev' check

[Brian Norris]

'card->dev' is initialized once and is never cleared. Drop the
unnecessary "safety" check, as it simply obscures things, and we don't
do this check everywhere (and therefore it's not really "safe").

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index a1907e8e620f..1993b9b339df 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2958,15 +2958,12 @@ static void mwifiex_cleanup_pcie(struct mwifiex_adapter 
*adapter)
"Failed to write driver not-ready 
signature\n");
}
 
-   if (pdev) {
-   pci_disable_device(pdev);
+   pci_disable_device(pdev);
 
-   pci_iounmap(pdev, card->pci_mmap);
-   pci_iounmap(pdev, card->pci_mmap1);
-   pci_disable_device(pdev);
-   pci_release_region(pdev, 2);
-   pci_release_region(pdev, 0);
-   }
+   pci_iounmap(pdev, card->pci_mmap);
+   pci_iounmap(pdev, card->pci_mmap1);
+   pci_release_region(pdev, 2);
+   pci_release_region(pdev, 0);
 
mwifiex_pcie_free_buffers(adapter);
 }
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 19/20] mwifiex: keep mwifiex_cancel_pending_ioctl() static

[Brian Norris]

It has some scary comments about "only being called" from the timeout
handler, so let's help keep it that way.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 4 +++-
 drivers/net/wireless/marvell/mwifiex/main.h   | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c 
b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 3f5e822673bf..0edc5d621304 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -26,6 +26,8 @@
 #include "11n.h"
 #include "11ac.h"
 
+static void mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter);
+
 /*
  * This function initializes a command node.
  *
@@ -1074,7 +1076,7 @@ mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter 
*adapter)
  * In case of scan commands, all pending commands in scan pending queue
  * are cancelled.
  */
-void
+static void
 mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter)
 {
struct cmd_ctrl_node *cmd_node = NULL;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index 909bd1ad3838..537a0ad795ff 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1080,7 +1080,6 @@ int mwifiex_alloc_cmd_buffer(struct mwifiex_adapter 
*adapter);
 void mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter);
 void mwifiex_free_cmd_buffers(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter);
-void mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_pending_scan_cmd(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_scan(struct mwifiex_adapter *adapter);
 
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 05/20] mwifiex: unregister wiphy before freeing resources

[Brian Norris]

It's possible for some control interfaces (e.g., scans, set freq) to be
active after we've stopped our main work queue and the netif TX queues.
These don't get completely shut out until we've unregistered the wdevs
and wiphy.

So let's only free command buffers and poison our lists after
wiphy_unregister().

This resolves various use-after-free issues seen when resetting the
device.

Cc: Johannes Berg <johan...@sipsolutions.net>
Signed-off-by: Brian Norris <briannor...@chromium.org>
---
new in v2
---
 drivers/net/wireless/marvell/mwifiex/init.c | 3 +++
 drivers/net/wireless/marvell/mwifiex/main.c | 7 ++-
 drivers/net/wireless/marvell/mwifiex/main.h | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c 
b/drivers/net/wireless/marvell/mwifiex/init.c
index 3ecb59f7405b..de96675e43d5 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -418,7 +418,10 @@ mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
mwifiex_cancel_all_pending_cmd(adapter);
wake_up_interruptible(>cmd_wait_q.wait);
wake_up_interruptible(>hs_activate_wait_q);
+}
 
+void mwifiex_free_cmd_buffers(struct mwifiex_adapter *adapter)
+{
/* Free lock variables */
mwifiex_free_lock_list(adapter);
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 9c8f7bcfef8b..77e491720664 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -653,6 +653,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, 
void *context)
if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
pr_debug("info: %s: shutdown mwifiex\n", __func__);
mwifiex_shutdown_drv(adapter);
+   mwifiex_free_cmd_buffers(adapter);
}
 
init_failed = true;
@@ -1404,11 +1405,13 @@ static void mwifiex_uninit_sw(struct mwifiex_adapter 
*adapter)
mwifiex_del_virtual_intf(adapter->wiphy, >wdev);
rtnl_unlock();
}
-   vfree(adapter->chan_stats);
 
wiphy_unregister(adapter->wiphy);
wiphy_free(adapter->wiphy);
adapter->wiphy = NULL;
+
+   vfree(adapter->chan_stats);
+   mwifiex_free_cmd_buffers(adapter);
 }
 
 /*
@@ -1515,6 +1518,7 @@ mwifiex_reinit_sw(struct mwifiex_adapter *adapter)
mwifiex_dbg(adapter, ERROR,
"info: %s: shutdown mwifiex\n", __func__);
mwifiex_shutdown_drv(adapter);
+   mwifiex_free_cmd_buffers(adapter);
}
 
complete_all(adapter->fw_done);
@@ -1662,6 +1666,7 @@ mwifiex_add_card(void *card, struct completion *fw_done,
if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
pr_debug("info: %s: shutdown mwifiex\n", __func__);
mwifiex_shutdown_drv(adapter);
+   mwifiex_free_cmd_buffers(adapter);
}
 err_kmalloc:
mwifiex_free_adapter(adapter);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h 
b/drivers/net/wireless/marvell/mwifiex/main.h
index f8cf3079ac7d..62ce4e81f695 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1078,6 +1078,7 @@ int mwifiex_get_debug_info(struct mwifiex_private *,
 
 int mwifiex_alloc_cmd_buffer(struct mwifiex_adapter *adapter);
 int mwifiex_free_cmd_buffer(struct mwifiex_adapter *adapter);
+void mwifiex_free_cmd_buffers(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_all_pending_cmd(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_pending_ioctl(struct mwifiex_adapter *adapter);
 void mwifiex_cancel_pending_scan_cmd(struct mwifiex_adapter *adapter);
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 04/20] mwifiex: re-register wiphy across reset

[Brian Norris]

In general, it's helpful to use the same code for device removal as for
device reset, as this tends to have fewer bugs. Let's move the wiphy
unregistration code into the common reset and removal code.

In particular, it's very hard to properly handle the reset sequence when
something fails. Currently, if mwifiex_reinit_sw() fails, we've failed
to unregister the associated wiphy, and so running something as simple
as "iw phy" can trigger an OOPS, as the wiphy still has hooks back into
freed mwifiex data structures. For example, KASAN complained:

[... see reset fail for other reasons ...]
[ 1184.821158] mwifiex_pcie :01:00.0: info: dnld wifi firmware from 174948 
bytes
[ 1186.870914] mwifiex_pcie :01:00.0: info: FW download over, size 608396 
bytes
[ 1187.685990] mwifiex_pcie :01:00.0: WLAN FW is active
[ 1187.692673] mwifiex_pcie :01:00.0: cmd_wait_q terminated: -512
[ 1187.699075] mwifiex_pcie :01:00.0: info: _mwifiex_fw_dpc: unregister 
device
[ 1187.713476] mwifiex: Failed to bring up adapter: -5
[ 1187.718644] mwifiex_pcie :01:00.0: reinit failed: -5

[... run `iw phy` ...]
[ 1212.902419] 
==
[ 1212.909806] BUG: KASAN: use-after-free in 
mwifiex_cfg80211_get_antenna+0x54/0xfc [mwifiex] at addr ffc0ad1a8028
[ 1212.920246] Read of size 1 by task iw/3127
[...]
[ 1212.934946] page dumped because: kasan: bad access detected
[...]
[ 1212.950665] Call trace:
[ 1212.953148] [] dump_backtrace+0x0/0x190
[ 1212.958572] [] show_stack+0x20/0x28
[ 1212.963648] [] dump_stack+0xa4/0xcc
[ 1212.968723] [] kasan_report+0x378/0x500
[ 1212.974140] [] __asan_load1+0x44/0x4c
[ 1212.979462] [] mwifiex_cfg80211_get_antenna+0x54/0xfc 
[mwifiex]
[ 1212.987131] [] nl80211_send_wiphy+0x75c/0x2de0 [cfg80211]
[ 1212.994246] [] nl80211_dump_wiphy+0x32c/0x438 [cfg80211]
[ 1213.001149] [] genl_lock_dumpit+0x48/0x64
[ 1213.006746] [] netlink_dump+0x178/0x398
[ 1213.012171] [] __netlink_dump_start+0x1bc/0x260
[...]

This all goes away if we just tear down the wiphy on the way down, and
set it back up if/when we bring the device back up.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/main.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 275cf8dc4f2a..9c8f7bcfef8b 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1405,6 +1405,10 @@ static void mwifiex_uninit_sw(struct mwifiex_adapter 
*adapter)
rtnl_unlock();
}
vfree(adapter->chan_stats);
+
+   wiphy_unregister(adapter->wiphy);
+   wiphy_free(adapter->wiphy);
+   adapter->wiphy = NULL;
 }
 
 /*
@@ -1686,9 +1690,6 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 
mwifiex_uninit_sw(adapter);
 
-   wiphy_unregister(adapter->wiphy);
-   wiphy_free(adapter->wiphy);
-
if (adapter->irq_wakeup >= 0)
device_init_wakeup(adapter->dev, false);
 
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 03/20] mwifiex: pcie: don't allow cmd buffer reuse after reset

[Brian Norris]

In rogue cases (due to other bugs) it's possible we try to process an
old command response *after* resetting the device. This could trigger a
double-free (or the SKB can get reallocated elsewhere...causing other
memory corruptions) in mwifiex_pcie_process_cmd_complete().

For safety (and symmetry) let's always NULL out the command buffer as we
free it up. We're already doing this for the command response buffer.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index b53ecf1eddda..5c07edd4e094 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -1030,12 +1030,14 @@ static int mwifiex_pcie_delete_cmdrsp_buf(struct 
mwifiex_adapter *adapter)
mwifiex_unmap_pci_memory(adapter, card->cmdrsp_buf,
 PCI_DMA_FROMDEVICE);
dev_kfree_skb_any(card->cmdrsp_buf);
+   card->cmdrsp_buf = NULL;
}
 
if (card && card->cmd_buf) {
mwifiex_unmap_pci_memory(adapter, card->cmd_buf,
 PCI_DMA_TODEVICE);
dev_kfree_skb_any(card->cmd_buf);
+   card->cmd_buf = NULL;
}
return 0;
 }
@@ -2921,7 +2923,6 @@ static void mwifiex_pcie_free_buffers(struct 
mwifiex_adapter *adapter)
mwifiex_pcie_delete_evtbd_ring(adapter);
mwifiex_pcie_delete_rxbd_ring(adapter);
mwifiex_pcie_delete_txbd_ring(adapter);
-   card->cmdrsp_buf = NULL;
 }
 
 /*
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 01/20] mwifiex: reunite copy-and-pasted remove/reset code

[Brian Norris]

When PCIe FLR code was added, it explicitly copy-and-pasted much of
mwifiex_remove_card() into mwifiex_shutdown_sw(). This is unnecessary,
as almost all of the code should be reused.

Let's reunite what we can for now.

The only functional changes for now:

 * call netif_device_detach() in the remove() code path -- this wasn't
   done before, but it really should be a no-op, when the device is
   getting totally unregistered soon anyway

 * call the ->down_dev() driver callback only after we've finished all
   SW teardown -- this should have no significant effect, since the only
   user (pcie.c) does very minimal work there, and it doesn't matter
   that we reorder this

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/main.c | 104 
 1 file changed, 28 insertions(+), 76 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index f2600b827e81..8615099468da 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1352,26 +1352,12 @@ static void mwifiex_main_work_queue(struct work_struct 
*work)
mwifiex_main_process(adapter);
 }
 
-/*
- * This function gets called during PCIe function level reset. Required
- * code is extracted from mwifiex_remove_card()
- */
-int
-mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
+/* Common teardown code used for both device removal and reset */
+static void mwifiex_uninit_sw(struct mwifiex_adapter *adapter)
 {
struct mwifiex_private *priv;
int i;
 
-   if (!adapter)
-   goto exit_return;
-
-   wait_for_completion(adapter->fw_done);
-   /* Caller should ensure we aren't suspending while this happens */
-   reinit_completion(adapter->fw_done);
-
-   priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
-   mwifiex_deauthenticate(priv, NULL);
-
/* We can no longer handle interrupts once we start doing the teardown
 * below.
 */
@@ -1393,12 +1379,9 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
}
 
mwifiex_dbg(adapter, CMD, "cmd: calling mwifiex_shutdown_drv...\n");
-
mwifiex_shutdown_drv(adapter);
-   if (adapter->if_ops.down_dev)
-   adapter->if_ops.down_dev(adapter);
-
mwifiex_dbg(adapter, CMD, "cmd: mwifiex_shutdown_drv done\n");
+
if (atomic_read(>rx_pending) ||
atomic_read(>tx_pending) ||
atomic_read(>cmd_pending)) {
@@ -1421,9 +1404,30 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
rtnl_unlock();
}
vfree(adapter->chan_stats);
+}
+
+/*
+ * This function gets called during PCIe function level reset.
+ */
+int mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
+{
+   struct mwifiex_private *priv;
+
+   if (!adapter)
+   return 0;
+
+   wait_for_completion(adapter->fw_done);
+   /* Caller should ensure we aren't suspending while this happens */
+   reinit_completion(adapter->fw_done);
+
+   priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
+   mwifiex_deauthenticate(priv, NULL);
+
+   mwifiex_uninit_sw(adapter);
+
+   if (adapter->if_ops.down_dev)
+   adapter->if_ops.down_dev(adapter);
 
-   mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
-exit_return:
return 0;
 }
 EXPORT_SYMBOL_GPL(mwifiex_shutdown_sw);
@@ -1676,61 +1680,10 @@ EXPORT_SYMBOL_GPL(mwifiex_add_card);
  */
 int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 {
-   struct mwifiex_private *priv = NULL;
-   int i;
-
if (!adapter)
-   goto exit_remove;
-
-   /* We can no longer handle interrupts once we start doing the teardown
-* below. */
-   if (adapter->if_ops.disable_int)
-   adapter->if_ops.disable_int(adapter);
-
-   adapter->surprise_removed = true;
-
-   mwifiex_terminate_workqueue(adapter);
-
-   /* Stop data */
-   for (i = 0; i < adapter->priv_num; i++) {
-   priv = adapter->priv[i];
-   if (priv && priv->netdev) {
-   mwifiex_stop_net_dev_queue(priv->netdev, adapter);
-   if (netif_carrier_ok(priv->netdev))
-   netif_carrier_off(priv->netdev);
-   }
-   }
-
-   mwifiex_dbg(adapter, CMD,
-   "cmd: calling mwifiex_shutdown_drv...\n");
-
-   mwifiex_shutdown_drv(adapter);
-   mwifiex_dbg(adapter, CMD,
-   "cmd: mwifiex_shutdown_drv done\n");
-   if (atomic_read(>rx_pending) ||
-   atomic_read(>tx_pending) ||
-   atomic_read(>cmd_pending)) {
-   mwifiex_dbg(adapter, ERROR,
-  

[PATCH v2 02/20] mwifiex: reset interrupt status across device reset

[Brian Norris]

When resetting the device, we might have queued up interrupts that
didn't get a chance to finish processing. We really don't need to handle
them at this point; we just want to make sure they don't cause us to try
to process old commands from before the device was reset.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
v2: no change
---
 drivers/net/wireless/marvell/mwifiex/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index 8615099468da..275cf8dc4f2a 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1366,6 +1366,7 @@ static void mwifiex_uninit_sw(struct mwifiex_adapter 
*adapter)
 
adapter->surprise_removed = true;
mwifiex_terminate_workqueue(adapter);
+   adapter->int_status = 0;
 
/* Stop data */
for (i = 0; i < adapter->priv_num; i++) {
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH v2 00/20] mwifiex: "reset" bugfixes and other refactorings

[Brian Norris]

Hello,

I've previously sent a stack of similar patches (with no cover letter),
starting at this patch:

[PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver 
callbacks
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1405062.html
https://patchwork.kernel.org/patch/9747263/

They fixed several bugs related to the "reset" codepath in this driver,
in which the driver tries to recover from a dead firmware, as well as making
various code improvements (e.g., refactorings, removing redunancy, improving
safety).

There were some valid criticisms of the first patch in that series (and some
real issues I hit with it later on in testing), and in the end, I believe
the concern there was not actually appearing in practice, so in order to
make some forward progress, I've dropped that patch (and related changes)
from this series. I've kept most of the rest and added a few bugfixes along
the way.

Thanks to Johannes for some brainstorming/ideas that yielded the patch
"mwifiex: unregister wiphy before freeing resources".

Regards,
Brian

Brian Norris (20):
  mwifiex: reunite copy-and-pasted remove/reset code
  mwifiex: reset interrupt status across device reset
  mwifiex: pcie: don't allow cmd buffer reuse after reset
  mwifiex: re-register wiphy across reset
  mwifiex: unregister wiphy before freeing resources
  mwifiex: don't short-circuit netdev notifiers on interface deletion
  mwifiex: fixup init_channel_scan_gap error case
  mwifiex: ensure "disable auto DS" struct is initialized
  mwifiex: fix misnomers in mwifiex_free_lock_list()
  mwifiex: make mwifiex_free_cmd_buffer() return void
  mwifiex: utilize netif_tx_{wake,stop}_all_queues()
  mwifiex: don't open-code ARRAY_SIZE()
  mwifiex: drop 'add_tail' param from mwifiex_insert_cmd_to_pending_q()
  mwifiex: pcie: remove unnecessary masks
  mwifiex: pcie: unify MSI-X / non-MSI-X interrupt process
  mwifiex: debugfs: allow card_reset() to cancel things
  mwifiex: pcie: disable device DMA before unmapping/freeing buffers
  mwifiex: pcie: remove unnecessary 'pdev' check
  mwifiex: keep mwifiex_cancel_pending_ioctl() static
  mwifiex: drop num CPU notice

 drivers/net/wireless/marvell/mwifiex/cfg80211.c|   4 -
 drivers/net/wireless/marvell/mwifiex/cfp.c |   4 +-
 drivers/net/wireless/marvell/mwifiex/cmdevt.c  |  15 +--
 drivers/net/wireless/marvell/mwifiex/debugfs.c |   2 -
 drivers/net/wireless/marvell/mwifiex/init.c|  32 ++
 drivers/net/wireless/marvell/mwifiex/main.c| 124 +++--
 drivers/net/wireless/marvell/mwifiex/main.h|   7 +-
 drivers/net/wireless/marvell/mwifiex/pcie.c| 100 +++--
 drivers/net/wireless/marvell/mwifiex/scan.c|   5 +-
 drivers/net/wireless/marvell/mwifiex/sta_cmd.c |   8 +-
 drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c |   5 +-
 drivers/net/wireless/marvell/mwifiex/sta_ioctl.c   |   6 +-
 12 files changed, 87 insertions(+), 225 deletions(-)

-- 
2.14.0.rc0.284.gd933b75aa4-goog

Re: Re: [PATCH] mwifiex: add tdls uapsd support module parameter

[Brian Norris]

Hi,

On Thu, Jul 20, 2017 at 10:54:16AM +, Xinming Hu wrote:
> Hi Brian,
> 
> > -Original Message-
> > From: Brian Norris [mailto:briannor...@chromium.org]
> > Sent: 2017年7月20日 4:10
> > To: Xinming Hu
> > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com; Zhiyuan
> > Yang; Tim Song; Cathy Luo; Xinming Hu
> > Subject: [EXT] Re: [PATCH] mwifiex: add tdls uapsd support module parameter
> > 
> > External Email
> > 
> > --
> > On Wed, Jul 19, 2017 at 06:36:27AM +, Xinming Hu wrote:
> > > From: Xinming Hu <h...@marvell.com>
> > >
> > > Add module parameter to control tdls uapsd support, which is default
> > > disabled.
> > 
> > Why default disabled, when it looks like it used to be on by default?
> 
> Oho, I should comment more in description, it is now confusing.
> We just fixed an tdls uapsd issue in latest firmware, so try to disable
> this feature as a workaround to the old firmware. At the same time, 
> it is optional to enable this feature in specific case.

That helps a bit, thanks.

> I will add more comments in V2.
> Please let us know if you have more comments.

I won't insist on changes, but for something like this, it seems
reasonable (if you have really fixed the issue in the latest firmware)
to just provide the knob to disable as a backup, not as a default. If
someone is going to update their kernel (to include this patch), but not
update their firmware, then they probably should know enough to be able
to provide the module parameter to disable.

Or alternatively: how is anyone supposed to know whether their current
firmware is broken or not? And how is this feature ever going to be
default-enabled again? New chipsets with new firmware should hopefully
not have the same bugs, no?

Brian

Re: [PATCH] mwifiex: add tdls uapsd support module parameter

[Brian Norris]

On Wed, Jul 19, 2017 at 06:36:27AM +, Xinming Hu wrote:
> From: Xinming Hu 
> 
> Add module parameter to control tdls uapsd support, which is
> default disabled.

Why default disabled, when it looks like it used to be on by default?

Re: [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

[Brian Norris]

On Mon, Jul 03, 2017 at 06:46:21PM +0800, Jeffy Chen wrote:
> Hi guys,
> 
> with this patch, the pci device's irq might be override by this
> wakeup irq when not using msi:

Hmm, good point. I believe I noticed this one at some point and then
didn't get to investigate further...

It kind of seems like we inadvertently conflicted with the PCI OF
interrupt spec [1]. There, the "interrupts" property for a device (if
present) is supposed to represent INT{A...D} with values of {1...4}.
IIUC, there should only be a single entry in this property.

If we were to extend this properly, I guess that would mean we'd need a
second "interrupts" entry, with a different parent. I think we can use
"interrupts-extended" for that.

So we'd need to document an optional "interrupt-names" for Marvell, and
have the driver try that first. The rough outline would be something
like this.

For the device tree (e.g., rk3399-gru):

-   interrupt-parent = <>;
-   interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
+   interrupts-extended = < 1>, < 8 
IRQ_TYPE_LEVEL_LOW>;
+   interrupt-names = "int-A", "wake";

Then mwifiex would need to check "byname" before trying "by index":

adapter->irq_wakeup = of_irq_get_byname(adapter->dt_node, "wake");
if (!adapter->irq_wakeup) {
adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
if (!adapter->irq_wakeup) {
dev_dbg(dev, "fail to parse irq_wakeup from device 
tree\n");
goto err_exit;
}
}

Or if we want to suggest the original binding was wrong and that we
should just ignore existing device trees that tried to use it, we can
skip the by-index fallback.

Brian

[1] Documentation/devicetree/bindings/pci/pci.txt points to
http://www.firmware.org/1275/practice/imap/imap0_9d.pdf
except that link is also dead now. I found the same doc here:
https://www.openfirmware.info/data/docs/rec.intmap.d09.pdf
Might want to update the binding doc... I've sent a patch for that
separately.

Re: [PATCH v2] mwifiex: uninit wakeup info in the error handling

[Brian Norris]

On Thu, Jul 06, 2017 at 03:55:28PM +0800, Jeffy Chen wrote:
> We inited wakeup info at the beginning of mwifiex_add_card, so we need
> to uninit it in the error handling.
> 
> It's much the same as what we did in:
> 36908c4 mwifiex: uninit wakeup info when removing device
> 
> Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
> 
> ---
> 
> Changes in v2:
> Uninit wakeup when _mwifiex_fw_dpc failed too.

Looks good to me:

Reviewed-by: Brian Norris <briannor...@chromium.org>

Re: [PATCH] mwifiex: fix compile warning of unused variable

[Brian Norris]

On Thu, Jul 06, 2017 at 03:50:33PM +0800, Shawn Lin wrote:
> We got a compile warning shows below:
> 
> drivers/net/wireless/marvell/mwifiex/sdio.c: In function
> 'mwifiex_sdio_remove':
> drivers/net/wireless/marvell/mwifiex/sdio.c:377:6: warning: variable
> 'ret' set but not used [-Wunused-but-set-variable]

It's probably worth noting that this is not a default warning [1],
especially if you resend. It already confused Kalle.

[1] In Makefile:

# These warnings generated too much noise in a regular build.
# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)

> Per the code, it didn't check if mwifiex_sdio_read_fw_status
> finish successfully. We should at least check the return of
> mwifiex_sdio_read_fw_status, otherwise the following check of
> firmware_stat and adapter->mfg_mode is pointless as the device
> is probably dead.
> 
> Signed-off-by: Shawn Lin <shawn@rock-chips.com>
> ---
> 
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c 
> b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index f81a006..fd5183c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -390,7 +390,8 @@ static int mwifiex_check_winner_status(struct 
> mwifiex_adapter *adapter)
>   mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
>  
>   ret = mwifiex_sdio_read_fw_status(adapter, _stat);
> - if (firmware_stat == FIRMWARE_READY_SDIO && !adapter->mfg_mode) {
> + if (!ret && firmware_stat == FIRMWARE_READY_SDIO &&
> + !adapter->mfg_mode) {

The PCIe driver has the same code structure. Might change both, if
you're changing one of them? The PCIe one is technically safe I guess,
since it will write to the 'firmware_stat' variable regardless of
success or failure, whereas this SDIO one will not. But it would keep
things clear and obvious.

With (or without) that change:

Reviewed-by: Brian Norris <briannor...@chromium.org>

Brian

>   mwifiex_deauthenticate_all(adapter);
>  
>   priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> -- 
> 1.9.1
> 
> 

Re: [PATCH] mwifiex: uninit wakeup info when failed to add card

[Brian Norris]

On Mon, Jul 03, 2017 at 03:54:30PM +0800, Jeffy Chen wrote:
> We inited wakeup info at the beginning of mwifiex_add_card, so we need
> to uninit it in the error handling.
> 
> It's much the same as what we did in:
> 36908c4 mwifiex: uninit wakeup info when removing device

Yeah, I noticed I hadn't covered all the error cases, so your change is
part of the fix. But you're not covering the error paths when the
firmware doesn't load correctly -- this happens asynchronously to
mwifiex_add_card(). i.e., you need to fixup the error paths in
_mwifiex_fw_dpc() too.

Brian

> Signed-off-by: Jeffy Chen 
> 
> ---
> 
>  drivers/net/wireless/marvell/mwifiex/main.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
> b/drivers/net/wireless/marvell/mwifiex/main.c
> index f2600b8..17d2cbe 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1655,6 +1655,8 @@ mwifiex_add_card(void *card, struct completion *fw_done,
>   mwifiex_shutdown_drv(adapter);
>   }
>  err_kmalloc:
> + if (adapter->irq_wakeup >= 0)
> + device_init_wakeup(adapter->dev, false);
>   mwifiex_free_adapter(adapter);
>  
>  err_init_sw:
> -- 
> 2.1.4
> 
> 

[PATCH] mwifiex: correct channel stat buffer overflows

[Brian Norris]

mwifiex records information about various channels as it receives scan
information. It does this by appending to a buffer that was sized
to the max number of supported channels on any band, but there are
numerous problems:

(a) scans can return info from more than one band (e.g., both 2.4 and 5
GHz), so the determined "max" is not large enough
(b) some firmware appears to return multiple results for a given
channel, so the max *really* isn't large enough
(c) there is no bounds checking when stashing these stats, so problems
(a) and (b) can easily lead to buffer overflows

Let's patch this by setting a slightly-more-correct max (that accounts
for a combination of both 2.4G and 5G bands) and adding a bounds check
when writing to our statistics buffer.

Due to problem (b), we still might not properly report all known survey
information (e.g., with "iw  survey dump"), since duplicate results
(or otherwise "larger than expected" results) will cause some
truncation. But that's a problem for a future bugfix.

(And because of this known deficiency, only log the excess at the WARN
level, since that isn't visible by default in this driver and would
otherwise be a bit too noisy.)

Fixes: bf35443314ac ("mwifiex: channel statistics support for mwifiex")
Cc: <sta...@vger.kernel.org>
Cc: Avinash Patil <pat...@marvell.com>
Cc: Xinming Hu <h...@marvell.com>
Signed-off-by: Brian Norris <briannor...@chromium.org>
---
I've got a ton of other patches still queued up locally, and I hope to send
them soon. But I realized this one is a nasty bug (with a trivial fix), so it's
probably best to get this out the door quickly.

 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 +-
 drivers/net/wireless/marvell/mwifiex/scan.c | 6 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c 
b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index a850ec0054e2..82f4e796ed39 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -4219,7 +4219,7 @@ int mwifiex_init_channel_scan_gap(struct mwifiex_adapter 
*adapter)
if (adapter->config_bands & BAND_A)
n_channels_a = mwifiex_band_5ghz.n_channels;
 
-   adapter->num_in_chan_stats = max_t(u32, n_channels_bg, n_channels_a);
+   adapter->num_in_chan_stats = n_channels_bg + n_channels_a;
adapter->chan_stats = vmalloc(sizeof(*adapter->chan_stats) *
  adapter->num_in_chan_stats);
 
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
b/drivers/net/wireless/marvell/mwifiex/scan.c
index ae9630b49342..9900855746ac 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -2492,6 +2492,12 @@ mwifiex_update_chan_statistics(struct mwifiex_private 
*priv,
  sizeof(struct mwifiex_chan_stats);
 
for (i = 0 ; i < num_chan; i++) {
+   if (adapter->survey_idx >= adapter->num_in_chan_stats) {
+   mwifiex_dbg(adapter, WARN,
+   "FW reported too many channel results (max 
%d)\n",
+   adapter->num_in_chan_stats);
+   return;
+   }
chan_stats.chan_num = fw_chan_stats->chan_num;
chan_stats.bandcfg = fw_chan_stats->bandcfg;
chan_stats.flags = fw_chan_stats->flags;
-- 
2.13.2.725.g09c95d1e9-goog

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

[Brian Norris]

Hi Johannes,

On Wed, Jun 28, 2017 at 09:28:49AM +0200, Johannes Berg wrote:
> On Tue, 2017-06-27 at 13:48 -0700, Brian Norris wrote:
> 
> > > There isn't really a good way to do this. You can, of course, call
> > > wiphy_unregister(), but if you could do that you'd already have the
> > > problem solved, I think?
> > 
> > That's probably along the right track. There are still some things
> > we'd need to do properly before that though, and this is where all
> > the problems are so far. (Also, this is what Kalle was already
> > objecting to; he didn't think we should be unregistering/recreating
> > the wiphy, but I think he ended up softening on that a bit.)
> > 
> > For one, I still expect I should be removing the wireless dev's
> > before unregistering the wihpy, no? Otherwise, there will be existing
> > wdevs backed by an unregistered wiphy?
> 
> Yeah, that's true - though once you get rid of those they can't be
> accessed any more.

Right. That's the whole idea for the current reset implementation in
this driver anyway.

> > And that gets to the heart of another bug: deleting interfaces (e.g.,
> > "iw dev foo del") races with a lot of stuff -- like see
> > 
> > mwifiex_process_sta_event() ->
> >   EVENT_EXT_SCAN_REPORT ->
> > netif_running(priv->netdev)
> > 
> > Because mwifiex_del_virtual_intf() doesn't stop any outstanding
> > commands, we can be both deleting the netdev and processing scans for
> > it.
> 
> Huh, well, I guess you need some kind of locking here anyway, since the
> user can always do things like deleting the interface while a scan is
> running?

Yes, some sort of locking, and maybe ability to cancel outstanding
commands on just the targeted interface. I gave the locking a try myself
previously and got something sorta working, before getting distracted by
other problems. I also reported this directly to Marvell to see if they
could be bothered to fix it. They might be working on that.

But actually I think the rmmod or reset code path has this a little
easier, since we're fine just killing all outstanding commands and
interfaces. So these two problems are somewhat orthogonal.

> > > > Also, IIUC, we need to wait for all control paths to complete (or
> > > > cancel) before we can free up the associated resources; so just
> > > > marking "unavailable" isn't enough.
> > > 
> > > Yeah, I suppose so. Though if you just do all the freeing after
> > > wiphy_unregister() it'll do that for you?
> > 
> > Yes, I think so. Then part of the problem is probably that some of
> > the current "cancel command" logic is tied up with the "free command
> > structures" logic. So we're freeing some stuff too early.
> > 
> > Anyway, those sorts of bugs aside, IIUC the full sequence for
> > teardown should probably be something like:
> > 
> > 1. Stop TX queues
> > 2. Cancel outstanding commands (let them fail or finish, etc.) -- but
> >    DON'T free their backing resources yet

I also failed to mention "don't queue new FW commands". The driver does
this before step 1 currently, though the code isn't beautiful:

mwifiex_send_cmd()
...
if (adapter->surprise_removed) {
mwifiex_dbg(adapter, ERROR,
"PREP_CMD: card is removed\n");
return -1;
}
... // continue on to prepare and queue (or sync) the command

static void mwifiex_uninit_sw(struct mwifiex_adapter *adapter)
{
...
adapter->surprise_removed = true;
... // continue on to step 1, 2, ...


(And now that I think about it, I'm pretty sure there's a race in there
somewhere... Someone could easily miss the "surprise removed" check, grab a
command node, and miss out on step 2 (since the command isn't sitting on any of
the queues that get "canceled" yet). I believe this can easily blow up once
they try to queue the command, as we are no longer ready to handle the command
queue...)

> > 3. Remove wdevs
> > 4. wiphy_unregister()
> > 5. Free up resources
> > 
> > Current problems are at least:
> > 
> > * we don't do step 4 in the right place (if at all; see this patch)
> > * step 2 mixes in "free"ing resources too early
> 
> So I'm not sure what you mean by splitting in 2/5 - this seems
> reasonable, but I don't understand why something like a scan request
> wouldn't be freed while you cancel it in 2? In fact, you really have to
> free it before you remove the corresponding wdev, or cfg80211 will
> complain?

I haven't validated all the related code, but I think the problem isn't
that a scan is still being processed

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

[Brian Norris]

On Thu, Jun 22, 2017 at 03:02:34PM +0200, Johannes Berg wrote:
> On Wed, 2017-06-21 at 11:27 -0700, Brian Norris wrote:

> > > Without checking the code now, it seems entirely plausible that
> > > this is
> > > holding some lock that would lock out the control path entirely,
> > > for
> > > the duration until the wiphy is actually unregistered?
> > > 
> > > Actually, you can't unregister with the relevant locks held
> > > (without
> > > causing deadlocks), so perhaps it's marking the wiphy as
> > > unavailable so
> > > that all operations fail?
> > 
> > One of the above two sounds along the right line. But it's something
> > I couldn't really figure out how to do quite right.
> > 
> > Dumb question: how would I mark the wiphy as unavailable? Is there
> > something I can do at the cfg80211 level? Or would I really have to
> > guard all the cfg80211 entry points into mwifiex with a flag or lock?
> 
> There isn't really a good way to do this. You can, of course, call
> wiphy_unregister(), but if you could do that you'd already have the
> problem solved, I think?

That's probably along the right track. There are still some things we'd
need to do properly before that though, and this is where all the
problems are so far. (Also, this is what Kalle was already objecting to;
he didn't think we should be unregistering/recreating the wiphy, but I
think he ended up softening on that a bit.)

For one, I still expect I should be removing the wireless dev's before
unregistering the wihpy, no? Otherwise, there will be existing wdevs
backed by an unregistered wiphy?

And that gets to the heart of another bug: deleting interfaces (e.g.,
"iw dev foo del") races with a lot of stuff -- like see

mwifiex_process_sta_event() ->
  EVENT_EXT_SCAN_REPORT ->
netif_running(priv->netdev)

Because mwifiex_del_virtual_intf() doesn't stop any outstanding
commands, we can be both deleting the netdev and processing scans for
it.

> I'm not really familiar enough with the context this happens in - can't
> you let all the operations that try to talk to the firmware fail
> (because the firmware is dead, or whatever) and then call
> wiphy_unregister()?

Yes, something like that, barring some of the other bugs mentioned.

> > Also, IIUC, we need to wait for all control paths to complete (or
> > cancel) before we can free up the associated resources; so just
> > marking "unavailable" isn't enough.
> 
> Yeah, I suppose so. Though if you just do all the freeing after
> wiphy_unregister() it'll do that for you?

Yes, I think so. Then part of the problem is probably that some of the
current "cancel command" logic is tied up with the "free command
structures" logic. So we're freeing some stuff too early.

Anyway, those sorts of bugs aside, IIUC the full sequence for teardown
should probably be something like:

1. Stop TX queues
2. Cancel outstanding commands (let them fail or finish, etc.) -- but
   DON'T free their backing resources yet
3. Remove wdevs
4. wiphy_unregister()
5. Free up resources

Current problems are at least:

* we don't do step 4 in the right place (if at all; see this patch)
* step 2 mixes in "free"ing resources too early

Brian

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

[Brian Norris]

(A little slow on follow-up here)

On Thu, Jun 22, 2017 at 02:59:49PM +0200, Johannes Berg wrote:
> On Wed, 2017-06-21 at 10:48 -0700, Brian Norris wrote:
> > 
> > Yes, that all sounds nice. But for my sake, can you describe better
> > what's actually going on there (e.g., can you point me at which code
> > does this)? 
> 
> It's much easier with mac80211, it has all the state. Basically the
> reconfig is in ieee80211_reconfig() :)

Wow, that's not exactly simple code; I expect it could be pretty
difficult to get that right today on mwifiex. The current approach
actually should be *easier* (for the kernel side) to avoid bugs, as it
should be basically the same thing as 'rmmod'. Nonetheless, there are
plenty of bugs.

Thanks for the pointer though.

> > I'm really not familiar with mac80211 (though I was aware of
> > the above general behavior). But to my knowledge, mac80211 drivers
> > keep a lot more state managed in the kernel, so it's a little easier
> > and more natural to get the driver/FW back to "the same state" than
> > it is with a full-MAC driver.
> 
> Indeed.

Brian

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

[Brian Norris]

Hi Johannes,

On Fri, Jun 09, 2017 at 11:03:38AM +0200, Johannes Berg wrote:
> On Mon, 2017-06-05 at 18:54 +0300, Kalle Valo wrote:
> 
> > > BTW, since you're taking an interest in this code now, can I
> > > trouble you with a question? Looking at mwifiex_uninit_sw() (after
> > > this patchset), you can see a loop like this:
> > > 
> > > /* Stop data */
> > > for (i = 0; i < adapter->priv_num; i++) {
> > > priv = adapter->priv[i];
> > > if (priv && priv->netdev) {
> > > mwifiex_stop_net_dev_queue(priv->netdev,
> > > adapter);
> > > if (netif_carrier_ok(priv->netdev))
> > > netif_carrier_off(priv->netdev);
> > > netif_device_detach(priv->netdev);
> > > }
> > > }
> > > 
> > > That seems to be the only attempt to prevent user space from
> > > talking to the device while we proceed to shut down
> > > (mwifiex_shutdown_drv()). AIUI, that's wholly insufficient, and we
> > > need to actually stop all the virtual interfaces (and possibly the
> > > wiphy as well) first. I'm looking at trying to move the
> > > mwifiex_del_virtual_intf() loop up much further in this function
> > > (but there are other bugs preventing me from doing that yet).
> > > 
> > > Does that sound like the right approach to you? I'm kinda figuring
> > > this should better mimic the mac80211 ieee80211_remove_interfaces()
> > > structure.
> > 
> > Johannes is much better person to answer this (CCed).
> 
> Wait, what? You're throwing me into pretty deep water ;-)

Regardless, thanks for the help :)

> I'm not sure what you mean by "we need to atually stop all the virtual
> interfaces ([...]) first".

Judging by your following comments, I may have been completely mistaken.
(But that's why I asked you folks!)

> There are essentially only two/three ways to reach this - data path,
> which is getting stopped here, and control path (both nl80211 and
> perhaps ndo ops like start/stop).

I think I was conflating virtual interfaces with control path (e.g.,
nl80211 scans, set freq, etc.). The idea is that control operations may
still get *started* after the above, and it's just plain impossible to
resolve the races with driver queue teardown if we're queueing up new
control ops at the same time.

But even if we kill off the wireless_dev's, I suppose there are still
control interfaces that can talk directly to the wiphy.

> Without checking the code now, it seems entirely plausible that this is
> holding some lock that would lock out the control path entirely, for
> the duration until the wiphy is actually unregistered?
> 
> Actually, you can't unregister with the relevant locks held (without
> causing deadlocks), so perhaps it's marking the wiphy as unavailable so
> that all operations fail?

One of the above two sounds along the right line. But it's something I
couldn't really figure out how to do quite right.

Dumb question: how would I mark the wiphy as unavailable? Is there
something I can do at the cfg80211 level? Or would I really have to
guard all the cfg80211 entry points into mwifiex with a flag or lock?

Also, IIUC, we need to wait for all control paths to complete (or
cancel) before we can free up the associated resources; so just marking
"unavailable" isn't enough.

Thanks,
Brian

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

[Brian Norris]

Hi Kalle (and Johannes; I'll reply to Johannes response separately too),

On Mon, Jun 05, 2017 at 06:54:18PM +0300, Kalle Valo wrote:
> Brian Norris <briannor...@chromium.org> writes:
> > That's not to say that there aren't such bugs out there. I'd still be
> > willing to bet there are. And IMO, it seems wise to just do the same
> > teardown/setup as one would do for (e.g.) 'rmmod', to prevent exposing
> > *too* many new permutations of "wiphy is available but rest of the
> > driver is torn down".
> 
> This feels like a sledge hammer approach causing all sort of problems

Yes, it is a sledge hammer. But I'm working with what we have here. With
this approach, it's also easier to tell that things aren't out-of-sync,
since I'm never quite sure how much state was held in the firmware (and
now won't match what user space thinks). A full removal / re-init makes
this clear -- user space should expect *everything* to be reset.

I'm open to learning better approaches if possible, but this also might
be difficult if I don't get any support from Marvell on this. (They seem
quite happy to let sleeping dogs lie.)

> for user space and I really like the mac80211 approach more. For
> example, if an ath10k firmware crash happens user only sees a few second
> pause in data traffic and a warning in kernel log, otherwise everything
> happens behind the scenes. Of course there are very likely races
> somewhere but at least I haven't seen that many reports related to
> firmware restart functionality.

Yes, that all sounds nice. But for my sake, can you describe better
what's actually going on there (e.g., can you point me at which code
does this)? I'm really not familiar with mac80211 (though I was aware of
the above general behavior). But to my knowledge, mac80211 drivers keep
a lot more state managed in the kernel, so it's a little easier and
more natural to get the driver/FW back to "the same state" than it is
with a full-MAC driver.

> > But if none of this is convincing to you, I can take a stab at a
> > different solution.
> 
> I don't have any problem applying this patch but more about being
> curious why doing it like this. And hopefully finding a less intrusive
> solution in the future.

OK, sure. I'll see what I can do, but I don't see an easy path at the
moment toward fixing (i.e., completely rewriting) this long-standing
driver behavior.

[trim]

Thanks,
Brian

Re: porting 'mwifiex: resolve races between async FW init (failure) and device removal' to kernel 4.1.x

[Brian Norris]

On Thu, Jun 08, 2017 at 01:11:28PM +, Chadzynski, MikolajX wrote:
> Races between async firmware initialization and device removal appears on 
> kernel 4.1.x.
> Whole scenario leading to the error together with logs is in 
> https://bugzilla.kernel.org/show_bug.cgi?id=196003
> Mapping the patch from kernel 4.11.x "mwifiex: resolve races between async FW 
> init (failure) and device removal" solved the problem.

FWIW, that patch was also in v4.10.

> In Bugzilla I've also attached patch which I've tested for pcie interface and 
> it works.

What are you asking for? An official backport to the 4.1.x stable
series? I'm not super interested in trying to do that. And IIUC, your
patch is a no-go, as it will break the SDIO and USB driver builds.

If you do try to go forward with this, do try to note any follow-up
patches. I don't recall how many other related bugs there might have
been. I tried to mark things correctly with Fixes tags, but I may have
missed something, especially if such bugfixes also made it into 4.10.

Brian

Re: [PATCH 05/14] mwifiex: re-register wiphy across reset

[Brian Norris]

On Thu, Jun 01, 2017 at 12:15:45PM +0300, Kalle Valo wrote:
> Brian Norris <briannor...@chromium.org> writes:
> 
> > In general, it's helpful to use the same code for device removal as for
> > device reset, as this tends to have fewer bugs. Let's move the wiphy
> > unregistration code into the common reset and removal code.
> >
> > In particular, it's very hard to properly handle the reset sequence when
> > something fails. Currently, if mwifiex_reinit_sw() fails, we've failed
> > to unregister the associated wiphy, and so running something as simple
> > as "iw phy" can trigger an OOPS, as the wiphy still has hooks back into
> > freed mwifiex data structures. For example, KASAN complained:
> >
> > [... see reset fail for other reasons ...]
> > [ 1184.821158] mwifiex_pcie :01:00.0: info: dnld wifi firmware from 
> > 174948 bytes
> > [ 1186.870914] mwifiex_pcie :01:00.0: info: FW download over, size 
> > 608396 bytes
> > [ 1187.685990] mwifiex_pcie :01:00.0: WLAN FW is active
> > [ 1187.692673] mwifiex_pcie :01:00.0: cmd_wait_q terminated: -512
> > [ 1187.699075] mwifiex_pcie :01:00.0: info: _mwifiex_fw_dpc: unregister 
> > device
> > [ 1187.713476] mwifiex: Failed to bring up adapter: -5
> > [ 1187.718644] mwifiex_pcie :01:00.0: reinit failed: -5
> >
> > [... run `iw phy` ...]
> > [ 1212.902419] 
> > ==
> > [ 1212.909806] BUG: KASAN: use-after-free in 
> > mwifiex_cfg80211_get_antenna+0x54/0xfc [mwifiex] at addr ffc0ad1a8028
> > [ 1212.920246] Read of size 1 by task iw/3127
> > [...]
> > [ 1212.934946] page dumped because: kasan: bad access detected
> > [...]
> > [ 1212.950665] Call trace:
> > [ 1212.953148] [] dump_backtrace+0x0/0x190
> > [ 1212.958572] [] show_stack+0x20/0x28
> > [ 1212.963648] [] dump_stack+0xa4/0xcc
> > [ 1212.968723] [] kasan_report+0x378/0x500
> > [ 1212.974140] [] __asan_load1+0x44/0x4c
> > [ 1212.979462] [] mwifiex_cfg80211_get_antenna+0x54/0xfc 
> > [mwifiex]
> > [ 1212.987131] [] nl80211_send_wiphy+0x75c/0x2de0 
> > [cfg80211]
> > [ 1212.994246] [] nl80211_dump_wiphy+0x32c/0x438 
> > [cfg80211]
> > [ 1213.001149] [] genl_lock_dumpit+0x48/0x64
> > [ 1213.006746] [] netlink_dump+0x178/0x398
> > [ 1213.012171] [] __netlink_dump_start+0x1bc/0x260
> > [...]
> >
> > This all goes away if we just tear down the wiphy on the way down, and
> > set it back up if/when we bring the device back up.
> 
> I don't know exactly what kind of reset this is about,

Marvell firmwares are known to be quite buggy, and there are plenty of
situations in which they crash (often resulting in a command timeout).
The current best workaround for these is to essentially unwind the whole
driver, reset the card, and reprobe the whole thing. See anywhere that
the ->card_reset() callback is called.

This has been around for a long time on SDIO, and you recently merged my
changes to enable this for PCIe:

6d7d579a8243 mwifiex: pcie: add card_reset() support

> but isn't this a
> quite intrusive solution? For example, phy name changes because of this?

Yes, it is a bit intrusive. But the whole process is intrusive, as it
deletes all the virtual interfaces and loses your settings. This all
relies on user space being prepared to clean up and reinitialize
everything afterward.

And yes, this causes a phy name change.

In favor: this is what the SDIO reset code *used* to do, before this
commit:

c742e623e941 mwifiex: sdio card reset enhancement

where the SDIO driver started using the half-baked reset solution
written for PCIe.

Lastly, I still need to analyze a few more things in this driver, but
AFAICT, if we *don't* unregister the wiphy, we are exposed to quite a
few more race conditions -- not just the easy-to-notice condition
described above. What happens if the wiphy still processes cfg80211
operations while we're still resetting the firmware? Much of the driver
may not be prepared for this. At the moment, I can't find anything
terribly wrong; if I slow down the reset (e.g., with msleep()s) I can
just trigger complaints about "cmd node not available" or "card is
removed", but I haven't yet found a true bug.

That's not to say that there aren't such bugs out there. I'd still be
willing to bet there are. And IMO, it seems wise to just do the same
teardown/setup as one would do for (e.g.) 'rmmod', to prevent exposing
*too* many new permutations of "wiphy is available but rest of the
driver is torn down".

But if none of this is convincing to you, I can take a stab at a
different solution.

BTW, since you're taking an interest in

Re: [PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks

[Brian Norris]

By the way, this had a few review comments elsewhere, which I'll
summarize here, since I plan to resubmit a new version sometime.

On Wed, May 24, 2017 at 05:11:06PM -0700, Brian Norris wrote:
> It seems that the implicit assumption of the mwifiex
> {enable,disable}_int() callbacks is that after ->disable_int(), all
> interrupt handling should be complete (synchronized) and not fire again
> until after ->enable_int(). Also, interrupts should not be serviced
> until after the first ->enable_int().
> 
> However, the PCIe driver does none of this. First, the existing
> interrupt mask programming appears to only have an effect for legacy
> interrupts. It doesn't actually prevent MSI/MSI-X interrupts. Second,
> even when it might mask interrupts, we're doing nothing to ensure that
> pending IRQs have finished processing; they could be already in-flight
> when a CPU masks them.
> 
> Another quirk of this driver's design is the use of a racy
> "surprise_removed" check in mwifiex_pcie_interrupt(). This appears to
> act like a racy poor-man's version of masking our interrupts -- it
> allows us to short-circuit the ISR if it fires when we're not prepared
> to handle more work.
> 
> We can resolve this all by:
> (a) disabling our IRQs after requesting them
> (b) call {enable,disable}_irq() in the {enable,disable}_int() callbacks
> (c) remove the racy '->surprise_removed' hack from
> mwifiex_pcie_interrupt()
> (d) document the effect (or lack thereof) of PCIE_HOST_INT_MASK, to
> clarify and possibly prevent future misuse
> 
> Along the way, I decided to use underscores to prefix the driver-private
> forms of "disabling interrupts" (instead of the awkward "_noerr" suffix
> used already), partly to discourage their use.
> 
> Signed-off-by: Brian Norris <briannor...@chromium.org>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 70 
> -
>  1 file changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 394224d6c219..ea75315bf19d 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -505,12 +505,10 @@ static int mwifiex_pm_wakeup_card_complete(struct 
> mwifiex_adapter *adapter)
>  }
>  
>  /*
> - * This function disables the host interrupt.
> - *
> - * The host interrupt mask is read, the disable bit is reset and
> - * written back to the card host interrupt mask register.
> + * This function masks the host interrupt. Effective only for legacy PCI
> + * interrupts.
>   */
> -static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
> +static int __mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
>  {
>   if (mwifiex_pcie_ok_to_access_hw(adapter)) {
>   if (mwifiex_write_reg(adapter, PCIE_HOST_INT_MASK,
> @@ -525,18 +523,30 @@ static int mwifiex_pcie_disable_host_int(struct 
> mwifiex_adapter *adapter)
>   return 0;
>  }
>  
> -static void mwifiex_pcie_disable_host_int_noerr(struct mwifiex_adapter 
> *adapter)
> +/*
> + * Disable interrupts, synchronizing with any outstanding interrupts.
> + */
> +static void mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
>  {
> - WARN_ON(mwifiex_pcie_disable_host_int(adapter));
> + struct pcie_service_card *card = adapter->card;
> + int i;
> +
> + WARN_ON(__mwifiex_pcie_disable_host_int(adapter));
> +
> + if (card->msix_enable) {
> + for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
> + disable_irq(card->msix_entries[i].vector);
> + }
> + } else {
> + disable_irq(card->dev->irq);

This approach is not safe for the non-MSI-X case, since we actually
requested this IRQ with IRQF_SHARED. That's likely mostly for the legacy
PCI interrupt case (where we *have* to support shared interrupts) and
could probably be modified, but at any rate, this is unsafe as written.

Also, I've fielded objections to using the host-level IRQ masking for
disabling MSI interrupts here. I'm still not completely sure *why* the
objection, but I'm investigating whether there's any device-level
mechanism for disabling MSI interrupts on the Wif card. (Marvell folks,
feel free to speak up here.)

> + }
>  }
>  
>  /*
> - * This function enables the host interrupt.
> - *
> - * The host interrupt enable mask is written to the card
> - * host interrupt mask register.
> + * This function unmasks the host interrupt. Effective only for legacy PCI
> + * interrupts.
>   */
> -static int mwifiex_pcie_enable_host_int(struct mwifiex_ad

Re: [PATCH] mwifiex: simplify the code around ra_list

[Brian Norris]

On Fri, May 26, 2017 at 09:41:49AM +0800, Shawn Lin wrote:
> We don't need to check if the list is empty separately
> as we could use list_first_entry_or_null to cover it.
> 
> Signed-off-by: Shawn Lin <shawn@rock-chips.com>

Looks fine to me.

Reviewed-by: Brian Norris <briannor...@chromium.org>

[PATCH 01/14] mwifiex: pcie: properly synchronize, disable interrupts in driver callbacks

[Brian Norris]

It seems that the implicit assumption of the mwifiex
{enable,disable}_int() callbacks is that after ->disable_int(), all
interrupt handling should be complete (synchronized) and not fire again
until after ->enable_int(). Also, interrupts should not be serviced
until after the first ->enable_int().

However, the PCIe driver does none of this. First, the existing
interrupt mask programming appears to only have an effect for legacy
interrupts. It doesn't actually prevent MSI/MSI-X interrupts. Second,
even when it might mask interrupts, we're doing nothing to ensure that
pending IRQs have finished processing; they could be already in-flight
when a CPU masks them.

Another quirk of this driver's design is the use of a racy
"surprise_removed" check in mwifiex_pcie_interrupt(). This appears to
act like a racy poor-man's version of masking our interrupts -- it
allows us to short-circuit the ISR if it fires when we're not prepared
to handle more work.

We can resolve this all by:
(a) disabling our IRQs after requesting them
(b) call {enable,disable}_irq() in the {enable,disable}_int() callbacks
(c) remove the racy '->surprise_removed' hack from
mwifiex_pcie_interrupt()
(d) document the effect (or lack thereof) of PCIE_HOST_INT_MASK, to
clarify and possibly prevent future misuse

Along the way, I decided to use underscores to prefix the driver-private
forms of "disabling interrupts" (instead of the awkward "_noerr" suffix
used already), partly to discourage their use.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 70 -
 1 file changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 394224d6c219..ea75315bf19d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -505,12 +505,10 @@ static int mwifiex_pm_wakeup_card_complete(struct 
mwifiex_adapter *adapter)
 }
 
 /*
- * This function disables the host interrupt.
- *
- * The host interrupt mask is read, the disable bit is reset and
- * written back to the card host interrupt mask register.
+ * This function masks the host interrupt. Effective only for legacy PCI
+ * interrupts.
  */
-static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
+static int __mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
 {
if (mwifiex_pcie_ok_to_access_hw(adapter)) {
if (mwifiex_write_reg(adapter, PCIE_HOST_INT_MASK,
@@ -525,18 +523,30 @@ static int mwifiex_pcie_disable_host_int(struct 
mwifiex_adapter *adapter)
return 0;
 }
 
-static void mwifiex_pcie_disable_host_int_noerr(struct mwifiex_adapter 
*adapter)
+/*
+ * Disable interrupts, synchronizing with any outstanding interrupts.
+ */
+static void mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
 {
-   WARN_ON(mwifiex_pcie_disable_host_int(adapter));
+   struct pcie_service_card *card = adapter->card;
+   int i;
+
+   WARN_ON(__mwifiex_pcie_disable_host_int(adapter));
+
+   if (card->msix_enable) {
+   for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
+   disable_irq(card->msix_entries[i].vector);
+   }
+   } else {
+   disable_irq(card->dev->irq);
+   }
 }
 
 /*
- * This function enables the host interrupt.
- *
- * The host interrupt enable mask is written to the card
- * host interrupt mask register.
+ * This function unmasks the host interrupt. Effective only for legacy PCI
+ * interrupts.
  */
-static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
+static int __mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
 {
if (mwifiex_pcie_ok_to_access_hw(adapter)) {
/* Simply write the mask to the register */
@@ -551,6 +561,26 @@ static int mwifiex_pcie_enable_host_int(struct 
mwifiex_adapter *adapter)
return 0;
 }
 
+static int mwifiex_pcie_enable_host_int(struct mwifiex_adapter *adapter)
+{
+   struct pcie_service_card *card = adapter->card;
+   int i, ret;
+
+   ret = __mwifiex_pcie_enable_host_int(adapter);
+   if (ret)
+   return ret;
+
+   if (card->msix_enable) {
+   for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++) {
+   enable_irq(card->msix_entries[i].vector);
+   }
+   } else {
+   enable_irq(card->dev->irq);
+   }
+
+   return 0;
+}
+
 /*
  * This function initializes TX buffer ring descriptors
  */
@@ -1738,7 +1768,7 @@ static int mwifiex_pcie_process_cmd_complete(struct 
mwifiex_adapter *adapter)
while (reg->sleep_cookie && (count++ < 10) &&
   mwifiex_pcie_ok_to_access_hw(adapter))

[PATCH 07/14] mwifiex: fixup init_channel_scan_gap error case

[Brian Norris]

In reading through _mwifiex_fw_dpc(), I noticed that after we've
registered our wiphy, we still have error paths that don't free it back
up. Let's do that.

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index be3badba028a..c6cdbc311471 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -584,7 +584,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, 
void *context)
if (mwifiex_init_channel_scan_gap(adapter)) {
mwifiex_dbg(adapter, ERROR,
"could not init channel stats table\n");
-   goto err_init_fw;
+   goto err_init_chan_scan;
}
 
if (driver_mode) {
@@ -632,6 +632,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, 
void *context)
 
 err_add_intf:
vfree(adapter->chan_stats);
+err_init_chan_scan:
wiphy_unregister(adapter->wiphy);
wiphy_free(adapter->wiphy);
 err_init_fw:
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 02/14] mwifiex: reunite copy-and-pasted remove/reset code

[Brian Norris]

When PCIe FLR code was added, it explicitly copy-and-pasted much of
mwifiex_remove_card() into mwifiex_shutdown_sw(). This is unnecessary,
as almost all of the code should be reused.

Let's reunite what we can for now.

The only functional changes for now:

 * call netif_device_detach() in the remove() code path -- this wasn't
   done before, but it really should be a no-op, when the device is
   getting totally unregistered soon anyway

 * call the ->down_dev() driver callback only after we've finished all
   SW teardown -- this should have no significant effect, since the only
   user (pcie.c) does very minimal work there, and it doesn't matter
   that we reorder this

Signed-off-by: Brian Norris <briannor...@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/main.c | 104 
 1 file changed, 28 insertions(+), 76 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c 
b/drivers/net/wireless/marvell/mwifiex/main.c
index dd87b9ff64c3..a1e98e36c1ce 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1348,26 +1348,12 @@ static void mwifiex_main_work_queue(struct work_struct 
*work)
mwifiex_main_process(adapter);
 }
 
-/*
- * This function gets called during PCIe function level reset. Required
- * code is extracted from mwifiex_remove_card()
- */
-int
-mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
+/* Common teardown code used for both device removal and reset */
+static void mwifiex_uninit_sw(struct mwifiex_adapter *adapter)
 {
struct mwifiex_private *priv;
int i;
 
-   if (!adapter)
-   goto exit_return;
-
-   wait_for_completion(adapter->fw_done);
-   /* Caller should ensure we aren't suspending while this happens */
-   reinit_completion(adapter->fw_done);
-
-   priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
-   mwifiex_deauthenticate(priv, NULL);
-
/* We can no longer handle interrupts once we start doing the teardown
 * below.
 */
@@ -1389,12 +1375,9 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
}
 
mwifiex_dbg(adapter, CMD, "cmd: calling mwifiex_shutdown_drv...\n");
-
mwifiex_shutdown_drv(adapter);
-   if (adapter->if_ops.down_dev)
-   adapter->if_ops.down_dev(adapter);
-
mwifiex_dbg(adapter, CMD, "cmd: mwifiex_shutdown_drv done\n");
+
if (atomic_read(>rx_pending) ||
atomic_read(>tx_pending) ||
atomic_read(>cmd_pending)) {
@@ -1417,9 +1400,30 @@ mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
rtnl_unlock();
}
vfree(adapter->chan_stats);
+}
+
+/*
+ * This function gets called during PCIe function level reset.
+ */
+int mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
+{
+   struct mwifiex_private *priv;
+
+   if (!adapter)
+   return 0;
+
+   wait_for_completion(adapter->fw_done);
+   /* Caller should ensure we aren't suspending while this happens */
+   reinit_completion(adapter->fw_done);
+
+   priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
+   mwifiex_deauthenticate(priv, NULL);
+
+   mwifiex_uninit_sw(adapter);
+
+   if (adapter->if_ops.down_dev)
+   adapter->if_ops.down_dev(adapter);
 
-   mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
-exit_return:
return 0;
 }
 EXPORT_SYMBOL_GPL(mwifiex_shutdown_sw);
@@ -1672,61 +1676,10 @@ EXPORT_SYMBOL_GPL(mwifiex_add_card);
  */
 int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 {
-   struct mwifiex_private *priv = NULL;
-   int i;
-
if (!adapter)
-   goto exit_remove;
-
-   /* We can no longer handle interrupts once we start doing the teardown
-* below. */
-   if (adapter->if_ops.disable_int)
-   adapter->if_ops.disable_int(adapter);
-
-   adapter->surprise_removed = true;
-
-   mwifiex_terminate_workqueue(adapter);
-
-   /* Stop data */
-   for (i = 0; i < adapter->priv_num; i++) {
-   priv = adapter->priv[i];
-   if (priv && priv->netdev) {
-   mwifiex_stop_net_dev_queue(priv->netdev, adapter);
-   if (netif_carrier_ok(priv->netdev))
-   netif_carrier_off(priv->netdev);
-   }
-   }
-
-   mwifiex_dbg(adapter, CMD,
-   "cmd: calling mwifiex_shutdown_drv...\n");
-
-   mwifiex_shutdown_drv(adapter);
-   mwifiex_dbg(adapter, CMD,
-   "cmd: mwifiex_shutdown_drv done\n");
-   if (atomic_read(>rx_pending) ||
-   atomic_read(>tx_pending) ||
-   atomic_read(>cmd_pending)) {
-   mwifiex_dbg(adapter, ERROR,
-