On Wed, Feb 4, 2009 at 7:10 AM, David Brownell <david-b@xxxxxxxxxxx> wrote:
> 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.
>
Yes, we have two different endpoint for EP0 input and output,
according to HW design.
> 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
