On Mon, 12 May 2014, Pratyush Anand wrote:
> OTG3 and EH Compliance Plan 1.0 talks about Super Speed OTG Verification
> system (SS-OVS) which consists of an excersizer and analyzer.
>
> USB Compliance Suite from Lecroy can act as such SS-OVS for Link Layer
> Validation (LVS).
>
> Some modifications are needed for an embedded Linux USB host to pass all
> these tests. Most of these tests require just Link to be in U0. They do
> not work with default Linux USB stack since, default stack does port
> reset and then starts sending setup packet, which is not expected by
> Link Layer Validation (LVS) device of Lecroy Compliance Suit. Then,
> There are many Link Layer Tests which need host to generate specific
> traffic.
>
> This patch supports specific traffic generation cases. As of now all the
> host Lecroy Link Layer-USBIF tests (except TD7.26 and TD7.34) passes
> with this patch for single run using Lecroy USB Compliance Suite
> Version 1.98 Build 239 and Lecroy USB Protocol Analyzer version 4.80
> Build 1603. Therefore patch seems to be a good candidate for inclusion.
> Further modification can be done on top of it.
>
> To avoid any port reset and setup packet generation, a simple step
> needed is to unbind the hub driver from root hub device. If 2-0:1.0 is
> the root hub, then execute following to unbind hub driver.
>
> echo 2-0:1.0 > /sys/bus/usb/drivers/hub/unbind
>
> Then bind lvs driver as follows
>
> echo 2-0:1.0 > /sys/bus/usb/drivers/lvs/bind
>
> Now connect LVS device with root hub port.
> Test case specific traffic can be generated as follows whenever needed:
>
> 1. To issue "Get Device descriptor" command for TD.7.06:
> echo 1 > /sys/bus/usb/devices/2-0\:1.0/get_dev_desc
>
> 2. To set U1 timeout to 127 for TD.7.18
> echo 127 > /sys/bus/usb/devices/2-0\:1.0/u1_timeout
>
> 3. To set U2 timeout to 0 for TD.7.18
> echo 0 > /sys/bus/usb/devices/2-0\:1.0/u2_timeout
>
> 4. To issue "Hot Reset" for TD.7.29
> echo 1 > /sys/bus/usb/devices/2-0\:1.0/hot_reset
>
> 5. To issue "U3 Entry" for TD.7.35
> echo 1 > /sys/bus/usb/devices/2-0\:1.0/u3_entry
>
> 6. To issue "U3 Exit" for TD.7.36
> echo 1 > /sys/bus/usb/devices/2-0\:1.0/u3_exit
>
> Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx>
This looks pretty much okay, but it has got a few bugs.
The biggest bug may not be an obvious one. Suppose the lvstest driver
has been built into the kernel. When the kernel boots and the root
hubs are registered, what will prevent them all from binding to lvstest
instead of the normal hub driver?
A similar question arises if lvstest was built as a module, and after
the module is loaded, the user hot-plugs a USB-3 host controller.
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-usb-lvstest
> @@ -0,0 +1,40 @@
> +Link Layer Validation Device is a standard device for testing of Super
> +Speed Link Layer tests. These nodes are available in sysfs only when lvs
> +driver is binded with root hub device.
> +
> +What: /sys/bus/usb/devices/.../get_dev_desc
> +Date: March 2014
> +Contact: Pratyush Anand <pratyush.anand@xxxxxx>
> +Description:
> + Write 1 to this node to issue "Get Device Descriptor"
> + for Link Layer Validation device. It is needed for TD.7.06.
Why does the user have to write "1"? Why not accept any write at all?
The same complaint applies to u3_entry, u3_exit, and hot_reset.
> +What: /sys/bus/usb/devices/.../u1_timeout
> +Date: March 2014
> +Contact: Pratyush Anand <pratyush.anand@xxxxxx>
> +Description:
> + Set "U1 timeout" for the downstream port where Link Layer
> + Validation device is connected. It is needed for TD.7.18,
> + TD.7.19, TD.7.20 and TD.7.21.
> +
> +What: /sys/bus/usb/devices/.../u2_timeout
> +Date: March 2014
> +Contact: Pratyush Anand <pratyush.anand@xxxxxx>
> +Description:
> + Set "U2 timeout" for the downstream port where Link Layer
> + Validation device is connected. It is needed for TD.7.18,
> + TD.7.19, TD.7.20 and TD.7.21.
> +
> +What: /sys/bus/usb/devices/.../hot_reset
> +Date: March 2014
> +Contact: Pratyush Anand <pratyush.anand@xxxxxx>
> +Description:
> + Write 1 to this node to issue "Reset" for Link Layer Validation
> + device. It is needed for TD.7.29, TD.7.31, TD.7.34 and TD.7.35.
> +
> +What: /sys/bus/usb/devices/.../u3_entry
> +Date: March 2014
> +Contact: Pratyush Anand <pratyush.anand@xxxxxx>
> +Description:
> + Write 1 to this node to issue "u3 entry" for Link Layer
> + Validation device. It is needed for TD.7.35 and TD.7.36.
Where's the documentation for u3_exit?
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index a51e7d6..6ef1f81 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -235,3 +235,9 @@ config USB_HSIC_USB3503
> depends on I2C
> help
> This option enables support for SMSC USB3503 HSIC to USB 2.0 Driver.
> +
> +config USB_LINK_LAYER_TEST
> + tristate "USB Link Layer Test driver"
> + help
> + This driver is for generating specific traffic for Super Speed Link
> + Layer Test Device.
Mention that unless the user wants to conduct low-level electrical
tests of USB-3 host controllers, this value should be 'N'.
...
> +static struct usb_device *create_lvs_device(struct usb_interface *intf)
> +{
> + struct usb_device *udev, *hdev;
> + struct usb_hcd *hcd;
> + struct lvs_rh *lvs = usb_get_intfdata(intf);
> +
> + if (!lvs->present) {
> + dev_err(&intf->dev, "No LVS device is present\n");
> + return NULL;
> + }
> +
> + hdev = interface_to_usbdev(intf);
> + hcd = bus_to_hcd(hdev->bus);
> + lvs = usb_get_intfdata(intf);
What is the reason for this line? lvs has already been assigned.
> +
> + udev = usb_alloc_dev(hdev, hdev->bus, lvs->portnum);
> + if (!udev) {
> + dev_err(&intf->dev, "Could not allocate lvs udev\n");
> + return NULL;
> + }
> + udev->speed = USB_SPEED_SUPER;
> + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512);
> + usb_set_device_state(udev, USB_STATE_DEFAULT);
> +
> + if (hcd->driver->enable_device) {
> + if (hcd->driver->enable_device(hcd, udev) < 0) {
> + dev_err(&intf->dev, "Failed to enable\n");
> + kfree(udev);
You need to call usb_put_dev(), not kfree().
> + return NULL;
> + }
> + }
> +
> + return udev;
> +}
> +
> +static void destroy_lvs_device(struct usb_device *udev)
> +{
> + kfree(udev);
This is wrong for two reasons. First, you forgot to call
hcd->driver->free_dev(), and second you called kfree() instead of
usb_put_dev().
> +}
> +
> +static ssize_t issue_u3_entry(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct usb_device *hdev = interface_to_usbdev(intf);
> + struct lvs_rh *lvs = usb_get_intfdata(intf);
> + struct usb_device *udev;
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &val);
> + if (ret < 0) {
> + dev_err(dev, "couldn't parse string %d\n", ret);
> + return ret;
> + }
> +
> + if (val != 1)
> + return -EINVAL;
> +
> + udev = create_lvs_device(intf);
> + if (!udev) {
> + dev_err(dev, "failed to create lvs device\n");
> + return -ENOMEM;
> + }
> +
> + ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> + USB_REQ_SET_FEATURE, USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
> + lvs->portnum, NULL, 0, 1000);
> + if (ret < 0)
> + dev_err(dev, "can't issue U3 entry %d\n", ret);
> +
> + destroy_lvs_device(udev);
> +
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> +static DEVICE_ATTR(u3_entry, S_IWUSR, NULL, issue_u3_entry);
> +
> +static ssize_t issue_u3_exit(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct usb_device *hdev = interface_to_usbdev(intf);
> + struct lvs_rh *lvs = usb_get_intfdata(intf);
> + struct usb_device *udev;
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &val);
> + if (ret < 0) {
> + dev_err(dev, "couldn't parse string %d\n", ret);
> + return ret;
> + }
> +
> + if (val != 1)
> + return -EINVAL;
> +
> + udev = create_lvs_device(intf);
> + if (!udev) {
> + dev_err(dev, "failed to create lvs device\n");
> + return -ENOMEM;
> + }
> +
> + ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> + USB_REQ_CLEAR_FEATURE, USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
> + lvs->portnum, NULL, 0, 1000);
> + if (ret < 0)
> + dev_err(dev, "can't issue U3 entry %d\n", ret);
Don't you mean "U3 exit"?
> +
> + destroy_lvs_device(udev);
> +
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> +static DEVICE_ATTR(u3_exit, S_IWUSR, NULL, issue_u3_exit);
> +
> +static ssize_t issue_hot_reset(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct usb_device *hdev = interface_to_usbdev(intf);
> + struct lvs_rh *lvs = usb_get_intfdata(intf);
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &val);
> + if (ret < 0) {
> + dev_err(dev, "couldn't parse string %d\n", ret);
> + return ret;
> + }
> +
> + if (val != 1)
> + return -EINVAL;
> +
> + ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> + USB_REQ_SET_FEATURE, USB_RT_PORT, USB_PORT_FEAT_RESET,
> + lvs->portnum, NULL, 0, 1000);
> + if (ret < 0) {
> + dev_err(dev, "can't issue hot reset %d\n", ret);
> + return ret;
> + }
> +
> + return count;
> +}
> +static DEVICE_ATTR(hot_reset, S_IWUSR, NULL, issue_hot_reset);
> +
> +static ssize_t issue_u2_timeout(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct usb_device *hdev = interface_to_usbdev(intf);
> + struct lvs_rh *lvs = usb_get_intfdata(intf);
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &val);
> + if (ret < 0) {
> + dev_err(dev, "couldn't parse string %d\n", ret);
> + return ret;
> + }
> +
> + if (val != 0 && val != 127)
> + return -EINVAL;
Why accept only 0 and 127? Why not also accept any value in between?
> +
> + ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> + USB_REQ_SET_FEATURE, USB_RT_PORT, USB_PORT_FEAT_U2_TIMEOUT,
> + lvs->portnum | (((val) & 0xff) << 8), NULL, 0, 1000);
You know that val lies between 0 and 127. Why bother to do "((val) &
0xff)"? And why do you have the extra parentheses around "val"?
> + if (ret < 0) {
> + dev_err(dev, "can't set u2 Timeout %d\n", ret);
Mention val in this error message, so the reader will know what you
were trying to set the U2 timeout to. Also, write "U2" instead of
"u2".
> + return ret;
> + }
> +
> + return count;
> +}
> +static DEVICE_ATTR(u2_timeout, S_IWUSR, NULL, issue_u2_timeout);
> +
> +static ssize_t issue_u1_timeout(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + struct usb_device *hdev = interface_to_usbdev(intf);
> + struct lvs_rh *lvs = usb_get_intfdata(intf);
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &val);
> + if (ret < 0) {
> + dev_err(dev, "couldn't parse string %d\n", ret);
> + return ret;
> + }
> +
> + if (val != 0 && val != 127)
> + return -EINVAL;
Same comments as above for U2.
> +
> + ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> + USB_REQ_SET_FEATURE, USB_RT_PORT, USB_PORT_FEAT_U1_TIMEOUT,
> + lvs->portnum | (((val) & 0xff) << 8), NULL, 0, 1000);
> + if (ret < 0) {
> + dev_err(dev, "can't set U1 Timeout %d\n", ret);
> + return ret;
> + }
> +
> + return count;
> +}
> +static DEVICE_ATTR(u1_timeout, S_IWUSR, NULL, issue_u1_timeout);
> +
> +static ssize_t issue_get_device_descriptor(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct usb_interface *intf = to_usb_interface(dev);
> + unsigned long val;
> + struct usb_device *udev;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &val);
> + if (ret < 0) {
> + dev_err(dev, "couldn't parse string %d\n", ret);
> + return ret;
> + }
> +
> + if (val != 1)
> + return -EINVAL;
> +
> + udev = create_lvs_device(intf);
> + if (!udev) {
> + dev_err(dev, "failed to create lvs device\n");
> + return -ENOMEM;
> + }
> +
> + ret = usb_control_msg(udev, (PIPE_CONTROL << 30) | USB_DIR_IN,
> + USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, USB_DT_DEVICE << 8,
> + 0, &udev->descriptor, sizeof(udev->descriptor),
> + USB_CTRL_GET_TIMEOUT);
An URB's buffer should be separately allocated. Not part of a larger
structure.
> + if (ret < 0)
> + dev_err(dev, "can't read device descriptor %d\n", ret);
> +
> + destroy_lvs_device(udev);
> +
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> +static DEVICE_ATTR(get_dev_desc, S_IWUSR, NULL, issue_get_device_descriptor);
> +
> +static struct attribute *lvs_attributes[] = {
> + &dev_attr_get_dev_desc.attr,
> + &dev_attr_u1_timeout.attr,
> + &dev_attr_u2_timeout.attr,
> + &dev_attr_hot_reset.attr,
> + &dev_attr_u3_entry.attr,
> + &dev_attr_u3_exit.attr,
> + NULL
> +};
> +
> +static const struct attribute_group lvs_attr_group = {
> + .attrs = lvs_attributes,
> +};
> +
> +static void lvs_rh_irq(struct urb *urb)
> +{
> + struct lvs_rh *lvs = urb->context;
> + struct usb_interface *intf = lvs->intf;
> + struct usb_device *hdev = interface_to_usbdev(intf);
> + struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
> + struct usb_hub_descriptor *descriptor = &lvs->descriptor;
> + struct usb_port_status port_status;
> + int i, ret = 0;
> +
> + /* Examine each root port */
> + for (i = 1; i <= descriptor->bNbrPorts; i++) {
> + ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
> + USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, i,
> + &port_status, sizeof(port_status), 1000);
You cannot call usb_control_msg() from within an URB completion
handler, because usb_control_msg() sleeps and completion handlers are
not allowed to sleep.
> + if (ret < 4)
> + continue;
> + /* handle only connection change notification */
> + if (!(port_status.wPortChange & USB_PORT_STAT_C_CONNECTION))
> + continue;
> + usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> + USB_REQ_CLEAR_FEATURE, USB_RT_PORT,
> + USB_PORT_FEAT_C_CONNECTION, i, NULL, 0, 1000);
> +
> + if (port_status.wPortStatus & USB_PORT_STAT_CONNECTION) {
> + lvs->present = true;
> + lvs->portnum = i;
> + if (hcd->phy)
> + usb_phy_notify_connect(hcd->phy,
> + USB_SPEED_SUPER);
> + } else {
> + lvs->present = false;
> + if (hcd->phy)
> + usb_phy_notify_disconnect(hcd->phy,
> + USB_SPEED_SUPER);
> + }
> + goto resubmit;
How about just "break;" instead? Then you could avoid using "goto" and
you could omit the statement label.
> + }
What happens if you exit the loop from the bottom? That is, if no
ports have a connect-change notification?
> +
> +resubmit:
> + ret = usb_submit_urb(lvs->urb, GFP_ATOMIC);
> + if (ret != 0 && ret != -ENODEV && ret != -EPERM)
> + dev_err(&intf->dev, "urb resubmit error %d\n", ret);
> +}
> +
> +static int lvs_rh_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + struct usb_device *hdev;
> + struct usb_host_interface *desc;
> + struct usb_endpoint_descriptor *endpoint;
> + struct lvs_rh *lvs;
> + unsigned int pipe;
> + int ret, maxp;
> +
> + hdev = interface_to_usbdev(intf);
> + desc = intf->cur_altsetting;
> + endpoint = &desc->endpoint[0].desc;
> +
> + /* valid only for SS root hub */
> + if (hdev->descriptor.bDeviceProtocol != USB_HUB_PR_SS || hdev->parent) {
> + dev_err(&intf->dev, "Bind LVS driver with SS root Hub only\n");
> + return -EINVAL;
> + }
> +
> + /* This Root hub endpoint must be interrupt */
> + if (!usb_endpoint_is_int_in(endpoint)) {
> + dev_err(&intf->dev, "Root hub does not have interrupt endpoint\n");
> + return -EINVAL;
> + }
You don't need to test this; it is guaranteed by usbcore.
> +
> + lvs = devm_kzalloc(&intf->dev, sizeof(*lvs), GFP_KERNEL);
> + if (!lvs) {
> + dev_err(&intf->dev, "couldn't allocate lvs_rh struct\n");
> + return -ENOMEM;
> + }
The devm_* routines print out their own error messages if an allocation
fails; you don't need to add your own.
> +
> + lvs->intf = intf;
> + usb_set_intfdata(intf, lvs);
> +
> + /* how many number of ports this root hub has */
> + ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
> + USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> + USB_DT_SS_HUB << 8, 0, &lvs->descriptor,
> + USB_DT_SS_HUB_SIZE, USB_CTRL_GET_TIMEOUT);
> + if (ret < (USB_DT_HUB_NONVAR_SIZE + 2)) {
> + dev_err(&hdev->dev, "wrong root hub descriptor read %d\n", ret);
> + return ret;
> + }
> +
> + /* submit urb to poll interrupt endpoint */
> + lvs->urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!lvs->urb) {
> + dev_err(&intf->dev, "couldn't allocate lvs urb\n");
> + return -ENOMEM;
> + }
> +
> + pipe = usb_rcvintpipe(hdev, endpoint->bEndpointAddress);
> + maxp = usb_maxpacket(hdev, pipe, usb_pipeout(pipe));
> + usb_fill_int_urb(lvs->urb, hdev, pipe, &lvs->buffer[0], maxp,
> + lvs_rh_irq, lvs, endpoint->bInterval);
> +
> + ret = usb_submit_urb(lvs->urb, GFP_NOIO);
> + if (ret < 0) {
> + dev_err(&intf->dev, "couldn't submit lvs urb %d\n", ret);
Here you leak lvs->urb.
> + return ret;
> + }
> +
> + ret = sysfs_create_group(&intf->dev.kobj, &lvs_attr_group);
> + if (ret < 0)
> + dev_err(&intf->dev, "Failed to create sysfs node %d\n", ret);
Here too.
> +
> + return ret;
> +}
> +
> +static void lvs_rh_disconnect(struct usb_interface *intf)
> +{
> + struct lvs_rh *lvs = usb_get_intfdata(intf);
> +
> + sysfs_remove_group(&intf->dev.kobj, &lvs_attr_group);
> + usb_free_urb(lvs->urb);
> + usb_set_intfdata(intf, NULL);
This line isn't needed; the device core automatically clears the driver
data field following an unbind.
> +}
> +
> +static const struct usb_device_id lvs_id_table[] = {
> + { .match_flags = USB_DEVICE_ID_MATCH_DEV_CLASS,
> + .bDeviceClass = USB_CLASS_HUB},
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(usb, lvs_id_table);
> +
> +static struct usb_driver lvs_driver = {
> + .name = "lvs",
> + .probe = lvs_rh_probe,
> + .disconnect = lvs_rh_disconnect,
> + .id_table = lvs_id_table,
> +};
> +
> +static int __init lvs_init(void)
> +{
> + return usb_register(&lvs_driver);
> +}
> +module_init(lvs_init);
> +
> +static void __exit lvs_exit(void)
> +{
> + usb_deregister(&lvs_driver);
> +}
> +module_exit(lvs_exit);
> +
> +MODULE_DESCRIPTION("Link Layer Validation System Driver");
> +MODULE_LICENSE("GPL");
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
