On Thu, 12 Jan 2012, Sebastian Andrzej Siewior wrote:
> dummy_hcd provides (alloc|free)_stream() callbacks but there are not
> doing anything. The transfer side also lacks matching of streams. This
> patch changes this and implements stream allocation / de-allocation
> support and proper urb <=> req matching.
> The UDC side exposes a limit of 16 streams. DWC3, the only USB3 UDC has
> no limitations in this regard except that it _needs_ to know that
> streams will be used at the ep_enable time. At the host side, there is
> no real limit either: XHCI can allocate any number of streams as long as
> it does not run out of memory. The UAS gadget currently requests 16
> streams and the UAS host side fallbacks from the requested 256 down to
> 16 which is fine.
> From the UASP point of view (the only specified user), the number of
> used streams does not really matter. The only limitation is that the
> host may not use a higher stream than the gadget requested and can deal
> with.
>
> The dummy stream support has been modelled after current UAS + XHCI +
> DWC3 + UASP usage which helps me testing:
> - the device announces that each ep supports 16 streams (even it could
> more than that).
> - the device side looks into Companion descriptor at ep_enable time and
> enables them according to it.
> - the host side tries to enable the requested number of streams but the
> upper limit is the Comanion descriptor. None (zero streams) is an
> error condition, less is okay.
> - The only user in tree (UAS, host side) is enabling/disabling all
> streams in one go. This should be done in one go in dummy as well. If
> it is not done, we lose a counter and a message is printed. If
> _someone_ requires to enable/disable streams in two steps, the driver
> can adjusted. It has been done simplify the checking of this corner
> case (stream number used in URB is greater than requested).
I haven't looked at this terribly carefully, but a few things stand
out.
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -88,6 +88,7 @@ struct dummy_ep {
> unsigned wedged : 1;
> unsigned already_seen : 1;
> unsigned setup_stage : 1;
> + unsigned stream_en;
Should be stream_en:1, right?
> @@ -1058,6 +1070,16 @@ static struct platform_driver dummy_udc_driver = {
>
> /*-------------------------------------------------------------------------*/
>
> +static u32 dummy_get_ep_idx(const struct usb_endpoint_descriptor *desc)
> +{
> + u32 index;
You're doing it again! I really prefer to see either "unsigned" or
"unsigned int" rather than "u32", unless there's some good reason for
the variable to be exactly 32 bits wide. Likewise for the function's
return value.
> @@ -1070,6 +1092,39 @@ static struct platform_driver dummy_udc_driver = {
> * usb 2.0 rules.
> */
>
> +static int dummy_validate_stream(struct usb_hcd *hcd, struct urb *urb)
Tab instead of space?
> +{
> + struct device *dev = hcd->self.controller;
> + struct dummy_hcd *dum_hcd;
> + int ep_idx;
> +
> + if (!usb_endpoint_xfer_bulk(&urb->ep->desc)) {
> + if (urb->stream_id) {
> + dev_err(dev, "Stream id %d on non-BULK ep\n",
> + urb->stream_id);
> + return -EINVAL;
> + }
> + return 0;
> + }
> +
> + dum_hcd = hcd_to_dummy_hcd(hcd);
> + ep_idx = dummy_get_ep_idx(&urb->ep->desc);
> + if (!((1 << ep_idx) & dum_hcd->stream_en_ep)) {
> + if (!urb->stream_id)
> + return 0;
> + }
> +
> + if (urb->stream_id == 0) {
> + dev_err(dev, "Stream id 0 on stream enabled endpoint\n");
> + return -EINVAL;
> + }
> + if (urb->stream_id > dum_hcd->max_stream_num) {
> + dev_err(dev, "Stream id %d is out of range.\n", urb->stream_id);
> + return -EINVAL;
> + }
What happens if the endpoint is bulk but isn't enabled for streams and
urb->stream_id > 0? This is what comes of making the logic more
complicated than it should be. The structure of this routine should be
more like:
enabled = dummy_ep_stream_en(dum_hcd, urb);
if (!urb->stream_id) {
if (enabled)
error;
} else {
if (!enabled || urb->stream_id is out of range)
error;
}
> + return 0;
> +}
> +
> static int dummy_urb_enqueue (
> struct usb_hcd *hcd,
> struct urb *urb,
> @@ -1088,6 +1143,13 @@ static int dummy_urb_enqueue (
>
> dum_hcd = hcd_to_dummy_hcd(hcd);
> spin_lock_irqsave(&dum_hcd->dum->lock, flags);
> +
> + rc = dummy_validate_stream(hcd, urb);
Why not pass dum_hcd as the argument instead of passing hcd and
converting it again?
> + if (rc) {
> + kfree(urbp);
> + goto done;
> + }
> +
> rc = usb_hcd_link_urb_to_ep(hcd, urb);
> if (rc) {
> kfree(urbp);
> @@ -1200,11 +1262,25 @@ static int dummy_perform_transfer(struct urb *urb, struct dummy_request *req,
> return trans;
> }
>
> +static int dummy_ep_stream_en(struct dummy_hcd *dum, struct urb *urb)
Don't misuse the naming scheme. "dum" is always struct dummy * and
"dum_hcd" is always struct dummy_hcd *.
> +{
> + const struct usb_endpoint_descriptor *desc = &urb->ep->desc;
> + u32 index;
> +
> + if (!usb_endpoint_xfer_bulk(desc))
> + return 0;
> +
> + index = dummy_get_ep_idx(desc);
> + if ((1 << index) & dum->stream_en_ep)
> + return 1;
> + return 0;
How about: return (1 << index) & dum_hcd->stream_en_ep;
If this function were defined earlier, you could use it in
dummy_validate_stream.
You didn't modify dummy_timer(). That routine won't allow transfers
for than one URB per endpoint (unless an URB completes). In
particular, it won't allow concurrent transfers on different streams
for the same endpoint. Is that important for your work?
> @@ -2290,11 +2372,45 @@ static int dummy_alloc_streams(struct usb_hcd *hcd, struct usb_device *udev,
> struct usb_host_endpoint **eps, unsigned int num_eps,
> unsigned int num_streams, gfp_t mem_flags)
> {
> - if (hcd->speed != HCD_USB3)
> - dev_dbg(dummy_dev(hcd_to_dummy_hcd(hcd)),
> - "%s() - ERROR! Not supported for USB2.0 roothub\n",
> - __func__);
> - return 0;
Don't you want to retain this behavior?
> + struct dummy_hcd *dum_hcd = hcd_to_dummy_hcd(hcd);
> + unsigned long flags;
> + int max_stream;
> + int ret_streams = num_streams;
> + u32 index;
> + int i;
> +
> + if (!num_eps)
> + return -EINVAL;
> +
> + spin_lock_irqsave(&dum_hcd->dum->lock, flags);
> + if (dum_hcd->stream_en_ep)
> + dev_err(dummy_dev(dum_hcd), "You should enable stream "
> + "capability on all eps in one step.\n");
Why? Why shouldn't the various drivers in a composite gadget enable
their own streams independently? Likewise for deallocating streams.
> +
> + for (i = 0; i < num_eps; i++) {
> + index = dummy_get_ep_idx(&eps[i]->desc);
> + if ((1 << index) & dum_hcd->stream_en_ep) {
> + ret_streams = -EINVAL;
> + goto out;
> + }
> + max_stream = usb_ss_max_streams(&eps[i]->ss_ep_comp);
> + if (max_stream < ret_streams) {
What if max_stream is 0? Shouldn't that be an error?
> + dev_dbg(dummy_dev(dum_hcd), "Ep 0x%x only supports %u "
> + "stream IDs.\n",
> + eps[i]->desc.bEndpointAddress,
> + max_stream);
> + ret_streams = max_stream;
> + }
> + }
> +
> + for (i = 0; i < num_eps; i++) {
> + index = dummy_get_ep_idx(&eps[i]->desc);
> + dum_hcd->stream_en_ep |= 1 << index;
> + }
> + dum_hcd->max_stream_num = ret_streams;
> +out:
> + spin_unlock_irqrestore(&dum_hcd->dum->lock, flags);
> + return ret_streams;
> }
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
