* Alan Stern | 2012-01-12 13:36:15 [-0500]:
>I haven't looked at this terribly carefully, but a few things stand
>out.
Okay.
>> --- 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?
Sorry, yes.
>
>> @@ -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.
Sorry. This from the time where I had index shifted. I changed it to
unsigned int and keep stream_en_ep as u32 since I need at least 30 bits.
>> @@ -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?
will change. Don't know where it is coming from but vim shows this tab
as a single 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;
> }
Updated.
>> + 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?
I have convert it anyway: Either to dummy_hcd for normal using or to
usb_hcd for the dev_err(). So now I pass it directly and use
dummy_dev(dum_hcd) for the dev_err().
>> + 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 *.
fixed.
>> +{
>> + 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;
This could be done.
>If this function were defined earlier, you could use it in
>dummy_validate_stream.
done.
>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?
Hmm. So if one URB spans over two usb_requests I can't continue the
transfer until the URB completes. The way streams are working in UAS it
does not matter because one stream does not depend on the other so it
won't stall the other streams forever.
Do you suggest removing '->already_seen'?
>> @@ -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?
Why should do something that has been done in usb_alloc_streams()?
>> + 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.
This was just to make the max_stream id check easy. It has been changed.
>> +
>> + 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?
yes, it is. I wrote it in the patch description that I did it but forgot
to do so. Updated.
>Alan Stern
Sebastian
--
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
