Re: [PATCH v2] Bluetooth: hci_sync: Fix not processing all entries on cmd_sync_work

[Marcel Holtmann <marcel@holtmann.org>]

Hi Luiz,

> hci_cmd_sync_queue can be called multiple times, each adding a
> hci_cmd_sync_work_entry, before hci_cmd_sync_work is run so this makes
> sure they are all dequeued properly otherwise it creates a backlog of
> entries that are never run.
> 
> Link: https://lore.kernel.org/all/CAJCQCtSeUtHCgsHXLGrSTWKmyjaQDbDNpP4rb0i+RE+L2FTXSA@mail.gmail.com/T/
> Fixes: 6a98e3836fa20 ("Bluetooth: Add helper for serialized HCI command execution")
> Tested-by: Chris Clayton <chris2553@googlemail.com>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_sync.c | 39 +++++++++++++++++++++------------------
> 1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index d146d4efae43..06c6e954dcbd 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -283,33 +283,36 @@ static void hci_cmd_sync_work(struct work_struct *work)
> 
> 	bt_dev_dbg(hdev, "");
> 
> -	mutex_lock(&hdev->cmd_sync_work_lock);
> -	entry = list_first_entry(&hdev->cmd_sync_work_list,
> -				 struct hci_cmd_sync_work_entry, list);
> -	if (entry) {
> -		list_del(&entry->list);
> +	/* Dequeue all entries and run them */
> +	while (1) {
> +		mutex_lock(&hdev->cmd_sync_work_lock);
> +		entry = list_first_entry_or_null(&hdev->cmd_sync_work_list,
> +						 struct hci_cmd_sync_work_entry,
> +						 list);
> +		if (entry)
> +			list_del(&entry->list);
> +		mutex_unlock(&hdev->cmd_sync_work_lock);
> +
> +		if (!entry)
> +			break;
> +
> 		func = entry->func;
> 		data = entry->data;
> 		destroy = entry->destroy;
> 		kfree(entry);
> -	} else {
> -		func = NULL;
> -		data = NULL;
> -		destroy = NULL;
> -	}
> -	mutex_unlock(&hdev->cmd_sync_work_lock);
> 
> -	if (func) {
> -		int err;
> +		if (func) {
> +			int err;
> 
> -		hci_req_sync_lock(hdev);
> +			hci_req_sync_lock(hdev);
> 
> -		err = func(hdev, data);
> +			err = func(hdev, data);
> 
> -		if (destroy)
> -			destroy(hdev, data, err);
> +			if (destroy)
> +				destroy(hdev, data, err);
> 
> -		hci_req_sync_unlock(hdev);
> +			hci_req_sync_unlock(hdev);
> +		}
> 	}
> }

I really don’t get this. I already gave you how I think this should look like and you still keep the massive amount of variables for no apparent reason. If I am stupid and made a mistake, then please enlighten me, otherwise this should look like this:

@@ -276,40 +276,37 @@ EXPORT_SYMBOL(__hci_cmd_sync_status);
 static void hci_cmd_sync_work(struct work_struct *work)
 {
        struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);
-       struct hci_cmd_sync_work_entry *entry;
-       hci_cmd_sync_work_func_t func;
-       hci_cmd_sync_work_destroy_t destroy;
-       void *data;
 
        bt_dev_dbg(hdev, "");
 
-       mutex_lock(&hdev->cmd_sync_work_lock);
-       entry = list_first_entry(&hdev->cmd_sync_work_list,
-                                struct hci_cmd_sync_work_entry, list);
-       if (entry) {
-               list_del(&entry->list);
-               func = entry->func;
-               data = entry->data;
-               destroy = entry->destroy;
+       /* Dequeue all entries and run them */
+       while (1) {
+               struct hci_cmd_sync_work_entry *entry;
+
+               mutex_lock(&hdev->cmd_sync_work_lock);
+               entry = list_first_entry_or_null(&hdev->cmd_sync_work_list,
+                                                struct hci_cmd_sync_work_entry,
+                                                list);
+               if (entry)
+                       list_del(&entry->list);
+               mutex_unlock(&hdev->cmd_sync_work_lock);
+
+               if (!entry)
+                       break;
+
+               bt_dev_dbg(hdev, "entry %p", entry);
+
+               if (entry->func) {
+                       int err;
+
+                       hci_req_sync_lock(hdev);
+                       err = entry->func(hdev, entry->data);
+                       if (entry->destroy)
+                               entry->destroy(hdev, entry->data, err);
+                       hci_req_sync_unlock(hdev);
+               }
+
                kfree(entry);
-       } else {
-               func = NULL;
-               data = NULL;
-               destroy = NULL;
-       }
-       mutex_unlock(&hdev->cmd_sync_work_lock);
-
-       if (func) {
-               int err;
-
-               hci_req_sync_lock(hdev);
-
-               err = func(hdev, data);
-
-               if (destroy)
-                       destroy(hdev, data, err);
-
-               hci_req_sync_unlock(hdev);
        }
 }

I amended the patch now and applied it to bluetooth-stable tree. I am not doing any more iterations to get this right.

Regards

Marcel