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

[Jeff Johnson]

On 7/30/2024 10:09 AM, Raj Kumar Bhagat wrote:
> From: Karthikeyan Periyasamy <quic_periyasa at 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 at quicinc.com>
> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag at 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)