On 2022/09/05 17:24, Schspa Shi wrote:
>
> Tejun Heo <tj@xxxxxxxxxx> writes:
>
>> Hello,
>>
>> On Sat, Sep 03, 2022 at 07:11:18PM -0700, Luiz Augusto von Dentz wrote:
>>> And we can check for __WQ_DRAINING? Anyway checking
>>
>> Please don't do that. That's an internal flag. It shouldn't be *that*
>> difficult to avoid this without peeking into wq internal state.
>>
>> Thanks.
>
> It seems we only need to change hdev->{cmd,ncmd}_timer to
> hdev->workqueue, there will be no race because drain_workqueue will
> flush all pending work internally.
True for queue_work(), not always true for queue_delayed_work(). Explained below.
> Any new timeout work will see HCI_CMD_DRAIN_WORKQUEUE flags after we
> cancel and flushed all the delayed work.
>
If you don't mind calling
queue_work(&hdev->cmd_work) followed by hci_cmd_work() (case A below)
and/or
queue_delayed_work(&hdev->ncmd_timer) potentially followed by hci_ncmd_timeout()/hci_reset_dev() (case B and C below)
after observing HCI_CMD_DRAIN_WORKQUEUE flag.
We need to use RCU protection if you mind one of these.
Case A:
hci_dev_do_reset() {
hci_cmd_work() {
if (test_bit(HCI_RESET, &hdev->flags) ||
hci_dev_test_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE))
cancel_delayed_work(&hdev->cmd_timer);
else
queue_delayed_work(hdev->workqueue, &hdev->cmd_timer, HCI_CMD_TIMEOUT);
} else {
skb_queue_head(&hdev->cmd_q, skb);
hci_dev_set_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE);
cancel_delayed_work(&hdev->cmd_timer);
cancel_delayed_work(&hdev->ncmd_timer);
queue_work(hdev->workqueue, &hdev->cmd_work); // Queuing after setting HCI_CMD_DRAIN_WORKQUEUE despite the intent of HCI_CMD_DRAIN_WORKQUEUE...
drain_workqueue(hdev->workqueue); // Will wait for hci_cmd_timeout() queued by queue_work() to complete.
}
// Actual flush() happens here.
hci_dev_clear_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE);
}
Case B:
hci_dev_do_reset() {
handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd) {
if (!hci_dev_test_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE))
hci_dev_set_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE);
cancel_delayed_work(&hdev->cmd_timer);
queue_delayed_work(hdev->workqueue, &hdev->ncmd_timer, HCI_NCMD_TIMEOUT);
cancel_delayed_work(&hdev->ncmd_timer); // May or may not cancel hci_ncmd_timeout() queued by queue_delayed_work().
drain_workqueue(hdev->workqueue); // Will wait for hci_ncmd_timeout() queued by queue_delayed_work() to complete if cancel_delayed_work() failed to cancel.
}
// Actual flush() happens here.
hci_dev_clear_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE);
}
Case C:
hci_dev_do_reset() {
handle_cmd_cnt_and_timer(struct hci_dev *hdev, u8 ncmd) {
if (!hci_dev_test_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE))
hci_dev_set_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE);
cancel_delayed_work(&hdev->cmd_timer);
cancel_delayed_work(&hdev->ncmd_timer); // Does nothing.
queue_delayed_work(hdev->workqueue, &hdev->ncmd_timer, HCI_NCMD_TIMEOUT);
drain_workqueue(hdev->workqueue); // Will wait for hci_ncmd_timeout() queued by queue_delayed_work() to complete if delay timer has expired.
}
// Actual flush() happens here, but hci_ncmd_timeout() queued by queue_delayed_work() can be running if delay timer has not expired as of calling drain_workqueue().
hci_dev_clear_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE);
}
