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@xxxxxxxxxxxxxxxxxxx> 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
