On Tue, 13 May 2014, Pratyush Anand wrote:
> > 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?
>
> Sorry, I do not have much knowledge of device model, so may be I am
> wrong, but this is what I understand.
>
> Since core comes before misc in makefile, so won't it insure that
> hub_diver is registered before lvs_driver on usb bus?
No. Order in the makefile has no direct connection with driver
probing.
> And if that is
> insured than, would n't a root hub device always binds to the first
> driver and ie hub_driver. Infact, I am working with lvstest driver
> being built into the kernel.
It works only because drivers get probed in the order they are
registered, and the hub driver is registered first. But the probing
order is not guaranteed, as far as I know. You shouldn't rely on it.
> > 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.
>
> I see issue only when both lvstest and hub driver are loaded as
> module, but lvstest is loaded before hub.
It is impossible to load lvstest before the hub driver, because the hub
driver is part of usbcore, which is a prerequisite for lvstest.
> > > --- /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.
>
> No issue in that..can check for any non zero value.
Who cares whether the value is nonzero or not? Accept any value at
all. You should even accept an empty line.
> > > + 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().
>
> and I think also hcd->driver->free_dev(), right?
No, because hcd->driver->enable_device() returned an error.
> So , better to call destroy_lvs_device().
No.
> > > + 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?
>
> Link Layer Test specification asks to set only these two values in
> different tests.
Why stop people from trying values that the LLT doesn't use? They
might want to do their own tests with settings that aren't in the
specs.
> > > + 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.
>
> You mean to use a local memory for descriptor and not to use the one
> which comes as default with udev for device descriptor?
Yes. That's how the hub driver does this; it uses locally-allocated
memory for I/O and then copies the descriptor data into the usb_device
structure.
> > > + /* 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.
>
> Hummmmm...So I need to create a thread like hub_thread. I wanted to avoid that.
> But It seems, I will have to..no other alternative.
You don't need to create a thread; you can use a work queue.
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
