diff mbox

[2/3] ath10k: use dma_zalloc_coherent()

Message ID 1485183876-27080-2-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

Srinivas Kandagatla Jan. 23, 2017, 3:04 p.m. UTC
use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/net/wireless/ath/ath10k/ce.c  | 9 +--------
 drivers/net/wireless/ath/ath10k/pci.c | 3 +--
 2 files changed, 2 insertions(+), 10 deletions(-)

Comments

Joe Perches Jan. 23, 2017, 11:19 p.m. UTC | #1
On Mon, 2017-01-23 at 15:04 +0000, Srinivas Kandagatla wrote:
> use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().
[]
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
[]
> @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
>  	 */
>  	alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
>  
> -	data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
> +	data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
>  						       alloc_nbytes,
>  						       &ce_data_base,
>  						       GFP_ATOMIC);

trivia:

Nicer to realign arguments and remove the unnecessary cast.

Perhaps:

	data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, &ce_data_base,
				       GFP_ATOMIC);
Kalle Valo Jan. 24, 2017, 5:18 a.m. UTC | #2
Joe Perches <joe@perches.com> writes:

> On Mon, 2017-01-23 at 15:04 +0000, Srinivas Kandagatla wrote:

>> use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().

> []

>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c

> []

>> @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,

>>  	 */

>>  	alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);

>>  

>> -	data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,

>> +	data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,

>>  						       alloc_nbytes,

>>  						       &ce_data_base,

>>  						       GFP_ATOMIC);

>

> trivia:

>

> Nicer to realign arguments and remove the unnecessary cast.

>

> Perhaps:

>

> 	data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, &ce_data_base,

> 				       GFP_ATOMIC);


Sure, but that should be in a separate patch.

-- 
Kalle Valo
Joe Perches Jan. 24, 2017, 5:25 a.m. UTC | #3
On Tue, 2017-01-24 at 05:18 +0000, Valo, Kalle wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > On Mon, 2017-01-23 at 15:04 +0000, Srinivas Kandagatla wrote:
> > > use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().
> > 
> > []
> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> > 
> > []
> > > @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
> > >  	 */
> > >  	alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
> > >  
> > > -	data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
> > > +	data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
> > >  						       alloc_nbytes,
> > >  						       &ce_data_base,
> > >  						       GFP_ATOMIC);
> > 
> > trivia:
> > 
> > Nicer to realign arguments and remove the unnecessary cast.
> > 
> > Perhaps:
> > 
> > 	data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, &ce_data_base,
> > 				       GFP_ATOMIC);
> 
> Sure, but that should be in a separate patch.

I don't think so, trivial patches can be combined.

It's also nicer to realign all modified multiline
arguments when performing these changes.

Coccinelle generally does it automatically.
Kalle Valo Jan. 24, 2017, 12:13 p.m. UTC | #4
Joe Perches <joe@perches.com> writes:

> On Tue, 2017-01-24 at 05:18 +0000, Valo, Kalle wrote:

>> Joe Perches <joe@perches.com> writes:

>> 

>> > On Mon, 2017-01-23 at 15:04 +0000, Srinivas Kandagatla wrote:

>> > > use dma_zalloc_coherent() instead of dma_alloc_coherent and memset().

>> > 

>> > []

>> > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c

>> > > b/drivers/net/wireless/ath/ath10k/pci.c

>> > 

>> > []

>> > > @@ -896,7 +896,7 @@ static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,

>> > >  	 */

>> > >  	alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);

>> > >  

>> > > -	data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,

>> > > +	data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,

>> > >  						       alloc_nbytes,

>> > >  						       &ce_data_base,

>> > >  						       GFP_ATOMIC);

>> > 

>> > trivia:

>> > 

>> > Nicer to realign arguments and remove the unnecessary cast.

>> > 

>> > Perhaps:

>> > 

>> > 	data_buf = dma_zalloc_coherent(ar->dev, alloc_nbytes, &ce_data_base,

>> > 				       GFP_ATOMIC);

>> 

>> Sure, but that should be in a separate patch.

>

> I don't think so, trivial patches can be combined.

>

> It's also nicer to realign all modified multiline

> arguments when performing these changes.

>

> Coccinelle generally does it automatically.


A matter of preference really. I prefer keeping style and functional
changes in separate patches, keeps the review simple. And style changes
can hide bugs.

-- 
Kalle Valo
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 0b4d796..c2b388f 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -958,7 +958,7 @@  ath10k_ce_alloc_dest_ring(struct ath10k *ar, unsigned int ce_id,
 	 * coherent DMA are unsupported
 	 */
 	dest_ring->base_addr_owner_space_unaligned =
-		dma_alloc_coherent(ar->dev,
+		dma_zalloc_coherent(ar->dev,
 				   (nentries * sizeof(struct ce_desc) +
 				    CE_DESC_RING_ALIGN),
 				   &base_addr, GFP_KERNEL);
@@ -969,13 +969,6 @@  ath10k_ce_alloc_dest_ring(struct ath10k *ar, unsigned int ce_id,
 
 	dest_ring->base_addr_ce_space_unaligned = base_addr;
 
-	/*
-	 * Correctly initialize memory to 0 to prevent garbage
-	 * data crashing system when download firmware
-	 */
-	memset(dest_ring->base_addr_owner_space_unaligned, 0,
-	       nentries * sizeof(struct ce_desc) + CE_DESC_RING_ALIGN);
-
 	dest_ring->base_addr_owner_space = PTR_ALIGN(
 			dest_ring->base_addr_owner_space_unaligned,
 			CE_DESC_RING_ALIGN);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index b541a1c..855e3de 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -896,7 +896,7 @@  static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
 	 */
 	alloc_nbytes = min_t(unsigned int, nbytes, DIAG_TRANSFER_LIMIT);
 
-	data_buf = (unsigned char *)dma_alloc_coherent(ar->dev,
+	data_buf = (unsigned char *)dma_zalloc_coherent(ar->dev,
 						       alloc_nbytes,
 						       &ce_data_base,
 						       GFP_ATOMIC);
@@ -905,7 +905,6 @@  static int ath10k_pci_diag_read_mem(struct ath10k *ar, u32 address, void *data,
 		ret = -ENOMEM;
 		goto done;
 	}
-	memset(data_buf, 0, alloc_nbytes);
 
 	remaining_bytes = nbytes;
 	ce_data = ce_data_base;