Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor

PATCH In review Lee, Chun-Yi 5 participants 6 messages Apr 24, 2024

Subsystems (2)

Bluetooth drivers Bluetooth

lore

Share & tools

lore

Lee, Chun-Yi

author

2024-04-24 13:59 UTC #0 lore raw

PATCH 1/1 Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor

From: Chun-Yi Lee <[email protected]>

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 <[email protected]>

Cc Manish Mandlik <[email protected]>

Cc Archie Pusaka <[email protected]>

Cc Miao-chen Chou <[email protected]>

Signed-off-by Chun-Yi Lee <[email protected]>

---
 net/bluetooth/mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/bluetooth/mgmt.c +1 -1

index 32ed6e9245a3..21962969411d 100644
@@ -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) {

Signature · 1 line

2.35.3

Takashi Iwai

2024-04-24 14:02 UTC #1 lore raw

Quoted 21 lines · Chun-Yi

From: Chun-Yi Lee <[email protected]>
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().

Quoted 6 lines

Fixes d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
Cc Luiz Augusto von Dentz <[email protected]>
Cc Manish Mandlik <[email protected]>
Cc Archie Pusaka <[email protected]>
Cc Miao-chen Chou <[email protected]>
Signed-off-by Chun-Yi Lee <[email protected]>

Is it a same issue tracked in
https://github.com/bluez/bluez/issues/809
??

thanks,

Takashi

Quoted 21 lines

---
 net/bluetooth/mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/bluetooth/mgmt.c +1 -1

index 32ed6e9245a3..21962969411d 100644
@@ -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) {

Signature · 1 line

2.35.3

joeyli

2024-04-24 15:18 UTC #2 lore raw

Hi Takashi,

Thanks for your reviewing and comment!

Quoted 30 lines · Takashi Iwai

Quoted 21 lines · Chun-Yi
From: Chun-Yi Lee <[email protected]>
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().
Quoted 6 lines
Fixes d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
Cc Luiz Augusto von Dentz <[email protected]>
Cc Manish Mandlik <[email protected]>
Cc Archie Pusaka <[email protected]>
Cc Miao-chen Chou <[email protected]>
Signed-off-by Chun-Yi Lee <[email protected]>

Quoted 3 lines

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

Quoted 27 lines

thanks,

Takashi

Quoted 21 lines

---
 net/bluetooth/mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/bluetooth/mgmt.c +1 -1

index 32ed6e9245a3..21962969411d 100644
@@ -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) {

Signature · 1 line

2.35.3

Paul Menzel

2024-04-24 14:13 UTC #3 lore raw

In reply to Lee, Chun-Yi

Dear Chun-Yi,

Thank you for your patch.

Am 24.04.24 um 15:59 schrieb Lee, Chun-Yi:

Quoted 1 line

From: Chun-Yi Lee <[email protected]>

Please use imperative mood in the commit message summary:

Use hci_cmd_sync_submit() when removing Adv Monitor

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 1 line

Normally this situation is happened when the bluetoothd deamon

1. happen*s*
2. d*ae*mon

Quoted 1 line

be started manually after system booting. Which means that

is started

Quoted 1 line

bluetoothd received MGMT_EV_INDEX_ADDED event after kernel

receive*s*

Quoted 3 lines

runs hci_power_off().
Base on doc/mgmt-api.txt, the MGMT_OP_REMOVE_ADV_MONITOR command

Base*d*

Quoted 1 line

can be used when the controller is not powered. This patch changes

“This patch …” is redundant. Use: Change the code …

Quoted 2 lines

the code in remove_adv_monitor() to use hci_cmd_sync_submit() instead of hci_cmd_sync_queue().

Please document the test setup.

Quoted 15 lines

Fixes d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
Cc Luiz Augusto von Dentz <[email protected]>
Cc Manish Mandlik <[email protected]>
Cc Archie Pusaka <[email protected]>
Cc Miao-chen Chou <[email protected]>
Signed-off-by Chun-Yi Lee <[email protected]>
---
  net/bluetooth/mgmt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
net/bluetooth/mgmt.c +0 -0
index 32ed6e9245a3..21962969411d 100644

net/bluetooth/mgmt.c static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev, · line 5481

Quoted 8 lines

net/bluetooth/mgmt.c +1 -1

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

joeyli

2024-04-24 15:23 UTC #4 lore raw

Hi Paul,

Thanks for your review. I will follow your suggestion to update my v2 patch.

Joey Lee

Quoted 82 lines · Paul Menzel

Dear Chun-Yi,
Thank you for your patch.
Am 24.04.24 um 15:59 schrieb Lee, Chun-Yi:
Quoted 1 line
From: Chun-Yi Lee <[email protected]>
Please use imperative mood in the commit message summary:
Use hci_cmd_sync_submit() when removing Adv Monitor
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 1 line
Normally this situation is happened when the bluetoothd deamon
1. happen*s*
2. d*ae*mon
Quoted 1 line
be started manually after system booting. Which means that
is started
Quoted 1 line
bluetoothd received MGMT_EV_INDEX_ADDED event after kernel
receive*s*
Quoted 3 lines
runs hci_power_off().
Base on doc/mgmt-api.txt, the MGMT_OP_REMOVE_ADV_MONITOR command
Base*d*
Quoted 1 line
can be used when the controller is not powered. This patch changes
“This patch …” is redundant. Use: Change the code …
Quoted 2 lines
the code in remove_adv_monitor() to use hci_cmd_sync_submit() instead of hci_cmd_sync_queue().
Please document the test setup.
Quoted 15 lines
Fixes d883a4669a1de ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
Cc Luiz Augusto von Dentz <[email protected]>
Cc Manish Mandlik <[email protected]>
Cc Archie Pusaka <[email protected]>
Cc Miao-chen Chou <[email protected]>
Signed-off-by Chun-Yi Lee <[email protected]>
---
  net/bluetooth/mgmt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
net/bluetooth/mgmt.c +0 -0
index 32ed6e9245a3..21962969411d 100644
net/bluetooth/mgmt.c static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev, · line 5479
Quoted 6 lines
net/bluetooth/mgmt.c +1 -1
@@ -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

[email protected]

2024-04-24 14:33 UTC #5 lore raw

In reply to Lee, Chun-Yi

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