linux-kernel - Re: [RFC/PATCH v2 3/3] usb: Adding SuperSpeed support to dummy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <217d07c2bcc41c06a975cbe7b2465d64.squirrel@www.codeaurora.org>
Date:	Tue, 19 Oct 2010 04:35:27 -0700 (PDT)
From:	tlinder@...eaurora.org
To:	"Alan Stern" <stern@...land.harvard.edu>
Cc:	"Tatyana Brokhman" <tlinder@...eaurora.org>,
	linux-usb@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	"David Brownell" <dbrownell@...rs.sourceforge.net>,
	"Greg Kroah-Hartman" <gregkh@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [RFC/PATCH v2 3/3] usb: Adding SuperSpeed support to dummy_hcd

> 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.

I'll go over them once more.

>
>> @@ -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?

removed.

>
>> +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.

If a device operates in SuperSpeed it supports high and full speeds as
well. If we don't set this flag we will fail for example in ep_matches().
We didn't want to add another flag for is_superspeed. Reusing this flag
suited our current purposes well and doesn't contradict it's meaning.

>
>> @@ -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.

We'll rethink this.

>> +/**
>> + * 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?

Upon new device connection the host addresses the device from
hub_set_address() that if the address_device cb was provided for the hcd -
calls it. This function begins with a verification that either this cb was
supplied or the usbcore already addresses the device (meaning udev->devnum
> 1).
usbcore addresses the device in choose_address() that is called from
hub_port_connect_change but only if it's not a SuperSpeed hcd:
if (!(hcd->driver->flags & HCD_USB3)) {
	/* set the address */
        choose_address(udev);
        ...
}
Since our hcd is SuperSpeed, choose_address() isn't called and
udev->devnum remains 0.
Due to the above in order for this implementation to work properly we have
to provide the address_device cb for a SuperSpeed hcd.
Perhaps this was already fixed but I'm not familiar with such patch. I
would be happy to pick it up if you could refer me to it.

> 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.

A better solution might be to call choose_address() for SuperSpeed hcd as
well. If that sounds good to everyone I can make the change.

> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


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