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

[Luben Tuikov <ltuikov@yahoo.com>]

--- 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
>