Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 22 Jan 2020 16:54:54 -0700
From:   Paul Zimmerman <pauldzim@gmail.com>
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        David Heinzelmann <heinzelmann.david@gmail.com>,
        <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org>
Subject: Re: [REGRESSION][BISECTED] 5.5-rc suspend/resume failure caused by
 patch a4f55d8b8c14 ("usb: hub: Check device descriptor before
 resusciation")
Message-ID: <20200122165454.757aaa25@EliteBook>
In-Reply-To: <Pine.LNX.4.44L0.2001221249190.1636-100000@iolanthe.rowland.org>
References: <20200121193131.070a28bf@EliteBook>
        <Pine.LNX.4.44L0.2001221249190.1636-100000@iolanthe.rowland.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 22 Jan 2020 14:31:29 -0500 (EST)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Tue, 21 Jan 2020, Paul Zimmerman wrote:
> 
> > On Mon, 20 Jan 2020 13:52:15 -0700 Paul Zimmerman <pauldzim@gmail.com> wrote:
> >   
> > > On Mon, 20 Jan 2020 10:23:11 -0500 (EST) Alan Stern <stern@rowland.harvard.edu> 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@gmail.com>