usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

[Mathias Nyman]

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@linux.intel.com>
> 
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

[Alan Stern]

On Fri, 15 Dec 2017, Felipe Balbi wrote:

> 
> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
> > 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@linux.intel.com>
> >
> > 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).
> 
> wouldn't we get EPIPE in cases where host's and device's data
> toggle/seqNum go out of sync?

No.  When a device receives a packet with the wrong toggle value, it is
supposed to return ACK and then ignore the packet contents (see section
8.6.4 in the USB-2 spec -- the device assumes the packet is a
retransmission of one it already received).  It doesn't return any sort
of error token.

If the device failed to reset its toggle to 0, the effect would be that
it would ignore the data in the next packet.  But since this test
doesn't check whether or not the data gets ignored, it can't tell what
really happened.

> > 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!  :-)
> 
> It's mandatory for devices to support Clear-Halt as means to reset data
> toggle/SeqNum. If such controllers exist, they aren't certifiable IIRC.

That's right.  Nevertheless, plenty of such devices do exist.

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

usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

[Felipe Balbi]

Hi,

Mathias Nyman <mathias.nyman@linux.intel.com> writes:
> 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@linux.intel.com>
>> 
>> 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

that's because we prevent clear halt when endpoint is not halted. That
was changed when a problem was found when enumerating against macOS. It
may be that we're actually hiding an IP bug by doing that, however I
never got an argument strong enough to revert the commit.

This, may just be the strong argument I need :-)

usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

[Felipe Balbi]

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> 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@linux.intel.com>
>
> 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).

wouldn't we get EPIPE in cases where host's and device's data
toggle/seqNum go out of sync?

> 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!  :-)

It's mandatory for devices to support Clear-Halt as means to reset data
toggle/SeqNum. If such controllers exist, they aren't certifiable IIRC.

usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

[Alan Stern]

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@linux.intel.com>

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!  :-)

> ---
>  drivers/usb/misc/usbtest.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index aedc9a7..2c9a15b 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -1710,6 +1710,35 @@ static int test_halt(struct usbtest_dev *tdev, int ep, struct urb *urb)
>  	return 0;
>  }
>  
> +static int test_toggle_sync(struct usbtest_dev *tdev, int ep, struct urb *urb)
> +{
> +	int	retval;
> +
> +	/* clear initial data toggle to DATA0 */
> +	retval = usb_clear_halt(urb->dev, urb->pipe);
> +	if (retval < 0) {
> +		ERROR(tdev, "ep %02x couldn't clear halt, %d\n", ep, retval);
> +		return retval;
> +	}
> +
> +	/* transfer 3 data packets, should be DATA0, DATA1, DATA0 */
> +	retval = simple_io(tdev, urb, 1, 0, 0, __func__);
> +	if (retval != 0)
> +		return -EINVAL;
> +
> +	/* clear halt resets device side data toggle, host should react to it */
> +	retval = usb_clear_halt(urb->dev, urb->pipe);
> +	if (retval < 0) {
> +		ERROR(tdev, "ep %02x couldn't clear halt, %d\n", ep, retval);
> +		return retval;
> +	}
> +
> +	/* host should use DATA0 again after clear halt */
> +	retval = simple_io(tdev, urb, 1, 0, 0, __func__);
> +
> +	return retval;
> +}
> +
>  static int halt_simple(struct usbtest_dev *dev)
>  {
>  	int			ep;
> @@ -1742,6 +1771,36 @@ static int halt_simple(struct usbtest_dev *dev)
>  	return retval;
>  }
>  
> +static int toggle_sync_simple(struct usbtest_dev *dev)
> +{
> +	int			ep;
> +	int			retval = 0;
> +	struct urb		*urb;
> +	struct usb_device	*udev = testdev_to_usbdev(dev);
> +
> +	/*
> +	 * 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.

> +	if (urb == NULL)
> +		return -ENOMEM;
> +
> +	urb->transfer_flags |= URB_ZERO_PACKET;
> +
> +	ep = usb_pipeendpoint(dev->out_pipe);
> +	urb->pipe = dev->out_pipe;
> +	retval = test_toggle_sync(dev, ep, urb);
> +
> +	simple_free_urb(urb);
> +	return retval;
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  
>  /* 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.

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

> +			retval = toggle_sync_simple(dev);
> +
> +		if (retval)
> +			ERROR(dev, "toggle sync failed, iterations left %d\n",
> +			      i);
> +		break;
>  	}
>  	return retval;
>  }

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

usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

[Mathias Nyman]

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@linux.intel.com>
---
 drivers/usb/misc/usbtest.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index aedc9a7..2c9a15b 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -1710,6 +1710,35 @@ static int test_halt(struct usbtest_dev *tdev, int ep, struct urb *urb)
 	return 0;
 }
 
+static int test_toggle_sync(struct usbtest_dev *tdev, int ep, struct urb *urb)
+{
+	int	retval;
+
+	/* clear initial data toggle to DATA0 */
+	retval = usb_clear_halt(urb->dev, urb->pipe);
+	if (retval < 0) {
+		ERROR(tdev, "ep %02x couldn't clear halt, %d\n", ep, retval);
+		return retval;
+	}
+
+	/* transfer 3 data packets, should be DATA0, DATA1, DATA0 */
+	retval = simple_io(tdev, urb, 1, 0, 0, __func__);
+	if (retval != 0)
+		return -EINVAL;
+
+	/* clear halt resets device side data toggle, host should react to it */
+	retval = usb_clear_halt(urb->dev, urb->pipe);
+	if (retval < 0) {
+		ERROR(tdev, "ep %02x couldn't clear halt, %d\n", ep, retval);
+		return retval;
+	}
+
+	/* host should use DATA0 again after clear halt */
+	retval = simple_io(tdev, urb, 1, 0, 0, __func__);
+
+	return retval;
+}
+
 static int halt_simple(struct usbtest_dev *dev)
 {
 	int			ep;
@@ -1742,6 +1771,36 @@ static int halt_simple(struct usbtest_dev *dev)
 	return retval;
 }
 
+static int toggle_sync_simple(struct usbtest_dev *dev)
+{
+	int			ep;
+	int			retval = 0;
+	struct urb		*urb;
+	struct usb_device	*udev = testdev_to_usbdev(dev);
+
+	/*
+	 * 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);
+	if (urb == NULL)
+		return -ENOMEM;
+
+	urb->transfer_flags |= URB_ZERO_PACKET;
+
+	ep = usb_pipeendpoint(dev->out_pipe);
+	urb->pipe = dev->out_pipe;
+	retval = test_toggle_sync(dev, ep, urb);
+
+	simple_free_urb(urb);
+	return retval;
+}
+
 /*-------------------------------------------------------------------------*/
 
 /* 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)
+			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 */)
+			retval = toggle_sync_simple(dev);
+
+		if (retval)
+			ERROR(dev, "toggle sync failed, iterations left %d\n",
+			      i);
+		break;
 	}
 	return retval;
 }