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