| Message ID | 20190614134726.3827-13-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [01/16] media: videobuf-dma-contig: use dma_mmap_coherent | expand |
On Fri, Jun 14, 2019 at 03:47:22PM +0200, Christoph Hellwig wrote: > comedi_buf.c abuse the DMA API in gravely broken ways, as it assumes it > can call virt_to_page on the result, and the just remap it as uncached > using vmap. Disable the driver until this API abuse has been fixed. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/staging/comedi/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig > index 049b659fa6ad..e7c021d76cfa 100644 > --- a/drivers/staging/comedi/Kconfig > +++ b/drivers/staging/comedi/Kconfig > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > config COMEDI > tristate "Data acquisition support (comedi)" > + depends on BROKEN Um, that's a huge sledgehammer. Perhaps a hint as to how we can fix this up? This is the first time I've heard of the comedi code not handling dma properly. thanks, greg k-h
On Fri, Jun 14, 2019 at 04:02:39PM +0200, Greg KH wrote: > Perhaps a hint as to how we can fix this up? This is the first time > I've heard of the comedi code not handling dma properly. It can be fixed by: a) never calling virt_to_page (or vmalloc_to_page for that matter) on dma allocation b) never remapping dma allocation with conflicting cache modes (no remapping should be doable after a) anyway).
On Fri, Jun 14, 2019 at 04:48:57PM +0200, Christoph Hellwig wrote: > On Fri, Jun 14, 2019 at 04:02:39PM +0200, Greg KH wrote: > > Perhaps a hint as to how we can fix this up? This is the first time > > I've heard of the comedi code not handling dma properly. > > It can be fixed by: > > a) never calling virt_to_page (or vmalloc_to_page for that matter) > on dma allocation > b) never remapping dma allocation with conflicting cache modes > (no remapping should be doable after a) anyway). Ok, fair enough, have any pointers of drivers/core code that does this correctly? I can put it on my todo list, but might take a week or so... Ian, want to look into doing this sooner? thanks, greg k-h
On Fri, Jun 14, 2019 at 05:30:32PM +0200, Greg KH wrote: > On Fri, Jun 14, 2019 at 04:48:57PM +0200, Christoph Hellwig wrote: > > On Fri, Jun 14, 2019 at 04:02:39PM +0200, Greg KH wrote: > > > Perhaps a hint as to how we can fix this up? This is the first time > > > I've heard of the comedi code not handling dma properly. > > > > It can be fixed by: > > > > a) never calling virt_to_page (or vmalloc_to_page for that matter) > > on dma allocation > > b) never remapping dma allocation with conflicting cache modes > > (no remapping should be doable after a) anyway). > > Ok, fair enough, have any pointers of drivers/core code that does this > correctly? I can put it on my todo list, but might take a week or so... Just about everyone else. They just need to remove the vmap and either do one large allocation, or live with the fact that they need helpers to access multiple array elements instead of one net vmap, which most of the users already seem to do anyway, with just a few using the vmap (which might explain why we didn't see blowups yet).
On 14/06/2019 16:34, Christoph Hellwig wrote: > On Fri, Jun 14, 2019 at 05:30:32PM +0200, Greg KH wrote: >> On Fri, Jun 14, 2019 at 04:48:57PM +0200, Christoph Hellwig wrote: >>> On Fri, Jun 14, 2019 at 04:02:39PM +0200, Greg KH wrote: >>>> Perhaps a hint as to how we can fix this up? This is the first time >>>> I've heard of the comedi code not handling dma properly. >>> >>> It can be fixed by: >>> >>> a) never calling virt_to_page (or vmalloc_to_page for that matter) >>> on dma allocation >>> b) never remapping dma allocation with conflicting cache modes >>> (no remapping should be doable after a) anyway). >> >> Ok, fair enough, have any pointers of drivers/core code that does this >> correctly? I can put it on my todo list, but might take a week or so... > > Just about everyone else. They just need to remove the vmap and > either do one large allocation, or live with the fact that they need > helpers to access multiple array elements instead of one net vmap, > which most of the users already seem to do anyway, with just a few > using the vmap (which might explain why we didn't see blowups yet). Avoiding the vmap in comedi should be do-able as it already has other means to get at the buffer pages. When comedi makes the buffer from DMA coherent memory, it currently allocates it as a series of page-sized chunks. That cannot be mmap'ed in one go with dma_mmap_coherent(), so I see the following solutions. 1. Change the buffer allocation to allocate a single chunk of DMA coherent memory and use dma_mmap_coherent() to mmap it. 2. Call dma_mmap_coherent() in a loop, adjusting vma->vm_start and vma->vm_end for each iteration (vma->vm_pgoff will be 0), and restoring the vma->vm_start and vma->vm_end at the end. I'm not sure if 2 is a legal option.
diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig index 049b659fa6ad..e7c021d76cfa 100644 --- a/drivers/staging/comedi/Kconfig +++ b/drivers/staging/comedi/Kconfig @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 config COMEDI tristate "Data acquisition support (comedi)" + depends on BROKEN help Enable support for a wide range of data acquisition devices for Linux.
comedi_buf.c abuse the DMA API in gravely broken ways, as it assumes it can call virt_to_page on the result, and the just remap it as uncached using vmap. Disable the driver until this API abuse has been fixed. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/staging/comedi/Kconfig | 1 + 1 file changed, 1 insertion(+)