On Fri, Jan 01, 2010 at 11:09:23AM -0500, Alan Stern wrote:
> On Thu, 31 Dec 2009, Borislav Petkov wrote:
>
> > Here's what I got with a new version of the dbg patch (both it and new
> > dmesg attached):
> >
> > [ 427.681810] Restarting tasks ...
> > [ 427.681995] hub 1-0:1.0: state 7 ports 6 chg 0004 evt 0000
> > [ 427.682019] usb usb3: usb resume
> > [ 427.682030] ohci_hcd 0000:00:12.0: wakeup root hub
> > [ 427.682191] hub 1-0:1.0: port 2, status 0501, change 0000, 480 Mb/s
> > [ 427.682205] usb 1-2: usb wakeup-resume
> > [ 427.682226] usb 1-2: finish reset-resume
> > [ 427.682886] done.
> > [ 427.734658] ehci_hcd 0000:00:12.2: port 2 high speed
> > [ 427.734663] ehci_hcd 0000:00:12.2: GetStatus port 2 status 001005 POWER sig=se0 PE CONNECT
> > [ 427.746682] hub 3-0:1.0: hub_reset_resume
> > [ 427.746693] hub 3-0:1.0: trying to enable port power on non-switchable hub
> > [ 427.786715] usb 1-2: reset high speed USB device using ehci_hcd and address 2
> > [ 427.839653] ehci_hcd 0000:00:12.2: port 2 high speed
> > [ 427.839666] ehci_hcd 0000:00:12.2: GetStatus port 2 status 001005 POWER sig=se0 PE CONNECT
> > [ 427.847717] ohci_hcd 0000:00:12.0: GetStatus roothub.portstatus [1] = 0x00010100 CSC PPS
> > [ 427.915497] hub 1-2:1.0: remove_intf_ep_devs: if: ffff88022f9e8800 ->ep_devs_created: 1
> > [ 427.915774] hub 1-2:1.0: remove_intf_ep_devs: bNumEndpoints: 1
> > [ 427.915934] hub 1-2:1.0: if: ffff88022f9e8800: endpoint devs removed.
> > [ 427.916158] hub 1-2:1.0: create_intf_ep_devs: if: ffff88022f9e8800 ->ep_devs_created: 0, ->unregistering: 0
> > [ 427.916434] hub 1-2:1.0: create_intf_ep_devs: bNumEndpoints: 1
> > [ 427.916609] ep_81: create, parent hub
> > [ 427.916632] ------------[ cut here ]------------
> > [ 427.916644] WARNING: at fs/sysfs/dir.c:477 sysfs_add_one+0x82/0x96()
> > [ 427.916649] Hardware name: System Product Name
> > [ 427.916653] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:12.2/usb1/1-2/1-2:1.0/ep_81'
> > [ 427.916658] Modules linked in: binfmt_misc kvm_amd kvm powernow_k8 cpufreq_ondemand cpufreq_powersave cpufreq_userspace freq_table cpufreq_conservative ipv6 vfat fat 8250_pnp 8250 pcspkr ohci_hcd serial_core k10temp edac_core
> > [ 427.916694] Pid: 278, comm: khubd Not tainted 2.6.33-rc2-00187-g08d869a-dirty #13
> > [ 427.916699] Call Trace:
> >
> >
> > So, it looks like this ep_81 is already there when create_intf_ep_devs()
> > is being called. I've added debug calls to usb_create_ep_devs() and
> > usb_remove_ep_devs() but the ep_81 thing is never being removed after
> > being created 2 secs into the boot:
>
> Right. I have figured out the cause of the problem; it is Sarah
> Sharp's commit 3f0479e00a3fca9590ae8d9edc4e9c47b7fa0610 (USB: Check
> bandwidth when switching alt settings).
>
> > [ 2.177389] hub 1-2:1.0: remove_intf_ep_devs: if: ffff88022f9e8800 ->ep_devs_created: 0
> > [ 2.177633] hub 1-2:1.0: create_intf_ep_devs: if: ffff88022f9e8800 ->ep_devs_created: 0, ->unregistering: 0
> > [ 2.177877] hub 1-2:1.0: create_intf_ep_devs: bNumEndpoints: 1
> > [ 2.178019] ep_81: create, parent hub
> > [ 2.178171] hub 1-2:1.0: if: ffff88022f9e8800: endpoint devs created.
> >
> > And it seems the idea is to recreate this sysfs entry upon resume but
> > why isn't it destroyed during suspend?
>
> That's not right; resume and suspend aren't directly connected with
> this. The real trigger is the device reset at timestamp 427.786715;
> you saw it occur after hibernation ecause the hibernation caused a
> reset.
>
> For now you should be able to eliminate the problem by getting rid of
> these lines from usb_reset_and_verify_device() in
> drivers/usb/core/hub.c:
>
> intf->cur_altsetting =
> usb_find_alt_setting(config, i, 0);
> if (!intf->cur_altsetting)
> intf->cur_altsetting =
> &config->intf_cache[i]->altsetting[0];
>
> But we'll need to find a real solution. The problem is that we mustn't
> change intf->cur_altsetting here, because usb_set_interface() needs to
> know exactly what endpoint devices already exist for this interface.
Let's see if I can repeat the issue. :) usb_reset_and_verify_device()
is attempting to reset the device, and then re-instate the old
configuration and alternate settings. It takes a short cut by not
calling usb_reset_configuration(), since it doesn't want to rebind the
drivers if necessary.
Thus, the sysfs files from the original configuration and a non-default
alternate setting are still there when it calls usb_set_interface(), but
the device (and xHC host controller) think the default alternate
settings for the configuration are installed.
Before, the hub driver hid the reset from usb_set_interface() by leaving
intf->cur_altsetting set to the non-default alternate setting. Now that
usb_set_interface() needs to know which alternate settings are
*actually* installed to allocated bandwidth, we can't rely on that.
Does that sound correct?
> Sarah, any ideas? Add an extra flag to struct usb_interface to
> indicate that the hardware thinks the interface is in altsetting 0
> because of a reset?
I can't think of any other way to do it. usb_set_interface() needs to
know the true device/xHCI state to allocate bandwidth, so we can't just
remove the lines in hub.c as you suggested for the temporary fix.
So you're suggesting that the extra flag be checked in this if statement
in message.c?
/* prevent submissions using previous endpoint settings */
if (iface->cur_altsetting != alt) {
remove_intf_ep_devs(iface);
usb_remove_sysfs_intf_files(iface);
}
usb_disable_interface(dev, iface, true);
Something like:
if (!iface->resetting && iface->cur_altsetting != alt) {
and then have hub.c set and then clear that flag around the
usb_set_interface() call in usb_reset_and_verify_device()? It seems
a bit silly not just to add the flag as an argument to
usb_set_interface(), but that would mean a lot of drivers would have to
change for an internal USB core flag.
> (There's a related problem. In the call to usb_find_alt_setting()
> above, the second argument is not the interface number. Instead of i,
> it should be desc->bInterfaceNumber. In fact, that entire call should
> be replaced with a call to usb_altnum_to_altsetting().)
Yep, that's correct. I recall this discussion for another set of code,
so I wonder how that slipped by. I'll submit a separate patch for
that.
Sarah
--
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
