Re: [PATCH 2/4] usb: usbfs: Use consistent mmap functions

[David Hildenbrand]

On 10.05.23 17:41, Ruihan Li wrote:
> Hi Alan,
>
> On Wed, May 10, 2023 at 10:38:48AM -0400, Alan Stern wrote:
> > On Wed, May 10, 2023 at 04:55:25PM +0800, Ruihan Li wrote:
> > > When hcd->localmem_pool is non-null, it is used to allocate DMA memory.
> > > In this case, the dma address will be properly returned (in dma_handle),
> > > and dma_mmap_coherent should be used to map this memory into the user
> > > space. However, the current implementation uses pfn_remap_range, which
> > > is supposed to map normal pages (instead of DMA pages).
> > >
> > > Instead of repeating the logic in the memory allocation function, this
> > > patch introduces a more robust solution. To address the previous issue,
> > > this patch checks the type of allocated memory by testing whether
> > > dma_handle is properly set. If dma_handle is properly returned, it means
> > > some DMA pages are allocated and dma_mmap_coherent should be used to map
> > > them. Otherwise, normal pages are allocated and pfn_remap_range should
> > > be called. This ensures that the correct mmap functions are used
> > > consistently, independently with logic details that determine which type
> > > of memory gets allocated.
> > >
> > > Fixes: a0e710a7def4 ("USB: usbfs: fix mmap dma mismatch")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Ruihan Li <lrh2000@xxxxxxxxxx>
> > > ---
> > >   drivers/usb/core/devio.c | 10 ++++++++--
> > >   1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > index b4cf9e860..5067030b7 100644
> > > --- a/drivers/usb/core/devio.c
> > > +++ b/drivers/usb/core/devio.c
> > > @@ -235,7 +235,7 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> > >   	size_t size = vma->vm_end - vma->vm_start;
> > >   	void *mem;
> > >   	unsigned long flags;
> > > -	dma_addr_t dma_handle;
> > > +	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
> > >   	int ret;
> > >   
> > >   	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> > > @@ -265,7 +265,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> > >   	usbm->vma_use_count = 1;
> > >   	INIT_LIST_HEAD(&usbm->memlist);
> > >   
> > > -	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
> > > +	/* In DMA-unavailable cases, hcd_buffer_alloc_pages allocates
> > > +	 * normal pages and assigns DMA_MAPPING_ERROR to dma_handle. Check
> > > +	 * whether we are in such cases, and then use remap_pfn_range (or
> > > +	 * dma_mmap_coherent) to map normal (or DMA) pages into the user
> > > +	 * space, respectively.
> > > +	 */
> >
> > Another stylistic issue.  For multi-line comments, the format we use is:
> >
> > 	/*
> > 	 * Blah, blah, blah
> > 	 * Blah, blah, blah
> > 	 */
> >
> > Alan Stern
>
> Yeah, I am pretty sure it is another style difference with the net
> subsystem. Again, in the next version, I'll follow the coding style that
> you have pointed out.

Documentation/process/coding-style.rst

spells out that net/ and drivers/net/ are "special".

Regarding breaking long lines, it's just an inconsistent, undocumented 
mess IIRC ...

--
Thanks,

David / dhildenb