On 14.12.2017 20:12, Alan Stern wrote:
On Thu, 14 Dec 2017, Mathias Nyman wrote:
Clear Feature Endpoint Halt should reset the data toggle even if the
endpoint isn't halted. Host should manage to clear the host side data
toggle to keep in sync with the device.
Test by sending a "3 data packet URB" before and after clearing the halt.
this should create a toggle sequence with two consecutive DATA0 packets.
A successful test sequence looks like this
ClearFeature(ENDPOINT_HALT) - initial toggle clear
DATA0 (max packet sized)
DATA1 (max packet sized)
DATA0 (zero length packet)
ClearFeature(ENDPOINT_HALT) - resets toggle
DATA0 (max packet sized), if clear halt fails then toggle is DATA1
DATA1 (max packet sized)
DATA0 (zero length packet)
Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
This test is a little unusual in that it doesn't contain a way to
detect failures. That is, you can't tell from the test results whether
the device and the host behaved the way they are supposed to (in the
case of the host, you could use a USB analyzer to find out).
Also, some devices don't handle Clear-Halt requests properly if the
endpoint isn't already halted. Presumably people wouldn't use one of
those devices for this test! :-)
I was hoping that the device (dwc3 with zero gadget in my case) would somehow react
if the host sends a DATA1 packet right after ClearFeature(ENDPOINT_HALT), but turns out
this device accepts it and continues to work just fine. So I ended up looking at the
traffic with an analyzer just as you said.
This case has been really useful while testing a xhci host side toggle reset
implementation for the case where a Clear-Halt request is issued for non-halted endpoints.
+
+ /*
+ * Create a URB that causes a transfer of uneven amount of data packets
+ * This way the clear toggle has an impact on the data toggle sequence.
+ * Use 2 max packet length packes and one zero packet.
+ */
+
+ if (udev->speed == USB_SPEED_SUPER)
+ urb = simple_alloc_urb(udev, 0, 2048, 0);
+ else
+ urb = simple_alloc_urb(udev, 0, 1024, 0);
Just use 2 * usb_endpoint_maxp(ep). No conditional, no funny
constants. And it will do what you want even for full-speed devices.
Yes, thanks, that is a lot better.
/* Control OUT tests use the vendor control requests from Intel's
@@ -2524,6 +2583,20 @@ usbtest_do_ioctl(struct usb_interface *intf, struct usbtest_param_32 *param)
retval = test_queue(dev, param,
dev->in_pipe, NULL, 0);
break;
+ /* Test data Toggle/seq_nr clear between bulk out transfers */
+ case 29:
+ if (dev->out_pipe == 0 && dev->in_pipe == 0)
Why check dev->in_pipe? The test only uses dev->out_pipe.
Copy-paste mistake. This was originally a hacked test 13 before turning it to a new test.
I'll fix it
+ break;
+ retval = 0;
+ dev_info(&intf->dev, "TEST 29: Clear toggle between bulk writes %d times\n",
+ param->iterations);
+ for (i = param->iterations; retval == 0 && i--; /* NOP */)
This is a little contorted. I would write:
for (i = 0; retval == 0 && i < param->iterations; ++i)
Yes, looks better.
Original code was again copied from test 13
There is however a small benefit in counting down when printing the "iterations left"
message below.
I'll clean it up a bit
+ retval = toggle_sync_simple(dev);
+
+ if (retval)
+ ERROR(dev, "toggle sync failed, iterations left %d\n",
+ i);
+ break;
}
return retval;
}
Alan Stern
Thanks for the review and comments
-Mathias
--
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
