* Re: [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor
2024-04-24 13:59 [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor Lee, Chun-Yi
@ 2024-04-24 14:02 ` Takashi Iwai
2024-04-24 15:18 ` joeyli
2024-04-24 14:13 ` Paul Menzel
2024-04-24 14:33 ` bluez.test.bot
2 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2024-04-24 14:02 UTC (permalink / raw)
To: Lee, Chun-Yi
Cc: Luiz Augusto von Dentz, Manish Mandlik, Archie Pusaka,
Miao-chen Chou, linux-kernel, linux-bluetooth, Chun-Yi Lee
On Wed, 24 Apr 2024 15:59:03 +0200,
Lee, Chun-Yi wrote:
>
> From: Chun-Yi Lee <jlee@suse.com>
>
> Since the d883a4669a1de be introduced in v6.4, bluetooth daemon
> got the following failed message of MGMT_OP_REMOVE_ADV_MONITOR
> command when controller is power-off:
>
> bluetoothd[20976]:
> src/adapter.c:reset_adv_monitors_complete() Failed to reset Adv
> Monitors: Failed>
>
> Normally this situation is happened when the bluetoothd deamon
> be started manually after system booting. Which means that
> bluetoothd received MGMT_EV_INDEX_ADDED event after kernel
> runs hci_power_off().
>
> Base on doc/mgmt-api.txt, the MGMT_OP_REMOVE_ADV_MONITOR command
> can be used when the controller is not powered. This patch changes
> the code in remove_adv_monitor() to use hci_cmd_sync_submit()
> instead of hci_cmd_sync_queue().
>
> Fixes: d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Cc: Manish Mandlik <mmandlik@google.com>
> Cc: Archie Pusaka <apusaka@chromium.org>
> Cc: Miao-chen Chou <mcchou@chromium.org>
> Signed-off-by: Chun-Yi Lee <jlee@suse.com>
Is it a same issue tracked in
https://github.com/bluez/bluez/issues/809
??
thanks,
Takashi
> ---
> net/bluetooth/mgmt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 32ed6e9245a3..21962969411d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -5475,7 +5475,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
> goto unlock;
> }
>
> - err = hci_cmd_sync_queue(hdev, mgmt_remove_adv_monitor_sync, cmd,
> + err = hci_cmd_sync_submit(hdev, mgmt_remove_adv_monitor_sync, cmd,
> mgmt_remove_adv_monitor_complete);
>
> if (err) {
> --
> 2.35.3
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor
2024-04-24 14:02 ` Takashi Iwai
@ 2024-04-24 15:18 ` joeyli
0 siblings, 0 replies; 6+ messages in thread
From: joeyli @ 2024-04-24 15:18 UTC (permalink / raw)
To: Takashi Iwai
Cc: Lee, Chun-Yi, Luiz Augusto von Dentz, Manish Mandlik,
Archie Pusaka, Miao-chen Chou, linux-kernel, linux-bluetooth
Hi Takashi,
Thanks for your reviewing and comment!
On Wed, Apr 24, 2024 at 04:02:37PM +0200, Takashi Iwai wrote:
> On Wed, 24 Apr 2024 15:59:03 +0200,
> Lee, Chun-Yi wrote:
> >
> > From: Chun-Yi Lee <jlee@suse.com>
> >
> > Since the d883a4669a1de be introduced in v6.4, bluetooth daemon
> > got the following failed message of MGMT_OP_REMOVE_ADV_MONITOR
> > command when controller is power-off:
> >
> > bluetoothd[20976]:
> > src/adapter.c:reset_adv_monitors_complete() Failed to reset Adv
> > Monitors: Failed>
> >
> > Normally this situation is happened when the bluetoothd deamon
> > be started manually after system booting. Which means that
> > bluetoothd received MGMT_EV_INDEX_ADDED event after kernel
> > runs hci_power_off().
> >
> > Base on doc/mgmt-api.txt, the MGMT_OP_REMOVE_ADV_MONITOR command
> > can be used when the controller is not powered. This patch changes
> > the code in remove_adv_monitor() to use hci_cmd_sync_submit()
> > instead of hci_cmd_sync_queue().
> >
> > Fixes: d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
> > Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > Cc: Manish Mandlik <mmandlik@google.com>
> > Cc: Archie Pusaka <apusaka@chromium.org>
> > Cc: Miao-chen Chou <mcchou@chromium.org>
> > Signed-off-by: Chun-Yi Lee <jlee@suse.com>
>
> Is it a same issue tracked in
> https://github.com/bluez/bluez/issues/809
> ??
It's similar problem but for different MGMT command.
The above issue and patch are against MGMT_OP_ADD_UUID/
MGMT_OP_REMOVE_UUID. My patch is against
MGMT_OP_REMOVE_ADV_MONITOR command.
Thanks
Joey Lee
>
>
> thanks,
>
> Takashi
>
> > ---
> > net/bluetooth/mgmt.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 32ed6e9245a3..21962969411d 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -5475,7 +5475,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
> > goto unlock;
> > }
> >
> > - err = hci_cmd_sync_queue(hdev, mgmt_remove_adv_monitor_sync, cmd,
> > + err = hci_cmd_sync_submit(hdev, mgmt_remove_adv_monitor_sync, cmd,
> > mgmt_remove_adv_monitor_complete);
> >
> > if (err) {
> > --
> > 2.35.3
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor
2024-04-24 13:59 [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor Lee, Chun-Yi
2024-04-24 14:02 ` Takashi Iwai
@ 2024-04-24 14:13 ` Paul Menzel
2024-04-24 15:23 ` joeyli
2024-04-24 14:33 ` bluez.test.bot
2 siblings, 1 reply; 6+ messages in thread
From: Paul Menzel @ 2024-04-24 14:13 UTC (permalink / raw)
To: Chun-Yi Lee
Cc: Chun-Yi Lee, Luiz Augusto von Dentz, Manish Mandlik,
Archie Pusaka, Miao-chen Chou, linux-kernel, linux-bluetooth
Dear Chun-Yi,
Thank you for your patch.
Am 24.04.24 um 15:59 schrieb Lee, Chun-Yi:
> From: Chun-Yi Lee <jlee@suse.com>
Please use imperative mood in the commit message summary:
Use hci_cmd_sync_submit() when removing Adv Monitor
> Since the d883a4669a1de be introduced in v6.4, bluetooth daemon
> got the following failed message of MGMT_OP_REMOVE_ADV_MONITOR
> command when controller is power-off:
Maybe:
Since commit d883a4669a1de, present since Linux v6.4, the daemon
bluetoothd logs the failure below, if the command
MGMT_OP_REMOVE_ADV_MONITOR is sent when the controller is powered off.
> bluetoothd[20976]:
> src/adapter.c:reset_adv_monitors_complete() Failed to reset Adv
> Monitors: Failed>
Please make this one line and indent it with four spaces.
> Normally this situation is happened when the bluetoothd deamon
1. happen*s*
2. d*ae*mon
> be started manually after system booting. Which means that
is started
> bluetoothd received MGMT_EV_INDEX_ADDED event after kernel
receive*s*
> runs hci_power_off().
>
> Base on doc/mgmt-api.txt, the MGMT_OP_REMOVE_ADV_MONITOR command
Base*d*
> can be used when the controller is not powered. This patch changes
“This patch …” is redundant. Use: Change the code …
> the code in remove_adv_monitor() to use hci_cmd_sync_submit()
> instead of hci_cmd_sync_queue().
Please document the test setup.
> Fixes: d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Cc: Manish Mandlik <mmandlik@google.com>
> Cc: Archie Pusaka <apusaka@chromium.org>
> Cc: Miao-chen Chou <mcchou@chromium.org>
> Signed-off-by: Chun-Yi Lee <jlee@suse.com>
> ---
> net/bluetooth/mgmt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 32ed6e9245a3..21962969411d 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -5475,7 +5475,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
> goto unlock;
> }
>
> - err = hci_cmd_sync_queue(hdev, mgmt_remove_adv_monitor_sync, cmd,
> + err = hci_cmd_sync_submit(hdev, mgmt_remove_adv_monitor_sync, cmd,
> mgmt_remove_adv_monitor_complete);
>
> if (err) {
Kind regards,
Paul
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor
2024-04-24 14:13 ` Paul Menzel
@ 2024-04-24 15:23 ` joeyli
0 siblings, 0 replies; 6+ messages in thread
From: joeyli @ 2024-04-24 15:23 UTC (permalink / raw)
To: Paul Menzel
Cc: Chun-Yi Lee, Luiz Augusto von Dentz, Manish Mandlik,
Archie Pusaka, Miao-chen Chou, linux-kernel, linux-bluetooth
Hi Paul,
Thanks for your review. I will follow your suggestion to update
my v2 patch.
Joey Lee
On Wed, Apr 24, 2024 at 04:13:09PM +0200, Paul Menzel wrote:
> Dear Chun-Yi,
>
>
> Thank you for your patch.
>
> Am 24.04.24 um 15:59 schrieb Lee, Chun-Yi:
> > From: Chun-Yi Lee <jlee@suse.com>
>
> Please use imperative mood in the commit message summary:
>
> Use hci_cmd_sync_submit() when removing Adv Monitor
>
> > Since the d883a4669a1de be introduced in v6.4, bluetooth daemon
> > got the following failed message of MGMT_OP_REMOVE_ADV_MONITOR
> > command when controller is power-off:
>
> Maybe:
>
> Since commit d883a4669a1de, present since Linux v6.4, the daemon bluetoothd
> logs the failure below, if the command MGMT_OP_REMOVE_ADV_MONITOR is sent
> when the controller is powered off.
>
> > bluetoothd[20976]:
> > src/adapter.c:reset_adv_monitors_complete() Failed to reset Adv
> > Monitors: Failed>
>
> Please make this one line and indent it with four spaces.
>
> > Normally this situation is happened when the bluetoothd deamon
>
> 1. happen*s*
> 2. d*ae*mon
>
> > be started manually after system booting. Which means that
>
> is started
>
> > bluetoothd received MGMT_EV_INDEX_ADDED event after kernel
>
> receive*s*
>
> > runs hci_power_off().
> >
> > Base on doc/mgmt-api.txt, the MGMT_OP_REMOVE_ADV_MONITOR command
>
> Base*d*
>
> > can be used when the controller is not powered. This patch changes
>
> “This patch …” is redundant. Use: Change the code …
>
> > the code in remove_adv_monitor() to use hci_cmd_sync_submit()
> > instead of hci_cmd_sync_queue().
>
> Please document the test setup.
>
> > Fixes: d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
> > Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > Cc: Manish Mandlik <mmandlik@google.com>
> > Cc: Archie Pusaka <apusaka@chromium.org>
> > Cc: Miao-chen Chou <mcchou@chromium.org>
> > Signed-off-by: Chun-Yi Lee <jlee@suse.com>
> > ---
> > net/bluetooth/mgmt.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 32ed6e9245a3..21962969411d 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -5475,7 +5475,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
> > goto unlock;
> > }
> > - err = hci_cmd_sync_queue(hdev, mgmt_remove_adv_monitor_sync, cmd,
> > + err = hci_cmd_sync_submit(hdev, mgmt_remove_adv_monitor_sync, cmd,
> > mgmt_remove_adv_monitor_complete);
> > if (err) {
>
>
> Kind regards,
>
> Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor
2024-04-24 13:59 [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor Lee, Chun-Yi
2024-04-24 14:02 ` Takashi Iwai
2024-04-24 14:13 ` Paul Menzel
@ 2024-04-24 14:33 ` bluez.test.bot
2 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2024-04-24 14:33 UTC (permalink / raw)
To: linux-bluetooth, joeyli.kernel
[-- Attachment #1: Type: text/plain, Size: 3971 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=847477
---Test result---
Test Summary:
CheckPatch FAIL 0.90 seconds
GitLint PASS 0.20 seconds
SubjectPrefix PASS 0.05 seconds
BuildKernel PASS 32.20 seconds
CheckAllWarning PASS 37.80 seconds
CheckSparse PASS 40.29 seconds
CheckSmatch FAIL 36.63 seconds
BuildKernel32 PASS 30.35 seconds
TestRunnerSetup PASS 528.17 seconds
TestRunner_l2cap-tester PASS 18.40 seconds
TestRunner_iso-tester FAIL 31.30 seconds
TestRunner_bnep-tester PASS 4.63 seconds
TestRunner_mgmt-tester PASS 108.20 seconds
TestRunner_rfcomm-tester PASS 7.22 seconds
TestRunner_sco-tester PASS 14.93 seconds
TestRunner_ioctl-tester PASS 7.56 seconds
TestRunner_mesh-tester PASS 5.66 seconds
TestRunner_smp-tester PASS 6.62 seconds
TestRunner_userchan-tester PASS 4.86 seconds
IncrementalBuild PASS 27.45 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor
WARNING: 'deamon' may be misspelled - perhaps 'daemon'?
#106:
Normally this situation is happened when the bluetoothd deamon
^^^^^^
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")'
#116:
Fixes: d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
CHECK: Alignment should match open parenthesis
#136: FILE: net/bluetooth/mgmt.c:5479:
+ err = hci_cmd_sync_submit(hdev, mgmt_remove_adv_monitor_sync, cmd,
mgmt_remove_adv_monitor_complete);
total: 0 errors, 2 warnings, 1 checks, 8 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/13641928.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.
Use of uninitialized value $cid in concatenation (.) or string at /github/workspace/src/src/scripts/checkpatch.pl line 3229.
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 122, Passed: 121 (99.2%), Failed: 1, Not Run: 0
Failed Test Cases
ISO Connect2 Suspend - Success Failed 4.232 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 6+ messages in thread