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 <cjhu...@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 <briannor...@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
>
