Return-Path: Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 7.1 \(1827\)) Subject: Re: [PATCH v2 3/4] rfcomm: always wait for a bt connection on open() From: Marcel Holtmann In-Reply-To: <1389033795-23895-4-git-send-email-gianluca@sottospazio.it> Date: Mon, 6 Jan 2014 11:17:06 -0800 Cc: "Gustavo F. Padovan" , peter@hurleysoftware.com, "linux-bluetooth@vger.kernel.org development" , Greg KH , jslaby@suse.cz, stable@vger.kernel.org Message-Id: <4321EF0E-97F4-4236-B763-0D0213BE97A8@holtmann.org> References: <1389033795-23895-1-git-send-email-gianluca@sottospazio.it> <1389033795-23895-4-git-send-email-gianluca@sottospazio.it> To: Gianluca Anzolin Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Gianluca, > This patch fixes two regressions introduced with the recent rfcomm tty > rework. > > The current code uses the carrier_raised() method to wait for the > bluetooth connection when a process opens the tty. > > However processes may open the port with the O_NONBLOCK flag or set the > CLOCAL termios flag: in these cases the open() syscall returns > immediately without waiting for the bluetooth connection to > complete. > > This behaviour confuses userspace which expects an established bluetooth > connection. > > The patch restores the old behaviour by waiting for the connection in > rfcomm_dev_activate() and removes carrier_raised() from the tty_port ops. > > As a side effect the new code also fixes the case in which the rfcomm > tty device is created with the flag RFCOMM_REUSE_DLC: the old code > didn't call device_move() and ModemManager skipped the detection > probe. Now device_move() is always called inside rfcomm_dev_activate(). > > Signed-off-by: Gianluca Anzolin > Reported-by: Andrey Vihrov > Reported-by: Beson Chow > --- > net/bluetooth/rfcomm/tty.c | 43 +++++++++++++++++++++++++++++++++++-------- > 1 file changed, 35 insertions(+), 8 deletions(-) > > diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c > index 32ef9f9..65c0699 100644 > --- a/net/bluetooth/rfcomm/tty.c > +++ b/net/bluetooth/rfcomm/tty.c > @@ -58,6 +58,7 @@ struct rfcomm_dev { > uint modem_status; > > struct rfcomm_dlc *dlc; > + wait_queue_head_t conn_wait; > > struct device *tty_dev; > > @@ -123,8 +124,37 @@ static struct device *rfcomm_get_device(struct rfcomm_dev *dev) > static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty) > { > struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port); > + DEFINE_WAIT(wait); > + int err; > + > + err = rfcomm_dlc_open(dev->dlc, &dev->src, &dev->dst, dev->channel); > + if (err) > + return err; > + > + while (1) { > + prepare_to_wait(&dev->conn_wait, &wait, TASK_INTERRUPTIBLE); > + > + if (dev->dlc->state == BT_CLOSED) { > + err = -dev->err; > + break; > + } else if (dev->dlc->state == BT_CONNECTED) > + break; > + else if (signal_pending(current)) { > + err = -ERESTARTSYS; > + break; > + } don?t do an if-else-else if statement here. Just break. if (dev->dlc->state == BT_CLOSED) { .. break; } if (dev->dlc->state == BT_CONNECTED) break; if (signal_pending(..)) { .. break; } > + > + tty_unlock(tty); > + schedule(); > + tty_lock(tty); > + } > + finish_wait(&dev->conn_wait, &wait); > > - return rfcomm_dlc_open(dev->dlc, &dev->src, &dev->dst, dev->channel); > + if (!err) > + device_move(dev->tty_dev, rfcomm_get_device(dev), > + DPM_ORDER_DEV_AFTER_PARENT); > + > + return err; > } > > /* we block the open until the dlc->state becomes BT_CONNECTED */ > @@ -151,7 +181,6 @@ static const struct tty_port_operations rfcomm_port_ops = { > .destruct = rfcomm_dev_destruct, > .activate = rfcomm_dev_activate, > .shutdown = rfcomm_dev_shutdown, > - .carrier_raised = rfcomm_dev_carrier_raised, > }; > > static struct rfcomm_dev *__rfcomm_dev_get(int id) > @@ -258,6 +287,7 @@ static int rfcomm_dev_add(struct rfcomm_dev_req *req, struct rfcomm_dlc *dlc) > > tty_port_init(&dev->port); > dev->port.ops = &rfcomm_port_ops; > + init_waitqueue_head(&dev->conn_wait); > > skb_queue_head_init(&dev->pending); > > @@ -576,12 +606,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err) > BT_DBG("dlc %p dev %p err %d", dlc, dev, err); > > dev->err = err; > - if (dlc->state == BT_CONNECTED) { > - device_move(dev->tty_dev, rfcomm_get_device(dev), > - DPM_ORDER_DEV_AFTER_PARENT); > + wake_up_interruptible(&dev->conn_wait); > > - wake_up_interruptible(&dev->port.open_wait); > - } else if (dlc->state == BT_CLOSED) > + if (dlc->state == BT_CLOSED) > tty_port_tty_hangup(&dev->port, false); > } > > @@ -1103,7 +1130,7 @@ int __init rfcomm_init_ttys(void) > rfcomm_tty_driver->subtype = SERIAL_TYPE_NORMAL; > rfcomm_tty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV; > rfcomm_tty_driver->init_termios = tty_std_termios; > - rfcomm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL; > + rfcomm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL | CLOCAL; Is adding CLOCAL by default intentional? > rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON; > tty_set_operations(rfcomm_tty_driver, &rfcomm_ops); Regards Marcel