| Message ID | 20170311013924.73348-5-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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.
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
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?
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
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
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.
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.
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 >
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); }
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(-)