[PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Luben Tuikov]

Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
---
 The long story is that we see some host controllers misreport their
 PSA as they solved 2^v = streams, instead of 2^(v+1) = streams. Thus
 They report that they support 32 streams when in fact they support 16.
 When the device attempts to return status for stream > 15, the host
 says ACK(NumP=0), the device goes in flow control, blah, blah, this
 module parameter allows you to set a max cap on the number of streams
 the driver will ask XHCI HCD to allocate.
 drivers/usb/storage/uasp.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/storage/uasp.c b/drivers/usb/storage/uasp.c
index 3b10efd..e5a26e9 100644
--- a/drivers/usb/storage/uasp.c
+++ b/drivers/usb/storage/uasp.c
@@ -30,6 +30,19 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_tcq.h>
 
+int MaxNumStreams = -1;
+
+module_param(MaxNumStreams, int, 0444);
+MODULE_PARM_DESC(MaxNumStreams, "\n"
+	"\tSome host controllers and/or devices report a larger number of\n"
+	"\tstreams that they in fact support. This parameter allows you\n"
+	"\tto limit the number of streams this driver will request the XHCI\n"
+	"\tHCD to allocate. If set to -1, the default value, then this driver\n"
+	"\twill use the value reported by the attached device. Else the\n"
+	"\tnumber of streams will be limited to the minimum reported by the\n"
+	"\tattached device and this value. Valid values are -1, default,\n"
+	"\tand 1 to 0xFFEF.");
+
 /* Information unit types
  */
 #define IU_CMD   1
@@ -938,6 +951,8 @@ static int uasp_ep_conf(struct uasp_tport_info *tpinfo)
 					 tpinfo->eps[3]->desc.bEndpointAddress);
 
 	if (udev->speed == USB_SPEED_SUPER) {
+		int max_streams;
+
 		for (i = 1; i < 4; i++) {
 			if (tpinfo->max_streams == 0)
 				tpinfo->max_streams = USB_SS_MAX_STREAMS(tpinfo->eps[i]->ss_ep_comp.bmAttributes);
@@ -952,9 +967,13 @@ static int uasp_ep_conf(struct uasp_tport_info *tpinfo)
 		}
 		
 		tpinfo->use_streams = 1;
+		if (1 <= MaxNumStreams && MaxNumStreams <= 0xFFEF)
+			max_streams = min(MaxNumStreams, tpinfo->max_streams);
+		else
+			max_streams = tpinfo->max_streams;
 		tpinfo->num_streams = usb_alloc_streams(iface,
 							&tpinfo->eps[1], 3,
-							tpinfo->max_streams,
+							max_streams,
 							GFP_KERNEL);
 		if (tpinfo->num_streams <= 0) {
 			dev_err(&udev->dev,
-- 
1.7.0.1

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Sarah Sharp]

On Wed, Dec 08, 2010 at 03:55:27PM -0800, Luben Tuikov wrote:
> Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> ---
>  The long story is that we see some host controllers misreport their
>  PSA as they solved 2^v = streams, instead of 2^(v+1) = streams. Thus
>  They report that they support 32 streams when in fact they support 16.
>  When the device attempts to return status for stream > 15, the host
>  says ACK(NumP=0), the device goes in flow control, blah, blah, this
>  module parameter allows you to set a max cap on the number of streams
>  the driver will ask XHCI HCD to allocate.

If this is an issue with a host then the work-around should be in the
xHCI driver instead.  It should be based on the vendor/device ID of the
offending host instead of being a module parameter in the UAS driver.
The only people who benefit from this patch are the people "in the know"
about which hosts are buggy, not normal Linux users.

Streams could be used for things other than UAS in the future, so the
fix should really be in the xHCI host controller driver.  And please put
the information about the host bug in the commit message, so your
private comments don't get lost in the bowels of the linux-usb mailing
list.

In the future, please Cc me on any patches that involve xHCI, or bulk
streams in general.  I'm not an enemy; I frankly don't care which UAS
driver gets in the kernel, I just want one good, well documented driver,
and for the xHCI driver to be as good as possible.

Sarah Sharp


>  drivers/usb/storage/uasp.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/storage/uasp.c b/drivers/usb/storage/uasp.c
> index 3b10efd..e5a26e9 100644
> --- a/drivers/usb/storage/uasp.c
> +++ b/drivers/usb/storage/uasp.c
> @@ -30,6 +30,19 @@
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_tcq.h>
>  
> +int MaxNumStreams = -1;
> +
> +module_param(MaxNumStreams, int, 0444);
> +MODULE_PARM_DESC(MaxNumStreams, "\n"
> +	"\tSome host controllers and/or devices report a larger number of\n"
> +	"\tstreams that they in fact support. This parameter allows you\n"
> +	"\tto limit the number of streams this driver will request the XHCI\n"
> +	"\tHCD to allocate. If set to -1, the default value, then this driver\n"
> +	"\twill use the value reported by the attached device. Else the\n"
> +	"\tnumber of streams will be limited to the minimum reported by the\n"
> +	"\tattached device and this value. Valid values are -1, default,\n"
> +	"\tand 1 to 0xFFEF.");
> +
>  /* Information unit types
>   */
>  #define IU_CMD   1
> @@ -938,6 +951,8 @@ static int uasp_ep_conf(struct uasp_tport_info *tpinfo)
>  					 tpinfo->eps[3]->desc.bEndpointAddress);
>  
>  	if (udev->speed == USB_SPEED_SUPER) {
> +		int max_streams;
> +
>  		for (i = 1; i < 4; i++) {
>  			if (tpinfo->max_streams == 0)
>  				tpinfo->max_streams = USB_SS_MAX_STREAMS(tpinfo->eps[i]->ss_ep_comp.bmAttributes);
> @@ -952,9 +967,13 @@ static int uasp_ep_conf(struct uasp_tport_info *tpinfo)
>  		}
>  		
>  		tpinfo->use_streams = 1;
> +		if (1 <= MaxNumStreams && MaxNumStreams <= 0xFFEF)
> +			max_streams = min(MaxNumStreams, tpinfo->max_streams);
> +		else
> +			max_streams = tpinfo->max_streams;
>  		tpinfo->num_streams = usb_alloc_streams(iface,
>  							&tpinfo->eps[1], 3,
> -							tpinfo->max_streams,
> +							max_streams,
>  							GFP_KERNEL);
>  		if (tpinfo->num_streams <= 0) {
>  			dev_err(&udev->dev,
> -- 
> 1.7.0.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Luben Tuikov]

--- On Thu, 12/9/10, Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:
> -0800, Luben Tuikov wrote:
> > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > ---
> >  The long story is that we see some host
> controllers misreport their
> >  PSA as they solved 2^v = streams, instead of
> 2^(v+1) = streams. Thus
> >  They report that they support 32 streams when in
> fact they support 16.
> >  When the device attempts to return status for
> stream > 15, the host
> >  says ACK(NumP=0), the device goes in flow
> control, blah, blah, this
> >  module parameter allows you to set a max cap on
> the number of streams
> >  the driver will ask XHCI HCD to allocate.
> 
> If this is an issue with a host then the work-around should
> be in the
> xHCI driver instead.  It should be based on the
> vendor/device ID of the
> offending host instead of being a module parameter in the
> UAS driver.
> The only people who benefit from this patch are the people
> "in the know"
> about which hosts are buggy, not normal Linux users.

The OS might want to limit the number of streams for reasons other than a buggy HC. So this module parameter add flexibility other than making a buggy HC work.

(I didn't know you were reading my patches and yet to a "competing" driver of a driver where you're listed as a co-author...)

> Streams could be used for things other than UAS in the
> future, so the
> fix should really be in the xHCI host controller
> driver.  And please put
> the information about the host bug in the commit message,
> so your
> private comments don't get lost in the bowels of the
> linux-usb mailing
> list.

I've an updated HC of this particular make sitting on my desk which I've not had time to test... The HC this was found in is an early version of that make.

> In the future, please Cc me on any patches that involve
> xHCI, or bulk
> streams in general.

In this case, you should probably not add 1 to the number of streams a protocol driver requests, if that number is already a power of 2.  To extend this and make it foolproof, the XHCI should probably know if the number of streams came from a descriptor or the driver made it up (in which case you know more precisely what to do). Now, since you return the number of streams the driver can use, implementing the former is probably just enough.

Also please add a comment as to the return value therein, to the effect that the range of streams the caller can use is [1, the value returned], WLG.

> I'm not an enemy; I frankly don't
> care which UAS
> driver gets in the kernel,

I see.

> I just want one good, well
> documented driver,
> and for the xHCI driver to be as good as possible.

Uh-huh.

> 
> Sarah Sharp
> 
> 
> >  drivers/usb/storage/uasp.c |   21
> ++++++++++++++++++++-
> >  1 files changed, 20 insertions(+), 1
> deletions(-)
> > 
> > diff --git a/drivers/usb/storage/uasp.c
> b/drivers/usb/storage/uasp.c
> > index 3b10efd..e5a26e9 100644
> > --- a/drivers/usb/storage/uasp.c
> > +++ b/drivers/usb/storage/uasp.c
> > @@ -30,6 +30,19 @@
> >  #include <scsi/scsi_host.h>
> >  #include <scsi/scsi_tcq.h>
> >  
> > +int MaxNumStreams = -1;
> > +
> > +module_param(MaxNumStreams, int, 0444);
> > +MODULE_PARM_DESC(MaxNumStreams, "\n"
> > +    "\tSome host controllers and/or
> devices report a larger number of\n"
> > +    "\tstreams that they in fact
> support. This parameter allows you\n"
> > +    "\tto limit the number of streams
> this driver will request the XHCI\n"
> > +    "\tHCD to allocate. If set to -1,
> the default value, then this driver\n"
> > +    "\twill use the value reported by
> the attached device. Else the\n"
> > +    "\tnumber of streams will be
> limited to the minimum reported by the\n"
> > +    "\tattached device and this value.
> Valid values are -1, default,\n"
> > +    "\tand 1 to 0xFFEF.");
> > +
> >  /* Information unit types
> >   */
> >  #define IU_CMD   1
> > @@ -938,6 +951,8 @@ static int uasp_ep_conf(struct
> uasp_tport_info *tpinfo)
> >         
>        
>      tpinfo->eps[3]->desc.bEndpointAddress);
> >  
> >      if (udev->speed ==
> USB_SPEED_SUPER) {
> > +        int
> max_streams;
> > +
> >          for (i =
> 1; i < 4; i++) {
> >         
>     if (tpinfo->max_streams == 0)
> >         
>         tpinfo->max_streams
> =
> USB_SS_MAX_STREAMS(tpinfo->eps[i]->ss_ep_comp.bmAttributes);
> > @@ -952,9 +967,13 @@ static int uasp_ep_conf(struct
> uasp_tport_info *tpinfo)
> >          }
> >          
> >         
> tpinfo->use_streams = 1;
> > +        if (1 <=
> MaxNumStreams && MaxNumStreams <= 0xFFEF)
> > +       
>     max_streams = min(MaxNumStreams,
> tpinfo->max_streams);
> > +        else
> > +       
>     max_streams = tpinfo->max_streams;
> >         
> tpinfo->num_streams = usb_alloc_streams(iface,
> >         
>            
>        
> &tpinfo->eps[1], 3,
> > -       
>            
>        
> tpinfo->max_streams,
> > +       
>            
>         max_streams,
> >         
>            
>         GFP_KERNEL);
> >          if
> (tpinfo->num_streams <= 0) {
> >         
>     dev_err(&udev->dev,
> > -- 
> > 1.7.0.1
> > 
> > 
> > --
> > To unsubscribe from this list: send the line
> "unsubscribe linux-usb" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Sarah Sharp]

On Thu, Dec 09, 2010 at 02:54:19PM -0800, Luben Tuikov wrote:
> --- On Thu, 12/9/10, Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:
> > -0800, Luben Tuikov wrote:
> > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > > ---
> > >  The long story is that we see some host
> > controllers misreport their
> > >  PSA as they solved 2^v = streams, instead of
> > 2^(v+1) = streams. Thus
> > >  They report that they support 32 streams when in
> > fact they support 16.
> > >  When the device attempts to return status for
> > stream > 15, the host
> > >  says ACK(NumP=0), the device goes in flow
> > control, blah, blah, this
> > >  module parameter allows you to set a max cap on
> > the number of streams
> > >  the driver will ask XHCI HCD to allocate.
> > 
> > If this is an issue with a host then the work-around should
> > be in the
> > xHCI driver instead.  It should be based on the
> > vendor/device ID of the
> > offending host instead of being a module parameter in the
> > UAS driver.
> > The only people who benefit from this patch are the people
> > "in the know"
> > about which hosts are buggy, not normal Linux users.
> 
> The OS might want to limit the number of streams for reasons other
> than a buggy HC. So this module parameter add flexibility other than
> making a buggy HC work.

Ok, that's fine, but my comments still stand about fixing this
particular bug in the xHCI driver, rather than in the UAS driver.

> (I didn't know you were reading my patches and yet to a "competing"
> driver of a driver where you're listed as a co-author...)

I always read emails that seem like they might be xHCI related.
Especially when they're a patch.

> > Streams could be used for things other than UAS in the future, so
> > the fix should really be in the xHCI host controller driver.  And
> > please put the information about the host bug in the commit message,
> > so your private comments don't get lost in the bowels of the
> > linux-usb mailing list.
> 
> I've an updated HC of this particular make sitting on my desk which
> I've not had time to test... The HC this was found in is an early
> version of that make.

If the earlier version of the HC is already out in the market, then the
fix needs to get into the xHCI driver, regardless of whether the new
version works.  Otherwise some poor user is going to plug in their
off-the-shelf UAS device into their off-the-shelf host controller and
wonder why it doesn't work.

> > In the future, please Cc me on any patches that involve xHCI, or
> > bulk streams in general.
> 
> In this case, you should probably not add 1 to the number of streams a
> protocol driver requests, if that number is already a power of 2.

Why?  What if the host and endpoints can handle 32 streams, but the
driver requests 16?  Is there a reason the xHCI driver shouldn't
allocate transfer rings for streams 1 through 16?  Something special to
do with powers of two?

There's a deeper design decision here that I never really resolved.
Say a driver asks for 16 streams.  Stream 0 is reserved, and can't be
used after streams are enabled.  The xHCI host controller spec forbids
allocating a transfer ring for stream 0.

Now, what should the contract between the host controller and the driver
be?  Should the xHCI driver allocate stream rings for stream IDs 1
through 15, thereby guaranteeing 15 streams can be used?  Or should it
allocate streams 1 through 16, giving the driver the 16 active streams
it actually wanted?  I chose the latter, which is why I add 1 to the
number of streams the driver requests.

> To extend this and make it foolproof, the XHCI should probably know if
> the number of streams came from a descriptor or the driver made it up
> (in which case you know more precisely what to do). Now, since you
> return the number of streams the driver can use, implementing the
> former is probably just enough.

In either case, I look at the endpoint descriptors that the driver
passes me, and never allocate more streams than those endpoints can
handle.  The driver also never allocates more streams than the host
controller can handle.  All that work is done in
xhci_calculate_streams_and_bitmask().

> Also please add a comment as to the return value therein, to the
> effect that the range of streams the caller can use is [1, the value
> returned], WLG.

It's documented in Documentation/usb/bulk-streams.txt under the "Picking
new Stream IDs to use" section.  But yes, it should probably be
documented in the function comments above usb_alloc_streams().

Sarah Sharp

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Greg KH]

On Thu, Dec 09, 2010 at 04:11:55PM -0800, Sarah Sharp wrote:
> On Thu, Dec 09, 2010 at 02:54:19PM -0800, Luben Tuikov wrote:
> > --- On Thu, 12/9/10, Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:
> > > -0800, Luben Tuikov wrote:
> > > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > > > ---
> > > >  The long story is that we see some host
> > > controllers misreport their
> > > >  PSA as they solved 2^v = streams, instead of
> > > 2^(v+1) = streams. Thus
> > > >  They report that they support 32 streams when in
> > > fact they support 16.
> > > >  When the device attempts to return status for
> > > stream > 15, the host
> > > >  says ACK(NumP=0), the device goes in flow
> > > control, blah, blah, this
> > > >  module parameter allows you to set a max cap on
> > > the number of streams
> > > >  the driver will ask XHCI HCD to allocate.
> > > 
> > > If this is an issue with a host then the work-around should
> > > be in the
> > > xHCI driver instead.  It should be based on the
> > > vendor/device ID of the
> > > offending host instead of being a module parameter in the
> > > UAS driver.
> > > The only people who benefit from this patch are the people
> > > "in the know"
> > > about which hosts are buggy, not normal Linux users.
> > 
> > The OS might want to limit the number of streams for reasons other
> > than a buggy HC. So this module parameter add flexibility other than
> > making a buggy HC work.
> 
> Ok, that's fine, but my comments still stand about fixing this
> particular bug in the xHCI driver, rather than in the UAS driver.

I agree.

Also, a module parameter will not work when your driver is connecting to
different xHCI devices in the same system.  Please never add module
parameters anymore, there is usually never a need for them at all.

thanks,

greg k-h

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Luben Tuikov]

--- On Thu, 12/9/10, Greg KH <greg@kroah.com> wrote:
> -0800, Sarah Sharp wrote:
> > On Thu, Dec 09, 2010 at 02:54:19PM -0800, Luben Tuikov
> wrote:
> > > --- On Thu, 12/9/10, Sarah Sharp <sarah.a.sharp@linux.intel.com>
> wrote:
> > > > -0800, Luben Tuikov wrote:
> > > > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > > > > ---
> > > > >  The long story is that we see some
> host
> > > > controllers misreport their
> > > > >  PSA as they solved 2^v = streams,
> instead of
> > > > 2^(v+1) = streams. Thus
> > > > >  They report that they support 32
> streams when in
> > > > fact they support 16.
> > > > >  When the device attempts to return
> status for
> > > > stream > 15, the host
> > > > >  says ACK(NumP=0), the device goes in
> flow
> > > > control, blah, blah, this
> > > > >  module parameter allows you to set a
> max cap on
> > > > the number of streams
> > > > >  the driver will ask XHCI HCD to
> allocate.
> > > > 
> > > > If this is an issue with a host then the
> work-around should
> > > > be in the
> > > > xHCI driver instead.  It should be based on
> the
> > > > vendor/device ID of the
> > > > offending host instead of being a module
> parameter in the
> > > > UAS driver.
> > > > The only people who benefit from this patch
> are the people
> > > > "in the know"
> > > > about which hosts are buggy, not normal
> Linux users.
> > > 
> > > The OS might want to limit the number of streams
> for reasons other
> > > than a buggy HC. So this module parameter add
> flexibility other than
> > > making a buggy HC work.
> > 
> > Ok, that's fine, but my comments still stand about
> fixing this
> > particular bug in the xHCI driver, rather than in the
> UAS driver.
> 
> I agree.
> 
> Also, a module parameter will not work when your driver is
> connecting to
> different xHCI devices in the same system.  Please
> never add module
> parameters anymore, there is usually never a need for them
> at all.

I hesitated a lot before adding this module parameter. I did not want to add it since of course the obvious solution is to have a "black list" in the XHCI HCD. I think everyone knows this.

But at the time and still in some ways, it seemed like a valuable addition.  Of course, when not set (default behavior) it doesn't impact the operation of the driver.

   Luben

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Alan Cox]

> > The only people who benefit from this patch are the people
> > "in the know"
> > about which hosts are buggy, not normal Linux users.
> 
> The OS might want to limit the number of streams for reasons other than a buggy HC. So this module parameter add flexibility other than making a buggy HC work.

Not of any use to anyone. Any stream limit stuff neees to b done
automatically otherwise it'll be of value to maybe 1 person if that.
It'll also never get tested so will in time rot, produce weird bugs or
show up locking horrors.


Alan

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Luben Tuikov]

--- On Thu, 12/9/10, Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:
> In the future, please Cc me on any patches that involve
> xHCI, or bulk
> streams in general.  I'm not an enemy; I frankly don't
> care which UAS
> driver gets in the kernel, I just want one good, well
> documented driver,
> and for the xHCI driver to be as good as possible.

Then do the noble thing and retract the still-born uas.c where you're listed as a co-author.

    Luben

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Greg KH]

On Fri, Dec 10, 2010 at 03:02:18PM -0800, Luben Tuikov wrote:
> --- On Thu, 12/9/10, Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote:
> > In the future, please Cc me on any patches that involve
> > xHCI, or bulk
> > streams in general.  I'm not an enemy; I frankly don't
> > care which UAS
> > driver gets in the kernel, I just want one good, well
> > documented driver,
> > and for the xHCI driver to be as good as possible.
> 
> Then do the noble thing and retract the still-born uas.c where you're
> listed as a co-author.

That's just plain rude and obnoxious.

You just earned a place in my kill-file.

*plonk*

Re: [PATCH 2/4] [USB] UASP: Add MaxNumStreams module parameter

[Luben Tuikov]

--- On Fri, 12/10/10, Greg KH <greg@kroah.com> wrote:
> 
> That's just plain rude and obnoxious.
> 
> You just earned a place in my kill-file.
> 
> *plonk*

But, Greg, it's what I would've done. If there is a better solution out there, let it be, instead of leading on this charade of your protecting someone else's interests.