usb: Add support for EHSET

It does sound like a lot of mucking around inside ehci-hcd for a relatively small benefit. But if people want it, I guess it can be added.

From: Manu Gautam <[email protected]>

The USB Embedded High-speed Host Electrical Test (EHSET) defines the SINGLE_STEP_SET_FEATURE test as follows:

1) The host enumerates the test device with VID:0x1A0A, PID:0x0108
2) The host sends the SETUP stage of a GetDescriptor(Device)
3) The device ACKs the request
4) The host issues SOFs for 15 seconds allowing the test operator to raise the scope trigger just above the SOF voltage level
5) The host sends the IN packet
6) The device sends data in response, triggering the scope
7) The host sends an ACK in response to the data

This patch adds additional handling to the EHCI hub driver and allows the EHSET driver to initiate this test mode by issuing a a SetFeature request to the root hub with a Test Selector value of 0x06. From there it mimics ehci_urb_enqueue() but separately submits QTDs for the SETUP and DATA/STATUS stages in order to insert a delay in between.

Signed-off-by Manu Gautam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

Quoted 20 lines · Jack Pham

From: Manu Gautam <[email protected]>

The USB Embedded High-speed Host Electrical Test (EHSET) defines the SINGLE_STEP_SET_FEATURE test as follows:

1) The host enumerates the test device with VID:0x1A0A, PID:0x0108
2) The host sends the SETUP stage of a GetDescriptor(Device)
3) The device ACKs the request
4) The host issues SOFs for 15 seconds allowing the test operator to raise the scope trigger just above the SOF voltage level
5) The host sends the IN packet
6) The device sends data in response, triggering the scope
7) The host sends an ACK in response to the data

This patch adds additional handling to the EHCI hub driver and allows the EHSET driver to initiate this test mode by issuing a a SetFeature request to the root hub with a Test Selector value of 0x06. From there it mimics ehci_urb_enqueue() but separately submits QTDs for the SETUP and DATA/STATUS stages in order to insert a delay in between.

Quoted 2 lines

Signed-off-by Manu Gautam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

Alan, any thoughts about this patch?

Quoted 21 lines · Greg KH

Quoted 20 lines · Jack Pham

From: Manu Gautam <[email protected]>

The USB Embedded High-speed Host Electrical Test (EHSET) defines the SINGLE_STEP_SET_FEATURE test as follows:

1) The host enumerates the test device with VID:0x1A0A, PID:0x0108
2) The host sends the SETUP stage of a GetDescriptor(Device)
3) The device ACKs the request
4) The host issues SOFs for 15 seconds allowing the test operator to raise the scope trigger just above the SOF voltage level
5) The host sends the IN packet
6) The device sends data in response, triggering the scope
7) The host sends an ACK in response to the data

This patch adds additional handling to the EHCI hub driver and allows the EHSET driver to initiate this test mode by issuing a a SetFeature request to the root hub with a Test Selector value of 0x06. From there it mimics ehci_urb_enqueue() but separately submits QTDs for the SETUP and DATA/STATUS stages in order to insert a delay in between.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Manu Gautam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

Alan, any thoughts about this patch?

Sorry, this slipped my mind.

It looks okay. I haven't tested it yet (and it's so specialized that it probably will never receive very much testing). It is somewhat fragile, in that it copies part of usbcore into ehci-hcd; updates to the core will have to be mirrored in the driver.

On the other hand, there's no real reason to reject it, and it could end up helping people who want to test new USB devices. So...

Acked-by Alan Stern <[email protected]>

Quoted 35 lines · Alan Stern

Quoted 21 lines · Greg KH

Quoted 20 lines · Jack Pham

From: Manu Gautam <[email protected]>

The USB Embedded High-speed Host Electrical Test (EHSET) defines the SINGLE_STEP_SET_FEATURE test as follows:

1) The host enumerates the test device with VID:0x1A0A, PID:0x0108
2) The host sends the SETUP stage of a GetDescriptor(Device)
3) The device ACKs the request
4) The host issues SOFs for 15 seconds allowing the test operator to raise the scope trigger just above the SOF voltage level
5) The host sends the IN packet
6) The device sends data in response, triggering the scope
7) The host sends an ACK in response to the data

This patch adds additional handling to the EHCI hub driver and allows the EHSET driver to initiate this test mode by issuing a a SetFeature request to the root hub with a Test Selector value of 0x06. From there it mimics ehci_urb_enqueue() but separately submits QTDs for the SETUP and DATA/STATUS stages in order to insert a delay in between.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Manu Gautam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

Alan, any thoughts about this patch?

Sorry, this slipped my mind.

It looks okay. I haven't tested it yet (and it's so specialized that it probably will never receive very much testing). It is somewhat fragile, in that it copies part of usbcore into ehci-hcd; updates to the core will have to be mirrored in the driver.

Quoted 4 lines

On the other hand, there's no real reason to reject it, and it could end up helping people who want to test new USB devices. So...

Acked-by Alan Stern <[email protected]>

Wait a minute, didn't we discuss a while back that these test features should be built into usbcore so that we could have a usbcv clone for linux ?

Quoted 41 lines · Alan Stern

Quoted 39 lines · Greg KH

Quoted 22 lines · Jack Pham

Quoted 20 lines

From: Manu Gautam <[email protected]>

The USB Embedded High-speed Host Electrical Test (EHSET) defines the SINGLE_STEP_SET_FEATURE test as follows:

1) The host enumerates the test device with VID:0x1A0A, PID:0x0108
2) The host sends the SETUP stage of a GetDescriptor(Device)
3) The device ACKs the request
4) The host issues SOFs for 15 seconds allowing the test operator to raise the scope trigger just above the SOF voltage level
5) The host sends the IN packet
6) The device sends data in response, triggering the scope
7) The host sends an ACK in response to the data

This patch adds additional handling to the EHCI hub driver and allows the EHSET driver to initiate this test mode by issuing a a SetFeature request to the root hub with a Test Selector value of 0x06. From there it mimics ehci_urb_enqueue() but separately submits QTDs for the SETUP and DATA/STATUS stages in order to insert a delay in between.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Manu Gautam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

Alan, any thoughts about this patch?

Sorry, this slipped my mind.

It looks okay. I haven't tested it yet (and it's so specialized that it probably will never receive very much testing). It is somewhat fragile, in that it copies part of usbcore into ehci-hcd; updates to the core will have to be mirrored in the driver.

On the other hand, there's no real reason to reject it, and it could end up helping people who want to test new USB devices. So...

Acked-by Alan Stern <[email protected]>

Quoted 3 lines

Wait a minute, didn't we discuss a while back that these test features should be built into usbcore so that we could have a usbcv clone for linux ?

There's no way this can be built into the core. This test requires the behavior of the host controller to be modified at a low level (introducing a 15-second delay between the Setup and Data stages of a control transfer). Only the HCD can do that.

Besides, doesn't USB-CV install its own version of Windows' EHCI driver while it runs its tests?

Quoted 8 lines

Quoted 3 lines

Wait a minute, didn't we discuss a while back that these test features should be built into usbcore so that we could have a usbcv clone for linux ?

There's no way this can be built into the core. This test requires the behavior of the host controller to be modified at a low level (introducing a 15-second delay between the Setup and Data stages of a control transfer). Only the HCD can do that.

heh, it doesn't need to be entirely in the core. Core could have the generic calls and HCDs could implement some callbacks, but I think quite a bit of the code will be similar if we implement the same thing on all HCDs.

Quoted 2 lines

Besides, doesn't USB-CV install its own version of Windows' EHCI driver while it runs its tests?

IIRC only USB20CV, the new USB30CV (which also tests USB2) doesn't do that.

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

Wait a minute, didn't we discuss a while back that these test features should be built into usbcore so that we could have a usbcv clone for linux ?

There's no way this can be built into the core. This test requires the behavior of the host controller to be modified at a low level (introducing a 15-second delay between the Setup and Data stages of a control transfer). Only the HCD can do that.

Quoted 4 lines

heh, it doesn't need to be entirely in the core. Core could have the generic calls and HCDs could implement some callbacks, but I think quite a bit of the code will be similar if we implement the same thing on all HCDs.

What generic calls and callbacks would you suggest? I assume you want enough to cover not just this one test but the entire USB-CV suite.

Quoted 5 lines

Quoted 2 lines

Besides, doesn't USB-CV install its own version of Windows' EHCI driver while it runs its tests?

IIRC only USB20CV, the new USB30CV (which also tests USB2) doesn't do that.

I don't have more than the foggiest notion of how these things really work under Windows.

Quoted 7 lines

Quoted 4 lines

heh, it doesn't need to be entirely in the core. Core could have the generic calls and HCDs could implement some callbacks, but I think quite a bit of the code will be similar if we implement the same thing on all HCDs.

What generic calls and callbacks would you suggest? I assume you want enough to cover not just this one test but the entire USB-CV suite.

maybe a single callback for supporting 'testmodes' ? which receives the test mode as argument ?

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

heh, it doesn't need to be entirely in the core. Core could have the generic calls and HCDs could implement some callbacks, but I think quite a bit of the code will be similar if we implement the same thing on all HCDs.

What generic calls and callbacks would you suggest? I assume you want enough to cover not just this one test but the entire USB-CV suite.

Quoted 2 lines

maybe a single callback for supporting 'testmodes' ? which receives the test mode as argument ?

I don't have a clear picture of how you would apply such an approach to this case. There would have to be a way to tell the HCD to insert a 15-second delay between the Setup and Data stages of a particular control URB. How would you do that? Whatever method you choose,

implementing it in every HCD would be a huge amount of work.

What other test modes would you want to support?

Is it worth adding this support to the standard host controller drivers, or should there be a special version (a Kconfig option like CONFIG_RCU_TORTURE_TEST) to enable it? Putting a lot of testing code in distribution kernels where it will never be used seems like a big waste.

Quoted 7 lines

Quoted 2 lines

maybe a single callback for supporting 'testmodes' ? which receives the test mode as argument ?

I don't have a clear picture of how you would apply such an approach to this case. There would have to be a way to tell the HCD to insert a 15-second delay between the Setup and Data stages of a particular control URB. How would you do that? Whatever method you choose,

each test is started after enumerating a known Vid/Pid pair. Based on that, you *know* which test to run.

Quoted 1 line

What other test modes would you want to support?

anything that USB[23]0CV supports today. There are even link layer tests for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one of a large(-ish) test suite which needs to be supported.

Quoted 5 lines

Is it worth adding this support to the standard host controller drivers, or should there be a special version (a Kconfig option like CONFIG_RCU_TORTURE_TEST) to enable it? Putting a lot of testing code in distribution kernels where it will never be used seems like a big waste.

right, I think it should be hidden by Kconfig, not arguing against that.

Quoted 10 lines · Felipe Balbi

Quoted 7 lines

Quoted 2 lines

maybe a single callback for supporting 'testmodes' ? which receives the test mode as argument ?

I don't have a clear picture of how you would apply such an approach to this case. There would have to be a way to tell the HCD to insert a 15-second delay between the Setup and Data stages of a particular control URB. How would you do that? Whatever method you choose,

each test is started after enumerating a known Vid/Pid pair. Based on that, you *know* which test to run.

That's not what I meant. Yes, the test-device driver knows what test it wants to run, based on the VID/PID. I was asking how it would communicate this knowledge to the HCD.

For example, it doesn't make sense to have a callback that means "Insert a 15-second delay into the next URB that I submit", because the HCD doesn't know where URBs come from.

Quoted 5 lines

Quoted 1 line

What other test modes would you want to support?

anything that USB[23]0CV supports today. There are even link layer tests for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one of a large(-ish) test suite which needs to be supported.

That's what I'm trying to find out. What are the special features that we would need to implement in order to support the entire test suite?

Quoted 6 lines

Quoted 5 lines

Is it worth adding this support to the standard host controller drivers, or should there be a special version (a Kconfig option like CONFIG_RCU_TORTURE_TEST) to enable it? Putting a lot of testing code in distribution kernels where it will never be used seems like a big waste.

Quoted 1 line

right, I think it should be hidden by Kconfig, not arguing against that.

Therefore we both agree the $SUBJECT patch should not be accepted in its current form. At the very least, the new code in ehci-hcd should be conditional on a CONFIG_USB_HCD_TEST_MODE option. In addition, we may want some of the work (though at this point I'm not still clear on exactly what parts) to be moved into usbcore.

Quoted 7 lines

That's not what I meant. Yes, the test-device driver knows what test it wants to run, based on the VID/PID. I was asking how it would communicate this knowledge to the HCD.

For example, it doesn't make sense to have a callback that means "Insert a 15-second delay into the next URB that I submit", because the HCD doesn't know where URBs come from.

static int ehci_test_mode(struct usb_hcd *hcd, unsigned int test)

{
	struct ehci_hcd *ehci = to_ehci(hcd);

	....

	switch (test) {
	case USB_TEST_SINGLE_STEP_GET_DESC:
		start_test();
		wait_15_seconds();
		finish_test();
		break;
	...

	default:
		return -ENOTSUP;
	}

	return ret;
}

...

static struct hc_driver ehci_hcd_driver = {
	....

	.test_mode	= ehci_test_mode,

	...
};

Quoted 7 lines · Alan Stern

Quoted 3 lines · Felipe Balbi

anything that USB[23]0CV supports today. There are even link layer tests for USB3 and a bunch of others. This SINGLE_STEP_SET_FEATURE is but one of a large(-ish) test suite which needs to be supported.

That's what I'm trying to find out. What are the special features that we would need to implement in order to support the entire test suite?

I haven't looked at USB??CV spec for a while, maybe Jack knows better ?

Quoted 3 lines

Therefore we both agree the $SUBJECT patch should not be accepted in its current form. At the very least, the new code in ehci-hcd should be conditional on a CONFIG_USB_HCD_TEST_MODE option.

Quoted 2 lines

In addition, we may want some of the work (though at this point I'm not still clear on exactly what parts) to be moved into usbcore.

Quoted 2 lines

Quoted 2 lines

In addition, we may want some of the work (though at this point I'm not still clear on exactly what parts) to be moved into usbcore.

+1. I am open to suggestions on how best to achieve this. I have a another patch in the for doing the same 15-second delay in xHCI but am hesitant to submit it just yet, pending ideas on a better way to do it in the common HCD core.

Quoted 7 lines · Jack Pham

Quoted 2 lines

Quoted 2 lines

In addition, we may want some of the work (though at this point I'm not still clear on exactly what parts) to be moved into usbcore.

+1. I am open to suggestions on how best to achieve this. I have a another patch in the for doing the same 15-second delay in xHCI but am hesitant to submit it just yet, pending ideas on a better way to do it in the common HCD core.

Well, here's one possibility, not fully thought out. Suppose there was a way to tell the HCD to carry out _just_ the Setup stage of a control transfer. Or to omit the Setup stage and go directly to the Data stage. Then the SINGLE_STEP_GET_DEVICE_DESCRIPTOR test could be implemented very simply by submitting two URBs with a delay between.

The USB Embedded High-speed Host Electrical Test (EHSET) defines the SINGLE_STEP_SET_FEATURE test as follows:

drivers/usb/host/ehci-hub.c +2 -0

index 2b70277..8e6dc09 100644
+static int ehset_single_step_set_feature(struct usb_hcd *hcd, int port)
+{

From: Manu Gautam <[email protected]>

I don't seem to have a patch 1/2 from you, did it not get sent somehow?

Hi Greg,

Quoted 1 line · Greg KH

I don't seem to have a patch 1/2 from you, did it not get sent somehow?

Here was patch 1/2: http://marc.info/?l=linux-usb&m=137282127113409&w=2
which you've applied to usb-next already as commit 1353aa53. I kept the sequence numbering in the subject as it was in response to the RFC version I originally sent which was the 2/2. Should I resend this as a standalone (i.e. with a revised subject tag) instead?