* [PATCH 1/4] mwifiex: pcie: don't leak DMA buffers when removing
2017-03-11 1:39 [PATCH 0/4] mwifiex: several bugfixes Brian Norris
@ 2017-03-11 1:39 ` Brian Norris
2017-03-16 8:14 ` [1/4] " Kalle Valo
2017-03-11 1:39 ` [PATCH 2/4] mwifiex: set adapter->dev before starting to use mwifiex_dbg() Brian Norris
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Brian Norris @ 2017-03-11 1:39 UTC (permalink / raw)
To: Amitkumar Karwar, Nishant Sarmukadam
Cc: Kalle Valo, linux-wireless, linux-kernel, Rajat Jain,
Brian Norris, stable
When PCIe FLR support was added, much of the remove/release code for
PCIe was migrated to ->down_dev(), but ->down_dev() is never called for
device removal. Let's refactor the cleanup to be done in both cases.
Also, drop the comments above mwifiex_cleanup_pcie(), because they were
clearly wrong, and it's better to have clear and obvious code than to
detail the code steps in comments anyway.
Fixes: 4c5dae59d2e9 ("mwifiex: add PCIe function level reset support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
I don't think there's any code path in which we see ->down_dev() followed by
->cleanup_if(), with no ->up_dev() in between? (Or, what happens if PCIe FLR
fails?) But if so, then this could cause a double free. It could use a close
review.
drivers/net/wireless/marvell/mwifiex/pcie.c | 38 ++++++++++++++---------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 5438483fcefe..eae1cc58a310 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2732,6 +2732,21 @@ static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
schedule_work(&card->work);
}
+static void mwifiex_pcie_free_buffers(struct mwifiex_adapter *adapter)
+{
+ struct pcie_service_card *card = adapter->card;
+ const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
+
+ if (reg->sleep_cookie)
+ mwifiex_pcie_delete_sleep_cookie_buf(adapter);
+
+ mwifiex_pcie_delete_cmdrsp_buf(adapter);
+ mwifiex_pcie_delete_evtbd_ring(adapter);
+ mwifiex_pcie_delete_rxbd_ring(adapter);
+ mwifiex_pcie_delete_txbd_ring(adapter);
+ card->cmdrsp_buf = NULL;
+}
+
/*
* This function initializes the PCI-E host memory space, WCB rings, etc.
*
@@ -2843,13 +2858,6 @@ static int mwifiex_init_pcie(struct mwifiex_adapter *adapter)
/*
* This function cleans up the allocated card buffers.
- *
- * The following are freed by this function -
- * - TXBD ring buffers
- * - RXBD ring buffers
- * - Event BD ring buffers
- * - Command response ring buffer
- * - Sleep cookie buffer
*/
static void mwifiex_cleanup_pcie(struct mwifiex_adapter *adapter)
{
@@ -2868,6 +2876,8 @@ 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_iounmap(pdev, card->pci_mmap);
pci_iounmap(pdev, card->pci_mmap1);
@@ -3119,10 +3129,7 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter)
pci_iounmap(pdev, card->pci_mmap1);
}
-/* This function cleans up the PCI-E host memory space.
- * Some code is extracted from mwifiex_unregister_dev()
- *
- */
+/* This function cleans up the PCI-E host memory space. */
static void mwifiex_pcie_down_dev(struct mwifiex_adapter *adapter)
{
struct pcie_service_card *card = adapter->card;
@@ -3133,14 +3140,7 @@ static void mwifiex_pcie_down_dev(struct mwifiex_adapter *adapter)
adapter->seq_num = 0;
- if (reg->sleep_cookie)
- mwifiex_pcie_delete_sleep_cookie_buf(adapter);
-
- mwifiex_pcie_delete_cmdrsp_buf(adapter);
- mwifiex_pcie_delete_evtbd_ring(adapter);
- mwifiex_pcie_delete_rxbd_ring(adapter);
- mwifiex_pcie_delete_txbd_ring(adapter);
- card->cmdrsp_buf = NULL;
+ mwifiex_pcie_free_buffers(adapter);
}
static struct mwifiex_if_ops pcie_ops = {
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [1/4] mwifiex: pcie: don't leak DMA buffers when removing
2017-03-11 1:39 ` [PATCH 1/4] mwifiex: pcie: don't leak DMA buffers when removing Brian Norris
@ 2017-03-16 8:14 ` Kalle Valo
0 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2017-03-16 8:14 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain, Brian Norris, stable
Brian Norris <briannorris@chromium.org> wrote:
> When PCIe FLR support was added, much of the remove/release code for
> PCIe was migrated to ->down_dev(), but ->down_dev() is never called for
> device removal. Let's refactor the cleanup to be done in both cases.
>
> Also, drop the comments above mwifiex_cleanup_pcie(), because they were
> clearly wrong, and it's better to have clear and obvious code than to
> detail the code steps in comments anyway.
>
> Fixes: 4c5dae59d2e9 ("mwifiex: add PCIe function level reset support")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
3 patches applied to wireless-drivers.git, thanks.
4e841d3eb929 mwifiex: pcie: don't leak DMA buffers when removing
ba1c7e45ec22 mwifiex: set adapter->dev before starting to use mwifiex_dbg()
36908c4e5b10 mwifiex: uninit wakeup info when removing device
--
https://patchwork.kernel.org/patch/9618297/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] mwifiex: set adapter->dev before starting to use mwifiex_dbg()
2017-03-11 1:39 [PATCH 0/4] mwifiex: several bugfixes Brian Norris
2017-03-11 1:39 ` [PATCH 1/4] mwifiex: pcie: don't leak DMA buffers when removing Brian Norris
@ 2017-03-11 1:39 ` Brian Norris
2017-03-11 1:39 ` [PATCH 3/4] mwifiex: uninit wakeup info when removing device Brian Norris
2017-03-11 1:39 ` [PATCH 4/4] mwifiex: pcie: de-duplicate buffer allocation code Brian Norris
3 siblings, 0 replies; 14+ messages in thread
From: Brian Norris @ 2017-03-11 1:39 UTC (permalink / raw)
To: Amitkumar Karwar, Nishant Sarmukadam
Cc: Kalle Valo, linux-wireless, linux-kernel, Rajat Jain, Brian Norris
The mwifiex_dbg() log handler utilizes the struct device in
adapter->dev. Without it, it decides not to print anything.
As of commit 2e02b5814217 ("mwifiex: Allow mwifiex early access to device
structure"), we started assigning that pointer only after we finished
mwifiex_register() -- this effectively neuters any mwifiex_dbg() logging
done before this point.
Let's move the device assignment into mwifiex_register().
Fixes: 2e02b5814217 ("mwifiex: Allow mwifiex early access to device structure")
Cc: Rajat Jain <rajatja@google.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/net/wireless/marvell/mwifiex/main.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 5ebca1d0cfc7..43d040e02e4d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -57,8 +57,8 @@ MODULE_PARM_DESC(mfg_mode, "manufacturing mode enable:1, disable:0");
* In case of any errors during inittialization, this function also ensures
* proper cleanup before exiting.
*/
-static int mwifiex_register(void *card, struct mwifiex_if_ops *if_ops,
- void **padapter)
+static int mwifiex_register(void *card, struct device *dev,
+ struct mwifiex_if_ops *if_ops, void **padapter)
{
struct mwifiex_adapter *adapter;
int i;
@@ -68,6 +68,7 @@ static int mwifiex_register(void *card, struct mwifiex_if_ops *if_ops,
return -ENOMEM;
*padapter = adapter;
+ adapter->dev = dev;
adapter->card = card;
/* Save interface specific operations in adapter */
@@ -1568,12 +1569,11 @@ mwifiex_add_card(void *card, struct completion *fw_done,
{
struct mwifiex_adapter *adapter;
- if (mwifiex_register(card, if_ops, (void **)&adapter)) {
+ if (mwifiex_register(card, dev, if_ops, (void **)&adapter)) {
pr_err("%s: software init failed\n", __func__);
goto err_init_sw;
}
- adapter->dev = dev;
mwifiex_probe_of(adapter);
adapter->iface_type = iface_type;
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/4] mwifiex: uninit wakeup info when removing device
2017-03-11 1:39 [PATCH 0/4] mwifiex: several bugfixes Brian Norris
2017-03-11 1:39 ` [PATCH 1/4] mwifiex: pcie: don't leak DMA buffers when removing Brian Norris
2017-03-11 1:39 ` [PATCH 2/4] mwifiex: set adapter->dev before starting to use mwifiex_dbg() Brian Norris
@ 2017-03-11 1:39 ` Brian Norris
2017-03-11 1:39 ` [PATCH 4/4] mwifiex: pcie: de-duplicate buffer allocation code Brian Norris
3 siblings, 0 replies; 14+ messages in thread
From: Brian Norris @ 2017-03-11 1:39 UTC (permalink / raw)
To: Amitkumar Karwar, Nishant Sarmukadam
Cc: Kalle Valo, linux-wireless, linux-kernel, Rajat Jain, Brian Norris
We manually init wakeup info, but we don't detach it on device removal.
This means that if we (for example) rmmod + modprobe the driver, the
device framework might return -EEXIST the second time, and we'll
complain in the logs:
[ 839.311881] mwifiex_pcie 0000:01:00.0: fail to init wakeup for mwifiex
AFAICT, there's no other negative effect.
But we can fix this by disabling wakeup on remove, similar to what a few
other drivers do (e.g., the power supply framework).
This code (and bug) has existed on SDIO for a while, but it got moved
around and enabled for PCIe with commit 853402a00823 ("mwifiex: Enable
WoWLAN for both sdio and pcie").
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/net/wireless/marvell/mwifiex/main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 43d040e02e4d..b62e03d11c2e 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1718,6 +1718,9 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter)
wiphy_unregister(adapter->wiphy);
wiphy_free(adapter->wiphy);
+ if (adapter->irq_wakeup >= 0)
+ device_init_wakeup(adapter->dev, false);
+
/* Unregister device */
mwifiex_dbg(adapter, INFO,
"info: unregister device\n");
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/4] mwifiex: pcie: de-duplicate buffer allocation code
2017-03-11 1:39 [PATCH 0/4] mwifiex: several bugfixes Brian Norris
` (2 preceding siblings ...)
2017-03-11 1:39 ` [PATCH 3/4] mwifiex: uninit wakeup info when removing device Brian Norris
@ 2017-03-11 1:39 ` Brian Norris
2017-03-20 17:08 ` [4/4] " Kalle Valo
` (3 more replies)
3 siblings, 4 replies; 14+ messages in thread
From: Brian Norris @ 2017-03-11 1:39 UTC (permalink / raw)
To: Amitkumar Karwar, Nishant Sarmukadam
Cc: Kalle Valo, linux-wireless, linux-kernel, Rajat Jain, Brian Norris
This code was duplicated as part of the PCIe FLR code added to this
driver. Let's de-duplicate it to:
* make things easier to read (mwifiex_pcie_free_buffers() now has a
corresponding mwifiex_pcie_alloc_buffers())
* reduce likelihood of bugs
* make error logging equally verbose
* save lines of code!
Also drop some of the commentary that isn't really needed.
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 157 ++++++++++++----------------
1 file changed, 66 insertions(+), 91 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index eae1cc58a310..889d87086913 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2732,6 +2732,61 @@ static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
schedule_work(&card->work);
}
+static int mwifiex_pcie_alloc_buffers(struct mwifiex_adapter *adapter)
+{
+ struct pcie_service_card *card = adapter->card;
+ const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
+ int ret;
+
+ card->cmdrsp_buf = NULL;
+ ret = mwifiex_pcie_create_txbd_ring(adapter);
+ if (ret) {
+ mwifiex_dbg(adapter, ERROR, "Failed to create txbd ring\n");
+ goto err_cre_txbd;
+ }
+
+ ret = mwifiex_pcie_create_rxbd_ring(adapter);
+ if (ret) {
+ mwifiex_dbg(adapter, ERROR, "Failed to create rxbd ring\n");
+ goto err_cre_rxbd;
+ }
+
+ ret = mwifiex_pcie_create_evtbd_ring(adapter);
+ if (ret) {
+ mwifiex_dbg(adapter, ERROR, "Failed to create evtbd ring\n");
+ goto err_cre_evtbd;
+ }
+
+ ret = mwifiex_pcie_alloc_cmdrsp_buf(adapter);
+ if (ret) {
+ mwifiex_dbg(adapter, ERROR, "Failed to allocate cmdbuf buffer\n");
+ goto err_alloc_cmdbuf;
+ }
+
+ if (reg->sleep_cookie) {
+ ret = mwifiex_pcie_alloc_sleep_cookie_buf(adapter);
+ if (ret) {
+ mwifiex_dbg(adapter, ERROR, "Failed to allocate sleep_cookie buffer\n");
+ goto err_alloc_cookie;
+ }
+ } else {
+ card->sleep_cookie_vbase = NULL;
+ }
+
+ return 0;
+
+err_alloc_cookie:
+ mwifiex_pcie_delete_cmdrsp_buf(adapter);
+err_alloc_cmdbuf:
+ mwifiex_pcie_delete_evtbd_ring(adapter);
+err_cre_evtbd:
+ mwifiex_pcie_delete_rxbd_ring(adapter);
+err_cre_rxbd:
+ mwifiex_pcie_delete_txbd_ring(adapter);
+err_cre_txbd:
+ return ret;
+}
+
static void mwifiex_pcie_free_buffers(struct mwifiex_adapter *adapter)
{
struct pcie_service_card *card = adapter->card;
@@ -2749,20 +2804,12 @@ static void mwifiex_pcie_free_buffers(struct mwifiex_adapter *adapter)
/*
* This function initializes the PCI-E host memory space, WCB rings, etc.
- *
- * The following initializations steps are followed -
- * - Allocate TXBD ring buffers
- * - Allocate RXBD ring buffers
- * - Allocate event BD ring buffers
- * - Allocate command response ring buffer
- * - Allocate sleep cookie buffer
*/
static int mwifiex_init_pcie(struct mwifiex_adapter *adapter)
{
struct pcie_service_card *card = adapter->card;
int ret;
struct pci_dev *pdev = card->dev;
- const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
pci_set_drvdata(pdev, card);
@@ -2811,37 +2858,13 @@ static int mwifiex_init_pcie(struct mwifiex_adapter *adapter)
pr_notice("PCI memory map Virt0: %p PCI memory map Virt2: %p\n",
card->pci_mmap, card->pci_mmap1);
- card->cmdrsp_buf = NULL;
- ret = mwifiex_pcie_create_txbd_ring(adapter);
+ ret = mwifiex_pcie_alloc_buffers(adapter);
if (ret)
- goto err_cre_txbd;
- ret = mwifiex_pcie_create_rxbd_ring(adapter);
- if (ret)
- goto err_cre_rxbd;
- ret = mwifiex_pcie_create_evtbd_ring(adapter);
- if (ret)
- goto err_cre_evtbd;
- ret = mwifiex_pcie_alloc_cmdrsp_buf(adapter);
- if (ret)
- goto err_alloc_cmdbuf;
- if (reg->sleep_cookie) {
- ret = mwifiex_pcie_alloc_sleep_cookie_buf(adapter);
- if (ret)
- goto err_alloc_cookie;
- } else {
- card->sleep_cookie_vbase = NULL;
- }
- return ret;
+ goto err_alloc_buffers;
-err_alloc_cookie:
- mwifiex_pcie_delete_cmdrsp_buf(adapter);
-err_alloc_cmdbuf:
- mwifiex_pcie_delete_evtbd_ring(adapter);
-err_cre_evtbd:
- mwifiex_pcie_delete_rxbd_ring(adapter);
-err_cre_rxbd:
- mwifiex_pcie_delete_txbd_ring(adapter);
-err_cre_txbd:
+ return 0;
+
+err_alloc_buffers:
pci_iounmap(pdev, card->pci_mmap1);
err_iomap2:
pci_release_region(pdev, 2);
@@ -3053,22 +3076,15 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
card->adapter = NULL;
}
-/* This function initializes the PCI-E host memory space, WCB rings, etc.
- *
- * The following initializations steps are followed -
- * - Allocate TXBD ring buffers
- * - Allocate RXBD ring buffers
- * - Allocate event BD ring buffers
- * - Allocate command response ring buffer
- * - Allocate sleep cookie buffer
- * Part of mwifiex_init_pcie(), not reset the PCIE registers
+/*
+ * This function initializes the PCI-E host memory space, WCB rings, etc.,
+ * similar to mwifiex_init_pcie(), but without resetting PCI-E state.
*/
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;
- const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
/* Bluetooth is not on pcie interface. Download Wifi only firmware
* during pcie FLR, so that bluetooth part of firmware which is
@@ -3081,51 +3097,10 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter)
*/
adapter->tx_buf_size = card->pcie.tx_buf_size;
- card->cmdrsp_buf = NULL;
- ret = mwifiex_pcie_create_txbd_ring(adapter);
- if (ret) {
- mwifiex_dbg(adapter, ERROR, "Failed to create txbd ring\n");
- goto err_cre_txbd;
- }
-
- ret = mwifiex_pcie_create_rxbd_ring(adapter);
- if (ret) {
- mwifiex_dbg(adapter, ERROR, "Failed to create rxbd ring\n");
- goto err_cre_rxbd;
- }
-
- ret = mwifiex_pcie_create_evtbd_ring(adapter);
- if (ret) {
- mwifiex_dbg(adapter, ERROR, "Failed to create evtbd ring\n");
- goto err_cre_evtbd;
- }
-
- ret = mwifiex_pcie_alloc_cmdrsp_buf(adapter);
- if (ret) {
- mwifiex_dbg(adapter, ERROR, "Failed to allocate cmdbuf buffer\n");
- goto err_alloc_cmdbuf;
- }
-
- if (reg->sleep_cookie) {
- ret = mwifiex_pcie_alloc_sleep_cookie_buf(adapter);
- if (ret) {
- mwifiex_dbg(adapter, ERROR, "Failed to allocate sleep_cookie buffer\n");
- goto err_alloc_cookie;
- }
- } else {
- card->sleep_cookie_vbase = NULL;
- }
- return;
+ ret = mwifiex_pcie_alloc_buffers(adapter);
+ if (!ret)
+ return;
-err_alloc_cookie:
- mwifiex_pcie_delete_cmdrsp_buf(adapter);
-err_alloc_cmdbuf:
- mwifiex_pcie_delete_evtbd_ring(adapter);
-err_cre_evtbd:
- mwifiex_pcie_delete_rxbd_ring(adapter);
-err_cre_rxbd:
- mwifiex_pcie_delete_txbd_ring(adapter);
-err_cre_txbd:
pci_iounmap(pdev, card->pci_mmap1);
}
--
2.12.0.246.ga2ecc84866-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
2017-03-11 1:39 ` [PATCH 4/4] mwifiex: pcie: de-duplicate buffer allocation code Brian Norris
@ 2017-03-20 17:08 ` Kalle Valo
[not found] ` <20170320170835.5ED1C609C6@smtp.codeaurora.org>
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2017-03-20 17:08 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain, Brian Norris
Brian Norris <briannorris@chromium.org> wrote:
> This code was duplicated as part of the PCIe FLR code added to this
> driver. Let's de-duplicate it to:
>
> * make things easier to read (mwifiex_pcie_free_buffers() now has a
> corresponding mwifiex_pcie_alloc_buffers())
> * reduce likelihood of bugs
> * make error logging equally verbose
> * save lines of code!
>
> Also drop some of the commentary that isn't really needed.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
Failed to apply:
fatal: sha1 information is lacking or useless (drivers/net/wireless/marvell/mwifiex/pcie.c).
error: could not build fake ancestor
Applying: mwifiex: pcie: de-duplicate buffer allocation code
Patch failed at 0001 mwifiex: pcie: de-duplicate buffer allocation code
The copy of the patch that failed is found in: .git/rebase-apply/patch
Patch set to Changes Requested.
--
https://patchwork.kernel.org/patch/9618309/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20170320170835.5ED1C609C6@smtp.codeaurora.org>]
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
[not found] ` <20170320170835.5ED1C609C6@smtp.codeaurora.org>
@ 2017-03-20 20:05 ` Brian Norris
2017-03-21 12:14 ` Kalle Valo
0 siblings, 1 reply; 14+ messages in thread
From: Brian Norris @ 2017-03-20 20:05 UTC (permalink / raw)
To: Kalle Valo
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain
Hi Kalle,
On Mon, Mar 20, 2017 at 05:08:35PM +0000, Kalle Valo wrote:
> Brian Norris <briannorris@chromium.org> wrote:
> > This code was duplicated as part of the PCIe FLR code added to this
> > driver. Let's de-duplicate it to:
> >
> > * make things easier to read (mwifiex_pcie_free_buffers() now has a
> > corresponding mwifiex_pcie_alloc_buffers())
> > * reduce likelihood of bugs
> > * make error logging equally verbose
> > * save lines of code!
> >
> > Also drop some of the commentary that isn't really needed.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
>
> Failed to apply:
>
> fatal: sha1 information is lacking or useless (drivers/net/wireless/marvell/mwifiex/pcie.c).
> error: could not build fake ancestor
> Applying: mwifiex: pcie: de-duplicate buffer allocation code
> Patch failed at 0001 mwifiex: pcie: de-duplicate buffer allocation code
> The copy of the patch that failed is found in: .git/rebase-apply/patch
>
> Patch set to Changes Requested.
This applies fine to your wireless-drivers/master branch for me, where
patches 1-3 were applied. Are you applying this to
wireless-drivers-next? It's quite understandable that patch 4 wouldn't
apply there, as you've stripped out the previous patches...
Brian
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
2017-03-20 20:05 ` Brian Norris
@ 2017-03-21 12:14 ` Kalle Valo
2017-03-21 15:59 ` Brian Norris
2017-04-28 16:50 ` Brian Norris
0 siblings, 2 replies; 14+ messages in thread
From: Kalle Valo @ 2017-03-21 12:14 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain
Brian Norris <briannorris@chromium.org> writes:
> On Mon, Mar 20, 2017 at 05:08:35PM +0000, Kalle Valo wrote:
>> Brian Norris <briannorris@chromium.org> wrote:
>> > This code was duplicated as part of the PCIe FLR code added to this
>> > driver. Let's de-duplicate it to:
>> >
>> > * make things easier to read (mwifiex_pcie_free_buffers() now has a
>> > corresponding mwifiex_pcie_alloc_buffers())
>> > * reduce likelihood of bugs
>> > * make error logging equally verbose
>> > * save lines of code!
>> >
>> > Also drop some of the commentary that isn't really needed.
>> >
>> > Signed-off-by: Brian Norris <briannorris@chromium.org>
>>
>> Failed to apply:
>>
>> fatal: sha1 information is lacking or useless
>> (drivers/net/wireless/marvell/mwifiex/pcie.c).
>> error: could not build fake ancestor
>> Applying: mwifiex: pcie: de-duplicate buffer allocation code
>> Patch failed at 0001 mwifiex: pcie: de-duplicate buffer allocation code
>> The copy of the patch that failed is found in: .git/rebase-apply/patch
>>
>> Patch set to Changes Requested.
>
> This applies fine to your wireless-drivers/master branch for me, where
> patches 1-3 were applied. Are you applying this to
> wireless-drivers-next? It's quite understandable that patch 4 wouldn't
> apply there, as you've stripped out the previous patches...
I (wrongly) understood that patches 1-3 are for 4.11 and patch 4 is for
4.12, don't remember anymore how I got that impression. But I don't
think a cleanup patch like this is justified for 4.11 so I'm not
comfortable applying this to wireless-drivers (which should only contain
fixes to important bugs or regressions).
What I could do is to wait for the patches 1-3 trickle down to w-d-next
and then apply this patch. It usually takes few weeks, but with bad luck
it might happen only after the merge window. Would that work?
--
Kalle Valo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
2017-03-21 12:14 ` Kalle Valo
@ 2017-03-21 15:59 ` Brian Norris
2017-04-28 16:50 ` Brian Norris
1 sibling, 0 replies; 14+ messages in thread
From: Brian Norris @ 2017-03-21 15:59 UTC (permalink / raw)
To: Kalle Valo
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain
On Tue, Mar 21, 2017 at 02:14:05PM +0200, Kalle Valo wrote:
> Brian Norris <briannorris@chromium.org> writes:
> > On Mon, Mar 20, 2017 at 05:08:35PM +0000, Kalle Valo wrote:
> >> Failed to apply:
> >>
> >> fatal: sha1 information is lacking or useless
> >> (drivers/net/wireless/marvell/mwifiex/pcie.c).
> >> error: could not build fake ancestor
> >> Applying: mwifiex: pcie: de-duplicate buffer allocation code
> >> Patch failed at 0001 mwifiex: pcie: de-duplicate buffer allocation code
> >> The copy of the patch that failed is found in: .git/rebase-apply/patch
> >>
> >> Patch set to Changes Requested.
> >
> > This applies fine to your wireless-drivers/master branch for me, where
> > patches 1-3 were applied. Are you applying this to
> > wireless-drivers-next? It's quite understandable that patch 4 wouldn't
> > apply there, as you've stripped out the previous patches...
>
> I (wrongly) understood that patches 1-3 are for 4.11 and patch 4 is for
> 4.12, don't remember anymore how I got that impression. But I don't
Well, you're not exactly wrong. I mentioned in the cover letter that the
first 3 are bugfixes (probably for 4.11) and the 4th is not.
> think a cleanup patch like this is justified for 4.11 so I'm not
> comfortable applying this to wireless-drivers (which should only contain
> fixes to important bugs or regressions).
Right.
> What I could do is to wait for the patches 1-3 trickle down to w-d-next
> and then apply this patch. It usually takes few weeks, but with bad luck
> it might happen only after the merge window. Would that work?
Yeah, I figured something like that would happen. Seems fine to me.
Brian
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
2017-03-21 12:14 ` Kalle Valo
2017-03-21 15:59 ` Brian Norris
@ 2017-04-28 16:50 ` Brian Norris
2017-05-04 13:11 ` Kalle Valo
1 sibling, 1 reply; 14+ messages in thread
From: Brian Norris @ 2017-04-28 16:50 UTC (permalink / raw)
To: Kalle Valo
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain
On Tue, Mar 21, 2017 at 02:14:05PM +0200, Kalle Valo wrote:
> What I could do is to wait for the patches 1-3 trickle down to w-d-next
> and then apply this patch. It usually takes few weeks, but with bad luck
> it might happen only after the merge window. Would that work?
Is this going to get picked up?
Brian
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
2017-04-28 16:50 ` Brian Norris
@ 2017-05-04 13:11 ` Kalle Valo
0 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2017-05-04 13:11 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain
Brian Norris <briannorris@chromium.org> writes:
> On Tue, Mar 21, 2017 at 02:14:05PM +0200, Kalle Valo wrote:
>> What I could do is to wait for the patches 1-3 trickle down to w-d-next
>> and then apply this patch. It usually takes few weeks, but with bad luck
>> it might happen only after the merge window. Would that work?
>
> Is this going to get picked up?
Oh man, I thought I had changed the state of this patch from Changes
Requested to Awaiting Upstream but apparently I didn't do that and hence
missed the patch. It's now back in active my queue in Under Review
state:
https://patchwork.kernel.org/patch/9618309/
Thanks for reminding me and sorry for the screwup.
--
Kalle Valo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
2017-03-11 1:39 ` [PATCH 4/4] mwifiex: pcie: de-duplicate buffer allocation code Brian Norris
2017-03-20 17:08 ` [4/4] " Kalle Valo
[not found] ` <20170320170835.5ED1C609C6@smtp.codeaurora.org>
@ 2017-05-18 13:33 ` Kalle Valo
[not found] ` <20170518133348.6C5C660F63@smtp.codeaurora.org>
3 siblings, 0 replies; 14+ messages in thread
From: Kalle Valo @ 2017-05-18 13:33 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain, Brian Norris
Brian Norris <briannorris@chromium.org> wrote:
> This code was duplicated as part of the PCIe FLR code added to this
> driver. Let's de-duplicate it to:
>
> * make things easier to read (mwifiex_pcie_free_buffers() now has a
> corresponding mwifiex_pcie_alloc_buffers())
> * reduce likelihood of bugs
> * make error logging equally verbose
> * save lines of code!
>
> Also drop some of the commentary that isn't really needed.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
Failed to apply:
fatal: sha1 information is lacking or useless (drivers/net/wireless/marvell/mwifiex/pcie.c).
error: could not build fake ancestor
Applying: mwifiex: pcie: de-duplicate buffer allocation code
Patch failed at 0001 mwifiex: pcie: de-duplicate buffer allocation code
The copy of the patch that failed is found in: .git/rebase-apply/patch
Patch set to Changes Requested.
--
https://patchwork.kernel.org/patch/9618309/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20170518133348.6C5C660F63@smtp.codeaurora.org>]
* Re: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
[not found] ` <20170518133348.6C5C660F63@smtp.codeaurora.org>
@ 2017-05-18 16:30 ` Brian Norris
0 siblings, 0 replies; 14+ messages in thread
From: Brian Norris @ 2017-05-18 16:30 UTC (permalink / raw)
To: Kalle Valo
Cc: Amitkumar Karwar, Nishant Sarmukadam, linux-wireless,
linux-kernel, Rajat Jain
On Thu, May 18, 2017 at 01:33:48PM +0000, Kalle Valo wrote:
> Brian Norris <briannorris@chromium.org> wrote:
> > This code was duplicated as part of the PCIe FLR code added to this
> > driver. Let's de-duplicate it to:
> >
> > * make things easier to read (mwifiex_pcie_free_buffers() now has a
> > corresponding mwifiex_pcie_alloc_buffers())
> > * reduce likelihood of bugs
> > * make error logging equally verbose
> > * save lines of code!
> >
> > Also drop some of the commentary that isn't really needed.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
>
> Failed to apply:
>
> fatal: sha1 information is lacking or useless (drivers/net/wireless/marvell/mwifiex/pcie.c).
> error: could not build fake ancestor
> Applying: mwifiex: pcie: de-duplicate buffer allocation code
> Patch failed at 0001 mwifiex: pcie: de-duplicate buffer allocation code
> The copy of the patch that failed is found in: .git/rebase-apply/patch
Hmm, well it still applies for me, but it does require a 3-way merge...
and I guess I have local history (because the 3 previous patches *used*
to be together...):
$ pwclient git-am -p linux-wireless 9618309
Applying patch #9618309 using u'git am'
Description: [4/4] mwifiex: pcie: de-duplicate buffer allocation code
Applying: mwifiex: pcie: de-duplicate buffer allocation code
Using index info to reconstruct a base tree...
M drivers/net/wireless/marvell/mwifiex/pcie.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/net/wireless/marvell/mwifiex/pcie.c
Anyway, I'll resend.
Brian
> Patch set to Changes Requested.
> https://patchwork.kernel.org/patch/9618309/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>
^ permalink raw reply [flat|nested] 14+ messages in thread