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
