diff mbox series

[21/22] mmc: wbsd: Use dma_alloc_noncoherent() for dma buffer

Message ID 20220219005221.634-22-bhe@redhat.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series Don't use kmalloc() with GFP_DMA | expand

Commit Message

Baoquan He Feb. 19, 2022, 12:52 a.m. UTC
Use dma_alloc_noncoherent() instead to get the DMA buffer.

[ 42.hyeyoo@gmail.com: Only keep label free.

  Remove unnecessary alignment checks. it's guaranteed by DMA API.
  Just use GFP_KERNEL as it's called in sleepable context.

  Specify its dma capability using  dma_set_mask_and_coherent() ]

Signed-off-by: Baoquan He <bhe@redhat.com>
Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: Pierre Ossman <pierre@ossman.eu>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: linux-mmc@vger.kernel.org
---
 drivers/mmc/host/wbsd.c | 45 +++++++++--------------------------------
 1 file changed, 9 insertions(+), 36 deletions(-)

Comments

Christoph Hellwig Feb. 19, 2022, 7:17 a.m. UTC | #1
On Sat, Feb 19, 2022 at 08:52:20AM +0800, Baoquan He wrote:
>  	if (request_dma(dma, DRIVER_NAME))
>  		goto err;
>  
> +	dma_set_mask_and_coherent(mmc_dev(host->mmc), DMA_BIT_MASK(24));

This also sets the streaming mask, but the driver doesn't seem to make
use of that.  Please document it in the commit log.

Also setting smaller than 32 bit masks can fail, so this should have
error handling.
Baoquan He Feb. 20, 2022, 8:40 a.m. UTC | #2
On 02/19/22 at 08:17am, Christoph Hellwig wrote:
> On Sat, Feb 19, 2022 at 08:52:20AM +0800, Baoquan He wrote:
> >  	if (request_dma(dma, DRIVER_NAME))
> >  		goto err;
> >  
> > +	dma_set_mask_and_coherent(mmc_dev(host->mmc), DMA_BIT_MASK(24));
> 
> This also sets the streaming mask, but the driver doesn't seem to make
> use of that.  Please document it in the commit log.

Thanks for reviewing. I will change it to dma_set_mask(), and describe
this change in patch log.

> 
> Also setting smaller than 32 bit masks can fail, so this should have
> error handling.

OK, will check and add error handling.
Christoph Hellwig Feb. 22, 2022, 8:45 a.m. UTC | #3
On Sun, Feb 20, 2022 at 04:40:44PM +0800, Baoquan He wrote:
> On 02/19/22 at 08:17am, Christoph Hellwig wrote:
> > On Sat, Feb 19, 2022 at 08:52:20AM +0800, Baoquan He wrote:
> > >  	if (request_dma(dma, DRIVER_NAME))
> > >  		goto err;
> > >  
> > > +	dma_set_mask_and_coherent(mmc_dev(host->mmc), DMA_BIT_MASK(24));
> > 
> > This also sets the streaming mask, but the driver doesn't seem to make
> > use of that.  Please document it in the commit log.
> 
> Thanks for reviewing. I will change it to dma_set_mask(), and describe
> this change in patch log.

No, if you change it, it should be dma_set_coherent_mask only as it is
not using streaming mappings.  I suspect dma_set_mask_and_coherent is
the right thing if the driver ever wants to use streaming mapping,
it would just need to be documented in the commit message.
Baoquan He Feb. 22, 2022, 9:14 a.m. UTC | #4
On 02/22/22 at 09:45am, Christoph Hellwig wrote:
> On Sun, Feb 20, 2022 at 04:40:44PM +0800, Baoquan He wrote:
> > On 02/19/22 at 08:17am, Christoph Hellwig wrote:
> > > On Sat, Feb 19, 2022 at 08:52:20AM +0800, Baoquan He wrote:
> > > >  	if (request_dma(dma, DRIVER_NAME))
> > > >  		goto err;
> > > >  
> > > > +	dma_set_mask_and_coherent(mmc_dev(host->mmc), DMA_BIT_MASK(24));
> > > 
> > > This also sets the streaming mask, but the driver doesn't seem to make
> > > use of that.  Please document it in the commit log.
> > 
> > Thanks for reviewing. I will change it to dma_set_mask(), and describe
> > this change in patch log.
> 
> No, if you change it, it should be dma_set_coherent_mask only as it is
> not using streaming mappings.  I suspect dma_set_mask_and_coherent is
> the right thing if the driver ever wants to use streaming mapping,
> it would just need to be documented in the commit message.

It will serve dma_alloc_noncoherent() calling later, should be streaming
mapping?
Christoph Hellwig Feb. 22, 2022, 1:11 p.m. UTC | #5
On Tue, Feb 22, 2022 at 05:14:16PM +0800, Baoquan He wrote:
> > No, if you change it, it should be dma_set_coherent_mask only as it is
> > not using streaming mappings.  I suspect dma_set_mask_and_coherent is
> > the right thing if the driver ever wants to use streaming mapping,
> > it would just need to be documented in the commit message.
> 
> It will serve dma_alloc_noncoherent() calling later, should be streaming
> mapping?

No, that also looks at the coherent mask.  Which is a bit misnamed these
days, it really should be the alloc mask.
Baoquan He Feb. 22, 2022, 1:40 p.m. UTC | #6
On 02/22/22 at 02:11pm, Christoph Hellwig wrote:
> On Tue, Feb 22, 2022 at 05:14:16PM +0800, Baoquan He wrote:
> > > No, if you change it, it should be dma_set_coherent_mask only as it is
> > > not using streaming mappings.  I suspect dma_set_mask_and_coherent is
> > > the right thing if the driver ever wants to use streaming mapping,
> > > it would just need to be documented in the commit message.
> > 
> > It will serve dma_alloc_noncoherent() calling later, should be streaming
> > mapping?
> 
> No, that also looks at the coherent mask.  Which is a bit misnamed these
> days, it really should be the alloc mask.

I noticed the misnamed code and have made two draft patches, please help
check if it's necessary.
diff mbox series

Patch

diff --git a/drivers/mmc/host/wbsd.c b/drivers/mmc/host/wbsd.c
index 67ecd342fe5f..50b0197583c7 100644
--- a/drivers/mmc/host/wbsd.c
+++ b/drivers/mmc/host/wbsd.c
@@ -1366,55 +1366,28 @@  static void wbsd_request_dma(struct wbsd_host *host, int dma)
 	if (request_dma(dma, DRIVER_NAME))
 		goto err;
 
+	dma_set_mask_and_coherent(mmc_dev(host->mmc), DMA_BIT_MASK(24));
+
 	/*
 	 * We need to allocate a special buffer in
 	 * order for ISA to be able to DMA to it.
 	 */
-	host->dma_buffer = kmalloc(WBSD_DMA_SIZE,
-		GFP_NOIO | GFP_DMA | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+	host->dma_buffer = dma_alloc_noncoherent(mmc_dev(host->mmc),
+					WBSD_DMA_SIZE, &host->dma_addr,
+					DMA_BIDIRECTIONAL,
+					GFP_KERNEL);
 	if (!host->dma_buffer)
 		goto free;
 
-	/*
-	 * Translate the address to a physical address.
-	 */
-	host->dma_addr = dma_map_single(mmc_dev(host->mmc), host->dma_buffer,
-		WBSD_DMA_SIZE, DMA_BIDIRECTIONAL);
-	if (dma_mapping_error(mmc_dev(host->mmc), host->dma_addr))
-		goto kfree;
-
-	/*
-	 * ISA DMA must be aligned on a 64k basis.
-	 */
-	if ((host->dma_addr & 0xffff) != 0)
-		goto unmap;
-	/*
-	 * ISA cannot access memory above 16 MB.
-	 */
-	else if (host->dma_addr >= 0x1000000)
-		goto unmap;
-
 	host->dma = dma;
 
 	return;
 
-unmap:
-	/*
-	 * If we've gotten here then there is some kind of alignment bug
-	 */
-	BUG_ON(1);
-
-	dma_unmap_single(mmc_dev(host->mmc), host->dma_addr,
-		WBSD_DMA_SIZE, DMA_BIDIRECTIONAL);
-	host->dma_addr = 0;
-
-kfree:
-	kfree(host->dma_buffer);
-	host->dma_buffer = NULL;
-
 free:
+	dma_free_noncoherent(mmc_dev(host->mmc), WBSD_DMA_SIZE, host->dma_buffer,
+			     host->dma_addr, DMA_BIDIRECTIONAL);
+	host->dma_buffer = NULL;
 	free_dma(dma);
-
 err:
 	pr_warn(DRIVER_NAME ": Unable to allocate DMA %d - falling back on FIFO\n",
 		dma);