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

[Peter Hurley]

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/