From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751552AbdJVO57 (ORCPT ); Sun, 22 Oct 2017 10:57:59 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:49552 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751017AbdJVO55 (ORCPT ); Sun, 22 Oct 2017 10:57:57 -0400 Subject: Re: [PATCH] Bluetooth: hci_ldisc: Allow sleeping while proto locks are held. To: "Life is hard, and then you die" CC: Marcel Holtmann , Gustavo Padovan , Johan Hedberg , Lukas Wunner , , References: <20171015104045.GA25513@innovation.ch> <20171018020035.GB12182@innovation.ch> From: Dean Jenkins Message-ID: <66738b01-c25d-cda7-07f0-dbf6500eed95@mentor.com> Date: Sun, 22 Oct 2017 15:57:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171018020035.GB12182@innovation.ch> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-Originating-IP: [137.202.0.87] X-ClientProxiedBy: svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) To SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.