Re: [PATCH] usb: usbtest: Add TEST 29, toggle sync, Clear toggle between bulk writes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux