DMA mapping of urb->setup_packet is not necessary for xHCI host
controllers. The xHCI specification says that Setup Stage TRB includes
whole Setup Data; therefore, urb->setup_dma will not be used in the xhci
HCD code.
Signed-off-by: Daewoong Kim <[email protected]>
---
drivers/usb/core/hcd.c | 4 +++-
drivers/usb/host/xhci.c | 1 +
include/linux/usb.h | 4 ++++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index ad5a0f405a75..b1f9eac93f0d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1411,7 +1411,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
if (usb_endpoint_xfer_control(&urb->ep->desc)) {
if (hcd->self.uses_pio_for_control)
return ret;
- if (hcd->localmem_pool) {
+ if (hcd->self.uses_pio_for_setup_pkt) {
+ ; /* do nothing */
+ } else if (hcd->localmem_pool) {
ret = hcd_alloc_coherent(
urb->dev->bus, mem_flags,
&urb->setup_dma,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e86940571b4c..c263aee82dc0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -643,6 +643,7 @@ int xhci_run(struct usb_hcd *hcd)
*/
hcd->uses_new_polling = 1;
+ hcd->self.uses_pio_for_setup_pkt = 1;
if (!usb_hcd_is_primary_hcd(hcd))
return xhci_run_finished(xhci);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7d72c4e0713c..76600e8de414 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -430,6 +430,10 @@ struct usb_bus {
* Does the host controller use PIO
* for control transfers?
*/
+ u8 uses_pio_for_setup_pkt; /*
+ * Does the host controller use PIO
+ * for setup packets?
+ */
u8 otg_port; /* 0, or number of OTG/HNP port */
unsigned is_b_host:1; /* true during some HNP roleswitches */
unsigned b_hnp_enable:1; /* OTG: did A-Host enable HNP? */
--
2.17.1
On 21-01-14 11:59:07, Daewoong Kim wrote:
> DMA mapping of urb->setup_packet is not necessary for xHCI host
> controllers. The xHCI specification says that Setup Stage TRB includes
> whole Setup Data; therefore, urb->setup_dma will not be used in the xhci
> HCD code.
>
How about bypass map/unmap operation for xHCI control transfer directly?
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 91ab81c3fc79..0a0ab14b7638 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1374,7 +1374,8 @@ static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
xhci = hcd_to_xhci(hcd);
- if (xhci_urb_suitable_for_idt(urb))
+ if (xhci_urb_suitable_for_idt(urb) ||
+ (usb_endpoint_xfer_control(&urb->ep->desc)))
return 0;
if (xhci->quirks & XHCI_SG_TRB_CACHE_SIZE_QUIRK) {
@@ -1389,6 +1390,9 @@ static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
struct xhci_hcd *xhci;
bool unmap_temp_buf = false;
+ if (usb_endpoint_xfer_control(&urb->ep->desc))
+ return;
+
xhci = hcd_to_xhci(hcd);
if (urb->num_sgs && (urb->transfer_flags & URB_DMA_MAP_SINGLE))
> Signed-off-by: Daewoong Kim <[email protected]>
> ---
> drivers/usb/core/hcd.c | 4 +++-
> drivers/usb/host/xhci.c | 1 +
> include/linux/usb.h | 4 ++++
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index ad5a0f405a75..b1f9eac93f0d 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1411,7 +1411,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> if (usb_endpoint_xfer_control(&urb->ep->desc)) {
> if (hcd->self.uses_pio_for_control)
> return ret;
> - if (hcd->localmem_pool) {
> + if (hcd->self.uses_pio_for_setup_pkt) {
> + ; /* do nothing */
> + } else if (hcd->localmem_pool) {
> ret = hcd_alloc_coherent(
> urb->dev->bus, mem_flags,
> &urb->setup_dma,
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e86940571b4c..c263aee82dc0 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -643,6 +643,7 @@ int xhci_run(struct usb_hcd *hcd)
> */
>
> hcd->uses_new_polling = 1;
> + hcd->self.uses_pio_for_setup_pkt = 1;
> if (!usb_hcd_is_primary_hcd(hcd))
> return xhci_run_finished(xhci);
>
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 7d72c4e0713c..76600e8de414 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -430,6 +430,10 @@ struct usb_bus {
> * Does the host controller use PIO
> * for control transfers?
> */
> + u8 uses_pio_for_setup_pkt; /*
> + * Does the host controller use PIO
> + * for setup packets?
> + */
> u8 otg_port; /* 0, or number of OTG/HNP port */
> unsigned is_b_host:1; /* true during some HNP roleswitches */
> unsigned b_hnp_enable:1; /* OTG: did A-Host enable HNP? */
> --
> 2.17.1
>
--
Thanks,
Peter Chen
On Thu, Jan 14, 2021 at 01:04:02PM +0800, Peter Chen wrote:
> On 21-01-14 11:59:07, Daewoong Kim wrote:
> > DMA mapping of urb->setup_packet is not necessary for xHCI host
> > controllers. The xHCI specification says that Setup Stage TRB includes
> > whole Setup Data; therefore, urb->setup_dma will not be used in the xhci
> > HCD code.
> >
>
> How about bypass map/unmap operation for xHCI control transfer directly?
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 91ab81c3fc79..0a0ab14b7638 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1374,7 +1374,8 @@ static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>
> xhci = hcd_to_xhci(hcd);
>
> - if (xhci_urb_suitable_for_idt(urb))
> + if (xhci_urb_suitable_for_idt(urb) ||
> + (usb_endpoint_xfer_control(&urb->ep->desc)))
> return 0;
Would this affect the map/unmap operations for the DATA packets in a
control transfer, along with the SETUP packet?
Alan Stern
On 21-01-14 13:00:21, Alan Stern wrote:
> On Thu, Jan 14, 2021 at 01:04:02PM +0800, Peter Chen wrote:
> > On 21-01-14 11:59:07, Daewoong Kim wrote:
> > > DMA mapping of urb->setup_packet is not necessary for xHCI host
> > > controllers. The xHCI specification says that Setup Stage TRB includes
> > > whole Setup Data; therefore, urb->setup_dma will not be used in the xhci
> > > HCD code.
> > >
> >
> > How about bypass map/unmap operation for xHCI control transfer directly?
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 91ab81c3fc79..0a0ab14b7638 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1374,7 +1374,8 @@ static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> >
> > xhci = hcd_to_xhci(hcd);
> >
> > - if (xhci_urb_suitable_for_idt(urb))
> > + if (xhci_urb_suitable_for_idt(urb) ||
> > + (usb_endpoint_xfer_control(&urb->ep->desc)))
> > return 0;
>
> Would this affect the map/unmap operations for the DATA packets in a
> control transfer, along with the SETUP packet?
>
Oh, you are right, Alan. We do need map/unmap operation for DATA
packet in control transfer.
--
Thanks,
Peter Chen
Peter Chen, thanks for your comments.
On Fri, 15 Jan 2021 08:32:51 +0800, Peter Chen wrote:
> On 21-01-14 13:00:21, Alan Stern wrote:
> > On Thu, Jan 14, 2021 at 01:04:02PM +0800, Peter Chen wrote:
> > > On 21-01-14 11:59:07, Daewoong Kim wrote:
> > > > DMA mapping of urb->setup_packet is not necessary for xHCI host
> > > > controllers. The xHCI specification says that Setup Stage TRB includes
> > > > whole Setup Data; therefore, urb->setup_dma will not be used in the xhci
> > > > HCD code.
> > > >
> > >
> > > How about bypass map/unmap operation for xHCI control transfer directly?
> > >
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index 91ab81c3fc79..0a0ab14b7638 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -1374,7 +1374,8 @@ static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > >
> > > xhci = hcd_to_xhci(hcd);
> > >
> > > - if (xhci_urb_suitable_for_idt(urb))
> > > + if (xhci_urb_suitable_for_idt(urb) ||
> > > + (usb_endpoint_xfer_control(&urb->ep->desc)))
> > > return 0;
> >
> > Would this affect the map/unmap operations for the DATA packets in a
> > control transfer, along with the SETUP packet?
> >
>
> Oh, you are right, Alan. We do need map/unmap operation for DATA
> packet in control transfer.
Is it possible to bypass map/unmap operations for MUSB devices' control transfer directly?
If possible, the uses_pio_for_control field in struct usb_bus could be removed.
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index ad5a0f405a75..54061764ffda 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1409,8 +1409,6 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
*/
if (usb_endpoint_xfer_control(&urb->ep->desc)) {
- if (hcd->self.uses_pio_for_control)
- return ret;
if (hcd->localmem_pool) {
ret = hcd_alloc_coherent(
urb->dev->bus, mem_flags,
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 30c5e7de0761..6eaff08cd1e4 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -2624,6 +2624,8 @@ static int musb_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
struct musb *musb = hcd_to_musb(hcd);
int ret;
+ if (usb_endpoint_xfer_control(&urb->ep->desc))
+ return 0;
/*
* The DMA engine in RTL1.8 and above cannot handle
* DMA addresses that are not aligned to a 4 byte boundary.
@@ -2700,7 +2702,6 @@ int musb_host_alloc(struct musb *musb)
return -EINVAL;
*musb->hcd->hcd_priv = (unsigned long) musb;
- musb->hcd->self.uses_pio_for_control = 1;
musb->hcd->uses_new_polling = 1;
musb->hcd->has_tt = 1;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7d72c4e0713c..f4b7d529f2eb 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -426,10 +426,6 @@ struct usb_bus {
struct device *sysdev; /* as seen from firmware or bus */
int busnum; /* Bus number (in order of reg) */
const char *bus_name; /* stable id (PCI slot_name etc) */
- u8 uses_pio_for_control; /*
- * Does the host controller use PIO
- * for control transfers?
- */
u8 otg_port; /* 0, or number of OTG/HNP port */
unsigned is_b_host:1; /* true during some HNP roleswitches */
unsigned b_hnp_enable:1; /* OTG: did A-Host enable HNP? */
--
With Best Regards,
Daewoong Kim