Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Peter Hurley <peter@hurleysoftware.com>]

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