usb:tools: usb unittests framework

This implementation adds a new proprietary device control requests (to be handled by the dummy_hcd) that initiates a connect/disconnect sequence. The bRequest value of the new control request is 0x52.
It is used by the user-space Unit testing application.

Signed-off-by Tatyana Linder <[email protected]>

---
 drivers/usb/gadget/dummy_hcd.c |   48 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

drivers/usb/gadget/dummy_hcd.c +1 -0

index 74b7655..7d0a6fe 100644
@@ -179,6 +179,7 @@ struct dummy_hcd {
 	unsigned			active:1;
 	unsigned			old_active:1;
 	unsigned			resuming:1;
+	struct work_struct		conn_disc_work;
 };
 
 struct dummy {

drivers/usb/gadget/dummy_hcd.c +17 -0

+ */
+static void dummy_hcd_conn_disc_work(struct work_struct *data)
+{
+	struct dummy_hcd *dum_hcd =
+		container_of(data, struct dummy_hcd, conn_disc_work);
+	int ret_val;
+
+	if (!dum_hcd->dum->driver) {
+		dev_err(dummy_dev(dum_hcd),
+			"dummy_hcd_conn_disc_work called without connected "
+			"device\n");
+		return;
+	}
+
+	dev_info(dummy_dev(dum_hcd), "disconnecting device...\n");
+	ret_val = dummy_pullup(&dum_hcd->dum->gadget, 0);
+	if (ret_val) {

drivers/usb/gadget/dummy_hcd.c +7 -0

+	ret_val = dummy_pullup(&dum_hcd->dum->gadget, 1);
+	if (ret_val)
+		dev_err(dummy_dev(dum_hcd), "dummy_hcd_conn_disc_work:"
+					    " couldn't re-connect device:"
+					    " ret_val=%d\n",
+				ret_val);
+}
 
 /**
  * handle_control_request() - handles all control transfers

Quoted 4 lines · Tatyana Brokhman

This implementation adds a new proprietary device control requests (to be handled by the dummy_hcd) that initiates a connect/disconnect sequence. The bRequest value of the new control request is 0x52.
It is used by the user-space Unit testing application.

This is not a bad idea. On the other hand, it is slightly peculiar -- it you're testing with real UDC hardware, you would do the disconnect/reconnect sequence by hand (unplug and replug the USB cable), not in software.

Hi,

Quoted 11 lines · Alan Stern

Quoted 4 lines · Tatyana Brokhman

This implementation adds a new proprietary device control requests (to be handled by the dummy_hcd) that initiates a connect/disconnect sequence. The bRequest value of the new control request is 0x52.
It is used by the user-space Unit testing application.

This is not a bad idea. On the other hand, it is slightly peculiar -- it you're testing with real UDC hardware, you would do the disconnect/reconnect sequence by hand (unplug and replug the USB cable), not in software.

actually, this is quite useful. Specially for the controllers which _can_ do soft-connect by toggling data pullups. I would rather have these sort of thing maybe in composite.c, so that we can build tests with all gadget drivers/controllers, not only dummy_hcd.

Quoted 15 lines · Felipe Balbi

Hi,

Quoted 11 lines · Alan Stern

Quoted 4 lines · Tatyana Brokhman

This implementation adds a new proprietary device control requests (to be handled by the dummy_hcd) that initiates a connect/disconnect sequence. The bRequest value of the new control request is 0x52.
It is used by the user-space Unit testing application.

This is not a bad idea. On the other hand, it is slightly peculiar -- it you're testing with real UDC hardware, you would do the disconnect/reconnect sequence by hand (unplug and replug the USB cable), not in software.

Quoted 4 lines

actually, this is quite useful. Specially for the controllers which _can_ do soft-connect by toggling data pullups. I would rather have these sort of thing maybe in composite.c, so that we can build tests with all gadget drivers/controllers, not only dummy_hcd.

It sounds like you don't understand the point of this patch. It allows the _host_ to control the pullup settings.

This implementation adds a new proprietary device control requests (to be handled by the dummy_hcd) that initiates a connect/disconnect sequence. The bRequest value of the new control request is 0x52.
It is used by the user-space Unit testing application.

Signed-off-by Tatyana Linder <[email protected]>

drivers/usb/gadget/dummy_hcd.c +26 -0

@@ -1362,6 +1363,43 @@ static struct dummy_ep *find_endpoint (struct dummy *dum, u8 address)
 #define Ep_Request	(USB_TYPE_STANDARD | USB_RECIP_ENDPOINT)
 #define Ep_InRequest	(Ep_Request | USB_DIR_IN)
 
+/**
+ * dummy_hcd_conn_disc_work() - performs a disconnect/connect sequence.
+ * @data: pointer to the sceduled work_struct
+ */
+static void dummy_hcd_conn_disc_work(struct work_struct *data)
+{
+	struct dummy_hcd *dum_hcd =
+		container_of(data, struct dummy_hcd, conn_disc_work);
+	int ret_val;
+
+	if (!dum_hcd->dum->driver) {
+		dev_err(dummy_dev(dum_hcd),
+			"dummy_hcd_conn_disc_work called without connected "
+			"device\n");
+		return;
+	}
+
+	dev_info(dummy_dev(dum_hcd), "disconnecting device...\n");
+	ret_val = dummy_pullup(&dum_hcd->dum->gadget, 0);
+	if (ret_val) {
+		dev_err(dummy_dev(dum_hcd), "dummy_hcd_conn_disc_work:"
+					    " couldn't disconnect device:"
+					    " ret_val=%d\n",
+				ret_val);
+		return;
+	}

This commit adds a new vendor specific request to be handled by the g_zero module in it's sourcesink configuration. The purpose if this request is to update the length of the BULK transfer to a given value. The bRequest value of the new control request is 0x5e.
It is used by the user-space Unit testing application for bulk in/out tests.

Signed-off-by Tatyana Brokhman <[email protected]>

---
 drivers/usb/gadget/f_sourcesink.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

drivers/usb/gadget/f_sourcesink.c +0 -0

index caf2f95..d417b8a 100644

drivers/usb/gadget/f_sourcesink.c +2 -0

 /*-------------------------------------------------------------------------*/
 
 static struct usb_interface_descriptor source_sink_intf = {

@@ -197,6 +198,7 @@ static void
 sourcesink_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	kfree(func_to_ss(f));
+	the_sourcesink = NULL;
 }
 
 /* optionally require specific source/sink data patterns  */

@@ -425,6 +427,7 @@ static int __init sourcesink_bind_config(struct usb_configuration *c)
 	status = usb_add_function(c, &ss->function);
 	if (status)
 		kfree(ss);
+	the_sourcesink = ss;
 	return status;
 }
 

@@ -472,6 +475,15 @@ static int sourcesink_setup(struct usb_configuration *c,
 		value = w_length;
 		break;
 

This is not the actual behavior of the udc, thus it's removed.

This patch is needed for several of UAS test to pass.

Allow user space applications such as LIBUSB, to request streams alloc/dealloc from HCD that implements XHCI.

Signed-off-by Amit Blay <ablay-zC7DfRvBq/JWk0Htik3J/[email protected]>

Signed-off-by Tatyana Brokhman <[email protected]>

include/linux/usbdevice_fs.h +1 -0

index 15591d2..133c216 100644
@@ -108,6 +108,7 @@ struct usbdevfs_urb {
 				  or 0 if none should be sent. */
 	void __user *usercontext;
 	struct usbdevfs_iso_packet_desc iso_frame_desc[0];
+	unsigned int stream_id;
 };
 
 /* ioctls for talking directly to drivers */

Allow user space applications such as LIBUSB, to request streams alloc/dealloc from HCD that implements XHCI.

Signed-off-by Amit Blay <[email protected]>

Signed-off-by Tatyana Brokhman <[email protected]>

Quoted 5 lines · Tatyana Brokhman

Allow user space applications such as LIBUSB, to request streams alloc/dealloc from HCD that implements XHCI.

Signed-off-by Amit Blay <[email protected]>

Signed-off-by Tatyana Brokhman <[email protected]>

other than what Alan said, this needs major splitting. This patch is too big. First we need to discuss the ABI details with the guys at libusb-devel, then we implement.

Quoted 10 lines · Tatyana Brokhman

Quoted 5 lines

Allow user space applications such as LIBUSB, to request streams alloc/dealloc from HCD that implements XHCI.

Signed-off-by Amit Blay <[email protected]>

Signed-off-by Tatyana Brokhman <[email protected]>

other than what Alan said, this needs major splitting. This patch is too big. First we need to discuss the ABI details with the guys at libusb-devel, then we implement.

No, we don't break the abi, but rather, create a new one for streams, and then we can work on adding them to libusb, which should be quite easy when done.

thanks,

greg k-h

My name is Amit Blay, I'm working with Tatyana Brokhman, I originally created this patch. I want to make sure I understand the solution:

We will create a new IOCTL, i.e., USBDEVFS_SUBMITURB_SS. This IOCTL will pass new URB structure, usbdevfs_urb_ss, which will hold the "legacy" URB and in addition the stream_id. libusb will be able to use the new ABI whenever SuperSpeed streams are required.

Is this correct?

Allow user space applications such as LIBUSB, to request streams alloc/dealloc from HCD that implements XHCI.

Signed-off-by Amit Blay <ablay-zC7DfRvBq/JWk0Htik3J/[email protected]>

Signed-off-by Tatyana Brokhman <[email protected]>

---
 drivers/usb/core/devio.c     |  128 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/usbdevice_fs.h |    5 ++
 2 files changed, 132 insertions(+), 1 deletions(-)

drivers/usb/core/devio.c +0 -0

index 37518df..7e73e35 100644

drivers/usb/core/devio.c +34 -0

+{
+	unsigned int ep_map;
+	int ret;
+	int intf_num;
+	struct usb_interface *intf = NULL;
+	const int max_eps = 32;
+	int max_streams = 0;
+	struct usb_host_endpoint *eps[max_eps];
+	int num_eps = 0;
+	int i;
+	unsigned int ep;
+
+	if (get_user(ep_map, (unsigned int __user *)arg))
+		return -EFAULT;
+
+	for (i = 0; i < max_eps; i++) {
+		if (ep_map & (0x1 << i)) {
+			/* Convert from i to ep address */
+			if (i < 16) /* IN EP */
+				ep = i | USB_ENDPOINT_DIR_MASK;
+			else /* OUT EP */
+				ep = (i - 16);
+
+			intf_num = findintfep(ps->dev, ep);
+			if (intf_num < 0)
+				return intf_num;
+			ret = checkintf(ps, intf_num);
+			if (ret)
+				return ret;
+			intf = usb_ifnum_to_if(ps->dev, intf_num);
+			if (!intf)
+				return -ENOENT;
+
+			if (ep & USB_ENDPOINT_DIR_MASK)

Allow user space applications such as LIBUSB, to request streams alloc/dealloc from HCD that implements XHCI.

Signed-off-by Amit Blay <[email protected]>

Signed-off-by Tatyana Brokhman <[email protected]>

---
 drivers/usb/core/devio.c     |  128 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/usbdevice_fs.h |    5 ++
 2 files changed, 132 insertions(+), 1 deletions(-)

drivers/usb/core/devio.c +0 -0

index 37518df..7e73e35 100644

drivers/usb/core/devio.c +34 -0

+{
+	unsigned int ep_map;
+	int ret;
+	int intf_num;
+	struct usb_interface *intf = NULL;
+	const int max_eps = 32;
+	int max_streams = 0;
+	struct usb_host_endpoint *eps[max_eps];
+	int num_eps = 0;
+	int i;
+	unsigned int ep;
+
+	if (get_user(ep_map, (unsigned int __user *)arg))
+		return -EFAULT;
+
+	for (i = 0; i < max_eps; i++) {
+		if (ep_map & (0x1 << i)) {
+			/* Convert from i to ep address */
+			if (i < 16) /* IN EP */
+				ep = i | USB_ENDPOINT_DIR_MASK;
+			else /* OUT EP */
+				ep = (i - 16);
+
+			intf_num = findintfep(ps->dev, ep);
+			if (intf_num < 0)
+				return intf_num;
+			ret = checkintf(ps, intf_num);
+			if (ret)
+				return ret;
+			intf = usb_ifnum_to_if(ps->dev, intf_num);
+			if (!intf)
+				return -ENOENT;
+
+			if (ep & USB_ENDPOINT_DIR_MASK)

Will this patch be used to create a userspace UAS driver, or are you envisioning other ways userspace might use streams? I ask because I can't think of another usage of streams in currently available devices.

It looks like userspace doesn't have a way to specify the number of streams USBFS needs to allocate. Not all userspace applications are

You probably also need a way to communicate back to userspace how many streams were actually allocated. The program could choose to free

So you're just taking the max_streams from the first endpoint that has streams? What if other endpoints have varying numbers of max streams?

What if you have endpoints on an interface that don't support streams?

So it seems like you also need a way for userspace to specify which endpoints get streams, and which endpoints have streams freed. I will

Quoted 2 lines

So it seems like you also need a way for userspace to specify which endpoints get streams, and which endpoints have streams freed. I will

What I have in mind is user space passing a structure holding:

1. Interface number [IN]
2. Bitmap indicating which EP to allocate streams for [IN]

3. Number of streams to allocate, one number [IN}
4. Number of streams actually allocated, one number [OUT]

Quoted 2 lines

1. Interface number [IN]
2. Bitmap indicating which EP to allocate streams for [IN]

If this is a bitmap,

Quoted 2 lines

3. Number of streams to allocate, one number [IN}
4. Number of streams actually allocated, one number [OUT]

how can those be scalars?

Quoted 5 lines

Quoted 2 lines

1. Interface number [IN]
2. Bitmap indicating which EP to allocate streams for [IN]

If this is a bitmap,

Quoted 5 lines

Quoted 2 lines

3. Number of streams to allocate, one number [IN}
4. Number of streams actually allocated, one number [OUT]

how can those be scalars?

The bitmap only indicates which EP the user space requests to allocate streams for. The actual number is taken from #3.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

1. Interface number [IN]
2. Bitmap indicating which EP to allocate streams for [IN]

If this is a bitmap,

Quoted 2 lines

3. Number of streams to allocate, one number [IN}
4. Number of streams actually allocated, one number [OUT]

Quoted 5 lines

Quoted 2 lines

how can those be scalars?

The bitmap only indicates which EP the user space requests to allocate streams for. The actual number is taken from #3.

Exactly. If you specify EP with a bitmap, what do you do if multiple bits are set in the map?

Hi Sarah,

I apologize for the delay responding to the thread. Thanks for your comments. I would like to continue and fix the previous patch according to the comments.
Also, I would like to split this patch, and implement the actual usage of streams for bulk EP transfers (passing the stream ID with the URB) to a different patch.

Quoted 7 lines

It looks like userspace doesn't have a way to specify the number of streams USBFS needs to allocate. Not all userspace applications are

You probably also need a way to communicate back to userspace how many streams were actually allocated. The program could choose to free

Quoted 7 lines

So you're just taking the max_streams from the first endpoint that has streams? What if other endpoints have varying numbers of max streams?

What if you have endpoints on an interface that don't support streams?

So it seems like you also need a way for userspace to specify which endpoints get streams, and which endpoints have streams freed. I will

What I have in mind is user space passing a structure holding:

1. Interface number [IN]
2. Bitmap indicating which EP to allocate streams for [IN]
3. Number of streams to allocate, one number [IN}
4. Number of streams actually allocated, one number [OUT]

The devio alloc function will double check if all EPs belong to the interface passed by the user space.
Then it will call the HCI alloc function with the required number of streams. It will return the number of streams actually allocated by the HCD. Items 3 & 4 can be merged to one IN/OUT parameter.

No error checking will be done in devio for number of required streams (i.e, comparing it with the reported max streams). This kind of error checking is already done by the xHCI driver.

Please let me know if the solution is acceptable.

Thanks,
Amit.

Hi Sarah,

A gentle reminder, can you please take a look at the proposed solution below for stream alloc/dealloc in devio.c? I responded to your comments about the original patch.

Quoted 13 lines · Amit Blay

What I have in mind is user space passing a structure holding:

1. Interface number [IN]
2. Bitmap indicating which EP to allocate streams for [IN]
3. Number of streams to allocate, one number [IN}
4. Number of streams actually allocated, one number [OUT]

The devio alloc function will double check if all EPs belong to the interface passed by the user space.
Then it will call the HCI alloc function with the required number of streams. It will return the number of streams actually allocated by the HCD. Items 3 & 4 can be merged to one IN/OUT parameter.

Quoted 5 lines

No error checking will be done in devio for number of required streams (i.e, comparing it with the reported max streams). This kind of error checking is already done by the xHCI driver.

Please let me know if the solution is acceptable.

I'm looking into implementing USB 3.0 streams support for FreeBSD and would like to have a solution in Linux which is not too far apart, also regarding API's for userspace.

I would suggest overloading the "unsigned int pipe", instead of breaking existing API's by adding a new stream ID value. Also for LibUSB.

./linux/usb.h: unsigned int pipe; /* (in) pipe information */

As per definition there are 15 bits available for "pipe". I think it is not important to support more than 255 streams in the first go, hence I see no real applications that would benefit from that many streams yet.

#define usb_pipeendpoint(pipe) (((pipe) >> 15) & 0xf)

Then I suggest a new function/IOCTL in libusb which can be used to switch on/off streams on a given endpoint. This is something which would need to be done before submitting any URB's on that endpoint. And would be similar to the clear-stall case.

If an URB is submitted on a stream when streams are disabled then it should just fail and vice versa.

Yes, I think this is the correct solution. Sorry for taking so long to get back to you.

Quoted 2 lines

I would suggest overloading the "unsigned int pipe", instead of breaking existing API's by adding a new stream ID value. Also for LibUSB.

I don't see this as a strong argument why we should arbitrarily limit a new API. It's very hard to deprecate kernel to userspace API, so I think we should do it right the first time. There are current applications (like an SSD behind a UAS device) that need as many concurrent streams in flight as possible, so I don't buy the argument that there aren't current applications that need that many streams.

Thanks Sarah, Hans for your comments,

Quoted 2 lines · Sarah Sharp

Yes, I think this is the correct solution. Sorry for taking so long to get back to you.

OK, so I will go ahead with implementing this solution.

Quoted 3 lines · Hans Petter Selasky

Quoted 2 lines

I would suggest overloading the "unsigned int pipe", instead of breaking existing API's by adding a new stream ID value. Also for LibUSB.

The purposed solution will not break any existing API, by doing the following:
a. Adding a new API for streams alloc/dealloc
b. Adding a new API for submitting a URB which is stream capable (I will implement this in a separate patch).

Quoted 6 lines

I don't see this as a strong argument why we should arbitrarily limit a new API. It's very hard to deprecate kernel to userspace API, so I think we should do it right the first time. There are current applications (like an SSD behind a UAS device) that need as many concurrent streams in flight as possible, so I don't buy the argument that there aren't current applications that need that many streams.

I agree with Sarah.
Hans, I appreciate your inputs. Please let me know if you see a good reason why not implementing the new APIs purposed above for streams allocation and usage. Otherwise I will go ahead with the implementation.

Quoted 7 lines

I'm looking into implementing USB 3.0 streams support for FreeBSD and would like to have a solution in Linux which is not too far apart, also regarding API's for userspace.

I would suggest overloading the "unsigned int pipe", instead of breaking existing API's by adding a new stream ID value. Also for LibUSB.

Quoted 2 lines

./linux/usb.h: unsigned int pipe; /* (in) pipe information
*/

I think this gets very close to breaking current API for usbfs. What happens if some application is putting garbage in the upper bits of the pipe?

A libusb application that worked before will break because the USB core will reject the endpoint as not having streams enabled. So I think it's better to have a new IOCTL for submitting URBs to endpoints with streams enabled, as Alan suggested:

http://marc.info/?l=linux-kernel&m=130832352124932&w=2

Quoted 3 lines

As per definition there are 15 bits available for "pipe". I think it is not important to support more than 255 streams in the first go, hence I see no real applications that would benefit from that many streams yet.

I don't see this as a strong argument why we should arbitrarily limit a new API. It's very hard to deprecate kernel to userspace API, so I think we should do it right the first time. There are current applications (like an SSD behind a UAS device) that need as many concurrent streams in flight as possible, so I don't buy the argument that there aren't current applications that need that many streams.

I don't want to limit future applications of streams without a very good reason. I would like to see the new API allow userspace to submit URBs for stream IDs up to the maximum limit specified by the USB 3.0 spec (65,533 streams).

Why do you think reusing the pipe variable would be a better solution? You're going to need a new IOCTL to enable streams anyway, so why not also have a new IOCTL for submitting an URB to an endpoint with streams enabled?

Quoted 7 lines

#define usb_pipeendpoint(pipe) (((pipe) >> 15) & 0xf)

Then I suggest a new function/IOCTL in libusb which can be used to switch on/off streams on a given endpoint. This is something which would need to be done before submitting any URB's on that endpoint. And would be similar to the clear-stall case.

Amit was discussing such an IOCTL. The problem is you just can't turn streams "on and off". The xHCI driver has to have an indication of how many streams your application will need. That's why Amit's proposal included a way for the application to pass in the number of streams to allocate.

Quoted 3 lines

If an URB is submitted on a stream when streams are disabled then it should just fail and vice versa.

That code is already included in the xHCI driver, and usbfs shouldn't do any checking of the stream ID for the URB, since it's already in the lower level driver.

Hi,

Quoted 12 lines · Hans Petter Selasky

Quoted 9 lines

I'm looking into implementing USB 3.0 streams support for FreeBSD and would like to have a solution in Linux which is not too far apart, also regarding API's for userspace.

I would suggest overloading the "unsigned int pipe", instead of breaking existing API's by adding a new stream ID value. Also for LibUSB.

./linux/usb.h: unsigned int pipe; /* (in) pipe information
*/

Quoted 3 lines

I think this gets very close to breaking current API for usbfs. What happens if some application is putting garbage in the upper bits of the pipe?

I think this will never happen, because all USB applications I know about are taught to use a set of macros to create the "pipe" value, which by default zero these bits.

Quoted 4 lines

Why do you think reusing the pipe variable would be a better solution? You're going to need a new IOCTL to enable streams anyway, so why not also have a new IOCTL for submitting an URB to an endpoint with streams enabled?

What I'm thinking is that you should try to integrate the streams functionality as much as possible, with existing API's, so that API's that do roughly the same are not duplicated. In FreeBSD 8/9 we have more free bits in the pipe variable, and I'll probably use that for the stream ID.

It's like
IPv4 and IPv6, that you extend fields when required and not duplicate them. I'm just worried that intel will come up with yet-another way to transfer data in a few years, and then we'll have to wreck API's again.

So why not foresee
that now and make 128-bits available for endpoint/stream/XXX addressing?