diff mbox series

[1/2] wifi: ath12k: Refactor ath12k_qmi_alloc_target_mem_chunk function

Message ID 20240730170910.3281816-2-quic_rajkbhag@quicinc.com (mailing list archive)
State Deferred
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: Add support to allocate MLO global memory region | expand

Commit Message

Raj Kumar Bhagat July 30, 2024, 5:09 p.m. UTC
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>

Currently, all QMI target memory types share the same allocation
logic within the function ath12k_qmi_alloc_target_mem_chunk().
However, for Multi Link Operation (MLO), firmware requests a new MLO
global memory region. This memory is shared across different firmware
(SoC) participating in the MLO. To accommodate this logic change,
refactor ath12k_qmi_alloc_target_mem_chunk() and introduce a helper
function ath12k_qmi_alloc_chunk() for memory chunk allocation.
Subsequent patch will add MLO global memory allocation logic.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00210-QCAHKSWPL_SILICONZ-1
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3

Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/qmi.c | 82 ++++++++++++++-------------
 1 file changed, 44 insertions(+), 38 deletions(-)

Comments

Jeff Johnson July 31, 2024, 3:45 p.m. UTC | #1
On 7/30/2024 10:09 AM, Raj Kumar Bhagat wrote:
> From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
> 
> Currently, all QMI target memory types share the same allocation
> logic within the function ath12k_qmi_alloc_target_mem_chunk().
> However, for Multi Link Operation (MLO), firmware requests a new MLO
> global memory region. This memory is shared across different firmware
> (SoC) participating in the MLO. To accommodate this logic change,
> refactor ath12k_qmi_alloc_target_mem_chunk() and introduce a helper
> function ath12k_qmi_alloc_chunk() for memory chunk allocation.
> Subsequent patch will add MLO global memory allocation logic.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00210-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> 
> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/qmi.c | 82 ++++++++++++++-------------
>  1 file changed, 44 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
> index 047393bc8bea..11bf16eaadd9 100644
> --- a/drivers/net/wireless/ath/ath12k/qmi.c
> +++ b/drivers/net/wireless/ath/ath12k/qmi.c
> @@ -2366,9 +2366,50 @@ static void ath12k_qmi_free_target_mem_chunk(struct ath12k_base *ab)
>  	}
>  }
>  
> +static int ath12k_qmi_alloc_chunk(struct ath12k_base *ab,
> +				  struct target_mem_chunk *chunk)
> +{
> +	/* Firmware reloads in recovery/resume.
> +	 * In such cases, no need to allocate memory for FW again.
> +	 */
> +	if (chunk->v.addr) {
> +		if (chunk->prev_type == chunk->type &&
> +		    chunk->prev_size == chunk->size)
> +			goto this_chunk_done;

since this is a refactor I appreciate the desire to exactly copy the existing
code. however since this is now a separate function, IMO we can make small
tweaks which take advantage of that, so here I would just:

			return 0;
> +
> +		/* cannot reuse the existing chunk */
> +		dma_free_coherent(ab->dev, chunk->prev_size,
> +				  chunk->v.addr, chunk->paddr);
> +		chunk->v.addr = NULL;
> +	}
> +
> +	chunk->v.addr = dma_alloc_coherent(ab->dev,
> +					   chunk->size,
> +					   &chunk->paddr,
> +					   GFP_KERNEL | __GFP_NOWARN);
> +	if (!chunk->v.addr) {
> +		if (chunk->size > ATH12K_QMI_MAX_CHUNK_SIZE) {
> +			ab->qmi.target_mem_delayed = true;
> +			ath12k_warn(ab,
> +				    "qmi dma allocation failed (%d B type %u), will try later with small size\n",
> +				    chunk->size,
> +				    chunk->type);
> +			ath12k_qmi_free_target_mem_chunk(ab);
> +			return 0;
> +		}
> +		ath12k_warn(ab, "memory allocation failure for %u size: %d\n",
> +			    chunk->type, chunk->size);
> +		return -ENOMEM;
> +	}
> +	chunk->prev_type = chunk->type;
> +	chunk->prev_size = chunk->size;
> +this_chunk_done:

with the above change this label is no longer needed

> +	return 0;
> +}
> +
>  static int ath12k_qmi_alloc_target_mem_chunk(struct ath12k_base *ab)
>  {
> -	int i;
> +	int i, ret = 0;
>  	struct target_mem_chunk *chunk;
>  
>  	ab->qmi.target_mem_delayed = false;
> @@ -2385,42 +2426,7 @@ static int ath12k_qmi_alloc_target_mem_chunk(struct ath12k_base *ab)
>  		case M3_DUMP_REGION_TYPE:
>  		case PAGEABLE_MEM_REGION_TYPE:
>  		case CALDB_MEM_REGION_TYPE:
> -			/* Firmware reloads in recovery/resume.
> -			 * In such cases, no need to allocate memory for FW again.
> -			 */
> -			if (chunk->v.addr) {
> -				if (chunk->prev_type == chunk->type &&
> -				    chunk->prev_size == chunk->size)
> -					goto this_chunk_done;
> -
> -				/* cannot reuse the existing chunk */
> -				dma_free_coherent(ab->dev, chunk->prev_size,
> -						  chunk->v.addr, chunk->paddr);
> -				chunk->v.addr = NULL;
> -			}
> -
> -			chunk->v.addr = dma_alloc_coherent(ab->dev,
> -							   chunk->size,
> -							   &chunk->paddr,
> -							   GFP_KERNEL | __GFP_NOWARN);
> -			if (!chunk->v.addr) {
> -				if (chunk->size > ATH12K_QMI_MAX_CHUNK_SIZE) {
> -					ab->qmi.target_mem_delayed = true;
> -					ath12k_warn(ab,
> -						    "qmi dma allocation failed (%d B type %u), will try later with small size\n",
> -						    chunk->size,
> -						    chunk->type);
> -					ath12k_qmi_free_target_mem_chunk(ab);
> -					return 0;
> -				}
> -				ath12k_warn(ab, "memory allocation failure for %u size: %d\n",
> -					    chunk->type, chunk->size);
> -				return -ENOMEM;
> -			}
> -
> -			chunk->prev_type = chunk->type;
> -			chunk->prev_size = chunk->size;
> -this_chunk_done:
> +			ret = ath12k_qmi_alloc_chunk(ab, chunk);

seems like you need to test ret and return if non-zero here since currently if
you get a bad ret in one loop but you continue and get a good ret on the
remaining iterations then you'll end up returning success from this function.

In other words, your refactoring doesn't correct handle that the original
"return -ENOMEM" would return from *this* function at this point

>  			break;
>  		default:
>  			ath12k_warn(ab, "memory type %u not supported\n",
> @@ -2430,7 +2436,7 @@ static int ath12k_qmi_alloc_target_mem_chunk(struct ath12k_base *ab)
>  			break;
>  		}
>  	}
> -	return 0;
> +	return ret;

if you test ret above then there is no need to make this change since this
will always be the success case

>  }
>  
>  static int ath12k_qmi_request_target_cap(struct ath12k_base *ab)
Jeff Johnson July 31, 2024, 4:08 p.m. UTC | #2
On 7/31/2024 8:45 AM, Jeff Johnson wrote:
> On 7/30/2024 10:09 AM, Raj Kumar Bhagat wrote:
>> +			ret = ath12k_qmi_alloc_chunk(ab, chunk);
> 
> seems like you need to test ret and return if non-zero here since currently if
> you get a bad ret in one loop but you continue and get a good ret on the
> remaining iterations then you'll end up returning success from this function.
> 
> In other words, your refactoring doesn't correct handle that the original
> "return -ENOMEM" would return from *this* function at this point

I see you handle this in patch 2, but that portion of patch 2 should be moved
to this patch
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
index 047393bc8bea..11bf16eaadd9 100644
--- a/drivers/net/wireless/ath/ath12k/qmi.c
+++ b/drivers/net/wireless/ath/ath12k/qmi.c
@@ -2366,9 +2366,50 @@  static void ath12k_qmi_free_target_mem_chunk(struct ath12k_base *ab)
 	}
 }
 
+static int ath12k_qmi_alloc_chunk(struct ath12k_base *ab,
+				  struct target_mem_chunk *chunk)
+{
+	/* Firmware reloads in recovery/resume.
+	 * In such cases, no need to allocate memory for FW again.
+	 */
+	if (chunk->v.addr) {
+		if (chunk->prev_type == chunk->type &&
+		    chunk->prev_size == chunk->size)
+			goto this_chunk_done;
+
+		/* cannot reuse the existing chunk */
+		dma_free_coherent(ab->dev, chunk->prev_size,
+				  chunk->v.addr, chunk->paddr);
+		chunk->v.addr = NULL;
+	}
+
+	chunk->v.addr = dma_alloc_coherent(ab->dev,
+					   chunk->size,
+					   &chunk->paddr,
+					   GFP_KERNEL | __GFP_NOWARN);
+	if (!chunk->v.addr) {
+		if (chunk->size > ATH12K_QMI_MAX_CHUNK_SIZE) {
+			ab->qmi.target_mem_delayed = true;
+			ath12k_warn(ab,
+				    "qmi dma allocation failed (%d B type %u), will try later with small size\n",
+				    chunk->size,
+				    chunk->type);
+			ath12k_qmi_free_target_mem_chunk(ab);
+			return 0;
+		}
+		ath12k_warn(ab, "memory allocation failure for %u size: %d\n",
+			    chunk->type, chunk->size);
+		return -ENOMEM;
+	}
+	chunk->prev_type = chunk->type;
+	chunk->prev_size = chunk->size;
+this_chunk_done:
+	return 0;
+}
+
 static int ath12k_qmi_alloc_target_mem_chunk(struct ath12k_base *ab)
 {
-	int i;
+	int i, ret = 0;
 	struct target_mem_chunk *chunk;
 
 	ab->qmi.target_mem_delayed = false;
@@ -2385,42 +2426,7 @@  static int ath12k_qmi_alloc_target_mem_chunk(struct ath12k_base *ab)
 		case M3_DUMP_REGION_TYPE:
 		case PAGEABLE_MEM_REGION_TYPE:
 		case CALDB_MEM_REGION_TYPE:
-			/* Firmware reloads in recovery/resume.
-			 * In such cases, no need to allocate memory for FW again.
-			 */
-			if (chunk->v.addr) {
-				if (chunk->prev_type == chunk->type &&
-				    chunk->prev_size == chunk->size)
-					goto this_chunk_done;
-
-				/* cannot reuse the existing chunk */
-				dma_free_coherent(ab->dev, chunk->prev_size,
-						  chunk->v.addr, chunk->paddr);
-				chunk->v.addr = NULL;
-			}
-
-			chunk->v.addr = dma_alloc_coherent(ab->dev,
-							   chunk->size,
-							   &chunk->paddr,
-							   GFP_KERNEL | __GFP_NOWARN);
-			if (!chunk->v.addr) {
-				if (chunk->size > ATH12K_QMI_MAX_CHUNK_SIZE) {
-					ab->qmi.target_mem_delayed = true;
-					ath12k_warn(ab,
-						    "qmi dma allocation failed (%d B type %u), will try later with small size\n",
-						    chunk->size,
-						    chunk->type);
-					ath12k_qmi_free_target_mem_chunk(ab);
-					return 0;
-				}
-				ath12k_warn(ab, "memory allocation failure for %u size: %d\n",
-					    chunk->type, chunk->size);
-				return -ENOMEM;
-			}
-
-			chunk->prev_type = chunk->type;
-			chunk->prev_size = chunk->size;
-this_chunk_done:
+			ret = ath12k_qmi_alloc_chunk(ab, chunk);
 			break;
 		default:
 			ath12k_warn(ab, "memory type %u not supported\n",
@@ -2430,7 +2436,7 @@  static int ath12k_qmi_alloc_target_mem_chunk(struct ath12k_base *ab)
 			break;
 		}
 	}
-	return 0;
+	return ret;
 }
 
 static int ath12k_qmi_request_target_cap(struct ath12k_base *ab)