| Message ID | 1478869818-4340-1-git-send-email-akarwar@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi Amit, Kalle, On Fri, Nov 11, 2016 at 06:40:08PM +0530, Amitkumar Karwar wrote: > From: Shengzhen Li <szli@marvell.com> > > We may get SLEEP event from firmware even if TXDone interrupt > for last Tx packet is still pending. In this case, we may > end up accessing PCIe memory for handling TXDone after power > save handshake is completed. This causes kernel crash with > external abort. > > This patch will only allow downloading sleep confirm > when no tx done interrupt is pending in the hardware. > > Signed-off-by: Cathy Luo <cluo@marvell.com> > Signed-off-by: Shengzhen Li <szli@marvell.com> > Tested-by: Xinming Hu <huxm@marvell.com> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > --- > v2: address format issues(Brain) > RESEND v2(Applicable for complete patch series): > 1) Fixed syntax issue "changelog not placed after the Sign-offs" > pointed by Brian. > 2) Dropped "[v2,03/12] mwifiex: don't do unbalanced free()'ing in > cleanup_if()" patch in this series. It was already sent by Brian > separately. > v3: Same as RESEND v2. > > Hi Kalle, > > There are multiple mwifiex patches under review. I want you consider them > in following sequence(first being oldest) to avoid conflicts Thanks for doing this! It's a little confusing about what's outstanding at the moment (and I think I was just confused on a review a bit ago; I wasn't 100% sure what it was based on), so this listing helps. If it helps, I'll put my comments here, since I've reviewed most of these: > [v3] mwifiex: report wakeup for wowlan Reviewed, SGMT. > mwifiex: add power save parameters in hs_cfg cmd Didn't review. No comment. > [2/2] mwifiex: ignore calibration data failure (Note: 1/2 has dropped) Didn't review. But FWIW, Kalle expressed a preference for full series, not partial. > [v6] mwifiex: parse device tree node for PCIe This one is marked Deferred in patchwork, and I had some comments about it, since it introduced a double-free issue. I'd prefer it get fixed and resent, and I expect Kalle is also waiting for this. > [v2,1/3] mwifiex: Allow mwifiex early access to device structure > [v2,2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties > [v2,3/3] mwifiex: Enable WoWLAN for both sdio and pcie You sent v3 for the above, and those LGTM (I provided my review). I was probably also confused because they were based on the above "[v6] mwifiex: parse device tree node for PCIe", which was not completely correct. > mwifiex: don't do unbalanced free()'ing in cleanup_if() > mwifiex: printk() overflow with 32-byte SSIDs > mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels() I wrote or reviewed the above 3. LGTM. > [v3,01/11] mwifiex: check tx_hw_pending before downloading sleep confirm > [v3,02/11] mwifiex: complete blocked power save handshake in main process > [v3,03/11] mwifiex: resolve races between async FW init (failure) and device removal > [v3,04/11] mwifiex: remove redundant pdev check in suspend/resume handlers > [v3,05/11] mwifiex: don't pretend to resume while remove()'ing > [v3,06/11] mwifiex: resolve suspend() race with async FW init failure > [v3,07/11] mwifiex: reset card->adapter during device unregister > [v3,08/11] mwifiex: usb: handle HS failures > [v3,09/11] mwifiex: sdio: don't check for NULL sdio_func > [v3,10/11] mwifiex: stop checking for NULL drvata/intfdata > [v3,11/11] mwifiex: pcie: stop checking for NULL adapter->card For this entire series, I looked over them again (and I wrote several in the first place), so for all 11: Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > drivers/net/wireless/marvell/mwifiex/cmdevt.c | 5 +++-- > drivers/net/wireless/marvell/mwifiex/init.c | 1 + > drivers/net/wireless/marvell/mwifiex/main.h | 1 + > drivers/net/wireless/marvell/mwifiex/pcie.c | 5 +++++ > 4 files changed, 10 insertions(+), 2 deletions(-) > [...]
Brian Norris <briannorris@chromium.org> writes: > On Fri, Nov 11, 2016 at 06:40:08PM +0530, Amitkumar Karwar wrote: > >> There are multiple mwifiex patches under review. I want you consider them >> in following sequence(first being oldest) to avoid conflicts > > Thanks for doing this! It's a little confusing about what's outstanding > at the moment (and I think I was just confused on a review a bit ago; I > wasn't 100% sure what it was based on), so this listing helps. > > If it helps, I'll put my comments here, since I've reviewed most of > these: > >> [v3] mwifiex: report wakeup for wowlan > > Reviewed, SGMT. > >> mwifiex: add power save parameters in hs_cfg cmd > > Didn't review. No comment. > >> [2/2] mwifiex: ignore calibration data failure (Note: 1/2 has dropped) > > Didn't review. But FWIW, Kalle expressed a preference for full series, > not partial. You are correct, but dropping patches in patchwork is easy so I usually can do that myself. Changing or adding patches to a patchset is the difficult part. So I dropped patch 1 now. >> [v6] mwifiex: parse device tree node for PCIe > > This one is marked Deferred in patchwork, and I had some comments about > it, since it introduced a double-free issue. I'd prefer it get fixed and > resent, and I expect Kalle is also waiting for this. Correct, I dropped v6. >> [v2,1/3] mwifiex: Allow mwifiex early access to device structure >> [v2,2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties >> [v2,3/3] mwifiex: Enable WoWLAN for both sdio and pcie > > You sent v3 for the above, and those LGTM (I provided my review). I was > probably also confused because they were based on the above "[v6] > mwifiex: parse device tree node for PCIe", which was not completely > correct. v4 of this patchset is now "Under Review", which in practise means that the patches are pending for commit. (Too bad that patchwork doesn't have a "Pending" state, so I have to use "Under Review" instead) >> mwifiex: don't do unbalanced free()'ing in cleanup_if() >> mwifiex: printk() overflow with 32-byte SSIDs >> mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels() > > I wrote or reviewed the above 3. LGTM. The first is now "Under Review" and the last two I have already applied. >> [v3,01/11] mwifiex: check tx_hw_pending before downloading sleep confirm >> [v3,02/11] mwifiex: complete blocked power save handshake in main process >> [v3,03/11] mwifiex: resolve races between async FW init (failure) and device removal >> [v3,04/11] mwifiex: remove redundant pdev check in suspend/resume handlers >> [v3,05/11] mwifiex: don't pretend to resume while remove()'ing >> [v3,06/11] mwifiex: resolve suspend() race with async FW init failure >> [v3,07/11] mwifiex: reset card->adapter during device unregister >> [v3,08/11] mwifiex: usb: handle HS failures >> [v3,09/11] mwifiex: sdio: don't check for NULL sdio_func >> [v3,10/11] mwifiex: stop checking for NULL drvata/intfdata >> [v3,11/11] mwifiex: pcie: stop checking for NULL adapter->card > > For this entire series, I looked over them again (and I wrote several in > the first place), so for all 11: > > Reviewed-by: Brian Norris <briannorris@chromium.org> These 11 are now "Under Review". So to summarise, this is what I'm planning to commit (it's sorted by date but I try to follow the order Amit specified when I commit these): [ 1] mwifiex: add power save parameters in hs_cfg cmd 2016-10-14 Amitkumar Ka Under Review [ 2] [2/2] mwifiex: ignore calibration data failure 2016-10-21 Amitkumar Ka Under Review [ 3] mwifiex: don't do unbalanced free()'ing in cleanup_if() 2016-10-26 Brian Norris Under Review [ 4] [v3,01/11] mwifiex: check tx_hw_pending before downloadin... 2016-11-11 Amitkumar Ka Under Review [ 5] [v3,02/11] mwifiex: complete blocked power save handshake... 2016-11-11 Amitkumar Ka Under Review [ 6] [v3,03/11] mwifiex: resolve races between async FW init (... 2016-11-11 Amitkumar Ka Under Review [ 7] [v3,04/11] mwifiex: remove redundant pdev check in suspen... 2016-11-11 Amitkumar Ka Under Review [ 8] [v3,05/11] mwifiex: don't pretend to resume while remove(... 2016-11-11 Amitkumar Ka Under Review [ 9] [v3,06/11] mwifiex: resolve suspend() race with async FW.... 2016-11-11 Amitkumar Ka Under Review [ 10] [v3,07/11] mwifiex: reset card->adapter during device unr... 2016-11-11 Amitkumar Ka Under Review [ 11] [v3,08/11] mwifiex: usb: handle HS failures 2016-11-11 Amitkumar Ka Under Review [ 12] [v3,09/11] mwifiex: sdio: don't check for NULL sdio_func 2016-11-11 Amitkumar Ka Under Review [ 13] [v3,10/11] mwifiex: stop checking for NULL drvata/intfdata 2016-11-11 Amitkumar Ka Under Review [ 14] [v3,11/11] mwifiex: pcie: stop checking for NULL adapter-... 2016-11-11 Amitkumar Ka Under Review [ 15] [v3,1/3] mwifiex: Allow mwifiex early access to device st... 2016-11-14 Amitkumar Ka Under Review [ 16] [v3,2/3] mwifiex: Introduce mwifiex_probe_of() to parse c... 2016-11-14 Amitkumar Ka Under Review [ 17] [v3,3/3] mwifiex: Enable WoWLAN for both sdio and pcie 2016-11-14 Amitkumar Ka Under Review [ 18] [v4,1/3] mwifiex: Allow mwifiex early access to device st... 2016-11-15 Amitkumar Ka Under Review [ 19] [v4,2/3] mwifiex: Introduce mwifiex_probe_of() to parse c... 2016-11-15 Amitkumar Ka Under Review [ 20] [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie 2016-11-15 Amitkumar Ka Under Review Patchwork link for the same: https://patchwork.kernel.org/project/linux-wireless/list/?state=2&q=mwifiex Does that look ok?
Amitkumar Karwar <akarwar@marvell.com> wrote: > From: Shengzhen Li <szli@marvell.com> > > We may get SLEEP event from firmware even if TXDone interrupt > for last Tx packet is still pending. In this case, we may > end up accessing PCIe memory for handling TXDone after power > save handshake is completed. This causes kernel crash with > external abort. > > This patch will only allow downloading sleep confirm > when no tx done interrupt is pending in the hardware. > > Signed-off-by: Cathy Luo <cluo@marvell.com> > Signed-off-by: Shengzhen Li <szli@marvell.com> > Tested-by: Xinming Hu <huxm@marvell.com> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > Reviewed-by: Brian Norris <briannorris@chromium.org> (A note to myself) Depends on this patchset: [ 12] [v4,1/3] mwifiex: Allow mwifiex early access to device st... 2016-11-15 Amitkumar Ka Awaiting Upstream [ 13] [v4,2/3] mwifiex: Introduce mwifiex_probe_of() to parse c... 2016-11-15 Amitkumar Ka Awaiting Upstream [ 14] [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie 2016-11-15 Amitkumar Ka Awaiting Upstream
Hi Kalle, > From: linux-wireless-owner@vger.kernel.org [mailto:linux-wireless- > owner@vger.kernel.org] On Behalf Of Kalle Valo > Sent: Friday, November 18, 2016 5:01 PM > To: Amitkumar Karwar > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam; > rajatja@google.com; briannorris@google.com; dmitry.torokhov@gmail.com; > Shengzhen Li; Amitkumar Karwar > Subject: Re: [v3, 01/11] mwifiex: check tx_hw_pending before > downloading sleep confirm > > Amitkumar Karwar <akarwar@marvell.com> wrote: > > From: Shengzhen Li <szli@marvell.com> > > > > We may get SLEEP event from firmware even if TXDone interrupt for > last > > Tx packet is still pending. In this case, we may end up accessing > PCIe > > memory for handling TXDone after power save handshake is completed. > > This causes kernel crash with external abort. > > > > This patch will only allow downloading sleep confirm when no tx done > > interrupt is pending in the hardware. > > > > Signed-off-by: Cathy Luo <cluo@marvell.com> > > Signed-off-by: Shengzhen Li <szli@marvell.com> > > Tested-by: Xinming Hu <huxm@marvell.com> > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > > Reviewed-by: Brian Norris <briannorris@chromium.org> > > (A note to myself) > > Depends on this patchset: > > [ 12] [v4,1/3] mwifiex: Allow mwifiex early access to device st... > 2016-11-15 Amitkumar Ka Awaiting Upstream [ 13] [v4,2/3] mwifiex: > Introduce mwifiex_probe_of() to parse c... 2016-11-15 Amitkumar Ka > Awaiting Upstream > [ 14] [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie > 2016-11-15 Amitkumar Ka Awaiting Upstream > There is a minor conflict while applying [v3, 3/11] on top of above patch series. I will submit v4 patch series(11 patches) with correction. Regards, Amitkumar
On Thu, Nov 17, 2016 at 02:41:11PM +0200, Kalle Valo wrote: > Patchwork link for the same: > > https://patchwork.kernel.org/project/linux-wireless/list/?state=2&q=mwifiex > > Does that look ok? FWIW, your merges (since you made the above comments) look sane to me. Thanks! Brian
diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c index 5347728..25a7475 100644 --- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c +++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c @@ -1118,13 +1118,14 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter) void mwifiex_check_ps_cond(struct mwifiex_adapter *adapter) { - if (!adapter->cmd_sent && + if (!adapter->cmd_sent && !atomic_read(&adapter->tx_hw_pending) && !adapter->curr_cmd && !IS_CARD_RX_RCVD(adapter)) mwifiex_dnld_sleep_confirm_cmd(adapter); else mwifiex_dbg(adapter, CMD, - "cmd: Delay Sleep Confirm (%s%s%s)\n", + "cmd: Delay Sleep Confirm (%s%s%s%s)\n", (adapter->cmd_sent) ? "D" : "", + atomic_read(&adapter->tx_hw_pending) ? "T" : "", (adapter->curr_cmd) ? "C" : "", (IS_CARD_RX_RCVD(adapter)) ? "R" : ""); } diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c index 82839d9..b36cb3f 100644 --- a/drivers/net/wireless/marvell/mwifiex/init.c +++ b/drivers/net/wireless/marvell/mwifiex/init.c @@ -270,6 +270,7 @@ static void mwifiex_init_adapter(struct mwifiex_adapter *adapter) adapter->adhoc_11n_enabled = false; mwifiex_wmm_init(adapter); + atomic_set(&adapter->tx_hw_pending, 0); sleep_cfm_buf = (struct mwifiex_opt_sleep_confirm *) adapter->sleep_cfm->data; diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 11abc49..b0c501d 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -857,6 +857,7 @@ struct mwifiex_adapter { atomic_t rx_pending; atomic_t tx_pending; atomic_t cmd_pending; + atomic_t tx_hw_pending; struct workqueue_struct *workqueue; struct work_struct main_work; struct workqueue_struct *rx_workqueue; diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index ba1fa17..509c156 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -522,6 +522,7 @@ static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter) } } + atomic_set(&adapter->tx_hw_pending, 0); return 0; } @@ -721,6 +722,7 @@ static void mwifiex_cleanup_txq_ring(struct mwifiex_adapter *adapter) card->tx_buf_list[i] = NULL; } + atomic_set(&adapter->tx_hw_pending, 0); return; } @@ -1158,6 +1160,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter) -1); else mwifiex_write_data_complete(adapter, skb, 0, 0); + atomic_dec(&adapter->tx_hw_pending); } card->tx_buf_list[wrdoneidx] = NULL; @@ -1250,6 +1253,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter) wrindx = (card->txbd_wrptr & reg->tx_mask) >> reg->tx_start_ptr; buf_pa = MWIFIEX_SKB_DMA_ADDR(skb); card->tx_buf_list[wrindx] = skb; + atomic_inc(&adapter->tx_hw_pending); if (reg->pfu_enabled) { desc2 = card->txbd_ring[wrindx]; @@ -1327,6 +1331,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter) done_unmap: mwifiex_unmap_pci_memory(adapter, skb, PCI_DMA_TODEVICE); card->tx_buf_list[wrindx] = NULL; + atomic_dec(&adapter->tx_hw_pending); if (reg->pfu_enabled) memset(desc2, 0, sizeof(*desc2)); else