Re: [1/3] ath6kl: fix busreqs so they can be reused when sg is cleaned up
James Minor wrote: > To reuse the busreqs in case of hardware restart, they must be > properly reinitialized. If the scat_req pointer isn't reset to > 0, __ath6kl_sdio_write_async() will assume there is sg work to be > done (causing a kernel OOPS). > > Signed-off-by: James Minor > Reviewed-by: Steve deRosier 3 patches applied to ath-next branch of ath.git, thanks. 3605d751d5dd ath6kl: fix busreqs so they can be reused when sg is cleaned up db14b18a73a1 ath6kl: after cleanup properly reflect that sg is disabled fdb6e4839e3a ath6kl: configure SDIO when power is reapplied -- https://patchwork.kernel.org/patch/9360777/ Documentation about submitting wireless patches and checking status from patchwork: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [1/2] ath10k: clean up HTT tx buffer allocation and free
Mohammed Shafi Shajakhan wrote:
> From: Mohammed Shafi Shajakhan
>
> cleanup 'ath10k_htt_tx_alloc' by introducing the API's
> 'ath10k_htt_tx_alloc/free_{cont_txbuf, txdone_fifo} and
> re-use them whereever needed
>
> Signed-off-by: Mohammed Shafi Shajakhan
Patch 1 doesn't apply and the conflict was not trivial
error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:388
error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
stg import: Diff does not apply cleanly
error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:283
error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
stg import: Diff does not apply cleanly
2 patches set to Changes Requested.
9361741 [1/2] ath10k: clean up HTT tx buffer allocation and free
9361743 [2/2] ath10k: Remove extraneous error message in tx alloc
--
https://patchwork.kernel.org/patch/9361741/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [1/2] ath10k: clean up HTT tx buffer allocation and free
On Thu, Oct 06, 2016 at 09:31:41AM +0200, Kalle Valo wrote:
> Mohammed Shafi Shajakhan wrote:
> > From: Mohammed Shafi Shajakhan
> >
> > cleanup 'ath10k_htt_tx_alloc' by introducing the API's
> > 'ath10k_htt_tx_alloc/free_{cont_txbuf, txdone_fifo} and
> > re-use them whereever needed
> >
> > Signed-off-by: Mohammed Shafi Shajakhan
>
> Patch 1 doesn't apply and the conflict was not trivial
[shafi] oops will rebase it, not sure how i missed it :-(
>
> error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:388
> error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
> stg import: Diff does not apply cleanly
> error: patch failed: drivers/net/wireless/ath/ath10k/htt_tx.c:283
> error: drivers/net/wireless/ath/ath10k/htt_tx.c: patch does not apply
> stg import: Diff does not apply cleanly
>
> 2 patches set to Changes Requested.
>
> 9361741 [1/2] ath10k: clean up HTT tx buffer allocation and free
> 9361743 [2/2] ath10k: Remove extraneous error message in tx alloc
>
> --
> https://patchwork.kernel.org/patch/9361741/
>
> Documentation about submitting wireless patches and checking status
> from patchwork:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>
Re: [PATCH] ath10k: cache calibration data when the core is stopped.
Marty Faltesek writes: > On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle wrote: >> Marty Faltesek writes: >> >>> Caching calibration data allows it to be accessed when the >>> device is not active. >>> >>> Signed-off-by: Marty Faltesek >> >> No comma in the title, please. >> >> What tree did you use as the baseline? This doesn't seem to apply to >> ath.git: > > We use backports 20160122 which has not been updated since earlier this year. > I can forward port it to your tree, and make sure > it builds but won't be able to test it. Will that be OK? Sure, I can test it. >> Also please note that this patch (which I'm queuing to 4.9) touches the >> same area: >> >> ath10k: fix debug cal data file >> >> https://patchwork.kernel.org/patch/9340953/ > > I've modified this too, and this won't be necessary, so can you drop > it? If not, let me know and I'll pull it in and make sure I'm based > off it too. Actually I was first planning to push 9340953 to 4.9 and take your patch to 4.10. But your patch is a cleaner approach to this and maybe I should push that to 4.9 instead? Need to think a bit more. -- Kalle Valo
Re: bcmdhd: Strange Power Save messages
On Wed, Oct 5, 2016 at 11:12 AM, Arend Van Spriel wrote: > On 4-10-2016 13:39, Gucea Doru wrote: >> On Sat, Oct 1, 2016 at 2:52 PM, Arend van Spriel >>> wrote: On 29-09-16 13:32, Gucea Doru wrote: > On Tue, Sep 27, 2016 at 12:03 PM, Gucea Doru wrote: >> What is the decision triggering the exit from the PS mode immediately >> after the ping request? I am asking this because 802.11 PS legacy >> specifies that the client should wait for a beacon with TIM set in >> order to wake up: in my case, there is no beacon between the ping >> request message and the Null frame that announces the exit from the PS >> mode. > > > Any help would be highly appreciated :) Actually though I already sent you are reply, but alas here it is. bcmdhd is our aosp driver. I am maintaining the upstream brcm80211 drivers. Regardless your question is more for firmware running on the device. So like the same behavior would be observed when using brcmfmac with same firmware. > IEEE Std 802.11-2012, section 10.2.1.8 specifies that "when the STA > detects that the bit corresponding to its AID is 1 i the TIM, the STA > shall issue a PS Poll". In my capture there are cases when the STA > exits the PS mode without waiting for a beacon. It is a bit tricky, but the standard does not explicitly say the STA should be in power-save at any other time. So it is difficult to say what event occurred on the STA side to exit PS mode. Also STA means P2P-Client as you say. That means that you have multiple interfaces: regular STA and P2P-Client. So is the STA connected to some other AP or just not connected. wpa_supplicant will do intermittent scan or initiate scheduled scan by which firmware will scan at a certain interval. That is just some things I can come up with and I am sure there are more. >> >> I agree that there may be some events belonging to the regular STA >> interface that could trigger the Null Frame (which includes the exit >> from PS Mode). However, I would expect to see some management frames >> in the air before/after the Null Packet (e.g.: a Probe request in case >> of a scheduled scan). But in my case the trigger for the Null frame >> seems to be the ping request packet, the scenario is the same every >> time: ping request -> Block ACK -> Null Frame (Wireshark trace >> confirms this behavior). >> >> I thought that you had a power save optimization algorithm that keeps >> the card on a few milliseconds just to see if we can have a fast reply >> from the peer. Does this ring a bell? :) > > It does not. That would be implemented in firmware. As said I am working > on brcmfmac/brcmsmac. So bcmdhd and firmware are not my expertise. > Arend, could you please redirect me to a Broadcom firmware maintainer? Thank you, Doru
Re: bcmdhd: Strange Power Save messages
On 6-10-2016 10:07, Gucea Doru wrote: > On Wed, Oct 5, 2016 at 11:12 AM, Arend Van Spriel > wrote: >> On 4-10-2016 13:39, Gucea Doru wrote: >>> On Sat, Oct 1, 2016 at 2:52 PM, Arend van Spriel wrote: > > > On 29-09-16 13:32, Gucea Doru wrote: >> On Tue, Sep 27, 2016 at 12:03 PM, Gucea Doru >> wrote: >>> What is the decision triggering the exit from the PS mode immediately >>> after the ping request? I am asking this because 802.11 PS legacy >>> specifies that the client should wait for a beacon with TIM set in >>> order to wake up: in my case, there is no beacon between the ping >>> request message and the Null frame that announces the exit from the PS >>> mode. >> >> >> Any help would be highly appreciated :) > > Actually though I already sent you are reply, but alas here it is. > > bcmdhd is our aosp driver. I am maintaining the upstream brcm80211 > drivers. Regardless your question is more for firmware running on the > device. So like the same behavior would be observed when using brcmfmac > with same firmware. > >> IEEE Std 802.11-2012, section 10.2.1.8 specifies that "when the STA >> detects that the bit corresponding to its AID is 1 i the TIM, the STA >> shall issue a PS Poll". In my capture there are cases when the STA >> exits the PS mode without waiting for a beacon. > > It is a bit tricky, but the standard does not explicitly say the STA > should be in power-save at any other time. So it is difficult to say > what event occurred on the STA side to exit PS mode. Also STA means > P2P-Client as you say. That means that you have multiple interfaces: > regular STA and P2P-Client. So is the STA connected to some other AP or > just not connected. wpa_supplicant will do intermittent scan or initiate > scheduled scan by which firmware will scan at a certain interval. That > is just some things I can come up with and I am sure there are more. >>> >>> I agree that there may be some events belonging to the regular STA >>> interface that could trigger the Null Frame (which includes the exit >>> from PS Mode). However, I would expect to see some management frames >>> in the air before/after the Null Packet (e.g.: a Probe request in case >>> of a scheduled scan). But in my case the trigger for the Null frame >>> seems to be the ping request packet, the scenario is the same every >>> time: ping request -> Block ACK -> Null Frame (Wireshark trace >>> confirms this behavior). >>> >>> I thought that you had a power save optimization algorithm that keeps >>> the card on a few milliseconds just to see if we can have a fast reply >>> from the peer. Does this ring a bell? :) >> >> It does not. That would be implemented in firmware. As said I am working >> on brcmfmac/brcmsmac. So bcmdhd and firmware are not my expertise. >> > > Arend, could you please redirect me to a Broadcom firmware maintainer? Can you please elaborate on your platform, broadcom chipset, and what version of bcmdhd you are using. Regards, Arend
[PATCH v2 2/2] ath10k: Remove extraneous error message in tx alloc
From: Mohammed Shafi Shajakhan
Remove extraneous error message in 'ath10k_htt_tx_alloc_cont_frag_desc'
as the caller 'ath10k_htt_tx_alloc' already dumps a proper error
message
Signed-off-by: Mohammed Shafi Shajakhan
---
[v2 rebased over top of tree]
drivers/net/wireless/ath/ath10k/htt_tx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c
b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 786fbd7..4255c1a 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -283,10 +283,8 @@ static int ath10k_htt_tx_alloc_cont_frag_desc(struct
ath10k_htt *htt)
htt->frag_desc.vaddr = dma_alloc_coherent(ar->dev, size,
&htt->frag_desc.paddr,
GFP_KERNEL);
- if (!htt->frag_desc.vaddr) {
- ath10k_err(ar, "failed to alloc fragment desc memory\n");
+ if (!htt->frag_desc.vaddr)
return -ENOMEM;
- }
return 0;
}
--
1.9.1
Re: nl80211 fine timing measurement support
On 10/4/2016 12:25 PM, Johannes Berg wrote: > >> If raw results are mainly used for analysis algorithms how about >> providing raw measurement data through debugfs. May even consider >> adding rtt calculation in cfg80211/mac80211 for drivers that choose >> to provide raw measurement data and still only report final RTT in >> nl80211 api. >> > > I think the "analysis algorithms" in this case are what actually gives > you the distance value. > > However, I don't think we should accept that everybody wants to run > their proprietary algorithms on top and expose only the values needed > for those. That makes the drivers only usable with additional > proprietary software, which may even be incompatible with the GPL. > > If the algorithms are in the device, then we can expose the final > results, and that's most useful for applications in the API. > > If they're not, then we need to expose something that can be used > without additional proprietary and device-specific algorithms. > > If those algorithms cannot be run in the device, then we should put > them into the driver instead. > After further internal discussion, we will be ok with reporting only the final RTT. In our current internal implementation, the location framework in user space gets the raw results (t1,t2,t3,t4) and does an algorithm which includes drift compensation in order to derive the final RTT. We will need to move this down to the driver, not trivial but can be done. However I think there might be platforms where you might need a more complicated algorithm which will need to run in user space so for the long term we may want to consider an option to report the raw results. The raw results are definitely important for debugging but Using debugfs is problematic because it is difficult to synchronize with the measurement session, especially if you have multiple bursts. It is probably best to report is as part of the session, together/in place of the RTT for each burst. Thanks, Lior > johannes >
Re: [PATCH] [wl18xx] Fix memory leakage if kzalloc fails
Hi,
On Wed, Oct 5, 2016 at 10:50 PM, Souptick Joarder wrote:
> This patch is added to properly handle memory leak if kzalloc fails
> in wl18xx_scan_send() and wl18xx_scan_sched_scan_config()
What memory leak?
> Signed-off-by: Souptick Joarder
> Signed-off-by: Rameshwar Sahu
Why two signed-off-bys?
> ---
> drivers/net/wireless/ti/wl18xx/scan.c | 20 ++--
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ti/wl18xx/scan.c
> b/drivers/net/wireless/ti/wl18xx/scan.c
> index 4e522154..aed22e1 100644
> --- a/drivers/net/wireless/ti/wl18xx/scan.c
> +++ b/drivers/net/wireless/ti/wl18xx/scan.c
> @@ -41,14 +41,13 @@ static void wl18xx_adjust_channels(struct
> wl18xx_cmd_scan_params *cmd,
> static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
> struct cfg80211_scan_request *req)
> {
> - struct wl18xx_cmd_scan_params *cmd;
> + struct wl18xx_cmd_scan_params *cmd = NULL;
> struct wlcore_scan_channels *cmd_channels = NULL;
> int ret;
>
> cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> if (!cmd) {
> - ret = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> }
>
> /* scan on the dev role if the regular one is not started */
> @@ -59,7 +58,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
> wl12xx_vif *wlvif,
>
> if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
> ret = -EINVAL;
> - goto out;
> + goto err_cmd_free;
> }
>
> cmd->scan_type = SCAN_TYPE_SEARCH;
> @@ -84,7 +83,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
> wl12xx_vif *wlvif,
> cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
> if (!cmd_channels) {
> ret = -ENOMEM;
> - goto out;
> + goto err_cmd_free;
> }
>
> wlcore_set_scan_chan_params(wl, cmd_channels, req->channels,
> @@ -153,6 +152,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
> wl12xx_vif *wlvif,
>
> out:
> kfree(cmd_channels);
> +err_cmd_free:
kfree(NULL) is valid, so therefore the out: and err_cmd_free: labels
are equivalent from a memory freeing perspective, so where exactly are
we leaking memory in this function?
> kfree(cmd);
> return ret;
> }
> @@ -171,7 +171,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
> struct cfg80211_sched_scan_request *req,
> struct ieee80211_scan_ies *ies)
> {
> - struct wl18xx_cmd_scan_params *cmd;
> + struct wl18xx_cmd_scan_params *cmd = NULL;
> struct wlcore_scan_channels *cmd_channels = NULL;
> struct conf_sched_scan_settings *c = &wl->conf.sched_scan;
> int ret;
> @@ -185,15 +185,14 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>
> cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> if (!cmd) {
> - ret = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> }
>
> cmd->role_id = wlvif->role_id;
>
> if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
> ret = -EINVAL;
> - goto out;
> + goto err_cmd_free;
> }
>
> cmd->scan_type = SCAN_TYPE_PERIODIC;
> @@ -218,7 +217,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
> cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
> if (!cmd_channels) {
> ret = -ENOMEM;
> - goto out;
> + goto err_cmd_free;
> }
>
> /* configure channels */
> @@ -296,6 +295,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>
> out:
> kfree(cmd_channels);
> +err_cmd_free:
Same question here.
> kfree(cmd);
> return ret;
> }
> --
> 1.9.1
>
--
Julian Calaby
Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
[PATCH v2 1/2] ath10k: clean up HTT tx buffer allocation and free
From: Mohammed Shafi Shajakhan
cleanup 'ath10k_htt_tx_alloc' by introducing the API's
'ath10k_htt_tx_alloc/free_{cont_txbuf, txdone_fifo} and
re-use them whereever needed
Signed-off-by: Mohammed Shafi Shajakhan
---
[v2 rebased over top of tree]
drivers/net/wireless/ath/ath10k/htt_tx.c | 76 ++--
1 file changed, 52 insertions(+), 24 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c
b/drivers/net/wireless/ath/ath10k/htt_tx.c
index ae5b33f..786fbd7 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -229,6 +229,33 @@ void ath10k_htt_tx_free_msdu_id(struct ath10k_htt *htt,
u16 msdu_id)
idr_remove(&htt->pending_tx, msdu_id);
}
+static void ath10k_htt_tx_free_cont_txbuf(struct ath10k_htt *htt)
+{
+ size_t size;
+
+ if (!htt->txbuf.vaddr)
+ return;
+
+ size = htt->max_num_pending_tx * sizeof(struct ath10k_htt_txbuf);
+ dma_free_coherent(htt->ar->dev, size, htt->txbuf.vaddr,
+ htt->txbuf.paddr);
+}
+
+static int ath10k_htt_tx_alloc_cont_txbuf(struct ath10k_htt *htt)
+{
+ struct ath10k *ar = htt->ar;
+ size_t size;
+
+ size = htt->max_num_pending_tx * sizeof(struct ath10k_htt_txbuf);
+ htt->txbuf.vaddr = dma_alloc_coherent(ar->dev, size,
+ &htt->txbuf.paddr,
+ GFP_KERNEL);
+ if (!htt->txbuf.vaddr)
+ return -ENOMEM;
+
+ return 0;
+}
+
static void ath10k_htt_tx_free_cont_frag_desc(struct ath10k_htt *htt)
{
size_t size;
@@ -310,10 +337,26 @@ static int ath10k_htt_tx_alloc_txq(struct ath10k_htt *htt)
return 0;
}
+static void ath10k_htt_tx_free_txdone_fifo(struct ath10k_htt *htt)
+{
+ WARN_ON(!kfifo_is_empty(&htt->txdone_fifo));
+ kfifo_free(&htt->txdone_fifo);
+}
+
+static int ath10k_htt_tx_alloc_txdone_fifo(struct ath10k_htt *htt)
+{
+ int ret;
+ size_t size;
+
+ size = roundup_pow_of_two(htt->max_num_pending_tx);
+ ret = kfifo_alloc(&htt->txdone_fifo, size, GFP_KERNEL);
+ return ret;
+}
+
int ath10k_htt_tx_alloc(struct ath10k_htt *htt)
{
struct ath10k *ar = htt->ar;
- int ret, size;
+ int ret;
ath10k_dbg(ar, ATH10K_DBG_BOOT, "htt tx max num pending tx %d\n",
htt->max_num_pending_tx);
@@ -321,13 +364,9 @@ int ath10k_htt_tx_alloc(struct ath10k_htt *htt)
spin_lock_init(&htt->tx_lock);
idr_init(&htt->pending_tx);
- size = htt->max_num_pending_tx * sizeof(struct ath10k_htt_txbuf);
- htt->txbuf.vaddr = dma_alloc_coherent(ar->dev, size,
- &htt->txbuf.paddr,
- GFP_KERNEL);
- if (!htt->txbuf.vaddr) {
- ath10k_err(ar, "failed to alloc tx buffer\n");
- ret = -ENOMEM;
+ ret = ath10k_htt_tx_alloc_cont_txbuf(htt);
+ if (ret) {
+ ath10k_err(ar, "failed to alloc cont tx buffer: %d\n", ret);
goto free_idr_pending_tx;
}
@@ -343,8 +382,7 @@ int ath10k_htt_tx_alloc(struct ath10k_htt *htt)
goto free_frag_desc;
}
- size = roundup_pow_of_two(htt->max_num_pending_tx);
- ret = kfifo_alloc(&htt->txdone_fifo, size, GFP_KERNEL);
+ ret = ath10k_htt_tx_alloc_txdone_fifo(htt);
if (ret) {
ath10k_err(ar, "failed to alloc txdone fifo: %d\n", ret);
goto free_txq;
@@ -359,10 +397,7 @@ free_frag_desc:
ath10k_htt_tx_free_cont_frag_desc(htt);
free_txbuf:
- size = htt->max_num_pending_tx *
- sizeof(struct ath10k_htt_txbuf);
- dma_free_coherent(htt->ar->dev, size, htt->txbuf.vaddr,
- htt->txbuf.paddr);
+ ath10k_htt_tx_free_cont_txbuf(htt);
free_idr_pending_tx:
idr_destroy(&htt->pending_tx);
@@ -388,22 +423,15 @@ static int ath10k_htt_tx_clean_up_pending(int msdu_id,
void *skb, void *ctx)
void ath10k_htt_tx_free(struct ath10k_htt *htt)
{
- int size;
+ tasklet_kill(&htt->txrx_compl_task);
idr_for_each(&htt->pending_tx, ath10k_htt_tx_clean_up_pending, htt->ar);
idr_destroy(&htt->pending_tx);
- if (htt->txbuf.vaddr) {
- size = htt->max_num_pending_tx *
- sizeof(struct ath10k_htt_txbuf);
- dma_free_coherent(htt->ar->dev, size, htt->txbuf.vaddr,
- htt->txbuf.paddr);
- }
-
+ ath10k_htt_tx_free_cont_txbuf(htt);
ath10k_htt_tx_free_txq(htt);
ath10k_htt_tx_free_cont_frag_desc(htt);
- WARN_ON(!kfifo_is_empty(&htt->txdone_fifo));
- kfifo_free(&htt->txdone_fifo);
+ ath10k_htt_tx_free_txdone_fifo(htt);
}
void ath10k_htt_htc_tx_complete(struct ath10k *ar, struct sk_buff *skb)
--
1.9.1
Re: [PATCH 2/2] Revert "staging: wilc1000: Replace kthread with workqueue for host interface"
On Fri, 30 Sep 2016 15:22:15 +0200
Greg KH wrote:
> On Fri, Sep 30, 2016 at 03:43:18PM +0530, Aditya Shankar wrote:
> > This reverts commit 2518ac59eb27 ("staging: wilc1000: Replace kthread
> > with workqueue for host interface")
> >
> > This commit breaks wilc1000 driver init. A crash was seen
> > everytime the wlan interface was brought up and wilc device
> > open was attempted. This change is being reverted until we
> > figure out the problem in this change. The driver is
> > usable now with this change reverted.
> >
> > Signed-off-by: Aditya Shankar
> >
> > Conflicts:
> > drivers/staging/wilc1000/host_interface.c
>
> What is this line doing here?
>
> And shouldn't we add a cc: stable tag as well? Or at the least, put a
> "fixes:" tag to let people know exactly what commit it is fixing (the
> id that it is reverting.)
>
> thanks,
>
> greg k-h
Apologies for this bad commit message.
I have an update on this wilc1000 crash issue. I've figured out
the cause for the crash and fixed it. Therefore,
I request you to ignore the patch I sent out to
revert the change causing the regression. The cause was a misplaced
call to destroy workqueue soon after creating it.
With this removed, the issue is not seen.
I will send out a separate patch to fix the issue.
Thanks,
Aditya
Re: [PATCH 2/2] Revert "staging: wilc1000: Replace kthread with workqueue for host interface"
On Thu, Oct 06, 2016 at 03:26:59PM +0530, Aditya Shankar wrote:
> On Fri, 30 Sep 2016 15:22:15 +0200
> Greg KH wrote:
>
> > On Fri, Sep 30, 2016 at 03:43:18PM +0530, Aditya Shankar wrote:
> > > This reverts commit 2518ac59eb27 ("staging: wilc1000: Replace kthread
> > > with workqueue for host interface")
> > >
> > > This commit breaks wilc1000 driver init. A crash was seen
> > > everytime the wlan interface was brought up and wilc device
> > > open was attempted. This change is being reverted until we
> > > figure out the problem in this change. The driver is
> > > usable now with this change reverted.
> > >
> > > Signed-off-by: Aditya Shankar
> > >
> > > Conflicts:
> > > drivers/staging/wilc1000/host_interface.c
> >
> > What is this line doing here?
> >
> > And shouldn't we add a cc: stable tag as well? Or at the least, put a
> > "fixes:" tag to let people know exactly what commit it is fixing (the
> > id that it is reverting.)
> >
> > thanks,
> >
> > greg k-h
>
> Apologies for this bad commit message.
>
> I have an update on this wilc1000 crash issue. I've figured out
> the cause for the crash and fixed it. Therefore,
> I request you to ignore the patch I sent out to
> revert the change causing the regression. The cause was a misplaced
> call to destroy workqueue soon after creating it.
> With this removed, the issue is not seen.
>
> I will send out a separate patch to fix the issue.
Wonderful, thanks for doing that, I'll drop these and use your fix when
you send it.
greg k-h
Re: [PATCHv4] mac80211: check A-MSDU inner frame source address on AP interfaces
Am 05.10.2016 16:59, schrieb Johannes Berg: So as you can see by my own version of this patch, I'm not super happy with the way you did things here :) I'm happy with your version, so lets just drop mine. Instead of adding 4 new arguments, (ta, ra, is_4addr, is_tdls_data), I opted to just add two (check_da and check_sa) and make those NULL when no checks are desired. I *think* that works equivalently, but it'd be great if you could take a look. The rules when to check sa/da should be independent of the driver and thus would likely be duplicated by each caller. This is why I had it in ieee80211_amsdu_to_8023s. My patch defaulted into not checking sa/da for interface type not explicitly given, whereas your code defaults to checking both for those. This makes a difference for IFTYPE_MONITOR, IFTYPE_P2P_*, IFTYPE_OCB, IFTYPE_UNSPECIFIED, IFTYPE_WDS. I don't know if filtering is appropiate for those or if they can actually occur there. This also conflicts with the earlier patch I sent to just always drop when it's multicast. Dropping multicast A-MSDU frames is fine for me. michael
Re: [PATCHv4] mac80211: check A-MSDU inner frame source address on AP interfaces
> The rules when to check sa/da should be independent of the driver > and thus would likely be duplicated by each caller. This is why I > had it in ieee80211_amsdu_to_8023s. That does make sense, I guess. But I feel that it's overly complicated, and most drivers don't actually support all those features, in particular, 4-addr station/AP is currently exclusive to mac80211. > My patch defaulted into not checking sa/da for interface type not > explicitly given, whereas your code defaults to checking both for > those. > This makes a difference for IFTYPE_MONITOR, IFTYPE_P2P_*, Monitor will never get there, and P2P_CLIENT == STATION, P2P_GO == AP as far as mac80211's vif.type is concerned, so those are handled. > IFTYPE_OCB, That can't do A-MSDU I believe. > IFTYPE_UNSPECIFIED, That is invalid and can never happen here. > IFTYPE_WDS. This is ... it can't even negotiate HT, so I doubt A-MSDU could be used in any way. This is old crap, and I'd actually rather remove it, the 4- addr station/AP superseded it. > I don't know if filtering is appropiate > for those or if they can actually occur there. I think it's all covered then. Could you test my patches in your scenario to see they do what we want? I'll resend as [PATCH] then, and think about applying them and perhaps backporting also. johannes
Re: [PATCH 3/3] mac80211: multicast to unicast conversion
Am 05.10.2016 13:58, schrieb Johannes Berg: Anyway, perhaps this needs to change to take DMS/per-station into account? Then again, this kind of setting - global multicast-to-unicast - fundamentally *cannot* be done on a per-station basis, since if you enable it for one station and not for another, the first station that has it enabled would get the packets twice... as I see it, that is exactly how DMS is standarized. IEEE 802.11-2012 section 10.23.15 DMS procedures: "If the requested DMS is accepted by the AP, the AP shall send subsequent group addressed MSDUs that match the frame classifier specified in the DMS Descriptors to the requesting STA as A-MSDU subframes within an individually addressed A-MSDU frame (see 8.3.2.2 and 9.11)." -> so the multicast packets shall go out as unicast A-MSDU frames to stations that requested this "The AP shall continue to transmit the matching frames as group addressed frames (see 9.3.6, and 10.2.1.16) if at least one associated STA has not requested DMS for these frames." -> so it will continue to send it as multicast frames as well. As with DMS the station requested DMS for a specific multicast address, it could then drop multicast frames addressed to the multicast address it registered for DMS. Regards, M. Braun
Re: [PATCH 3/3] mac80211: multicast to unicast conversion
On Thu, 2016-10-06 at 13:53 +0200, michael-dev wrote: > Am 05.10.2016 13:58, schrieb Johannes Berg: > > > > > > Anyway, perhaps this needs to change to take DMS/per-station into > > account? > > > > Then again, this kind of setting - global multicast-to-unicast - > > fundamentally *cannot* be done on a per-station basis, since if you > > enable it for one station and not for another, the first station > > that has it enabled would get the packets twice... > > as I see it, that is exactly how DMS is standarized. > > IEEE 802.11-2012 section 10.23.15 DMS procedures: > > "If the requested DMS is accepted by the AP, the AP shall send > subsequent group addressed MSDUs that > match the frame classifier specified in the DMS Descriptors to the > requesting STA as A-MSDU subframes > within an individually addressed A-MSDU frame (see 8.3.2.2 and > 9.11)." > > -> so the multicast packets shall go out as unicast A-MSDU frames > to stations that requested this Correct. > "The AP shall continue to transmit the matching frames as group > addressed frames (see 9.3.6, and 10.2.1.16) if at least one > associated > STA has not requested DMS for these frames." > > -> so it will continue to send it as multicast frames as well. > > As with DMS the station requested DMS for a specific multicast > address, it could then drop multicast frames addressed to the > multicast address it registered for DMS. Yes, the DMS spec tells it to do this. However, we can't implement non- DMS similarly, because then the station won't request it and won't drop the duplicates. So for this non-standard multicast-to-unicast, it's all or nothing, it can't be done for some stations only. johannes
RE: [PATCH v2 1/2] mwifiex: reset card->adapter during device unregister
Hi Brian, > From: linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless- > ow...@vger.kernel.org] On Behalf Of Brian Norris > Sent: Wednesday, October 05, 2016 10:00 PM > To: Amitkumar Karwar > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > raja...@google.com; Xinming Hu > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during device > unregister > > Hi, > > On Wed, Oct 05, 2016 at 02:04:53PM +, Amitkumar Karwar wrote: > > > From: Brian Norris [mailto:briannor...@chromium.org] > > > Sent: Wednesday, October 05, 2016 3:28 AM > > > To: Amitkumar Karwar > > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > > > raja...@google.com; briannor...@google.com; Xinming Hu > > > Subject: Re: [PATCH v2 1/2] mwifiex: reset card->adapter during > > > device unregister > > > > > > Hi, > > > > > > On Tue, Oct 04, 2016 at 10:38:24PM +0530, Amitkumar Karwar wrote: > > > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > > > > @@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct > > > mwifiex_adapter *adapter) > > > > pci_disable_msi(pdev); > > > >} > > > > } > > > > + card->adapter = NULL; > > > > > > I think you have a similar problem here as in patch 2; there is no > > > locking to protect fields in struct pcie_service_card or struct > > > sdio_mmc_card below. That problem kind of already exists, except > > > that you only write the value of card->adapter once at registration > > > time, so it's not actually unsafe. But now that you're introducing a > > > second write, you have a problem. > > > > > > Brian > > > > > > > We have a global "add_remove_card_sem" semaphore in our code for > > synchronizing initialization and teardown threads. Ideally "init + > > teardown/reboot" should not have a race issue with this logic > > > > Later there was a loophole introduced in this after using async > > firmware download API. During initialization, firmware downloads > > asynchronously in a separate thread where might have released the > > semaphore. I am working on a patch to fix this. > > > > So "card->adapter" doesn't need to have locking. Even if we have two > > write operations, those two threads can't run simultaneously due to > > above mentioned logic. > > What about writes racing with reads? You have lots of unsynchronized > cases that read this, although most of them should be halted by now > (e.g., cmd processing). I was looking at suspend() in particular, which > I thought you were looking at in this patch series. Please note that "card->adapter" is used only in pcie.c/sdio.c/usb.c files Writes won't have race with reads. 1) write 1 --- "card->adapter = adapter;" in mwifiex_register_dev() This place is at the beginning of initialization. mwifiex_pcie_probe() -> mwifiex_add_card() -> adapter->if_ops.register_dev() There is no chance that "card->adapter" is read anywhere at this point. FW is not yet downloaded 2) write 2 "card->adapter = NULL;" in mwifiex_unregister_dev() This place the end of teardown phase. Interrupts are disabled and all cleanup is done. We have "card->adapter" NULL checks at entry point of suspend/remove/resume, if they get called after this. Regards, Amitkumar
RE: [PATCH v2 2/2] mwifiex: check hw_status in suspend and resume handlers
Hi Brain,
> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: Wednesday, October 05, 2016 10:12 PM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> raja...@google.com; Xinming Hu
> Subject: Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and
> resume handlers
>
> Hi Amit,
>
> On Wed, Oct 05, 2016 at 12:26:36PM +, Amitkumar Karwar wrote:
> > > From: Brian Norris [mailto:briannor...@chromium.org]
> > > Sent: Wednesday, October 05, 2016 2:35 AM
> > > To: Amitkumar Karwar
> > > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > raja...@google.com; briannor...@google.com; Xinming Hu
> > > Subject: Re: [PATCH v2 2/2] mwifiex: check hw_status in suspend and
> > > resume handlers
> > >
> > > On Tue, Oct 04, 2016 at 10:38:25PM +0530, Amitkumar Karwar wrote:
> > > > From: Xinming Hu
> > > >
> > > > We have observed a kernel crash when system immediately suspends
> > > > after booting. There is a race between suspend and driver
> > > > initialization paths.
> > > > This patch adds hw_status checks to fix the problem
> > > >
> > > > Signed-off-by: Xinming Hu
> > > > Signed-off-by: Amitkumar Karwar
> > > > ---
> > > > v2: Return failure in suspend/resume handler in this scenario.
> > > > ---
> > > > drivers/net/wireless/marvell/mwifiex/pcie.c | 10 ++
> > > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > index ba9e068..fa6bf85 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > > > @@ -122,9 +122,10 @@ static int mwifiex_pcie_suspend(struct device
> > > > *dev)
> > > >
> > > > if (pdev) {
> > > > card = pci_get_drvdata(pdev);
> > > > - if (!card || !card->adapter) {
> > > > + if (!card || !card->adapter ||
> > > > + card->adapter->hw_status !=
> MWIFIEX_HW_STATUS_READY) {
> > >
> > > Wait, is there no locking on the 'hw_status' field? That is
> > > inherently an unsafe race all on its own; you're not guaranteed that
> > > this will be read/written atomically. And you also aren't guaranteed
> > > that writes to this happen in the order they appear in the code --
> > > in other words, reading this flag doesn't necessarily guarantee that
> > > initialization is actually complete (even if that's very likely to
> > > be true, given that it's probably just a single-instruction
> > > word-access, and any prior HW polling or interrupts likely have done
> > > some synchronization and can't be reordered).
> > > actually complete
> >
> > Here is the brief info on how "hw_status" flag is updated.
> > 1) It gets changed incrementally during initialization.
> > MWIFIEX_HW_STATUS_INITIALIZING -> MWIFIEX_HW_STATUS_INIT_DONE ->
> > MWIFIEX_HW_STATUS_READY
> >
> > 2) Status will remain READY once driver+firmware is up and running.
> >
> > 3) Below is status during teardown
> > MWIFIEX_HW_STATUS_READY -> MWIFIEX_HW_STATUS_RESET ->
> > MWIFIEX_HW_STATUS_CLOSING -> MWIFIEX_HW_STATUS_NOT_READY
> >
> > As the events occur one after another, we don't expect a race and
> > don't need locking here for the flag. Flag status
> > MWIFIEX_HW_STATUS_READY guarantees that initialization is completed.
>
> It seems like, as with patch 1, you're mostly arguing about the writes
> to this variable. But writes race with reads as well; how do you
> guarantee that you're not seeing incorrect values of 'hw_status' here in
> suspend() -- e.g., either old or new values of it, or even partially-
> written values, if for some reason the compiler decides it can't
> read/write this all in one go?
Got it. I think, we need to define "hw_status" as atomic variable to resolve
the issue. I will create separate patch for this.
>
> > In worst case scenario, only first system suspend attempt issued
> > immediately after system boot will be aborted with BUSY error. I
> > think, that should be fine.
>
> (For the record, my concern about -EBUSY is separate from my concern
> about the potential race condition.)
>
> > Let me know if you have any concerns.
>
> Sorry, I probably didn't completely flesh out my thought here.
>
> I think I was concerned about a failed initialization causing the system
> to never enter suspend again. So specifically: what happens if (e.g.)
> the firmware fails to load? AFAICT, the device doesn't actually unbind
> itself from the driver, so instead, you have a device in limbo that will
> always return -EBUSY in suspend(), and your system can never again enter
> suspend. Am I correct? If so, that doesn't sound great.
You are right. I will add an extra check for this so that both the cases would
be handled
Case 1:
Firmware has gone bad or has failed to initialize. We will return zero
here, so that it won't block subsequent suspend attempts.
Case 2:
[PATCH v3 1/3] mwifiex: reset card->adapter during device unregister
From: Xinming Hu
card->adapter gets initialized during device registration.
As it's not cleared, we may end up accessing invalid memory
in some corner cases. This patch fixes the problem.
Signed-off-by: Xinming Hu
Signed-off-by: Amitkumar Karwar
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index f1eeb73..ba9e068 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter
*adapter)
pci_disable_msi(pdev);
}
}
+ card->adapter = NULL;
}
/* This function initializes the PCI-E host memory space, WCB rings, etc.
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8718950..4cad1c2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
struct sdio_mmc_card *card = adapter->card;
if (adapter->card) {
+ card->adapter = NULL;
sdio_claim_host(card->func);
sdio_disable_func(card->func);
sdio_release_host(card->func);
--
1.9.1
[PATCH v3 2/3] mwifiex: remove redundant pdev check in suspend/resume handlers
to_pci_dev() would just do struct offset arithmetic on struct
device to get 'pdev' pointer. We never get NULL pdev pointer
Signed-off-by: Amitkumar Karwar
---
New patch prepared as per inputs from Brian Norris.
It wasn't part of v1 and v2 series
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index ba9e068..1e27dbf 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -120,14 +120,9 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ pr_err("Card or adapter structure is not valid\n");
return 0;
}
@@ -164,14 +159,9 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ pr_err("Card or adapter structure is not valid\n");
return 0;
}
--
1.9.1
[PATCH v3 3/3] mwifiex: check hw_status in suspend and resume handlers
We have observed a kernel crash when system immediately suspends
after booting. There is a race between suspend and driver initialization
paths. This patch adds hw_status checks in suspend/resume to fix this issue
and other corner cases.
Signed-off-by: Amitkumar Karwar
---
v2: Return failure in suspend/resume handler in this scenario.
v3: Handle "hw_status" not READY cases carefully. Return 0 if
init or shutdown is in progress. Return -EBUSY if firmware download
failed or command timeout ocurred(non-recoverable). (Brian Norris)
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 27 +++
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 1e27dbf..cc18e4d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -121,13 +121,22 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
+ if (!card || !card->adapter ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
+ pr_err("Card or adapter structure is not valid or hw_status not
ready\n");
return 0;
}
adapter = card->adapter;
+ if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) {
+ pr_err("We are in the middle of initialzaion or closing\n");
+ return -EBUSY;
+ }
+
/* Enable the Host Sleep */
if (!mwifiex_enable_hs(adapter)) {
mwifiex_dbg(adapter, ERROR,
@@ -160,13 +169,23 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
+
+ if (!card || !card->adapter ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
+ pr_err("Card or adapter structure is not valid or hw_status not
ready\n");
return 0;
}
adapter = card->adapter;
+ if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) {
+ pr_err("We are in the middle of initialzaion or closing\n");
+ return -EBUSY;
+ }
+
if (!adapter->is_suspended) {
mwifiex_dbg(adapter, WARN,
"Device already resumed\n");
--
1.9.1
[PATCH v4 1/3] mwifiex: reset card->adapter during device unregister
From: Xinming Hu
card->adapter gets initialized during device registration.
As it's not cleared, we may end up accessing invalid memory
in some corner cases. This patch fixes the problem.
Signed-off-by: Xinming Hu
Signed-off-by: Amitkumar Karwar
---
v4: Same as v1, v2, v3
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index f1eeb73..ba9e068 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3042,6 +3042,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter
*adapter)
pci_disable_msi(pdev);
}
}
+ card->adapter = NULL;
}
/* This function initializes the PCI-E host memory space, WCB rings, etc.
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 8718950..4cad1c2 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2066,6 +2066,7 @@ mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
struct sdio_mmc_card *card = adapter->card;
if (adapter->card) {
+ card->adapter = NULL;
sdio_claim_host(card->func);
sdio_disable_func(card->func);
sdio_release_host(card->func);
--
1.9.1
[PATCH v4 2/3] mwifiex: remove redundant pdev check in suspend/resume handlers
to_pci_dev() would just do struct offset arithmetic on struct
device to get 'pdev' pointer. We never get NULL pdev pointer
Signed-off-by: Amitkumar Karwar
---
New patch introduced in v3 as per inputs from Brian Norris.
v4: Same as v3
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index ba9e068..1e27dbf 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -120,14 +120,9 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ pr_err("Card or adapter structure is not valid\n");
return 0;
}
@@ -164,14 +159,9 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ pr_err("Card or adapter structure is not valid\n");
return 0;
}
--
1.9.1
[PATCH v4 3/3] mwifiex: check hw_status in suspend and resume handlers
We have observed a kernel crash when system immediately suspends
after booting. There is a race between suspend and driver initialization
paths. This patch adds hw_status checks in suspend/resume to fix this issue
and other corner cases.
Signed-off-by: Amitkumar Karwar
---
v2: Return failure in suspend/resume handler in this scenario.
v3: Handle "hw_status" not READY cases carefully. Return 0 if
init or shutdown is in progress. Return -EBUSY if firmware download
failed or command timeout ocurred(non-recoverable). (Brian Norris)
v4: In resume, we need not return failure.
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 1e27dbf..e7c6980 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -121,13 +121,22 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
+ if (!card || !card->adapter ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_RESET ||
+ card->adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY) {
+ pr_err("Card or adapter structure is not valid or hw_status
shows failure\n");
return 0;
}
adapter = card->adapter;
+ if (adapter->hw_status == MWIFIEX_HW_STATUS_INITIALIZING ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_INIT_DONE ||
+ adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) {
+ pr_err("We are in the middle of initialzaion or closing\n");
+ return -EBUSY;
+ }
+
/* Enable the Host Sleep */
if (!mwifiex_enable_hs(adapter)) {
mwifiex_dbg(adapter, ERROR,
@@ -160,8 +169,10 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
+
+ if (!card || !card->adapter ||
+ card->adapter->hw_status != MWIFIEX_HW_STATUS_READY) {
+ pr_err("Card or adapter structure is not valid or hw_status is
not ready\n");
return 0;
}
--
1.9.1
Re: [PATCH] [wl18xx] Fix memory leakage if kzalloc fails
Hi Julian,
On Thu, Oct 6, 2016 at 3:05 PM, Julian Calaby wrote:
> Hi,
>
> On Wed, Oct 5, 2016 at 10:50 PM, Souptick Joarder
> wrote:
>> This patch is added to properly handle memory leak if kzalloc fails
>> in wl18xx_scan_send() and wl18xx_scan_sched_scan_config()
>
> What memory leak?
My Apologies here. I was addressing here about freeing the invalid memories.
But I put wrong description. I will resend this patch with proper
descriptions and addressing your comments.
>
>> Signed-off-by: Souptick Joarder
>> Signed-off-by: Rameshwar Sahu
>
> Why two signed-off-bys?
We both were involved in addressing this.
>
>> ---
>> drivers/net/wireless/ti/wl18xx/scan.c | 20 ++--
>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ti/wl18xx/scan.c
>> b/drivers/net/wireless/ti/wl18xx/scan.c
>> index 4e522154..aed22e1 100644
>> --- a/drivers/net/wireless/ti/wl18xx/scan.c
>> +++ b/drivers/net/wireless/ti/wl18xx/scan.c
>> @@ -41,14 +41,13 @@ static void wl18xx_adjust_channels(struct
>> wl18xx_cmd_scan_params *cmd,
>> static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
>> struct cfg80211_scan_request *req)
>> {
>> - struct wl18xx_cmd_scan_params *cmd;
>> + struct wl18xx_cmd_scan_params *cmd = NULL;
>> struct wlcore_scan_channels *cmd_channels = NULL;
>> int ret;
>>
>> cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> if (!cmd) {
>> - ret = -ENOMEM;
>> - goto out;
>> + return -ENOMEM;
>> }
>>
>> /* scan on the dev role if the regular one is not started */
>> @@ -59,7 +58,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
>> wl12xx_vif *wlvif,
>>
>> if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
>> ret = -EINVAL;
>> - goto out;
>> + goto err_cmd_free;
>> }
>>
>> cmd->scan_type = SCAN_TYPE_SEARCH;
>> @@ -84,7 +83,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
>> wl12xx_vif *wlvif,
>> cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
>> if (!cmd_channels) {
>> ret = -ENOMEM;
>> - goto out;
>> + goto err_cmd_free;
>> }
>>
>> wlcore_set_scan_chan_params(wl, cmd_channels, req->channels,
>> @@ -153,6 +152,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
>> wl12xx_vif *wlvif,
>>
>> out:
>> kfree(cmd_channels);
>> +err_cmd_free:
>
> kfree(NULL) is valid,
I agree with you. As *cmd not initialized with NULL, so it can hold a
garbage address.
In case of memory allocation failure, kernel may enter in abnormal
behavior while freeing this memory.
so therefore the out: and err_cmd_free: labels
> are equivalent from a memory freeing perspective, so where exactly are
> we leaking memory in this function?
I want to avoid kfree(cmd_channels) calls when we have not allocated the memory.
>
>> kfree(cmd);
>> return ret;
>> }
>> @@ -171,7 +171,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>> struct cfg80211_sched_scan_request *req,
>> struct ieee80211_scan_ies *ies)
>> {
>> - struct wl18xx_cmd_scan_params *cmd;
>> + struct wl18xx_cmd_scan_params *cmd = NULL;
>> struct wlcore_scan_channels *cmd_channels = NULL;
>> struct conf_sched_scan_settings *c = &wl->conf.sched_scan;
>> int ret;
>> @@ -185,15 +185,14 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>>
>> cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> if (!cmd) {
>> - ret = -ENOMEM;
>> - goto out;
>> + return -ENOMEM;
>> }
>>
>> cmd->role_id = wlvif->role_id;
>>
>> if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
>> ret = -EINVAL;
>> - goto out;
>> + goto err_cmd_free;
>> }
>>
>> cmd->scan_type = SCAN_TYPE_PERIODIC;
>> @@ -218,7 +217,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>> cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
>> if (!cmd_channels) {
>> ret = -ENOMEM;
>> - goto out;
>> + goto err_cmd_free;
>> }
>>
>> /* configure channels */
>> @@ -296,6 +295,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
>>
>> out:
>> kfree(cmd_channels);
>> +err_cmd_free:
>
> Same question here.
Please refer above comment for the same.
>
>> kfree(cmd);
>> return ret;
>> }
>> --
>> 1.9.1
>>
>
>
>
> --
> Julian Calaby
>
> Email: julian.cal...@gmail.com
> Profile: http://www.google.com/profiles/julian.calaby/
Re: [PATCH] ath10k: cache calibration data when the core is stopped.
OK the v2 patch is not based on 9340953. If you queue this in 4.10 as planned then just let me know and I'll rev v3 based on top of 9340953. On Thu, Oct 6, 2016 at 3:40 AM, Valo, Kalle wrote: > Marty Faltesek writes: > >> On Mon, Oct 3, 2016 at 3:46 AM, Valo, Kalle wrote: >>> Marty Faltesek writes: >>> Caching calibration data allows it to be accessed when the device is not active. Signed-off-by: Marty Faltesek >>> >>> No comma in the title, please. >>> >>> What tree did you use as the baseline? This doesn't seem to apply to >>> ath.git: >> >> We use backports 20160122 which has not been updated since earlier this year. >> I can forward port it to your tree, and make sure >> it builds but won't be able to test it. Will that be OK? > > Sure, I can test it. > >>> Also please note that this patch (which I'm queuing to 4.9) touches the >>> same area: >>> >>> ath10k: fix debug cal data file >>> >>> https://patchwork.kernel.org/patch/9340953/ >> >> I've modified this too, and this won't be necessary, so can you drop >> it? If not, let me know and I'll pull it in and make sure I'm based >> off it too. > > Actually I was first planning to push 9340953 to 4.9 and take your patch > to 4.10. But your patch is a cleaner approach to this and maybe I should > push that to 4.9 instead? Need to think a bit more. > > -- > Kalle Valo
Non-fatal errors when disabling WiFi - Intel 7260
Hi,
since commit b7a08b284dcf ("iwlwifi: pcie: extend device reset delay")
I get the following errors when I disable the WiFi:
[ 66.702845] iwlwifi :02:00.0: RF_KILL bit toggled to disable radio.
[ 66.737893] iwlwifi :02:00.0: Failed to wake NIC for hcmd
[ 66.737927] iwlwifi :02:00.0: Error sending TXPATH_FLUSH: enqueue_hcmd
failed: -5
[ 66.737933] iwlwifi :02:00.0: Failed to send flush command (-5)
[ 66.772678] iwlwifi :02:00.0: Failed to wake NIC for hcmd
[ 66.772698] iwlwifi :02:00.0: Error sending REMOVE_STA: enqueue_hcmd
failed: -5
[ 66.772699] iwlwifi :02:00.0: Failed to remove station. Id=2
[ 66.772701] iwlwifi :02:00.0: Failed sending remove station
[ 66.807518] iwlwifi :02:00.0: Failed to wake NIC for hcmd
[ 66.807531] iwlwifi :02:00.0: Error sending BINDING_CONTEXT_CMD:
enqueue_hcmd failed: -5
[ 66.807533] iwlwifi :02:00.0: Failed to send binding (action:3): -5
[ 66.842413] iwlwifi :02:00.0: Failed to wake NIC for hcmd
[ 66.842430] iwlwifi :02:00.0: Error sending MAC_PM_POWER_TABLE:
enqueue_hcmd failed: -5
[ 66.877120] iwlwifi :02:00.0: Failed to wake NIC for hcmd
[ 66.877150] iwlwifi :02:00.0: Error sending MAC_CONTEXT_CMD:
enqueue_hcmd failed: -5
[ 66.877153] iwlwifi :02:00.0: Failed to remove MAC context: -5
[ 66.911995] iwlwifi :02:00.0: Failed to wake NIC for hcmd
[ 66.912015] iwlwifi :02:00.0: Error sending SCD_QUEUE_CFG: enqueue_hcmd
failed: -5
[ 66.912019] iwlwifi :02:00.0: Failed to disable queue 8 (ret=-5)
[ 66.947134] iwlwifi :02:00.0: Failed to wake NIC for hcmd
[ 66.947163] iwlwifi :02:00.0: Error sending TXPATH_FLUSH: enqueue_hcmd
failed: -5
[ 66.947165] iwlwifi :02:00.0: Failed to send flush command (-5)
Other than these, there seems to be no noticeable issue, I can
re-enable the WiFi and use it normally.
Reverting the commit on top of 4.8 get rids of the errors.
My laptop has an Intel Corporation Wireless 7260 (rev 6b),
the firmware currently used is 17.352738.0.
Regards,
Gabriele
Re: [PATCH] [wl18xx] Fix memory leakage if kzalloc fails
Souptick Joarder writes: > Hi Julian, > > > On Thu, Oct 6, 2016 at 3:05 PM, Julian Calaby wrote: >> Hi, >> >> On Wed, Oct 5, 2016 at 10:50 PM, Souptick Joarder >> wrote: >>> This patch is added to properly handle memory leak if kzalloc fails >>> in wl18xx_scan_send() and wl18xx_scan_sched_scan_config() >> >> What memory leak? > > My Apologies here. I was addressing here about freeing the invalid memories. > But I put wrong description. I will resend this patch with proper > descriptions and addressing your comments. Also the prefix in the title should be "wl18xx: ", not "[wl18xx]". See: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#subject_name -- Kalle Valo
[PATCH] wl18xx: Handle kfree() in better way when kzalloc fails
This patch is added to handle kfree and return error in a better way
if kzalloc fails in wl18xx_scan_send() and wl18xx_scan_sched_scan_config().
Signed-off-by: Souptick joarder
---
drivers/net/wireless/ti/wl18xx/scan.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/ti/wl18xx/scan.c
b/drivers/net/wireless/ti/wl18xx/scan.c
index 4e522154..aed22e1 100644
--- a/drivers/net/wireless/ti/wl18xx/scan.c
+++ b/drivers/net/wireless/ti/wl18xx/scan.c
@@ -41,14 +41,13 @@ static void wl18xx_adjust_channels(struct
wl18xx_cmd_scan_params *cmd,
static int wl18xx_scan_send(struct wl1271 *wl, struct wl12xx_vif *wlvif,
struct cfg80211_scan_request *req)
{
- struct wl18xx_cmd_scan_params *cmd;
+ struct wl18xx_cmd_scan_params *cmd = NULL;
struct wlcore_scan_channels *cmd_channels = NULL;
int ret;
cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
if (!cmd) {
- ret = -ENOMEM;
- goto out;
+ return -ENOMEM;
}
/* scan on the dev role if the regular one is not started */
@@ -59,7 +58,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
wl12xx_vif *wlvif,
if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
ret = -EINVAL;
- goto out;
+ goto err_cmd_free;
}
cmd->scan_type = SCAN_TYPE_SEARCH;
@@ -84,7 +83,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
wl12xx_vif *wlvif,
cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
if (!cmd_channels) {
ret = -ENOMEM;
- goto out;
+ goto err_cmd_free;
}
wlcore_set_scan_chan_params(wl, cmd_channels, req->channels,
@@ -153,6 +152,7 @@ static int wl18xx_scan_send(struct wl1271 *wl, struct
wl12xx_vif *wlvif,
out:
kfree(cmd_channels);
+err_cmd_free:
kfree(cmd);
return ret;
}
@@ -171,7 +171,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
struct cfg80211_sched_scan_request *req,
struct ieee80211_scan_ies *ies)
{
- struct wl18xx_cmd_scan_params *cmd;
+ struct wl18xx_cmd_scan_params *cmd = NULL;
struct wlcore_scan_channels *cmd_channels = NULL;
struct conf_sched_scan_settings *c = &wl->conf.sched_scan;
int ret;
@@ -185,15 +185,14 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
if (!cmd) {
- ret = -ENOMEM;
- goto out;
+ return -ENOMEM;
}
cmd->role_id = wlvif->role_id;
if (WARN_ON(cmd->role_id == WL12XX_INVALID_ROLE_ID)) {
ret = -EINVAL;
- goto out;
+ goto err_cmd_free;
}
cmd->scan_type = SCAN_TYPE_PERIODIC;
@@ -218,7 +217,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
cmd_channels = kzalloc(sizeof(*cmd_channels), GFP_KERNEL);
if (!cmd_channels) {
ret = -ENOMEM;
- goto out;
+ goto err_cmd_free;
}
/* configure channels */
@@ -296,6 +295,7 @@ int wl18xx_scan_sched_scan_config(struct wl1271 *wl,
out:
kfree(cmd_channels);
+err_cmd_free:
kfree(cmd);
return ret;
}
--
1.9.1
