On Sun, Jul 21, 2013 at 01:04:03PM -0400, Peter Hurley wrote: > On 07/21/2013 04:08 AM, Gianluca Anzolin wrote: > >On Sat, Jul 20, 2013 at 10:11:50AM -0400, Peter Hurley wrote: > >>Sorry Gianluca, I should have been more specific here. > >> > >>There's no need to test for dlc->state == BT_CLOSED in carrier_raised(). > >>At the point where port->carrier_raised is called, the tty will have been > >>linked with the file descriptor, so if the dlc->state goes to BT_CLOSED, > >>then rfcomm_dev_state_change() will call > >> tty_hangup() -> driver hangup() -> tty_port_hangup() -> tty_port_shutdown() > >>This call chain will > >> 1. set the file_ops to hung_up_tty_fops which will cause tty_hung_up_p() to > >> return true > >> 2. clear ASYNCB_INITIALIZED in port->flags > >> 3. wakeup port->open_wait > >> > >>So an open() parked in the schedule loop of tty_port_block_til_ready() > >>will wake and exit the loop with either -EAGAIN or -ERESTARTSYS. > >> > >>rfcomm_dev_state_change() should only do a wakeup on port->open_wait when > >>dlc->state == BT_CONNECTED. > >> > >>>In case of success I should also call some device_move, > >>>rfcomm_tty_copy_pending and rfcomm_dlc_unthrottle. Could I do it in > >>>carrier_raised directly? > >> > >>I wouldn't. That would be a nasty hack and a potential problem if a > >>signal occurred. > >> > >>The device_move() isn't dependent on success, and can stay in .activate(). > > > >I changed the code and it's cleaner than before, very nice. However the > >device_move() is really dependent on success: the parent device is there only > >when the connection has been successfully established. > > Oh, right. > > >So I have to call that function after the carrier is raised, or right before. > >Since you already told me that calling it in the .carrier_raised method is > >unwise the only place left is the state_change callback of the dlc. > > That seems fine. [ Another place would be in rfcomm_tty_open() just after > tty_port_open() returns success -- the tty is still locked here so it won't > race with .close/.hangup() ] I will do it in state_change because open could be called multiple times by several user space programs. > > I do wonder why the tty device is re-parented to the host controller device. > It's obviously not for subsystem teardown. Maybe one of the bluetooth > maintainers could clarify this? Are there userspace components waiting for > this uevent? > > >Conversely in .shutdown the check for the device parent == NULL takes care of > >the scenario in which the aforementioned device_move() is never called. > > > >Another unrelated question: I'm working on the rfcomm_dev_add function to avoid > >the two nested locks. When the patch is ready should I send it separately or > >can I include it with the other patches? > > Your preference. > > Just a reminder: the dlc lock will still need to be dropped after bumping the > rfcomm_dev ref count via rfcomm_dev_get(), because when you subsequently drop > both references, rfcomm_dev destruction will attempt to gain the dlc lock, > resulting in deadlock. [Or do it as a work item] Yes I noticed that and before the last tty_port_put I will unlock the dlc. > > Regards, > Peter Hurley > Thanks, Gianluca -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
