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.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
