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:   Fri, 11 Jan 2019 10:12:07 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Kai Heng Feng <kai.heng.feng@canonical.com>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] USB: Don't enable LPM if it's already enabled
Message-ID: <20190111091207.GE15610@kroah.com>
References: <98BF2E31-2F7F-47FA-B591-40B2FFBC65F1@canonical.com>
 <Pine.LNX.4.44L0.1901081230560.1355-100000@iolanthe.rowland.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Pine.LNX.4.44L0.1901081230560.1355-100000@iolanthe.rowland.org>
User-Agent: Mutt/1.11.2 (2019-01-07)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jan 08, 2019 at 12:34:17PM -0500, Alan Stern wrote:
> On Wed, 9 Jan 2019, Kai Heng Feng wrote:
> 
> > > On Jan 8, 2019, at 11:41 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
> > >> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
> > >> after S3:
> > >> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_00000302.bin
> > >> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
> > >> 
> > >> After some experiments, I found that disabling LPM can workaround the
> > >> issue.
> > >> 
> > >> On some platforms, the USB power is cut during S3, so the driver uses
> > >> reset-resume to resume the device. During port resume, LPM gets enabled
> > >> twice, by usb_reset_and_verify_device() and usb_port_resume().
> > >> 
> > >> So let's enable LPM for just once, as this solves the issue for the
> > >> device in question.
> > >> 
> > >> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
> > >> and usb_disable_usb2_hardware_lpm().
> > > 
> > > I thought I asked for this to be two different patches.  One that does
> > > the "consolidation", and then one that fixes the bug.  You are mixing
> > > two different things here together, making it harder to review.
> > > 
> > > Can you please break this up and send a patch series, with the correct
> > > "Fixes:" tag added to the second patch that actually fixes the issue?
> > 
> > The consolidation itself is the fix, so I am not sure how to break this up.
> > 
> > In reset-resume case, LPM gets enabled twice, by
> > usb_reset_and_verify_device() and usb_port_resume().
> > 
> > If it’s a normal resume, LPM only gets enabled once, by
> > usb_port_resume().
> > 
> > Since all three checks (capable, allowed and enabled) are merged to
> > a single place, enabling LPM twice can be avoided, hence fixing the
> > issue.
> 
> One approach would be to have the first patch add the new functions and
> change the code to call them instead of the original function, but
> leaves the checks the way they are now.  Then the second patch could
> add the checks to the new functions and remove them from the call
> sites.

Yes, that is what I was looking for.  That way, if the "change" really
does cause problems, it is easier to revert/fix.

thanks,

greg k-h