| Message ID | 20220219005221.634-22-bhe@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Don't use kmalloc() with GFP_DMA | expand |
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.
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.
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.
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?
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.
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 --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);