Return-Path: Message-ID: <52AF55B4.6000303@hurleysoftware.com> Date: Mon, 16 Dec 2013 14:34:12 -0500 From: Peter Hurley MIME-Version: 1.0 To: Gianluca Anzolin CC: Alexander Holler , marcel@holtmann.org, Gustavo Padovan , linux-bluetooth@vger.kernel.org, gregkh@linuxfoundation.org, jslaby@suse.cz, linux-kernel@vger.kernel.org Subject: Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b References: <1377620926-23370-1-git-send-email-gianluca@sottospazio.it> <20130919162413.GG4006@joana> <52AA1854.500@ahsoftware.de> <52AA1E62.1030109@hurleysoftware.com> <52AA483E.4080706@ahsoftware.de> <20131215112413.GA8980@sottospazio.it> <52ADB6B7.5010503@hurleysoftware.com> <20131215150847.GA10288@sottospazio.it> In-Reply-To: <20131215150847.GA10288@sottospazio.it> Content-Type: text/plain; charset=UTF-8; format=flowed List-ID: On 12/15/2013 10:08 AM, Gianluca Anzolin wrote: > On Sun, Dec 15, 2013 at 09:03:35AM -0500, Peter Hurley wrote: >> On 12/15/2013 06:24 AM, Gianluca Anzolin wrote: >>> On Fri, Dec 13, 2013 at 12:35:26AM +0100, Alexander Holler wrote: >>>> Am 12.12.2013 21:36, schrieb Peter Hurley: >>>> >>>>>> What currently happens is that when one kills rfcomm (and any other >>>>>> terminal which might use that tty), the entry in /dev doesn't >>>>>> disappear. That means the same call to refcomm with the same device >>>>>> (e.g. [/dev/]rfcomm1 doesn't work. >>>>> >>>>> Thanks for the report, Alexander. >>>>> >>>>> Point 4 above details a different situation; something else is >>>>> happening. >>>>> >>>>> Would you please detail the necessary steps to reproduce this regression? >>>>> (How do you 'kill' rfcomm? etc. Shell command lines would be best.) >>>> >>>> Just call >>>> >>>> rfcomm connect rfcomm9 01:23:45:67:89:ab >>>> >>>> wait until the connection happened (a message will appear) and then >>>> press ctrl-c. This still terminates the bluetooth connection, but the >>>> device in /dev is now left. >>> >>> Yes I'm able to reproduce the regression which is indeed caused by that >>> commit. >>> >>> However I'm puzzled. Surely there is a fifth case I didn't cover because >>> when rfcomm_dev_state_change() is called, the tty_port is there but the tty is >>> not, and therefore I cannot get a reference to it and send the HUP. >> >> There is a fifth case, but it's crazy. >> >> The tty has been properly shutdown and destroyed because the tty file handle >> was closed, not hungup. The rfcomm device reference was properly put >> when the tty was released. >> >> But when the remote hangs up (and sends disc), then rfcomm_dev_state_change() >> is called -- to kill the port reference (thus the rfcomm device) that was >> instantiated locally! Ridiculous. Doubly ridiculous because it's the local >> port shutdown that closes the dlc locally that sends the disconnect (and sets >> the local dlc state) that triggers the received rfcomm_dev_state_change()! >> >> If this behavior is desirable (or necessary because it's been exposed to >> userspace), then why was the design ever reference-counted to begin with? >> >> Regards, >> Peter Hurley > > The attached patch fixes the regression by releasing the tty_port in the > shutdown method(). This way we can avoid strange games in the dlc callback > where we are constrained by the dlc lock. > > If this kind of approach is acceptable I will submit the patch for inclusion in > a separate email. This solution is acceptable to me, but I think the comment should briefly explain why this fix is necessary, and the changelog should explain why in detail. Perhaps with a fixme comment that rfcomm_tty_install() should just take over the port reference (instead of adding one) and rfcomm_tty_cleanup() should conditionally release on RFCOMM_RELEASE_ONHUP. Because then: 1) this fix would not be necessary. 2) the release in rfcomm_tty_hangup() would not be necessary 3) the second release in rfcomm_release_dev would not be necessary 4) the RFCOMM_TTY_RELEASED bit could be removed Regards, Peter Hurley