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 >