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

[Alan Stern <stern@rowland.harvard.edu>]

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.

> Only question here is 
> if it is good to use DMA_MAP_SINGLE flag for the consolidated SG list 
> into a single temporary buffer.

If you call dma_map_single() then USB_DMA_MAP_SINGLE is the appropriate 
flag to sest.

Alan Stern