Bluetooth: Host: Fix use of local variable as atomic target

In bt_conn_unref(), a local variable is used as an atomic target:

    atomic_val_t old = atomic_dec(&conn->ref);
    /* Prevent from accessing connection object */
    bool deallocated = (atomic_get(&old) == 1);
    conn = NULL;

The above call to atomic_get() cannot prevent any data race
or value staleness, and only causes confusion.
The comment that seems associated can also be misleading.

If pedantic, we can observe that in any case atomic_get() expects
an atomic_t* argument (target), not an atomic_val_t* (value).
This compiles and /works/ just fine since the Zephyr Atomic API defines
both to be the same integer type.
The equivalent C11 code, where _Atomic(T) and T are different types,
wouldn't compile.

Also add a comment to remind that automatic advertiser resumption
is deprecated.

Signed-off-by: Christophe Dufaza <[email protected]>

Author: dottspina

subsys/bluetooth/host/conn.c

@@ -1492,33 +1492,46 @@ void bt_conn_unref(struct bt_conn *conn)
 
 	__ASSERT(conn, "Invalid connection reference");
 
-	/* Storing parameters of interest so we don't access the object
-	 * after decrementing its ref-count
+	/* If we're removing the last reference, the connection object will be
+	 * considered freed and possibly reused by the Bluetooth Host stack
+	 * as soon as we decrement the counter.
+	 * To prevent access to such a recycled object, we store the parameters
+	 * of interest before decrementing the reference count,
+	 * then unset the connection pointer.
 	 */
 	conn_type = conn->type;
 	conn_role = conn->role;
 	conn_handle = conn->handle;
-
-	old = atomic_dec(&conn->ref);
-	/* Prevent from accessing connection object */
-	deallocated = (atomic_get(&old) == 1);
 	IF_ENABLED(CONFIG_BT_CONN_TX,
-		   (__ASSERT(!(deallocated && k_work_is_pending(&conn->tx_complete_work)),
-			     "tx_complete_work is pending when conn is deallocated")));
+		   (bool conn_tx_is_pending = k_work_is_pending(&conn->tx_complete_work);));
+	old = atomic_dec(&conn->ref);
 	conn = NULL;
 
 	LOG_DBG("handle %u ref %ld -> %ld", conn_handle, old, (old - 1));
 
 	__ASSERT(old > 0, "Conn reference counter is 0");
 
-	/* Slot has been freed and can be taken. No guarantees are made on requests
-	 * to claim connection object as only the first claim will be served.
+	deallocated = (old == 1);
+	IF_ENABLED(CONFIG_BT_CONN_TX,
+		   (__ASSERT(!(deallocated && conn_tx_is_pending),
+			     "tx_complete_work is pending when conn is deallocated")));
+
+	/* Notify listeners that a slot has been freed and can be taken.
+	 * No guarantees are made on requests to claim connection object
+	 * as only the first claim will be served.
 	 */
 	if (deallocated) {
 		k_sem_give(&pending_recycled_events);
 		k_work_submit(&recycled_work);
 	}
 
+	/* Use the freed slot to automatically resume LE peripheral advertising.
+	 *
+	 * This behavior is deprecated:
+	 * - 8cfad44: Bluetooth: Deprecate adv auto-resume
+	 * - #72567: Bluetooth: Advertising resume functionality is broken
+	 * - Migration guide to Zephyr v4.0.0, Automatic advertiser resumption is deprecated
+	 */
 	if (IS_ENABLED(CONFIG_BT_PERIPHERAL) && conn_type == BT_CONN_TYPE_LE &&
 	    conn_role == BT_CONN_ROLE_PERIPHERAL && deallocated) {
 		bt_le_adv_resume();