On Thursday 08 January 2009, Qiuping Chen wrote:
> +
> +static const char *const ep_name[] = {
> + "ep0-in", "ep0-out",
The naming convention doesn't have "-" before the direction;
after a "-" should always be a type. Except ... that ep0 is
always a control endpoint, so it doesn't need that.
> + ...
> +};
> +
I'm still trying to figure this out ... two different endpoint
objects, one for each side of EP0? That's the wrong model.
Just have one endpoint exposed to the gadget drivers; you can
even have a different usb_ep_ops structure for ep0, to keep
any nasty special cases out of the main code flow.
> +static void iusbc_reinit(struct iusbc *dev)
> +{
> + unsigned i;
> +
> + INIT_LIST_HEAD(&dev->gadget.ep_list);
> +
> + /* ep0-in */
> + dev->gadget.ep0 = &dev->ep[0].ep;
> +
> + ...
OK, so everything will work OK for control IN transfers.
But how about control OUT? I guess that's just a bit of
minor ugliness, letting gadget drivers submit OUT requests
to the IN endpoint as needed...
However ...
> + /* For RNDIS */
> + /* SEND_ENCAPSULATED_COMMAND and GET_ENCAPSULATED_RESPONSE */
> + if ((ctrl.bRequestType == 0x21 && ctrl.bRequest == 0x00)
> + || (ctrl.bRequestType == 0xa1 && ctrl.bRequest == 0x01))
> + goto delegate;
> +
Gaackh!
The fact that you need a special case indicates you're doing something
wrong. In fact, your entire control transfer routine needs to be
scrubbed ... I see stuff like
> + case USB_REQ_SET_DESCRIPTOR:
> + goto stall;
... preventing a driver from using that method number for its own
purposes ...
> + break;
> +
> + case USB_REQ_GET_CONFIGURATION:
> + goto delegate;
> +
> + case USB_REQ_SET_CONFIGURATION:
> + goto delegate;
> +
> + case USB_REQ_GET_INTERFACE:
> + goto delegate;
> +
... wasteful code, trying to enumerate what should be the default
case ("I don't explicitly recognize it as something that needs to
be handled here, so delegate it") ...
> + case USB_REQ_SYNCH_FRAME:
> + goto stall;
... same again, it's completely legit for a class or vendor
request to use that number for one of its methods ...
In short, change that ep0_setup() routine so that it handles
only the stuff it's *got* to handle: standard requests for
endpoint and device features, and interface features for now;
SET_ADDRESS; and maybe some other bits because of hardware quirks.
> + /* For RNDIS */
> + /* CDC: SEND_ENCAPSULATED_COMMAND */
> + if ((ctrl.bRequestType == 0x21)
> + && (ctrl.bRequest == 0x00)) {
> + /* CDC: SEND_ENCAPSULATED_COMMAND */
> + dev_dbg(dbg_dev,
> + "SEND_ENCAPSULATED_COMMAND\n");
> + dev->gadget.ep0 = &dev->ep[1].ep;
> + spin_unlock(&dev->lock);
> + tmp = dev->driver->setup(&dev->gadget, &ctrl);
> + spin_lock(&dev->lock);
> +
Aha! I see now. You really *do* have goofy handling of control
OUT transfers. Which means you aren't going to be able to handle
usbtest case 14 correctly.
Can you fix the EP0 handling?
- Dave
p.s. I'll send my copy of this driver, which has some minor
bits of cleanup -- style etc -- offline.
--
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
