| Message ID | 20241014202326.381559-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1608d6243da9d479910919e7bc0195050b43aa2e |
| Headers | show |
| Series | [v1] Bluetooth: btusb: Fix not being able to reconnect after suspend | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | warning | WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #100: Reported-by: Rafael J. Wysocki <rafael@kernel.org> Reported-by: Heiner Kallweit <hkallweit1@gmail.com> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #101: Reported-by: Heiner Kallweit <hkallweit1@gmail.com> Reported-by: Kenneth Crudup <kenny@panix.com> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #102: Reported-by: Kenneth Crudup <kenny@panix.com> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests") total: 0 errors, 3 warnings, 38 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13835440.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
Dear Luiz, Thank you for the patch. Am 14.10.24 um 22:23 schrieb Luiz Augusto von Dentz: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Calls to hci_suspend_dev assumes the system-suspend which doesn't work > well when just the device is being suspended because wakeup flag is only > set for remote devices that can wakeup the system. Please mention that you revert most parts of the problematic commit. > Reported-by: Rafael J. Wysocki <rafael@kernel.org> > Reported-by: Heiner Kallweit <hkallweit1@gmail.com> > Reported-by: Kenneth Crudup <kenny@panix.com> > Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests") That commit is not in the master branch, 610712298b11b2914be00b35abe9326b5dbb62c8 is. > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > drivers/bluetooth/btusb.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index d14b941bfde8..c0b6ef8ee5da 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -4075,7 +4075,6 @@ static void btusb_disconnect(struct usb_interface *intf) > static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > { > struct btusb_data *data = usb_get_intfdata(intf); > - int err; > > BT_DBG("intf %p", intf); > > @@ -4088,16 +4087,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > if (data->suspend_count++) > return 0; > > - /* Notify Host stack to suspend; this has to be done before stopping > - * the traffic since the hci_suspend_dev itself may generate some > - * traffic. > - */ > - err = hci_suspend_dev(data->hdev); > - if (err) { > - data->suspend_count--; > - return err; > - } > - > spin_lock_irq(&data->txlock); > if (!(PMSG_IS_AUTO(message) && data->tx_in_flight)) { > set_bit(BTUSB_SUSPENDING, &data->flags); > @@ -4105,7 +4094,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > } else { > spin_unlock_irq(&data->txlock); > data->suspend_count--; > - hci_resume_dev(data->hdev); > return -EBUSY; > } > > @@ -4226,8 +4214,6 @@ static int btusb_resume(struct usb_interface *intf) > spin_unlock_irq(&data->txlock); > schedule_work(&data->work); > > - hci_resume_dev(data->hdev); > - > return 0; > > failed: Kind regards, Paul
[CCing Stephen JFYI] On 16.10.24 07:12, Paul Menzel wrote: > > Thank you for the patch. +1 >> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend >> requests") > > That commit is not in the master branch, > 610712298b11b2914be00b35abe9326b5dbb62c8 is. Luiz, please allow me to ask: is there a reason why the bluetooth tree does not use a dedicated "-fixes" branch like many other subsystems do? That would avoid mishaps like the one above and all those "duplicate patches in the bluetooth tree" messages Stephen has to sent every few weeks (https://lore.kernel.org/all/?q=f%3Astephen+duplicate+%22bluetooth+tree%22 ); reminder, you can have both your -fixes and your -for-next branch in linux-next for test coverage. Ciao, Thorsten
On Mon, Oct 14, 2024 at 10:24 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Calls to hci_suspend_dev assumes the system-suspend which doesn't work > well when just the device is being suspended because wakeup flag is only > set for remote devices that can wakeup the system. > > Reported-by: Rafael J. Wysocki <rafael@kernel.org> First of all, the patch works here, so Tested-by: Rafael J. Wysocki <rafael@kernel.org> > Reported-by: Heiner Kallweit <hkallweit1@gmail.com> > Reported-by: Kenneth Crudup <kenny@panix.com> > Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests") However, this is not the commit ID referred to in my report, as already mentioned by Paul. > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > drivers/bluetooth/btusb.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index d14b941bfde8..c0b6ef8ee5da 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -4075,7 +4075,6 @@ static void btusb_disconnect(struct usb_interface *intf) > static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > { > struct btusb_data *data = usb_get_intfdata(intf); > - int err; > > BT_DBG("intf %p", intf); > > @@ -4088,16 +4087,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > if (data->suspend_count++) > return 0; > > - /* Notify Host stack to suspend; this has to be done before stopping > - * the traffic since the hci_suspend_dev itself may generate some > - * traffic. > - */ > - err = hci_suspend_dev(data->hdev); > - if (err) { > - data->suspend_count--; > - return err; > - } > - > spin_lock_irq(&data->txlock); > if (!(PMSG_IS_AUTO(message) && data->tx_in_flight)) { > set_bit(BTUSB_SUSPENDING, &data->flags); > @@ -4105,7 +4094,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > } else { > spin_unlock_irq(&data->txlock); > data->suspend_count--; > - hci_resume_dev(data->hdev); > return -EBUSY; > } > > @@ -4226,8 +4214,6 @@ static int btusb_resume(struct usb_interface *intf) > spin_unlock_irq(&data->txlock); > schedule_work(&data->work); > > - hci_resume_dev(data->hdev); > - > return 0; > > failed: > -- > 2.47.0 >
On Wed, Oct 16, 2024 at 7:12 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Luiz, > > > Thank you for the patch. > > > Am 14.10.24 um 22:23 schrieb Luiz Augusto von Dentz: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > Calls to hci_suspend_dev assumes the system-suspend which doesn't work > > well when just the device is being suspended because wakeup flag is only > > set for remote devices that can wakeup the system. > > Please mention that you revert most parts of the problematic commit. Yes, it would be good to say in the changelog that this is a partial revert. > > Reported-by: Rafael J. Wysocki <rafael@kernel.org> > > Reported-by: Heiner Kallweit <hkallweit1@gmail.com> > > Reported-by: Kenneth Crudup <kenny@panix.com> > > Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests") > > That commit is not in the master branch, > 610712298b11b2914be00b35abe9326b5dbb62c8 is. Right.
Hi Rafael, Paul, On Wed, Oct 16, 2024 at 8:06 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Oct 16, 2024 at 7:12 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > > > Dear Luiz, > > > > > > Thank you for the patch. > > > > > > Am 14.10.24 um 22:23 schrieb Luiz Augusto von Dentz: > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > > > Calls to hci_suspend_dev assumes the system-suspend which doesn't work > > > well when just the device is being suspended because wakeup flag is only > > > set for remote devices that can wakeup the system. > > > > Please mention that you revert most parts of the problematic commit. > > Yes, it would be good to say in the changelog that this is a partial revert. Ack. > > > Reported-by: Rafael J. Wysocki <rafael@kernel.org> > > > Reported-by: Heiner Kallweit <hkallweit1@gmail.com> > > > Reported-by: Kenneth Crudup <kenny@panix.com> > > > Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests") > > > > That commit is not in the master branch, > > 610712298b11b2914be00b35abe9326b5dbb62c8 is. Right, looks like I need to rebase to get the updated hashes. > Right.
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Mon, 14 Oct 2024 16:23:26 -0400 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > Calls to hci_suspend_dev assumes the system-suspend which doesn't work > well when just the device is being suspended because wakeup flag is only > set for remote devices that can wakeup the system. > > Reported-by: Rafael J. Wysocki <rafael@kernel.org> > Reported-by: Heiner Kallweit <hkallweit1@gmail.com> > Reported-by: Kenneth Crudup <kenny@panix.com> > Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend requests") > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > [...] Here is the summary with links: - [v1] Bluetooth: btusb: Fix not being able to reconnect after suspend https://git.kernel.org/bluetooth/bluetooth-next/c/1608d6243da9 You are awesome, thank you!
Hi Thorsten, On Wed, Oct 16, 2024 at 2:29 AM Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > [CCing Stephen JFYI] > > On 16.10.24 07:12, Paul Menzel wrote: > > > > Thank you for the patch. > > +1 > > >> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend > >> requests") > > > > That commit is not in the master branch, > > 610712298b11b2914be00b35abe9326b5dbb62c8 is. > > Luiz, please allow me to ask: is there a reason why the bluetooth tree > does not use a dedicated "-fixes" branch like many other subsystems do? > That would avoid mishaps like the one above and all those "duplicate > patches in the bluetooth tree" messages Stephen has to sent every few > weeks > (https://lore.kernel.org/all/?q=f%3Astephen+duplicate+%22bluetooth+tree%22 > ); reminder, you can have both your -fixes and your -for-next branch in > linux-next for test coverage. Not sure I follow, we do have bluetooth tree (https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git) for fixes during the RC phase, or are you saying the fixes for RC shall not be integrated thru bluetooth-next but directly into bluetooth tree and then once merged they are pulled into bluetooth-next by rebasing to avoid changing the hash? While possible this would be hard with our CI which only tests patches against bluetooth-next tree so by not integrating the RC fixes we may be able to detect similar changes. Regarding the duplicate detection, I wonder if that really a problem or some script failing to detect it is just a hash change, because git seems fine with those and in most cases it will just say it has already been applied and move on. > Ciao, Thorsten
On 16.10.24 21:01, Luiz Augusto von Dentz wrote: > On Wed, Oct 16, 2024 at 2:29 AM Thorsten Leemhuis > <regressions@leemhuis.info> wrote: >> On 16.10.24 07:12, Paul Menzel wrote: >>> >>> Thank you for the patch. >> +1 >> >>>> Fixes: 81b3e33bb054 ("Bluetooth: btusb: Don't fail external suspend >>>> requests") >>> >>> That commit is not in the master branch, >>> 610712298b11b2914be00b35abe9326b5dbb62c8 is. >> >> Luiz, please allow me to ask: is there a reason why the bluetooth tree >> does not use a dedicated "-fixes" branch like many other subsystems do? >> That would avoid mishaps like the one above and all those "duplicate >> patches in the bluetooth tree" messages Stephen has to sent every few >> weeks >> (https://lore.kernel.org/all/?q=f%3Astephen+duplicate+%22bluetooth+tree%22 >> ); reminder, you can have both your -fixes and your -for-next branch in >> linux-next for test coverage. > > Not sure I follow, we do have bluetooth tree > (https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git) > for fixes during the RC phase, Ahh, I see, you have two different trees, and not different branches in one tree. Sorry, should have noticed that. > or are you saying the fixes for RC > shall not be integrated thru bluetooth-next but directly into > bluetooth tree and then once merged they are pulled into > bluetooth-next by rebasing to avoid changing the hash? I'm no expert here, but subsystems use different strategies afaics. Most afaics have two branches in one tree or (like you) two trees. Both are in next. And they only merge the -fixes branch into their -next tree when there is a need, not regularly (that iirc would upset Linus). > While possible > this would be hard with our CI which only tests patches against > bluetooth-next tree Can't that be changed? Also: that sounds (but I might be wrong there!) like -fixes you send to Linus don't get tested independently. Wouldn't that be better? > so by not integrating the RC fixes we may be able > to detect similar changes. > > Regarding the duplicate detection, I wonder if that really a problem > or some script failing to detect it is just a hash change, because git > seems fine with those and in most cases it will just say it has > already been applied and move on. Stephen might be the better one to answer that, but from his mails and my understanding of it it's more like "if duplicates happen occasionally (for example because some change queued for -next turns out needs to be send through -fixes quickly), it's not a problem. But it shouldn't be something that happens due to the regular workflow. I've also seen a few people including Greg complain about Fixes: tags for commits that don't exist -- which is the case until the duplicate commit (like the 81b3e33bb054 that triggered this discussion) lands during the next merge window. But during that time window it can easily confuse people I guess. Anyway, maybe I shouldn't have send anything, this is not my area of expertise. It's just that I noticed the mails from Stephen, the complains from Greg, and that one discussion at maintainers summit or LPC where "more trees should have their -fixes branch in next" very briefly came up. Ciao, Thorsten
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index d14b941bfde8..c0b6ef8ee5da 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -4075,7 +4075,6 @@ static void btusb_disconnect(struct usb_interface *intf) static int btusb_suspend(struct usb_interface *intf, pm_message_t message) { struct btusb_data *data = usb_get_intfdata(intf); - int err; BT_DBG("intf %p", intf); @@ -4088,16 +4087,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) if (data->suspend_count++) return 0; - /* Notify Host stack to suspend; this has to be done before stopping - * the traffic since the hci_suspend_dev itself may generate some - * traffic. - */ - err = hci_suspend_dev(data->hdev); - if (err) { - data->suspend_count--; - return err; - } - spin_lock_irq(&data->txlock); if (!(PMSG_IS_AUTO(message) && data->tx_in_flight)) { set_bit(BTUSB_SUSPENDING, &data->flags); @@ -4105,7 +4094,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) } else { spin_unlock_irq(&data->txlock); data->suspend_count--; - hci_resume_dev(data->hdev); return -EBUSY; } @@ -4226,8 +4214,6 @@ static int btusb_resume(struct usb_interface *intf) spin_unlock_irq(&data->txlock); schedule_work(&data->work); - hci_resume_dev(data->hdev); - return 0; failed: