Re: [RFC PATCH v2 4/4] usb: xhci: Use temporary buffer to consolidate SG

[Tejas Joglekar <Tejas.Joglekar@synopsys.com>]

Hi,
On 5/22/2020 7:51 PM, Alan Stern wrote:
> On Fri, May 22, 2020 at 01:58:43AM +0000, Tejas Joglekar wrote:
>> Hi,
>> On 5/20/2020 11:14 PM, Alan Stern wrote:
>>> On Wed, May 20, 2020 at 05:00:50PM +0000, Tejas Joglekar wrote:
>>>> Hi,
>>>> On 5/20/2020 10:20 PM, Alan Stern wrote:
>>>>> On Wed, May 20, 2020 at 06:41:16AM +0000, Tejas Joglekar wrote:
>>>>>>>> +	urb->transfer_buffer = temp;
>>>>>>>> +	urb->transfer_dma = dma_map_single(hcd->self.sysdev,
>>>>>>>> +					   urb->transfer_buffer,
>>>>>>>> +					   urb->transfer_buffer_length,
>>>>>>>> +					   dir);
>>>>>>>> +
>>>>>>>> +	if (dma_mapping_error(hcd->self.sysdev,
>>>>>>>> +			      urb->transfer_dma)) {
>>>>>>>> +		ret = -EAGAIN;
>>>>>>>> +		kfree(temp);
>>>>>>>> +	} else {
>>>>>>>> +		urb->transfer_flags |= URB_DMA_MAP_SINGLE;
>>>>>>>
>>>>>>> Not sure if using URB_DMA_MAP_SINGLE to flag that this urb is boucebuffered is
>>>>>>> appropriate.
>>>>>>>
>>>>>>> If Greg or Alan don't object then it's fine for me as well.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> @Greg/Alan do you suggest any change for the flag here?
>>>>>
>>>>> This requires care, because the core will already have set other flags 
>>>>> for this URB.  I don't remember the details and I don't have time to 
>>>>> check them now.  But it wouldn't be at all surprising if the core 
>>>>> doesn't expect an URB both to use SG and to use DMA_MAP_SINGLE.
>>>>>
>>>> When this quirk is enabled and it is required to consolidate SG list to 
>>>> single buffer no other flags are set for the URB.
>>>
>>> I don't believe that.  What about URB_DMA_MAP_SG or 
>>> URB_DMA_MAP_PAGE?  The USB core certainly would have set one of them 
>>> already.  Not only would you need to clear that flag, you want also need 
>>> to undo the mapping.
>>>
>> URB_DMA_MAP_SG or URB_DMA_MAP_PAGE flags are set for an URB in core within
>> function usb_hcd_map_urb_for_dma(), with my patch the xhci_map_urb_for_dma()
>> does not call usb_hcd_map_urb_for_dma() when SG list consolidation is required
>> but it calls the newly added function xhci_map_temp_buffer() within that function
>> only dma_map_single() is called to map consolidated SG list.
> 
> Ah, in your patch xhci-hcd's map_urb_for_dma() routine bypasses 
> usb_hcd_map_urb_for_dma().  I hadn't noticed that.
> 
> Then yes, your use of the URB flags is okay.
> 
> Alan Stern
> 
Thanks for the review Alan
@Mathias: I will send v3 after updating the unmap function.

Thanks & Regards,
Tejas Joglekar