Messages in this thread | | | Subject | Re: [PATCH] Bluetooth: hci_ldisc: Allow sleeping while proto locks are held. | From | Dean Jenkins <> | Date | Sun, 22 Oct 2017 15:57:50 +0100 |
| |
Hi Ronald,
Sorry for my delay in replying to you.
On 18/10/17 03:00, Life is hard, and then you die wrote: > >> In fact, hci_uart_tty_close() is really a bit of a mess because it >> progressively removes resources. It is is based on old code which has been >> patched over the many years. Therefore it has kept code which is probably >> obsolete or duplicated. Ideally, hci_uart_tty_close() should be rewritten. >> >> For example, there is a call >> cancel_work_sync(&hu->write_work); >> in hci_uart_tty_close() which at first glance seems to be helpful but it is >> flawed because hci_uart_tx_wakeup() can schedule a new work item AFTER >> cancel_work_sync(&hu->write_work) runs. Therefore, locking is needed to >> prevent hci_uart_tx_wakeup() from being scheduled whilst >> HCI_UART_PROTO_READY is being cleared in hci_uart_tty_close(). > Actually, I think there's still problem: in hci_uart_tty_close() > cancel_work_sync() is called before the HCI_UART_PROTO_READY flag is > cleared, opening the following race: > > P1 P2 > cancel_work_sync() > hci_uart_tx_wakeup() > clear_bit(HCI_UART_PROTO_READY) > clean up > hci_uart_write_work() Yes, this looks bad. There is some protection in hci_uart_write_work() because hci_uart_dequeue(hu) checks HCI_UART_PROTO_READY but it will not be foolproof due to resources being removed by hci_uart_tty_close(). > > So AFAICT cancel_work_sync() needs to be done after clearing the flag: > > if (test_bit(HCI_UART_PROTO_READY, &hu->flags)) { > write_lock_irqsave(&hu->proto_lock, flags); > clear_bit(HCI_UART_PROTO_READY, &hu->flags); > write_unlock_irqrestore(&hu->proto_lock, flags); > > cancel_work_sync(&hu->write_work); // <--- I agree with this solution. I was going to suggest this but you beat me to it ;-) > > if (hdev) { > > (if HCI_UART_PROTO_READY is already clear, then no work can be pending, > so no need to cancel work in that case). > I agree with your statement.
Regards, Dean
-- Dean Jenkins Embedded Software Engineer Linux Transportation Solutions Mentor Embedded Software Division Mentor Graphics (UK) Ltd.
| |