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
> >> 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.
> >
> Yes, within xhci_map_temp_buffer(), after consolidating the SG list in a temp
> buffer I call dma_map_single() to map the temp buffer.
>
> > Alan Stern
> >
>
> Thanks & Regards,
> Tejas Joglekar
