Hi Felipe, thanks for the review!
On Wed, 2019-02-06 at 08:35 +0200, Felipe Balbi wrote:
> Hi,
>
> Nicolas Saenz Julienne <nsaenzjulie...@suse.de> writes:
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 40fa25c4d041..a4efbe62a1a3 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3272,8 +3272,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> > field |= TRB_IOC;
> > more_trbs_coming = false;
> > td->last_trb = ring->enqueue;
> > +
> > + if (xhci_urb_suitable_for_idt(urb)) {
> > + memcpy(&send_addr, urb->transfer_buffer,
> > + trb_buff_len);
> > + field |= TRB_IDT;
> > + }
> > }
> >
> > +
>
> trailing change
Noted
>
> > @@ -3411,6 +3418,12 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> > if (urb->transfer_buffer_length > 0) {
> > u32 length_field, remainder;
> >
> > + if (xhci_urb_suitable_for_idt(urb)) {
> > + memcpy(&urb->transfer_dma, urb->transfer_buffer,
> > + urb->transfer_buffer_length);
> > + field |= TRB_IDT;
> > + }
> > +
> > remainder = xhci_td_remainder(xhci, 0,
> > urb->transfer_buffer_length,
> > urb->transfer_buffer_length,
> > @@ -3420,6 +3433,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> > TRB_INTR_TARGET(0);
> > if (setup->bRequestType & USB_DIR_IN)
> > field |= TRB_DIR_IN;
> > +
>
> trailing change
Noted
>
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 005e65922608..dec62f7f5dc8 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
> >
> > /*-------------------------------------------------------------------------
> > */
> >
> > +/*
> > + * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
> > + * we'll copy the actual data into the TRB address register. This is
> > limited to
> > + * transfers up to 8 bytes on output endpoints of any kind with
> > wMaxPacketSize
> > + * >= 8 bytes.
> > + */
> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > + gfp_t mem_flags)
> > +{
> > + if (xhci_urb_suitable_for_idt(urb))
> > + return 0;
> > +
> > + return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
> > +}
>
> don't you need a matching unmap_urb_for_dma()??
Not really as every DMA mapping sets a matching URB flag to track it. For
example when usb_hcd_map_urb_for_dma() uses dma_map_single() it will set
URB_DMA_MAP_SINGLE in urb->transfer_flags, later on unmap_urb_for_dma() will
catch it and unmap it. As I bypass the mapping altogether there are no
flags set, so unmap_urb_for_dma() won't have any effect.
I could still add it for clarity, and well, I guess it'll save some
instructions on the IDT suitable side.
>
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index 652dc36e3012..9d77b0901ab7 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
> > #define TRB_IOC (1<<5)
> > /* The buffer pointer contains immediate data */
> > #define TRB_IDT (1<<6)
> > +/* TDs smaller than this might use IDT */
>
> Technically, "TDs at most this" since you're 8 itself is an allowed
> size.
>
Noted
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
