Re: [PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

Re: [PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

[Christoph Hellwig]

On Tue, Apr 15, 2025 at 12:35:59PM +0100, Ian Abbott wrote:
> +			vma->vm_start = start;
> +			vma->vm_end = start + PAGE_SIZE;
> +			retval = dma_mmap_coherent(bm->dma_hw_dev, vma,
> +						   buf->virt_addr,
> +						   buf->dma_addr, PAGE_SIZE);

I'm not  fan of the vm_start/vm_end manipulation, but I've seen it in
other places.  In a perfect world we'd have a dma_mmap_coherent_offset
or similar helper that encapsulates it, and then maybe later replace
that hack with passing on the offset.

> +			if (retval)
> +				break;
> +
> +			start += PAGE_SIZE;
> +		}
> +		vma->vm_start = vm_start;
> +		vma->vm_end = vm_end;
>  	} else {
>  		for (i = 0; i < n_pages; ++i) {
>  			unsigned long pfn;
> @@ -2407,19 +2421,18 @@ static int comedi_mmap(struct file *file, struct vm_area_struct *vma)
>  
>  			start += PAGE_SIZE;
>  		}
> +	}
>  
>  #ifdef CONFIG_MMU
> -		/*
> -		 * Leaving behind a partial mapping of a buffer we're about to
> -		 * drop is unsafe, see remap_pfn_range_notrack().
> -		 * We need to zap the range here ourselves instead of relying
> -		 * on the automatic zapping in remap_pfn_range() because we call
> -		 * remap_pfn_range() in a loop.
> -		 */
> -		if (retval)
> -			zap_vma_ptes(vma, vma->vm_start, size);
> +	/*
> +	 * Leaving behind a partial mapping of a buffer we're about to drop is
> +	 * unsafe, see remap_pfn_range_notrack().  We need to zap the range
> +	 * here ourselves instead of relying on the automatic zapping in
> +	 * remap_pfn_range() because we call remap_pfn_range() in a loop.
> +	 */
> +	if (retval)
> +		zap_vma_ptes(vma, vma->vm_start, size);
>  #endif
> -	}
>  
>  	if (retval == 0) {
>  		vma->vm_ops = &comedi_vm_ops;
> -- 
> 2.47.2
---end quoted text---

Re: [PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

[Marek Szyprowski]

On 28.04.2025 14:56, Christoph Hellwig wrote:
> On Tue, Apr 15, 2025 at 12:35:59PM +0100, Ian Abbott wrote:
>> +			vma->vm_start = start;
>> +			vma->vm_end = start + PAGE_SIZE;
>> +			retval = dma_mmap_coherent(bm->dma_hw_dev, vma,
>> +						   buf->virt_addr,
>> +						   buf->dma_addr, PAGE_SIZE);
> I'm not  fan of the vm_start/vm_end manipulation, but I've seen it in
> other places.  In a perfect world we'd have a dma_mmap_coherent_offset
> or similar helper that encapsulates it, and then maybe later replace
> that hack with passing on the offset.

Indeed the dma_mmap_*() makes too many assumptions about the vma. The 
case You mentioned is probably in drivers/infiniband/hw/hfi1/file_ops.c 
but I also see that the vma->vm_pgoff is being adjusted before most 
dma_mmap_*() calls, which proves that the current API is somehow 
limited. It would be great to fix this too while touching the 
dma_mmap_attrs() API.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 4/4] comedi: allocate DMA coherent buffer as individual pages

[Ian Abbott]

On 28/04/2025 16:33, Marek Szyprowski wrote:
> On 28.04.2025 14:56, Christoph Hellwig wrote:
>> On Tue, Apr 15, 2025 at 12:35:59PM +0100, Ian Abbott wrote:
>>> +			vma->vm_start = start;
>>> +			vma->vm_end = start + PAGE_SIZE;
>>> +			retval = dma_mmap_coherent(bm->dma_hw_dev, vma,
>>> +						   buf->virt_addr,
>>> +						   buf->dma_addr, PAGE_SIZE);
>> I'm not  fan of the vm_start/vm_end manipulation, but I've seen it in
>> other places.  In a perfect world we'd have a dma_mmap_coherent_offset
>> or similar helper that encapsulates it, and then maybe later replace
>> that hack with passing on the offset.
> 
> Indeed the dma_mmap_*() makes too many assumptions about the vma. The
> case You mentioned is probably in drivers/infiniband/hw/hfi1/file_ops.c
> but I also see that the vma->vm_pgoff is being adjusted before most
> dma_mmap_*() calls, which proves that the current API is somehow
> limited. It would be great to fix this too while touching the
> dma_mmap_attrs() API.

Drivers would probably have to continue manipulating vma->vm_pgoff 
anyway if they use its value in a special way, like 
drivers/infiniband/hw/hfil/file_ops.c or drivers/uio/uio.c.  The 
dma_mmap_*() calls already use vma->vm_pgoff as an offset into the VMA 
area, so I think all the new API would need is a parameter to restrict 
the number of pages being mapped, or something similar.

The new API doesn't necessarily have to be reflected all the way down to 
the dma_mmap_direct(), iommu_dma_mmap(), and ops->mmap() functions, as 
the new dma_mmap_*() function could modify vma->vm_end temporarily in 
order to restrict the number of pages being mapped by the lower-level 
functions.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-