Messages in this thread | | | Date | Mon, 16 Dec 2013 14:34:12 -0500 | From | Peter Hurley <> | Subject | Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b |
| |
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
| |