On Wed, 22 Jan 2020 14:31:29 -0500 (EST) Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Tue, 21 Jan 2020, Paul Zimmerman wrote: > > > On Mon, 20 Jan 2020 13:52:15 -0700 Paul Zimmerman <pauldzim@xxxxxxxxx> wrote: > > > > > On Mon, 20 Jan 2020 10:23:11 -0500 (EST) Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > On Sun, 19 Jan 2020, Paul Zimmerman wrote: > > > > > > > > > I reported this regression last week (see > > > > > https://lore.kernel.org/linux-usb/20200115153714.03d5b3aa@EliteBook/T/#u) > > > > > but I got no response to my email. Today I have retested with > > > > > 5.5-rc7 and verified that the problem still exists. So I am > > > > > resending with a different subject line to see if anyone responds. > > > > > > > > > > The $subject patch causes a regression on my HP EliteBook laptop > > > > > with a built-in USB bluetooth adapter. About 50% of the time, a > > > > > suspend/resume cycle will cause the bluetooth adapter to stop > > > > > working. > > > > > > > > > > The dmesg log below shows two suspend/resume cycles. At time > > > > > 63.928 you can see the bluetooth adapter being successfully > > > > > resumed, and at time 140.969 you can see it fail. After reverting > > > > > the patch, the bluetooth adapter resumes 100% of the time. > > > > > > > > > > I also included below a lsusb -v of the bluetooth adapter. Is > > > > > there any other debugging info you'd like me to send? > > > > > > > > It looks like your dmesg log was made without enabling debugging > > > > messages in usbcore. Can you collect another log with debugging > > > > messages turned on? > > > > > > > > echo 'module usbcore =p' > > > > >/sys/kernel/debug/dynamic_debug/control > > > > > > > > Also, it might not hurt to collect and post a usbmon trace for a bad > > > > suspend-resume cycle. > > > > > > Hi Alan, > > > > > > Thanks for responding. The new dmesg log and the usbmon trace are > > > below. The dmesg shows a good suspend/resume followed by a bad one. > > > The bluetooth device is usb 2-3.2 I believe. The usbmon trace is only > > > for the failed suspend/resume case. > > It might be interesting to have a usbmon trace of a successful resume > as well, for comparison. However I suspect it would just show that > the new Get-Device-Descriptor request worked and everything else > continued on normally. < snip > > > So if I'm understanding this correctly, there are two threads that are > > trying to access the USB bluetooth device at the same time. I have no > > idea if that is how it's supposed to work. > > In fact it isn't, although in principle this shouldn't cause any > trouble. It looks like your bluetooth device is deficient: It > sometimes crashes if it receives a Get-Device-Descriptor request while > it is busy with something else. > > Still, since there was no real connection change at the port, there's > no reason to call hub_port_connect_change() here. Let's see if the > patch below fixes your problem. > > Alan Stern > > > > Index: usb-devel/drivers/usb/core/hub.c > =================================================================== > --- usb-devel.orig/drivers/usb/core/hub.c > +++ usb-devel/drivers/usb/core/hub.c > @@ -1216,11 +1216,6 @@ static void hub_activate(struct usb_hub > #ifdef CONFIG_PM > udev->reset_resume = 1; > #endif > - /* Don't set the change_bits when the device > - * was powered off. > - */ > - if (test_bit(port1, hub->power_bits)) > - set_bit(port1, hub->change_bits); > > } else { > /* The power session is gone; tell hub_wq */ > I can confirm this fixes the issue for me, I did a couple dozen suspend/resume cycles without seeing a failure. I see the code you removed was added by Lan Tianyu in commit ad493e5e5805 ("usb: add usb port auto power off mechanism"). I wonder if your patch would break that? I don't know what that is or how to test it. In any case: Tested-by: Paul Zimmerman <pauldzim@xxxxxxxxx>
