Re: [RFC/PATCH v2 3/3] usb: Adding SuperSpeed support to dummy_hcd

[Alan Stern]

On Mon, 18 Oct 2010, Tatyana Brokhman wrote:

> USB 3.0 hub includes 2 hubs - HS and SS ones.
> Thus, when dummy_hcd enabled it will register 2 root hubs (SS and HS).

All of your new kerneldoc comments are invalid.  The patch cannot be
accepted until you fix them.

> @@ -750,16 +865,24 @@ static DEVICE_ATTR (function, S_IRUGO, show_function, NULL);
>  int
>  usb_gadget_register_driver (struct usb_gadget_driver *driver)
>  {
> -	struct dummy	*dum = the_controller;
> +	struct dummy	*dum;
>  	int		retval, i;
> +	enum usb_device_speed	speed = USB_SPEED_UNKNOWN;
> +
> +	if (!driver->bind || !driver->setup
> +			|| driver->speed == USB_SPEED_UNKNOWN)
> +		return -EINVAL;
> +
> +	speed = driver->speed;

What do you need "speed" for?  Can't you use driver->speed?

> +static int dummy_ss_udc_probe(struct platform_device *pdev)
> +{
> +	struct dummy	*dum = the_ss_controller;
> +	int		rc;
> +
> +	dum->gadget.name = gadget_name;
> +	dum->gadget.ops = &dummy_ops;
> +	dum->gadget.is_dualspeed = 1;

Is this setting appropriate?  The "is_dualspeed" field indicates that
the gadget runs at both full speed and high speed.  But that's not what
you want -- you want to indicate that the gadget runs at SuperSpeed.

> @@ -1384,6 +1559,9 @@ static void dummy_timer (unsigned long _dum)
>  	case USB_SPEED_HIGH:
>  		total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
>  		break;
> +	case USB_SPEED_SUPER:
> +		total = 1024/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
> +		break;

I don't know what the transfer parameters for SuperSpeed are, but I do
know that these are wrong.  With over 60000 bytes per uframe there is
certainly room for more than thirteen 1024-byte packets.

> +/**
> + * dummy_address_device() - Assign an address for the connected
> + * device
> + * @param hcd - host controller of the device
> + * @param udev - device to address
> + *
> + * @return int - 0 on success, or an error code (refer to
> + *	   errno-base.h for details)
> + *
> + * Issue an Address Device command (which will issue a
> + * SetAddress request to the device). We should be protected by
> + * the usb_address0_mutex in khubd's hub_port_init, so we should
> + * only issue and wait on one address command at the same time.
> + *
> + * This function is used only for SS hcd drivers.
> + */
> +static int dummy_address_device(struct usb_hcd *hcd, struct usb_device *udev)
> +{
> +	udev->devnum = 3;
> +	return usb_control_msg(udev, (PIPE_CONTROL << 30),
> +		USB_REQ_SET_ADDRESS, 0, udev->devnum, 0,
> +		NULL, 0, USB_CTRL_SET_TIMEOUT);
> +}

This looks very suspicious.  Why have this function if it's only going 
to do the same thing that usbcore would do if it weren't present?

Also, the address_device routine should not change udev->devnum.  The
code that does this in the xhci driver is being removed, because it
causes bugs.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/