lkml.org 
[lkml]   [2017]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] Bluetooth: hci_ldisc: Allow sleeping while proto locks are held.
From
Date
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.

\
 
 \ /
  Last update: 2017-10-22 17:54    [W:0.055 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site