Hi Luiz,
> The could be multiple hci_cmd_sync_work_entry enqueued when
> hci_cmd_sync_work is executed so this makes sure they are all
> dequeued properly.
might want to be a bit more verbose in the issue description.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> ---
> net/bluetooth/hci_sync.c | 65 ++++++++++++++++++++++++----------------
> 1 file changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index d146d4efae43..724d34bdd62a 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -273,43 +273,56 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen,
> }
> EXPORT_SYMBOL(__hci_cmd_sync_status);
>
> -static void hci_cmd_sync_work(struct work_struct *work)
> +
> +static void hci_cmd_sync_work_entry_run(struct hci_dev *hdev,
> + struct hci_cmd_sync_work_entry *entry)
> {
> - 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;
> + int err;
>
> - bt_dev_dbg(hdev, "");
> + bt_dev_dbg(hdev, "entry %p", entry);
>
> - 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;
> - kfree(entry);
> - } else {
> - func = NULL;
> - data = NULL;
> - destroy = NULL;
> - }
> - mutex_unlock(&hdev->cmd_sync_work_lock);
> + func = entry->func;
> + data = entry->data;
> + destroy = entry->destroy;
> + kfree(entry);
>
> - if (func) {
> - int err;
> + if (!func)
> + return;
> +
> + hci_req_sync_lock(hdev);
> +
> + err = func(hdev, data);
>
> - hci_req_sync_lock(hdev);
> + if (destroy)
> + destroy(hdev, data, err);
>
> - err = func(hdev, data);
> + hci_req_sync_unlock(hdev);
> +}
>
> - if (destroy)
> - destroy(hdev, data, err);
> +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;
> +
> + bt_dev_dbg(hdev, "");
> +
> + 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) {
> + mutex_unlock(&hdev->cmd_sync_work_lock);
> + break;
> + }
> +
> + list_del(&entry->list);
> + mutex_unlock(&hdev->cmd_sync_work_lock);
>
> - hci_req_sync_unlock(hdev);
> + hci_cmd_sync_work_entry_run(hdev, entry);
> }
> }
In general I don’t like having two unlock vs one lock in the code. I know it is correct and the tools will also understand it is correct, but I just dislike it. I think it makes it hard for human eyes to verify locking balance.
I would prefer something like this:
while (1) {
struct hci_cmd_sync_work_entry *entry;
int err;
mutex_lock(&hdev->cmd_sync_work_lock);
entry = list_first_entry_or_null(&hdev->cmd_..
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) {
hci_req_sync_lock(hdev);
err = func(hdev, entry->data);
if (entry->destroy)
entry->destroy(hdev, entry->data, err);
hci_req_sync_unlock(hdev);
}
kfree(entry);
}
This looks a lot easy to read and verify to me.
Regards
Marcel
