Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <48a47f2e-0506-ca0f-07d5-15918865cd19@redhat.com>
Date:   Wed, 10 May 2023 18:34:15 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH 2/4] usb: usbfs: Use consistent mmap functions
Content-Language: en-US
To:     Ruihan Li <lrh2000@pku.edu.cn>,
        Alan Stern <stern@rowland.harvard.edu>
Cc:     linux-mm@kvack.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Christoph Hellwig <hch@infradead.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        stable@vger.kernel.org
References: <20230510085527.57953-1-lrh2000@pku.edu.cn>
 <20230510085527.57953-3-lrh2000@pku.edu.cn>
 <e197f549-0ee7-446e-86af-ac173d047df5@rowland.harvard.edu>
 <w6keszmqdkwsuw5k3dsyl67zgndorxsoeenysjyzlzf5v4p6bl@mvztdsgt7qjj>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <w6keszmqdkwsuw5k3dsyl67zgndorxsoeenysjyzlzf5v4p6bl@mvztdsgt7qjj>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@vger.kernel.org
>>> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
>>> ---
>>>   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