[PATCH v3] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[PATCH v3] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[Xiuzhuo Shang]

start_discovery_internal(), start_service_discovery() and stop_discovery()
queue a cmd_sync work item and only then move the discovery state machine
into its transient value (DISCOVERY_STARTING / DISCOVERY_STOPPING):

  err = hci_cmd_sync_queue(hdev, ..._sync, cmd, ..._complete);
  if (err < 0) { ... }
  hci_discovery_set_state(hdev, DISCOVERY_STARTING /* or STOPPING */);

The matching completion callbacks run on hdev->req_workqueue serialised
by hci_req_sync_lock, which is independent of hdev->lock. So once the
work has been queued, the worker can be scheduled, run the sync function
and invoke the completion before the caller has executed the trailing
hci_discovery_set_state(). The completion's success path writes the
terminal state (DISCOVERY_STOPPED for stop, DISCOVERY_FINDING for start);
the caller then overwrites it with the transient value, and the state
machine is wedged: every subsequent Start (Service) Discovery is
rejected by the DISCOVERY_STOPPED gate with MGMT_STATUS_BUSY (0x0a),
with no HCI traffic generated, until bluetoothd or the adapter is
restarted.

Fix it in three parts:

  1. In all three call sites move hci_discovery_set_state(STARTING /
     STOPPING) to before hci_cmd_sync_queue(). The transient state is
     therefore always published before any worker can run the
     completion. On queue-submit failure, roll back to
     DISCOVERY_STOPPED.

  2. In start_discovery_complete() and stop_discovery_complete(), wrap
     the terminal hci_discovery_set_state() call with
     hci_dev_lock() / hci_dev_unlock(). These callbacks run without
     hdev->lock; serialising the state write matches the pattern used
     by mgmt_set_powered_complete() and removes any residual ordering
     hazard against a concurrent mgmt path holding hdev->lock.

  3. Generalise the "ignore -ECANCELED" early return in both completion
     callbacks to "on any non-zero err, also reset the transient state
     to STOPPED". With (1) in place the state observed at completion
     time is always known.

     For the stop path this also fixes a pre-existing wedge: when any
     sub-command issued from hci_stop_discovery_sync() returns an
     error, stop_discovery_complete() is invoked with err != 0. The
     existing "if (!err) set_state(STOPPED)" tail then skips the reset
     and the state machine sits in DISCOVERY_STOPPING forever.

Fixes: abfeea476c68 ("Bluetooth: hci_sync: Convert MGMT_OP_START_DISCOVERY")
Signed-off-by: Xiuzhuo Shang <xiuzhuo.shang@oss.qualcomm.com>
---
Changes in v3:
 - Replace inline patch title with lore.kernel.org URL in v2 link
   reference to fix GitLint B1 line-length check.
 - Link to v2:
   https://lore.kernel.org/all/20260708062009.3047447-1-xiuzhuo.shang@oss.qualcomm.com/

Changes in v2:
 - Fix if (err < 0) to if (err) in both start_discovery_complete() and
   stop_discovery_complete() to also catch positive HCI status codes,
   flagged by Sashiko.
 - Add Fixes: tag for commit abfeea476c68 as requested.
 - Update commit message wording from "err < 0" to "non-zero err" to
   match the code change.
 - Link to v1:
   https://lore.kernel.org/all/20260707093426.372897-1-xiuzhuo.shang@oss.qualcomm.com/

 net/bluetooth/mgmt.c | 86 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 733a4b70e10c..81c09c24a14b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5975,15 +5975,38 @@ static void start_discovery_complete(struct hci_dev *hdev, void *data, int err)
 
 	bt_dev_dbg(hdev, "err %d", err);
 
-	if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
+	if (err) {
+		/* The queued start-discovery work failed before the normal
+		 * completion path could advance the state machine. The
+		 * caller already moved the state to DISCOVERY_STARTING
+		 * (under hdev->lock, before queueing). Reset it here so the
+		 * gate in start_discovery_internal()/start_service_discovery()
+		 * does not wedge in STARTING and reject every future Start
+		 * (Service) Discovery with MGMT_STATUS_BUSY.
+		 */
+		hci_dev_lock(hdev);
+		if (hdev->discovery.state == DISCOVERY_STARTING)
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		hci_dev_unlock(hdev);
+
+		if (err == -ECANCELED)
+			return;
+	}
+
+	if (!mgmt_pending_valid(hdev, cmd))
 		return;
 
 	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
 			  cmd->param, 1);
 	mgmt_pending_free(cmd);
 
-	hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED:
+	/* Serialise discovery.state writes against any concurrent mgmt path
+	 * holding hdev->lock; this callback runs on req_workqueue without it.
+	 */
+	hci_dev_lock(hdev);
+	hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED :
 				DISCOVERY_FINDING);
+	hci_dev_unlock(hdev);
 }
 
 static int start_discovery_sync(struct hci_dev *hdev, void *data)
@@ -6051,15 +6074,23 @@ static int start_discovery_internal(struct sock *sk, struct hci_dev *hdev,
 		goto failed;
 	}
 
+	/* Publish the transient state BEFORE queueing the work. The
+	 * completion callback runs on hdev->req_workqueue serialised by
+	 * hci_req_sync_lock, which is independent of hdev->lock; setting
+	 * the state after the queue allowed the worker to win the race
+	 * and have its terminal STOPPED/FINDING write overwritten by this
+	 * trailing STARTING write, wedging discovery in STARTING.
+	 */
+	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
+
 	err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
 				 start_discovery_complete);
 	if (err < 0) {
+		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		mgmt_pending_remove(cmd);
 		goto failed;
 	}
 
-	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
-
 failed:
 	hci_dev_unlock(hdev);
 	return err;
@@ -6178,15 +6209,19 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
 		}
 	}
 
+	/* Publish the transient state BEFORE queueing; see the comment in
+	 * start_discovery_internal() for the race details.
+	 */
+	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
+
 	err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
 				 start_discovery_complete);
 	if (err < 0) {
+		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		mgmt_pending_remove(cmd);
 		goto failed;
 	}
 
-	hci_discovery_set_state(hdev, DISCOVERY_STARTING);
-
 failed:
 	hci_dev_unlock(hdev);
 	return err;
@@ -6196,17 +6231,40 @@ static void stop_discovery_complete(struct hci_dev *hdev, void *data, int err)
 {
 	struct mgmt_pending_cmd *cmd = data;
 
-	if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
-		return;
-
 	bt_dev_dbg(hdev, "err %d", err);
 
+	if (err) {
+		/* The queued stop-discovery work failed before the normal
+		 * completion path could advance the state machine. The
+		 * caller already moved the state to DISCOVERY_STOPPING
+		 * (under hdev->lock, before queueing). Reset it here so
+		 * the gate does not wedge in STOPPING.
+		 */
+		hci_dev_lock(hdev);
+		if (hdev->discovery.state == DISCOVERY_STOPPING)
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		hci_dev_unlock(hdev);
+
+		if (err == -ECANCELED)
+			return;
+	}
+
+	if (!mgmt_pending_valid(hdev, cmd))
+		return;
+
 	mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
 			  cmd->param, 1);
 	mgmt_pending_free(cmd);
 
-	if (!err)
+	if (!err) {
+		/* Serialise discovery.state writes against any concurrent
+		 * mgmt path holding hdev->lock; this callback runs on
+		 * req_workqueue without it.
+		 */
+		hci_dev_lock(hdev);
 		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		hci_dev_unlock(hdev);
+	}
 }
 
 static int stop_discovery_sync(struct hci_dev *hdev, void *data)
@@ -6248,15 +6306,19 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
+	/* Publish the transient state BEFORE queueing; see the comment in
+	 * start_discovery_internal() for the race details.
+	 */
+	hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
+
 	err = hci_cmd_sync_queue(hdev, stop_discovery_sync, cmd,
 				 stop_discovery_complete);
 	if (err < 0) {
+		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		mgmt_pending_remove(cmd);
 		goto unlock;
 	}
 
-	hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
-
 unlock:
 	hci_dev_unlock(hdev);
 	return err;
-- 
2.43.0

RE: [v3] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1903 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=1123700

---Test result---

Test Summary:
CheckPatch                    PASS      0.78 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.13 seconds
GitLint                       PASS      0.33 seconds
SubjectPrefix                 PASS      0.16 seconds
BuildKernel                   PASS      25.73 seconds
CheckAllWarning               PASS      28.80 seconds
CheckSparse                   PASS      27.56 seconds
BuildKernel32                 PASS      25.48 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      464.04 seconds
TestRunner_mgmt-tester        FAIL      206.88 seconds
TestRunner_mesh-tester        FAIL      25.01 seconds
IncrementalBuild              PASS      28.33 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 489 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.238 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    1.832 seconds
Mesh - Send cancel - 2                               Timed out    1.976 seconds


https://github.com/bluez/bluetooth-next/pull/410

---
Regards,
Linux Bluetooth

Re: [PATCH v3] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[Luiz Augusto von Dentz]

Hi Xiuzhuo,

On Wed, Jul 8, 2026 at 5:38 AM Xiuzhuo Shang
<xiuzhuo.shang@oss.qualcomm.com> wrote:
>
> start_discovery_internal(), start_service_discovery() and stop_discovery()
> queue a cmd_sync work item and only then move the discovery state machine
> into its transient value (DISCOVERY_STARTING / DISCOVERY_STOPPING):
>
>   err = hci_cmd_sync_queue(hdev, ..._sync, cmd, ..._complete);
>   if (err < 0) { ... }
>   hci_discovery_set_state(hdev, DISCOVERY_STARTING /* or STOPPING */);
>
> The matching completion callbacks run on hdev->req_workqueue serialised
> by hci_req_sync_lock, which is independent of hdev->lock. So once the
> work has been queued, the worker can be scheduled, run the sync function
> and invoke the completion before the caller has executed the trailing
> hci_discovery_set_state(). The completion's success path writes the
> terminal state (DISCOVERY_STOPPED for stop, DISCOVERY_FINDING for start);
> the caller then overwrites it with the transient value, and the state
> machine is wedged: every subsequent Start (Service) Discovery is
> rejected by the DISCOVERY_STOPPED gate with MGMT_STATUS_BUSY (0x0a),
> with no HCI traffic generated, until bluetoothd or the adapter is
> restarted.
>
> Fix it in three parts:
>
>   1. In all three call sites move hci_discovery_set_state(STARTING /
>      STOPPING) to before hci_cmd_sync_queue(). The transient state is
>      therefore always published before any worker can run the
>      completion. On queue-submit failure, roll back to
>      DISCOVERY_STOPPED.
>
>   2. In start_discovery_complete() and stop_discovery_complete(), wrap
>      the terminal hci_discovery_set_state() call with
>      hci_dev_lock() / hci_dev_unlock(). These callbacks run without
>      hdev->lock; serialising the state write matches the pattern used
>      by mgmt_set_powered_complete() and removes any residual ordering
>      hazard against a concurrent mgmt path holding hdev->lock.
>
>   3. Generalise the "ignore -ECANCELED" early return in both completion
>      callbacks to "on any non-zero err, also reset the transient state
>      to STOPPED". With (1) in place the state observed at completion
>      time is always known.
>
>      For the stop path this also fixes a pre-existing wedge: when any
>      sub-command issued from hci_stop_discovery_sync() returns an
>      error, stop_discovery_complete() is invoked with err != 0. The
>      existing "if (!err) set_state(STOPPED)" tail then skips the reset
>      and the state machine sits in DISCOVERY_STOPPING forever.
>
> Fixes: abfeea476c68 ("Bluetooth: hci_sync: Convert MGMT_OP_START_DISCOVERY")
> Signed-off-by: Xiuzhuo Shang <xiuzhuo.shang@oss.qualcomm.com>
> ---
> Changes in v3:
>  - Replace inline patch title with lore.kernel.org URL in v2 link
>    reference to fix GitLint B1 line-length check.
>  - Link to v2:
>    https://lore.kernel.org/all/20260708062009.3047447-1-xiuzhuo.shang@oss.qualcomm.com/
>
> Changes in v2:
>  - Fix if (err < 0) to if (err) in both start_discovery_complete() and
>    stop_discovery_complete() to also catch positive HCI status codes,
>    flagged by Sashiko.
>  - Add Fixes: tag for commit abfeea476c68 as requested.
>  - Update commit message wording from "err < 0" to "non-zero err" to
>    match the code change.
>  - Link to v1:
>    https://lore.kernel.org/all/20260707093426.372897-1-xiuzhuo.shang@oss.qualcomm.com/
>
>  net/bluetooth/mgmt.c | 86 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 74 insertions(+), 12 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 733a4b70e10c..81c09c24a14b 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -5975,15 +5975,38 @@ static void start_discovery_complete(struct hci_dev *hdev, void *data, int err)
>
>         bt_dev_dbg(hdev, "err %d", err);
>
> -       if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
> +       if (err) {
> +               /* The queued start-discovery work failed before the normal
> +                * completion path could advance the state machine. The
> +                * caller already moved the state to DISCOVERY_STARTING
> +                * (under hdev->lock, before queueing). Reset it here so the
> +                * gate in start_discovery_internal()/start_service_discovery()
> +                * does not wedge in STARTING and reject every future Start
> +                * (Service) Discovery with MGMT_STATUS_BUSY.
> +                */
> +               hci_dev_lock(hdev);
> +               if (hdev->discovery.state == DISCOVERY_STARTING)
> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +               hci_dev_unlock(hdev);
> +
> +               if (err == -ECANCELED)
> +                       return;
> +       }
> +
> +       if (!mgmt_pending_valid(hdev, cmd))
>                 return;
>
>         mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
>                           cmd->param, 1);
>         mgmt_pending_free(cmd);
>
> -       hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED:
> +       /* Serialise discovery.state writes against any concurrent mgmt path
> +        * holding hdev->lock; this callback runs on req_workqueue without it.
> +        */
> +       hci_dev_lock(hdev);
> +       hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED :
>                                 DISCOVERY_FINDING);
> +       hci_dev_unlock(hdev);
>  }
>
>  static int start_discovery_sync(struct hci_dev *hdev, void *data)
> @@ -6051,15 +6074,23 @@ static int start_discovery_internal(struct sock *sk, struct hci_dev *hdev,
>                 goto failed;
>         }
>
> +       /* Publish the transient state BEFORE queueing the work. The
> +        * completion callback runs on hdev->req_workqueue serialised by
> +        * hci_req_sync_lock, which is independent of hdev->lock; setting
> +        * the state after the queue allowed the worker to win the race
> +        * and have its terminal STOPPED/FINDING write overwritten by this
> +        * trailing STARTING write, wedging discovery in STARTING.
> +        */
> +       hci_discovery_set_state(hdev, DISCOVERY_STARTING);

I guess I will need to repeat myself, now that the callbacks do
acquire hdev lock the comments above are invalid, the callback will
attempt to hdev->lock making it wait the hci_dev_unlock bellow past
setting DISCOVERY_STARTING, so the change above is no longer needed.

>         err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
>                                  start_discovery_complete);
>         if (err < 0) {
> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>                 mgmt_pending_remove(cmd);
>                 goto failed;
>         }
>
> -       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
> -
>  failed:
>         hci_dev_unlock(hdev);
>         return err;
> @@ -6178,15 +6209,19 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
>                 }
>         }
>
> +       /* Publish the transient state BEFORE queueing; see the comment in
> +        * start_discovery_internal() for the race details.
> +        */
> +       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
> +
>         err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
>                                  start_discovery_complete);
>         if (err < 0) {
> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>                 mgmt_pending_remove(cmd);
>                 goto failed;
>         }
>
> -       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
> -
>  failed:
>         hci_dev_unlock(hdev);
>         return err;
> @@ -6196,17 +6231,40 @@ static void stop_discovery_complete(struct hci_dev *hdev, void *data, int err)
>  {
>         struct mgmt_pending_cmd *cmd = data;
>
> -       if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
> -               return;
> -
>         bt_dev_dbg(hdev, "err %d", err);
>
> +       if (err) {
> +               /* The queued stop-discovery work failed before the normal
> +                * completion path could advance the state machine. The
> +                * caller already moved the state to DISCOVERY_STOPPING
> +                * (under hdev->lock, before queueing). Reset it here so
> +                * the gate does not wedge in STOPPING.
> +                */
> +               hci_dev_lock(hdev);
> +               if (hdev->discovery.state == DISCOVERY_STOPPING)
> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +               hci_dev_unlock(hdev);
> +
> +               if (err == -ECANCELED)
> +                       return;
> +       }
> +
> +       if (!mgmt_pending_valid(hdev, cmd))
> +               return;
> +
>         mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
>                           cmd->param, 1);
>         mgmt_pending_free(cmd);
>
> -       if (!err)
> +       if (!err) {
> +               /* Serialise discovery.state writes against any concurrent
> +                * mgmt path holding hdev->lock; this callback runs on
> +                * req_workqueue without it.
> +                */
> +               hci_dev_lock(hdev);
>                 hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +               hci_dev_unlock(hdev);
> +       }
>  }
>
>  static int stop_discovery_sync(struct hci_dev *hdev, void *data)
> @@ -6248,15 +6306,19 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>                 goto unlock;
>         }
>
> +       /* Publish the transient state BEFORE queueing; see the comment in
> +        * start_discovery_internal() for the race details.
> +        */
> +       hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
> +
>         err = hci_cmd_sync_queue(hdev, stop_discovery_sync, cmd,
>                                  stop_discovery_complete);
>         if (err < 0) {
> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>                 mgmt_pending_remove(cmd);
>                 goto unlock;
>         }
>
> -       hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
> -
>  unlock:
>         hci_dev_unlock(hdev);
>         return err;
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v3] Bluetooth: MGMT: Fix discovery state race against cmd_sync worker

[Xiuzhuo Shang]

On 7/8/2026 9:51 PM, Luiz Augusto von Dentz wrote:
> Hi Xiuzhuo,
> 
> On Wed, Jul 8, 2026 at 5:38 AM Xiuzhuo Shang
> <xiuzhuo.shang@oss.qualcomm.com> wrote:
>>
>> start_discovery_internal(), start_service_discovery() and stop_discovery()
>> queue a cmd_sync work item and only then move the discovery state machine
>> into its transient value (DISCOVERY_STARTING / DISCOVERY_STOPPING):
>>
>>   err = hci_cmd_sync_queue(hdev, ..._sync, cmd, ..._complete);
>>   if (err < 0) { ... }
>>   hci_discovery_set_state(hdev, DISCOVERY_STARTING /* or STOPPING */);
>>
>> The matching completion callbacks run on hdev->req_workqueue serialised
>> by hci_req_sync_lock, which is independent of hdev->lock. So once the
>> work has been queued, the worker can be scheduled, run the sync function
>> and invoke the completion before the caller has executed the trailing
>> hci_discovery_set_state(). The completion's success path writes the
>> terminal state (DISCOVERY_STOPPED for stop, DISCOVERY_FINDING for start);
>> the caller then overwrites it with the transient value, and the state
>> machine is wedged: every subsequent Start (Service) Discovery is
>> rejected by the DISCOVERY_STOPPED gate with MGMT_STATUS_BUSY (0x0a),
>> with no HCI traffic generated, until bluetoothd or the adapter is
>> restarted.
>>
>> Fix it in three parts:
>>
>>   1. In all three call sites move hci_discovery_set_state(STARTING /
>>      STOPPING) to before hci_cmd_sync_queue(). The transient state is
>>      therefore always published before any worker can run the
>>      completion. On queue-submit failure, roll back to
>>      DISCOVERY_STOPPED.
>>
>>   2. In start_discovery_complete() and stop_discovery_complete(), wrap
>>      the terminal hci_discovery_set_state() call with
>>      hci_dev_lock() / hci_dev_unlock(). These callbacks run without
>>      hdev->lock; serialising the state write matches the pattern used
>>      by mgmt_set_powered_complete() and removes any residual ordering
>>      hazard against a concurrent mgmt path holding hdev->lock.
>>
>>   3. Generalise the "ignore -ECANCELED" early return in both completion
>>      callbacks to "on any non-zero err, also reset the transient state
>>      to STOPPED". With (1) in place the state observed at completion
>>      time is always known.
>>
>>      For the stop path this also fixes a pre-existing wedge: when any
>>      sub-command issued from hci_stop_discovery_sync() returns an
>>      error, stop_discovery_complete() is invoked with err != 0. The
>>      existing "if (!err) set_state(STOPPED)" tail then skips the reset
>>      and the state machine sits in DISCOVERY_STOPPING forever.
>>
>> Fixes: abfeea476c68 ("Bluetooth: hci_sync: Convert MGMT_OP_START_DISCOVERY")
>> Signed-off-by: Xiuzhuo Shang <xiuzhuo.shang@oss.qualcomm.com>
>> ---
>> Changes in v3:
>>  - Replace inline patch title with lore.kernel.org URL in v2 link
>>    reference to fix GitLint B1 line-length check.
>>  - Link to v2:
>>    https://lore.kernel.org/all/20260708062009.3047447-1-xiuzhuo.shang@oss.qualcomm.com/
>>
>> Changes in v2:
>>  - Fix if (err < 0) to if (err) in both start_discovery_complete() and
>>    stop_discovery_complete() to also catch positive HCI status codes,
>>    flagged by Sashiko.
>>  - Add Fixes: tag for commit abfeea476c68 as requested.
>>  - Update commit message wording from "err < 0" to "non-zero err" to
>>    match the code change.
>>  - Link to v1:
>>    https://lore.kernel.org/all/20260707093426.372897-1-xiuzhuo.shang@oss.qualcomm.com/
>>
>>  net/bluetooth/mgmt.c | 86 +++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 74 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 733a4b70e10c..81c09c24a14b 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -5975,15 +5975,38 @@ static void start_discovery_complete(struct hci_dev *hdev, void *data, int err)
>>
>>         bt_dev_dbg(hdev, "err %d", err);
>>
>> -       if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
>> +       if (err) {
>> +               /* The queued start-discovery work failed before the normal
>> +                * completion path could advance the state machine. The
>> +                * caller already moved the state to DISCOVERY_STARTING
>> +                * (under hdev->lock, before queueing). Reset it here so the
>> +                * gate in start_discovery_internal()/start_service_discovery()
>> +                * does not wedge in STARTING and reject every future Start
>> +                * (Service) Discovery with MGMT_STATUS_BUSY.
>> +                */
>> +               hci_dev_lock(hdev);
>> +               if (hdev->discovery.state == DISCOVERY_STARTING)
>> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +               hci_dev_unlock(hdev);
>> +
>> +               if (err == -ECANCELED)
>> +                       return;
>> +       }
>> +
>> +       if (!mgmt_pending_valid(hdev, cmd))
>>                 return;
>>
>>         mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
>>                           cmd->param, 1);
>>         mgmt_pending_free(cmd);
>>
>> -       hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED:
>> +       /* Serialise discovery.state writes against any concurrent mgmt path
>> +        * holding hdev->lock; this callback runs on req_workqueue without it.
>> +        */
>> +       hci_dev_lock(hdev);
>> +       hci_discovery_set_state(hdev, err ? DISCOVERY_STOPPED :
>>                                 DISCOVERY_FINDING);
>> +       hci_dev_unlock(hdev);
>>  }
>>
>>  static int start_discovery_sync(struct hci_dev *hdev, void *data)
>> @@ -6051,15 +6074,23 @@ static int start_discovery_internal(struct sock *sk, struct hci_dev *hdev,
>>                 goto failed;
>>         }
>>
>> +       /* Publish the transient state BEFORE queueing the work. The
>> +        * completion callback runs on hdev->req_workqueue serialised by
>> +        * hci_req_sync_lock, which is independent of hdev->lock; setting
>> +        * the state after the queue allowed the worker to win the race
>> +        * and have its terminal STOPPED/FINDING write overwritten by this
>> +        * trailing STARTING write, wedging discovery in STARTING.
>> +        */
>> +       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
> 
> I guess I will need to repeat myself, now that the callbacks do
> acquire hdev lock the comments above are invalid, the callback will
> attempt to hdev->lock making it wait the hci_dev_unlock bellow past
> setting DISCOVERY_STARTING, so the change above is no longer needed.
 Hi Luiz,
Thank you for the clarification. You are correct.

With the hci_dev_lock() / hci_dev_unlock() added to the completion callbacks in Part 2, the callbacks are forced to
wait until the caller releases hdev->lock. Since the caller holds hdev->lock across the entire hci_cmd_sync_queue()
call and the trailing hci_discovery_set_state(STARTING / STOPPING), by the time any callback can acquire hci_dev_lock,
the transient state is already published. Part 1 is therefore redundant and has been removed in v4.
Changes in v4:
- Drop the set_state-before-queue reordering (Part 1 in v1–v3)
- Update commit message from "Fix it in three parts" to "two parts" and revise the description to explain the locking

Will send v4.
> 
>>         err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
>>                                  start_discovery_complete);
>>         if (err < 0) {
>> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>                 mgmt_pending_remove(cmd);
>>                 goto failed;
>>         }
>>
>> -       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
>> -
>>  failed:
>>         hci_dev_unlock(hdev);
>>         return err;
>> @@ -6178,15 +6209,19 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
>>                 }
>>         }
>>
>> +       /* Publish the transient state BEFORE queueing; see the comment in
>> +        * start_discovery_internal() for the race details.
>> +        */
>> +       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
>> +
>>         err = hci_cmd_sync_queue(hdev, start_discovery_sync, cmd,
>>                                  start_discovery_complete);
>>         if (err < 0) {
>> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>                 mgmt_pending_remove(cmd);
>>                 goto failed;
>>         }
>>
>> -       hci_discovery_set_state(hdev, DISCOVERY_STARTING);
>> -
>>  failed:
>>         hci_dev_unlock(hdev);
>>         return err;
>> @@ -6196,17 +6231,40 @@ static void stop_discovery_complete(struct hci_dev *hdev, void *data, int err)
>>  {
>>         struct mgmt_pending_cmd *cmd = data;
>>
>> -       if (err == -ECANCELED || !mgmt_pending_valid(hdev, cmd))
>> -               return;
>> -
>>         bt_dev_dbg(hdev, "err %d", err);
>>
>> +       if (err) {
>> +               /* The queued stop-discovery work failed before the normal
>> +                * completion path could advance the state machine. The
>> +                * caller already moved the state to DISCOVERY_STOPPING
>> +                * (under hdev->lock, before queueing). Reset it here so
>> +                * the gate does not wedge in STOPPING.
>> +                */
>> +               hci_dev_lock(hdev);
>> +               if (hdev->discovery.state == DISCOVERY_STOPPING)
>> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +               hci_dev_unlock(hdev);
>> +
>> +               if (err == -ECANCELED)
>> +                       return;
>> +       }
>> +
>> +       if (!mgmt_pending_valid(hdev, cmd))
>> +               return;
>> +
>>         mgmt_cmd_complete(cmd->sk, cmd->hdev->id, cmd->opcode, mgmt_status(err),
>>                           cmd->param, 1);
>>         mgmt_pending_free(cmd);
>>
>> -       if (!err)
>> +       if (!err) {
>> +               /* Serialise discovery.state writes against any concurrent
>> +                * mgmt path holding hdev->lock; this callback runs on
>> +                * req_workqueue without it.
>> +                */
>> +               hci_dev_lock(hdev);
>>                 hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +               hci_dev_unlock(hdev);
>> +       }
>>  }
>>
>>  static int stop_discovery_sync(struct hci_dev *hdev, void *data)
>> @@ -6248,15 +6306,19 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>                 goto unlock;
>>         }
>>
>> +       /* Publish the transient state BEFORE queueing; see the comment in
>> +        * start_discovery_internal() for the race details.
>> +        */
>> +       hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>> +
>>         err = hci_cmd_sync_queue(hdev, stop_discovery_sync, cmd,
>>                                  stop_discovery_complete);
>>         if (err < 0) {
>> +               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>                 mgmt_pending_remove(cmd);
>>                 goto unlock;
>>         }
>>
>> -       hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>> -
>>  unlock:
>>         hci_dev_unlock(hdev);
>>         return err;
>> --
>> 2.43.0
>>
> 
>