public inbox for linux-bluetooth@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Bluetooth: hci_sync: Using hci_cmd_sync_submit when removing Adv Monitor
@ 2024-04-24 13:59 Lee, Chun-Yi
  2024-04-24 14:02 ` Takashi Iwai
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Lee, Chun-Yi @ 2024-04-24 13:59 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: Manish Mandlik, Archie Pusaka, Miao-chen Chou, linux-kernel,
	linux-bluetooth, Chun-Yi Lee

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>
---
 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 related	[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 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 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: 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

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

end of thread, other threads:[~2024-04-24 15:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 15:23   ` joeyli
2024-04-24 14:33 ` bluez.test.bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox