[PATCH] wifi: mwifiex: fix error recovery in PCIE buffer descriptor management

[PATCH] wifi: mwifiex: fix error recovery in PCIE buffer descriptor management

[Dmitry Antipov]

Add missing 'kfree_skb()' in 'mwifiex_init_rxq_ring()' and never do
'kfree(card->rxbd_ring_vbase)' because this area is DMAed and should
be released with 'dma_free_coherent()'. The latter is performed in
'mwifiex_pcie_delete_rxbd_ring()', which is now called to recover
from possible errors in 'mwifiex_pcie_create_rxbd_ring()'. Likewise
for 'mwifiex_pcie_init_evt_ring()', 'kfree(card->evtbd_ring_vbase)'
'mwifiex_pcie_delete_evtbd_ring()' and 'mwifiex_pcie_create_rxbd_ring()'.

Fixes: 0732484b47b5 ("mwifiex: separate ring initialization and ring creation routines")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 25 ++++++++++++++-------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 9a698a16a8f3..6697132ecc97 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -189,6 +189,8 @@ static int mwifiex_pcie_probe_of(struct device *dev)
 }
 
 static void mwifiex_pcie_work(struct work_struct *work);
+static int mwifiex_pcie_delete_rxbd_ring(struct mwifiex_adapter *adapter);
+static int mwifiex_pcie_delete_evtbd_ring(struct mwifiex_adapter *adapter);
 
 static int
 mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
@@ -792,14 +794,15 @@ static int mwifiex_init_rxq_ring(struct mwifiex_adapter *adapter)
 		if (!skb) {
 			mwifiex_dbg(adapter, ERROR,
 				    "Unable to allocate skb for RX ring.\n");
-			kfree(card->rxbd_ring_vbase);
 			return -ENOMEM;
 		}
 
 		if (mwifiex_map_pci_memory(adapter, skb,
 					   MWIFIEX_RX_DATA_BUF_SIZE,
-					   DMA_FROM_DEVICE))
-			return -1;
+					   DMA_FROM_DEVICE)) {
+			kfree_skb(skb);
+			return -ENOMEM;
+		}
 
 		buf_pa = MWIFIEX_SKB_DMA_ADDR(skb);
 
@@ -849,7 +852,6 @@ static int mwifiex_pcie_init_evt_ring(struct mwifiex_adapter *adapter)
 		if (!skb) {
 			mwifiex_dbg(adapter, ERROR,
 				    "Unable to allocate skb for EVENT buf.\n");
-			kfree(card->evtbd_ring_vbase);
 			return -ENOMEM;
 		}
 		skb_put(skb, MAX_EVENT_SIZE);
@@ -857,8 +859,7 @@ static int mwifiex_pcie_init_evt_ring(struct mwifiex_adapter *adapter)
 		if (mwifiex_map_pci_memory(adapter, skb, MAX_EVENT_SIZE,
 					   DMA_FROM_DEVICE)) {
 			kfree_skb(skb);
-			kfree(card->evtbd_ring_vbase);
-			return -1;
+			return -ENOMEM;
 		}
 
 		buf_pa = MWIFIEX_SKB_DMA_ADDR(skb);
@@ -1058,6 +1059,7 @@ static int mwifiex_pcie_delete_txbd_ring(struct mwifiex_adapter *adapter)
  */
 static int mwifiex_pcie_create_rxbd_ring(struct mwifiex_adapter *adapter)
 {
+	int ret;
 	struct pcie_service_card *card = adapter->card;
 	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
 
@@ -1096,7 +1098,10 @@ static int mwifiex_pcie_create_rxbd_ring(struct mwifiex_adapter *adapter)
 		    (u32)((u64)card->rxbd_ring_pbase >> 32),
 		    card->rxbd_ring_size);
 
-	return mwifiex_init_rxq_ring(adapter);
+	ret = mwifiex_init_rxq_ring(adapter);
+	if (ret)
+		mwifiex_pcie_delete_rxbd_ring(adapter);
+	return ret;
 }
 
 /*
@@ -1127,6 +1132,7 @@ static int mwifiex_pcie_delete_rxbd_ring(struct mwifiex_adapter *adapter)
  */
 static int mwifiex_pcie_create_evtbd_ring(struct mwifiex_adapter *adapter)
 {
+	int ret;
 	struct pcie_service_card *card = adapter->card;
 	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
 
@@ -1161,7 +1167,10 @@ static int mwifiex_pcie_create_evtbd_ring(struct mwifiex_adapter *adapter)
 		    (u32)((u64)card->evtbd_ring_pbase >> 32),
 		    card->evtbd_ring_size);
 
-	return mwifiex_pcie_init_evt_ring(adapter);
+	ret = mwifiex_pcie_init_evt_ring(adapter);
+	if (ret)
+		mwifiex_pcie_delete_evtbd_ring(adapter);
+	return ret;
 }
 
 /*
-- 
2.41.0

Re: [PATCH] wifi: mwifiex: fix error recovery in PCIE buffer descriptor management

[Brian Norris]

On Mon, Jul 31, 2023 at 10:43:07AM +0300, Dmitry Antipov wrote:
> Add missing 'kfree_skb()' in 'mwifiex_init_rxq_ring()' and never do
> 'kfree(card->rxbd_ring_vbase)' because this area is DMAed and should
> be released with 'dma_free_coherent()'. The latter is performed in
> 'mwifiex_pcie_delete_rxbd_ring()', which is now called to recover
> from possible errors in 'mwifiex_pcie_create_rxbd_ring()'. Likewise
> for 'mwifiex_pcie_init_evt_ring()', 'kfree(card->evtbd_ring_vbase)'
> 'mwifiex_pcie_delete_evtbd_ring()' and 'mwifiex_pcie_create_rxbd_ring()'.
> 
> Fixes: 0732484b47b5 ("mwifiex: separate ring initialization and ring creation routines")

I'm not sure that's truly an appropriate "Fixes" target, as that commit
just shuffled the existing bad code around. I'd either remove that line,
or else also add:

Fixes: d930faee141b ("mwifiex: add support for Marvell pcie8766 chipset")

where the buggy stuff was first introduced.

I don't think you need to resubmit just for this; Kalle can usually make
small adjustments like this when applying. And even if not, the issue is
trivial.

> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 25 ++++++++++++++-------
>  1 file changed, 17 insertions(+), 8 deletions(-)

Patch looks good, thanks:

Acked-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH] wifi: mwifiex: fix error recovery in PCIE buffer descriptor management

[Dmitry Antipov]

On 8/1/23 21:27, Brian Norris wrote:

> I'm not sure that's truly an appropriate "Fixes" target, as that commit
> just shuffled the existing bad code around. I'd either remove that line,
> or else also add:
> 
> Fixes: d930faee141b ("mwifiex: add support for Marvell pcie8766 chipset")
> 
> where the buggy stuff was first introduced.
> 
> I don't think you need to resubmit just for this; Kalle can usually make
> small adjustments like this when applying. And even if not, the issue is
> trivial.

I have some more PCIe bits; would it be better to submit them separately or
create a series (of 3 patches I think) and make this one a part of the series?

Dmitry

Re: wifi: mwifiex: fix error recovery in PCIE buffer descriptor management

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Add missing 'kfree_skb()' in 'mwifiex_init_rxq_ring()' and never do
> 'kfree(card->rxbd_ring_vbase)' because this area is DMAed and should
> be released with 'dma_free_coherent()'. The latter is performed in
> 'mwifiex_pcie_delete_rxbd_ring()', which is now called to recover
> from possible errors in 'mwifiex_pcie_create_rxbd_ring()'. Likewise
> for 'mwifiex_pcie_init_evt_ring()', 'kfree(card->evtbd_ring_vbase)'
> 'mwifiex_pcie_delete_evtbd_ring()' and 'mwifiex_pcie_create_rxbd_ring()'.
> 
> Fixes: d930faee141b ("mwifiex: add support for Marvell pcie8766 chipset")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> Acked-by: Brian Norris <briannorris@chromium.org>

Patch applied to wireless-next.git, thanks.

288c63d5cb46 wifi: mwifiex: fix error recovery in PCIE buffer descriptor management

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230731074334.56463-1-dmantipov@yandex.ru/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches