Re: [PATCH v1] Bluetooth: hci_sync: clear cmd_sync_work_list when power off

[Luiz Augusto von Dentz]

Hi Jiayang,

On Tue, Dec 3, 2024 at 12:19 PM Jiayang Mao <quic_jiaymao@xxxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> On 2024/12/3 4:41, Luiz Augusto von Dentz wrote:
> > Hi Jiayang,
> >
> > On Mon, Nov 25, 2024 at 12:51 PM Jiayang Mao <quic_jiaymao@xxxxxxxxxxx> wrote:
> >>
> >> Clear the remaining command in cmd_sync_work_list when BT is
> >> performing power off. In some cases, this list is not empty after
> >> power off. BT host will try to send more HCI commands.
> >> This can cause unexpected results.
> >
> > What commands are in the queue?
>
> If turning off BT during pairing, "hci_acl_create_conn_sync" has chances
> to be left in the queue. Then the driver will try to send the HCI
> command of creating connection but failed.

There shouldn't be happening though:

    /* Terminated due to Power Off */
    err = hci_disconnect_all_sync(hdev, HCI_ERROR_REMOTE_POWER_OFF);
    if (err)
        goto out;

    err = hci_dev_close_sync(hdev);

Perhaps there is something attempting to connect after
hci_disconnect_all_sync has completed, in that case there is a bug
around this sequence or we need to check HCI_POWERING_DOWN to not
attempt to process the connection attempts.

> >
> >> Signed-off-by: Jiayang Mao <quic_jiaymao@xxxxxxxxxxx>
> >> ---
> >>   net/bluetooth/hci_sync.c | 6 ++++++
> >>   1 file changed, 6 insertions(+)
> >>
> >> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> >> index c86f4e42e..bc622d074 100644
> >> --- a/net/bluetooth/hci_sync.c
> >> +++ b/net/bluetooth/hci_sync.c
> >> @@ -5139,6 +5139,7 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> >>   {
> >>          bool auto_off;
> >>          int err = 0;
> >> +       struct hci_cmd_sync_work_entry *entry, *tmp;
> >>
> >>          bt_dev_dbg(hdev, "");
> >>
> >> @@ -5258,6 +5259,11 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> >>          clear_bit(HCI_RUNNING, &hdev->flags);
> >>          hci_sock_dev_event(hdev, HCI_DEV_CLOSE);
> >>
> >> +       mutex_lock(&hdev->cmd_sync_work_lock);
> >> +       list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list)
> >> +               _hci_cmd_sync_cancel_entry(hdev, entry, -ECANCELED);
> >> +       mutex_unlock(&hdev->cmd_sync_work_lock);
> >
> > Seems equivalent to hci_cmd_sync_clear, that said we should have been
> > running with that lock already, also if there is a sequence like
> > close/open the close may cancel the subsequent open, so I don't think
> > we should be canceling every subsequent callback like this.
>
> In hci_cmd_sync_clear, the work cmd_sync_work and reenable_adv_work are
> canceled. hci_cmd_sync_clear is not directly called because these two
> works should not be canceled during power off.
> Do you mean the added code should be moved to other functions to avoid
> the risk of lock?
>
> Yes. This change lacks considering sequence of close/open. I will update
> the implementation to ensure it does not remove the opening and the
> operations after re-opening.
> >
> >>          /* After this point our queues are empty and no tasks are scheduled. */
> >>          hdev->close(hdev);
> >>
> >> --
> >> 2.25.1
> >>
> >
> >
>


-- 
Luiz Augusto von Dentz