[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Sergey Temerkhanov]

Increase xHCI transfer size for USB storage devices. This helps to
achieve 10-20x speedup for large transfers

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>
---

 common/usb_storage.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index b978430..ee5acca 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -97,13 +97,15 @@ struct us_data {
 	trans_cmnd	transport;		/* transport routine */
 };
 
-#ifdef CONFIG_USB_EHCI
+#if defined(CONFIG_USB_EHCI)
 /*
  * The U-Boot EHCI driver can handle any transfer length as long as there is
  * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
  * limited to 65535 blocks.
  */
 #define USB_MAX_XFER_BLK	65535
+#elif defined(CONFIG_USB_XHCI)
+#define USB_MAX_XFER_BLK	4096
 #else
 #define USB_MAX_XFER_BLK	20
 #endif
-- 
2.2.0

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Marek Vasut]

On Thursday, August 13, 2015 at 09:00:04 PM, Sergey Temerkhanov wrote:
> Increase xHCI transfer size for USB storage devices. This helps to
> achieve 10-20x speedup for large transfers
> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>

Hi!

> ---
> 
>  common/usb_storage.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index b978430..ee5acca 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -97,13 +97,15 @@ struct us_data {
>  	trans_cmnd	transport;		/* transport routine */
>  };
> 
> -#ifdef CONFIG_USB_EHCI
> +#if defined(CONFIG_USB_EHCI)
>  /*
>   * The U-Boot EHCI driver can handle any transfer length as long as there
> is * enough free heap space left, but the SCSI READ(10) and WRITE(10)
> commands are * limited to 65535 blocks.
>   */
>  #define USB_MAX_XFER_BLK	65535
> +#elif defined(CONFIG_USB_XHCI)

Why don't you keep the XHCI consistent with EHCI, ie 64k transfers as well ?

> +#define USB_MAX_XFER_BLK	4096
>  #else
>  #define USB_MAX_XFER_BLK	20
>  #endif

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Sergei Temerkhanov]

Tried different values but transfer sizes larger than ~8k blocks never
complete on some controllers causing timeouts and crashes. So, 4k blocks is
a safe enough xfer size

Regards,
Sergey

On Thu, Aug 13, 2015 at 10:09 PM, Marek Vasut <marex@denx.de> wrote:

> On Thursday, August 13, 2015 at 09:00:04 PM, Sergey Temerkhanov wrote:
> > Increase xHCI transfer size for USB storage devices. This helps to
> > achieve 10-20x speedup for large transfers
> >
> > Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> > Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>
>
> Hi!
>
> > ---
> >
> >  common/usb_storage.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/usb_storage.c b/common/usb_storage.c
> > index b978430..ee5acca 100644
> > --- a/common/usb_storage.c
> > +++ b/common/usb_storage.c
> > @@ -97,13 +97,15 @@ struct us_data {
> >       trans_cmnd      transport;              /* transport routine */
> >  };
> >
> > -#ifdef CONFIG_USB_EHCI
> > +#if defined(CONFIG_USB_EHCI)
> >  /*
> >   * The U-Boot EHCI driver can handle any transfer length as long as
> there
> > is * enough free heap space left, but the SCSI READ(10) and WRITE(10)
> > commands are * limited to 65535 blocks.
> >   */
> >  #define USB_MAX_XFER_BLK     65535
> > +#elif defined(CONFIG_USB_XHCI)
>
> Why don't you keep the XHCI consistent with EHCI, ie 64k transfers as well
> ?
>
> > +#define USB_MAX_XFER_BLK     4096
> >  #else
> >  #define USB_MAX_XFER_BLK     20
> >  #endif
>
> Best regards,
> Marek Vasut
>

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Marek Vasut]

On Thursday, August 13, 2015 at 09:13:52 PM, Sergei Temerkhanov wrote:
> Tried different values but transfer sizes larger than ~8k blocks never
> complete on some controllers causing timeouts and crashes. So, 4k blocks is
> a safe enough xfer size

Would you please elaborate on this ?

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Marek Vasut]

On Thursday, August 13, 2015 at 10:56:27 PM, Sergei Temerkhanov wrote:

Hi!

(please stop top-posting ; please keep the list in the loop)

> Well, when I was working on this, setting large transfer sizes resulted in,
> AFAIR, xhci_wait_for_event() timeout and changing XHCI_TIMEOUT doesn't
> help. This function returns NULL which is being dereferenced somewhere else
> (I don't remember where exactly), there are several places in the generic
> xhci support code where this is possible.

I suspect we might need a quirk for the XHCI instead them (quirk that the
controller gets stuck when you try to queue arbitrary number of descriptors).
But I wonder, don't you have some cache problems instead ? Try disabling
dcache (CONFIG_SYS_DCACHE_OFF) on your machine and see if that fixes the
problem .

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Sergei Temerkhanov]

Well, when I was working on this, setting large transfer sizes resulted in,
AFAIR, xhci_wait_for_event() timeout and changing XHCI_TIMEOUT doesn't
help. This function returns NULL which is being dereferenced somewhere else
(I don't remember where exactly), there are several places in the generic
xhci support code where this is possible - I think it was abort_td().

Regards,
Sergey

On Thu, Aug 13, 2015 at 10:34 PM, Marek Vasut <marex@denx.de> wrote:

> On Thursday, August 13, 2015 at 09:13:52 PM, Sergei Temerkhanov wrote:
> > Tried different values but transfer sizes larger than ~8k blocks never
> > complete on some controllers causing timeouts and crashes. So, 4k blocks
> is
> > a safe enough xfer size
>
> Would you please elaborate on this ?
>
> Best regards,
> Marek Vasut
>

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Marek Vasut]

On Friday, August 14, 2015 at 12:48:57 AM, Sergei Temerkhanov wrote:

Hi,

please stop top-posting :(

> Well, when I was working on this, setting large transfer sizes resulted in,
> AFAIR, xhci_wait_for_event() timeout and changing XHCI_TIMEOUT doesn't
> help. This function returns NULL which is being dereferenced somewhere else
> (I don't remember where exactly), there are several places in the generic
> xhci support code where this is possible - I think it was abort_td().

Can you please send fixes for those null-pointer dereferences you're triggering?
Also, I suspect that if your controller cannot support arbitrary lenght of the
descriptors, it might be a controller bug and a quirk should be introduced. Does
the controller work in Linux ?

> Regards,
> Sergey
> 
> On Thu, Aug 13, 2015 at 10:34 PM, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, August 13, 2015 at 09:13:52 PM, Sergei Temerkhanov wrote:
> > > Tried different values but transfer sizes larger than ~8k blocks never
> > > complete on some controllers causing timeouts and crashes. So, 4k
> > > blocks
> > 
> > is
> > 
> > > a safe enough xfer size
> > 
> > Would you please elaborate on this ?
> > 
> > Best regards,
> > Marek Vasut

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Sergei Temerkhanov]

OK, I'll try to find some time for this. I remember trying it with DCACHE
disabled but for no luck

Regards,
Sergey

On Fri, Aug 14, 2015 at 1:42 AM, Marek Vasut <marex@denx.de> wrote:

> On Thursday, August 13, 2015 at 10:56:27 PM, Sergei Temerkhanov wrote:
>
> Hi!
>
> (please stop top-posting ; please keep the list in the loop)
>
> > Well, when I was working on this, setting large transfer sizes resulted
> in,
> > AFAIR, xhci_wait_for_event() timeout and changing XHCI_TIMEOUT doesn't
> > help. This function returns NULL which is being dereferenced somewhere
> else
> > (I don't remember where exactly), there are several places in the generic
> > xhci support code where this is possible.
>
> I suspect we might need a quirk for the XHCI instead them (quirk that the
> controller gets stuck when you try to queue arbitrary number of
> descriptors).
> But I wonder, don't you have some cache problems instead ? Try disabling
> dcache (CONFIG_SYS_DCACHE_OFF) on your machine and see if that fixes the
> problem .
>
> Best regards,
> Marek Vasut
>

[U-Boot] [PATCH] usb_storage: USB storage transfer size increase for xHCI

[Marek Vasut]

On Friday, August 14, 2015 at 02:09:50 PM, Sergei Temerkhanov wrote:
> OK, I'll try to find some time for this. I remember trying it with DCACHE
> disabled but for no luck

Cool, thanks. Btw please stop top-posting ;)

> Regards,
> Sergey
> 
> On Fri, Aug 14, 2015 at 1:42 AM, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, August 13, 2015 at 10:56:27 PM, Sergei Temerkhanov wrote:
> > 
> > Hi!
> > 
> > (please stop top-posting ; please keep the list in the loop)
> > 
> > > Well, when I was working on this, setting large transfer sizes resulted
> > 
> > in,
> > 
> > > AFAIR, xhci_wait_for_event() timeout and changing XHCI_TIMEOUT doesn't
> > > help. This function returns NULL which is being dereferenced somewhere
> > 
> > else
> > 
> > > (I don't remember where exactly), there are several places in the
> > > generic xhci support code where this is possible.
> > 
> > I suspect we might need a quirk for the XHCI instead them (quirk that the
> > controller gets stuck when you try to queue arbitrary number of
> > descriptors).
> > But I wonder, don't you have some cache problems instead ? Try disabling
> > dcache (CONFIG_SYS_DCACHE_OFF) on your machine and see if that fixes the
> > problem .
> > 
> > Best regards,
> > Marek Vasut

Best regards,
Marek Vasut