Re: [PATCH] Bluetooth: hci_ldisc: Allow sleeping while proto locks are held.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux