Re: [PATCH] ath10k: allocate small size dma memory in ath10k_pci_diag_write_mem

[Brian Norris <briannorris@chromium.org>]

Hi Carl,

On Mon, Oct 08, 2018 at 05:49:42PM +0800, Carl Huang wrote:
> ath10k_pci_diag_write_mem may allocate big size of the dma memory
> based on the parameter nbytes. Take firmware diag download as
> example, the biggest size is about 500K. In some systems, the
> allocation is likely to fail because it can't acquire such a large
> contiguous dma memory.
> 
> The fix is to allocate a small size dma memory. In the loop,
> driver copies the data to the allocated dma memory and writes to
> the destination until all the data is written.
> 
> Tested with QCA6174 PCI with
> firmware-6.bin_WLAN.RM.4.4.1-00119-QCARMSWP-1, this also affects
> QCA9377 PCI.
> 
> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>

Thanks for the patch! With one small comment, I think this otherwise
looks good, so feel free to add my:

Reviewed-by: Brian Norris <briannorris@chomium.org>

> ---
>  drivers/net/wireless/ath/ath10k/pci.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 873dbb6..1872671 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -1071,7 +1071,7 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
>  	struct ath10k_ce *ce = ath10k_ce_priv(ar);
>  	int ret = 0;
>  	u32 *buf;
> -	unsigned int completed_nbytes, orig_nbytes, remaining_bytes;
> +	unsigned int completed_nbytes, alloc_nbytes, remaining_bytes;
>  	struct ath10k_ce_pipe *ce_diag;
>  	void *data_buf = NULL;
>  	u32 ce_data;	/* Host buffer address in CE space */

^^ I don't think this is needed any more.

> @@ -1088,9 +1088,10 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
>  	 *   1) 4-byte alignment
>  	 *   2) Buffer in DMA-able space
>  	 */
> -	orig_nbytes = nbytes;
> +	alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
> +
>  	data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
> -						       orig_nbytes,
> +						       alloc_nbytes,
>  						       &ce_data_base,
>  						       GFP_ATOMIC);
>  	if (!data_buf) {
> @@ -1098,9 +1099,6 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
>  		goto done;
>  	}
>  
> -	/* Copy caller's data to allocated DMA buf */
> -	memcpy(data_buf, data, orig_nbytes);
> -
>  	/*
>  	 * The address supplied by the caller is in the
>  	 * Target CPU virtual address space.
> @@ -1113,12 +1111,15 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
>  	 */
>  	address = ath10k_pci_targ_cpu_to_ce_addr(ar, address);
>  
> -	remaining_bytes = orig_nbytes;
> +	remaining_bytes = nbytes;
>  	ce_data = ce_data_base;

Because you no longer need to increment 'ce_data', you can just pass
'ce_data_base' to ath10k_ce_send_nolock() now.

Regards,
Brian

>  	while (remaining_bytes) {
>  		/* FIXME: check cast */
>  		nbytes = min_t(int, remaining_bytes, DIAG_TRANSFER_LIMIT);
>  
> +		/* Copy caller's data to allocated DMA buf */
> +		memcpy(data_buf, data, nbytes);
> +
>  		/* Set up to receive directly into Target(!) address */
>  		ret = ce_diag->ops->ce_rx_post_buf(ce_diag, &address, address);
>  		if (ret != 0)
> @@ -1171,12 +1172,12 @@ int ath10k_pci_diag_write_mem(struct ath10k *ar, u32 address,
>  
>  		remaining_bytes -= nbytes;
>  		address += nbytes;
> -		ce_data += nbytes;
> +		data += nbytes;
>  	}
>  
>  done:
>  	if (data_buf) {
> -		dma_free_coherent(ar->dev, orig_nbytes, data_buf,
> +		dma_free_coherent(ar->dev, alloc_nbytes, data_buf,
>  				  ce_data_base);
>  	}
>  
> -- 
> 2.7.4
> 

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k