Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor

Commit Message

Chun-Yi Lee April 24, 2024, 1:59 p.m. UTC

From: Chun-Yi Lee <[e-mail]>

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 <[e-mail]>
Cc: Manish Mandlik <[e-mail]>
Cc: Archie Pusaka <[e-mail]>
Cc: Miao-chen Chou <[e-mail]>
Signed-off-by: Chun-Yi Lee <[e-mail]>
---
 net/bluetooth/mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Takashi Iwai April 24, 2024, 2:02 p.m. UTC | #1

On Wed, 24 Apr 2024 15:59:03 +0200,
Lee, Chun-Yi wrote:
> 
> From: Chun-Yi Lee <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Manish Mandlik <[e-mail]>
> Cc: Archie Pusaka <[e-mail]>
> Cc: Miao-chen Chou <[e-mail]>
> Signed-off-by: Chun-Yi Lee <[e-mail]>

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

Paul Menzel April 24, 2024, 2:13 p.m. UTC | #2

Dear Chun-Yi,


Thank you for your patch.

Am 24.04.24 um 15:59 schrieb Lee, Chun-Yi:
> From: Chun-Yi Lee <[e-mail]>

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 <[e-mail]>
> Cc: Manish Mandlik <[e-mail]>
> Cc: Archie Pusaka <[e-mail]>
> Cc: Miao-chen Chou <[e-mail]>
> Signed-off-by: Chun-Yi Lee <[e-mail]>
> ---
>   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

[e-mail] April 24, 2024, 2:33 p.m. UTC | #3

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

joeyli April 24, 2024, 3:18 p.m. UTC | #4

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Cc: Manish Mandlik <[e-mail]>
> > Cc: Archie Pusaka <[e-mail]>
> > Cc: Miao-chen Chou <[e-mail]>
> > Signed-off-by: Chun-Yi Lee <[e-mail]>
> 
> 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
> > 
> >

joeyli April 24, 2024, 3:23 p.m. UTC | #5

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 <[e-mail]>
> 
> 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 <[e-mail]>
> > Cc: Manish Mandlik <[e-mail]>
> > Cc: Archie Pusaka <[e-mail]>
> > Cc: Miao-chen Chou <[e-mail]>
> > Signed-off-by: Chun-Yi Lee <[e-mail]>
> > ---
> >   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

Patch

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) {