On Mon, Jul 02, 2012 at 02:43:48PM -0700, Sarah Sharp wrote:
> On Tue, Jun 26, 2012 at 10:17:31AM -0400, Alan Stern wrote:
> > On Mon, 25 Jun 2012, Sarah Sharp wrote:
> > > +static int usb_disable_ltm(struct usb_device *udev);
> > > +static void usb_enable_ltm(struct usb_device *udev);
> >
> > If the function definitions were brought forward, these declarations
> > wouldn't be needed.
>
> Ok. I'll have to add a couple of #ifdef CONFIG_PM, since the USB core
> suspend, LPM, and LTM functionality is all turned off if CONFIG_PM is
> off. It's the same amount of lines either way, but if you just dislike
> forward declarations, I can change it.
Ok, looks like I won't need the extra CONFIG_PM check after all, so
ignore this comment.
> > > @@ -4705,6 +4759,8 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
> > > if (ret) {
> > > dev_err(&udev->dev, "%s Failed to disable LPM\n.", __func__);
> > > mutex_unlock(hcd->bandwidth_mutex);
> > > + /* Re-enable LTM, since the default is disabled after reset. */
> > > + usb_enable_ltm(udev);
> >
> > These enable calls don't belong all over the place in this function.
> > They should occur in exactly one place, in hub_port_reset(), since they
> > are needed after each reset.
The Set Feature LTM_ENABLE request is only valid when the device is in
the Configured state, so I can't push it down into hub_port_reset(). I
can't even push it down to hub_port_init() because that function doesn't
set the configuration.
Actually, the Set and Clear Feature U1/U2 Enable control transfer is
also only defined when the device is in a configured state. That means
this code shouldn't have worked:
for (i = 0; i < SET_CONFIG_TRIES; ++i) {
/* ep0 maxpacket size may change; let the HCD know about it.
* Other endpoints will be handled by re-enumeration. */
usb_ep0_reinit(udev);
ret = hub_port_init(parent_hub, udev, port1, i);
if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV)
break;
}
...
mutex_lock(hcd->bandwidth_mutex);
/* Disable LPM while we reset the device and reinstall the alt settings.
* Device-initiated LPM settings, and system exit latency settings are
* cleared when the device is reset, so we have to set them up again.
*/
ret = usb_disable_lpm(udev);
if (ret) {
dev_err(&udev->dev, "%s Failed to disable LPM\n.", __func__);
mutex_unlock(hcd->bandwidth_mutex);
/* Re-enable LTM, since the default is disabled after reset. */
usb_enable_ltm(udev);
That call will never succeed, since the device is not configured.
goto done;
}
ret = usb_hcd_alloc_bandwidth(udev, udev->actconfig, NULL, NULL);
if (ret < 0) {
dev_warn(&udev->dev,
"Busted HC? Not enough HCD resources for "
"old configuration.\n");
usb_enable_lpm(udev);
mutex_unlock(hcd->bandwidth_mutex);
usb_enable_ltm(udev);
Neither the LTM or LPM enable calls should succeed here either.
goto re_enumerate;
}
ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
USB_REQ_SET_CONFIGURATION, 0,
udev->actconfig->desc.bConfigurationValue, 0,
NULL, 0, USB_CTRL_SET_TIMEOUT);
My suspicion is that I've never hit this error path with the new LPM
patches.
I could change hub_port_reset() to disable LPM and LTM before the port
is reset, if the device is configured. Callers of hub_port_init() will
need to re-enable LPM and LTM once the device is configured.
Igor, this might actually be related to the errors you've seen about U1
Enable failing for your USB 3.0 camera.
Sarah Sharp
--
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
