* [PATCH v5 6/6] rfcomm: Purge the dlc->tx_queue to avoid circular dependency
From: Gianluca Anzolin @ 2013-07-29 15:08 UTC (permalink / raw)
To: gustavo; +Cc: peter, marcel, linux-bluetooth, gregkh, jslaby, Gianluca Anzolin
In-Reply-To: <1375110493-5237-1-git-send-email-gianluca@sottospazio.it>
In rfcomm_tty_cleanup we purge the dlc->tx_queue which may contain
socket buffers referencing the tty_port and thus preventing the tty_port
destruction.
Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
net/bluetooth/rfcomm/tty.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 3e078b7..6d126fa 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -668,6 +668,12 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
tty->driver_data = NULL;
rfcomm_dlc_unlock(dev->dlc);
+ /*
+ * purge the dlc->tx_queue to avoid circular dependencies
+ * between dev and dlc
+ */
+ skb_queue_purge(&dev->dlc->tx_queue);
+
tty_port_put(&dev->port);
}
--
1.8.3.4
^ permalink raw reply related
* [PATCH v5 5/6] rfcomm: Fix the reference counting of tty_port
From: Gianluca Anzolin @ 2013-07-29 15:08 UTC (permalink / raw)
To: gustavo; +Cc: peter, marcel, linux-bluetooth, gregkh, jslaby, Gianluca Anzolin
In-Reply-To: <1375110493-5237-1-git-send-email-gianluca@sottospazio.it>
The tty_port can be released in two cases: when we get a HUP in the
functions rfcomm_tty_hangup() and rfcomm_dev_state_change(). Or when the
user releases the device in rfcomm_release_dev().
In these cases we set the flag RFCOMM_TTY_RELEASED so that no other
function can get a reference to the tty_port.
The use of !test_and_set_bit(RFCOMM_TTY_RELEASED) ensures that the
'initial' tty_port reference is only dropped once.
The rfcomm_dev_del function is removed becase it isn't used anymore.
Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
Notes:
v5: always use !test_and_set_bit() to release the tty_port
net/bluetooth/rfcomm/tty.c | 46 ++++++++++++----------------------------------
1 file changed, 12 insertions(+), 34 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 583f713..3e078b7 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -324,23 +324,6 @@ free:
return err;
}
-static void rfcomm_dev_del(struct rfcomm_dev *dev)
-{
- unsigned long flags;
- BT_DBG("dev %p", dev);
-
- BUG_ON(test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags));
-
- spin_lock_irqsave(&dev->port.lock, flags);
- if (dev->port.count > 0) {
- spin_unlock_irqrestore(&dev->port.lock, flags);
- return;
- }
- spin_unlock_irqrestore(&dev->port.lock, flags);
-
- tty_port_put(&dev->port);
-}
-
/* ---- Send buffer ---- */
static inline unsigned int rfcomm_room(struct rfcomm_dlc *dlc)
{
@@ -454,8 +437,9 @@ static int rfcomm_release_dev(void __user *arg)
tty_kref_put(tty);
}
- if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
- rfcomm_dev_del(dev);
+ if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+ tty_port_put(&dev->port);
+
tty_port_put(&dev->port);
return 0;
}
@@ -607,6 +591,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
* rfcomm_dev_lock -> dlc lock
* 2. tty_port_put will deadlock if it's
* the last reference
+ *
+ * FIXME: when we release the lock anything
+ * could happen to dev, even its destruction
*/
rfcomm_dlc_unlock(dlc);
if (rfcomm_dev_get(dev->id) == NULL) {
@@ -614,7 +601,10 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
return;
}
- rfcomm_dev_del(dev);
+ if (!test_and_set_bit(RFCOMM_TTY_RELEASED,
+ &dev->flags))
+ tty_port_put(&dev->port);
+
tty_port_put(&dev->port);
rfcomm_dlc_lock(dlc);
}
@@ -741,16 +731,10 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
{
struct rfcomm_dev *dev = (struct rfcomm_dev *) tty->driver_data;
- if (!dev)
- return;
-
BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc,
dev->port.count);
tty_port_close(&dev->port, tty, filp);
-
- if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
}
static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
@@ -1050,17 +1034,11 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
BT_DBG("tty %p dev %p", tty, dev);
- if (!dev)
- return;
-
tty_port_hangup(&dev->port);
- if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
- if (rfcomm_dev_get(dev->id) == NULL)
- return;
- rfcomm_dev_del(dev);
+ if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
+ !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
tty_port_put(&dev->port);
- }
}
static int rfcomm_tty_tiocmget(struct tty_struct *tty)
--
1.8.3.4
^ permalink raw reply related
* [PATCH v5 4/6] rfcomm: Implement .activate, .shutdown and .carrier_raised methods
From: Gianluca Anzolin @ 2013-07-29 15:08 UTC (permalink / raw)
To: gustavo; +Cc: peter, marcel, linux-bluetooth, gregkh, jslaby, Gianluca Anzolin
In-Reply-To: <1375110493-5237-1-git-send-email-gianluca@sottospazio.it>
Implement .activate, .shutdown and .carrier_raised methods of tty_port
to manage the dlc, moving the code from rfcomm_tty_install() and
rfcomm_tty_cleanup() functions.
At the same time the tty .open()/.close() and .hangup() methods are
changed to use the tty_port helpers that properly call the
aforementioned tty_port methods.
Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
net/bluetooth/rfcomm/tty.c | 117 ++++++++++++++++++---------------------------
1 file changed, 47 insertions(+), 70 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 73dd615..583f713 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -58,7 +58,6 @@ struct rfcomm_dev {
uint modem_status;
struct rfcomm_dlc *dlc;
- wait_queue_head_t wait;
struct device *tty_dev;
@@ -104,8 +103,39 @@ static void rfcomm_dev_destruct(struct tty_port *port)
module_put(THIS_MODULE);
}
+/* device-specific initialization: open the dlc */
+static int rfcomm_dev_activate(struct tty_port *port, struct tty_struct *tty)
+{
+ struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
+
+ return rfcomm_dlc_open(dev->dlc, &dev->src, &dev->dst, dev->channel);
+}
+
+/* we block the open until the dlc->state becomes BT_CONNECTED */
+static int rfcomm_dev_carrier_raised(struct tty_port *port)
+{
+ struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
+
+ return (dev->dlc->state == BT_CONNECTED);
+}
+
+/* device-specific cleanup: close the dlc */
+static void rfcomm_dev_shutdown(struct tty_port *port)
+{
+ struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
+
+ if (dev->tty_dev->parent)
+ device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
+
+ /* close the dlc */
+ rfcomm_dlc_close(dev->dlc, 0);
+}
+
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)
@@ -228,7 +258,6 @@ 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->wait);
skb_queue_head_init(&dev->pending);
@@ -563,9 +592,12 @@ 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;
- wake_up_interruptible(&dev->wait);
+ if (dlc->state == BT_CONNECTED) {
+ device_move(dev->tty_dev, rfcomm_get_device(dev),
+ DPM_ORDER_DEV_AFTER_PARENT);
- if (dlc->state == BT_CLOSED) {
+ wake_up_interruptible(&dev->port.open_wait);
+ } else if (dlc->state == BT_CLOSED) {
tty = tty_port_tty_get(&dev->port);
if (!tty) {
if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
@@ -640,17 +672,10 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
{
struct rfcomm_dev *dev = tty->driver_data;
- if (dev->tty_dev->parent)
- device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
-
- /* Close DLC and dettach TTY */
- rfcomm_dlc_close(dev->dlc, 0);
-
clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
rfcomm_dlc_lock(dev->dlc);
tty->driver_data = NULL;
- dev->port.tty = NULL;
rfcomm_dlc_unlock(dev->dlc);
tty_port_put(&dev->port);
@@ -662,7 +687,6 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
*/
static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
{
- DECLARE_WAITQUEUE(wait, current);
struct rfcomm_dev *dev;
struct rfcomm_dlc *dlc;
int err;
@@ -676,72 +700,30 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
/* Attach TTY and open DLC */
rfcomm_dlc_lock(dlc);
tty->driver_data = dev;
- dev->port.tty = tty;
rfcomm_dlc_unlock(dlc);
set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
/* install the tty_port */
err = tty_port_install(&dev->port, driver, tty);
- if (err < 0)
- goto error_no_dlc;
-
- err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
- if (err < 0)
- goto error_no_dlc;
+ if (err)
+ rfcomm_tty_cleanup(tty);
- /* Wait for DLC to connect */
- add_wait_queue(&dev->wait, &wait);
- while (1) {
- set_current_state(TASK_INTERRUPTIBLE);
-
- if (dlc->state == BT_CLOSED) {
- err = -dev->err;
- break;
- }
-
- if (dlc->state == BT_CONNECTED)
- break;
-
- if (signal_pending(current)) {
- err = -EINTR;
- break;
- }
-
- tty_unlock(tty);
- schedule();
- tty_lock(tty);
- }
- set_current_state(TASK_RUNNING);
- remove_wait_queue(&dev->wait, &wait);
-
- if (err < 0)
- goto error_no_connection;
-
- device_move(dev->tty_dev, rfcomm_get_device(dev),
- DPM_ORDER_DEV_AFTER_PARENT);
- return 0;
-
-error_no_connection:
- rfcomm_dlc_close(dlc, err);
-error_no_dlc:
- clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
- tty_port_put(&dev->port);
return err;
}
static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
{
struct rfcomm_dev *dev = tty->driver_data;
- unsigned long flags;
+ int err;
BT_DBG("tty %p id %d", tty, tty->index);
BT_DBG("dev %p dst %pMR channel %d opened %d", dev, &dev->dst,
dev->channel, dev->port.count);
- spin_lock_irqsave(&dev->port.lock, flags);
- dev->port.count++;
- spin_unlock_irqrestore(&dev->port.lock, flags);
+ err = tty_port_open(&dev->port, tty, filp);
+ if (err)
+ return err;
/*
* FIXME: rfcomm should use proper flow control for
@@ -758,7 +740,6 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
{
struct rfcomm_dev *dev = (struct rfcomm_dev *) tty->driver_data;
- unsigned long flags;
if (!dev)
return;
@@ -766,14 +747,10 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc,
dev->port.count);
- spin_lock_irqsave(&dev->port.lock, flags);
- if (!--dev->port.count) {
- spin_unlock_irqrestore(&dev->port.lock, flags);
+ tty_port_close(&dev->port, tty, filp);
- if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags))
- tty_port_put(&dev->port);
- } else
- spin_unlock_irqrestore(&dev->port.lock, flags);
+ if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+ tty_port_put(&dev->port);
}
static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
@@ -1076,7 +1053,7 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
if (!dev)
return;
- rfcomm_tty_flush_buffer(tty);
+ tty_port_hangup(&dev->port);
if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
if (rfcomm_dev_get(dev->id) == NULL)
@@ -1166,7 +1143,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 | CLOCAL;
+ rfcomm_tty_driver->init_termios.c_cflag = B9600 | CS8 | CREAD | HUPCL;
rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON;
tty_set_operations(rfcomm_tty_driver, &rfcomm_ops);
--
1.8.3.4
^ permalink raw reply related
* [PATCH v5 3/6] rfcomm: Move the tty initialization and cleanup out of open/close
From: Gianluca Anzolin @ 2013-07-29 15:08 UTC (permalink / raw)
To: gustavo; +Cc: peter, marcel, linux-bluetooth, gregkh, jslaby, Gianluca Anzolin
In-Reply-To: <1375110493-5237-1-git-send-email-gianluca@sottospazio.it>
Move the tty_struct initialization from rfcomm_tty_open() to
rfcomm_tty_install() and do the same for the cleanup moving the code from
rfcomm_tty_close() to rfcomm_tty_cleanup().
Add also extra error handling in rfcomm_tty_install() because, unlike
.open()/.close(), .cleanup() is not called if .install() fails.
Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
Notes:
v5: left the debug message in rfcomm_tty_open()
net/bluetooth/rfcomm/tty.c | 114 ++++++++++++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 42 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 9c0e142..73dd615 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -633,49 +633,61 @@ static void rfcomm_tty_copy_pending(struct rfcomm_dev *dev)
tty_flip_buffer_push(&dev->port);
}
-static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
+/* do the reverse of install, clearing the tty fields and releasing the
+ * reference to tty_port
+ */
+static void rfcomm_tty_cleanup(struct tty_struct *tty)
+{
+ struct rfcomm_dev *dev = tty->driver_data;
+
+ if (dev->tty_dev->parent)
+ device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
+
+ /* Close DLC and dettach TTY */
+ rfcomm_dlc_close(dev->dlc, 0);
+
+ clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
+
+ rfcomm_dlc_lock(dev->dlc);
+ tty->driver_data = NULL;
+ dev->port.tty = NULL;
+ rfcomm_dlc_unlock(dev->dlc);
+
+ tty_port_put(&dev->port);
+}
+
+/* we acquire the tty_port reference since it's here the tty is first used
+ * by setting the termios. We also populate the driver_data field and install
+ * the tty port
+ */
+static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
{
DECLARE_WAITQUEUE(wait, current);
struct rfcomm_dev *dev;
struct rfcomm_dlc *dlc;
- unsigned long flags;
- int err, id;
-
- id = tty->index;
+ int err;
- BT_DBG("tty %p id %d", tty, id);
-
- /* We don't leak this refcount. For reasons which are not entirely
- clear, the TTY layer will call our ->close() method even if the
- open fails. We decrease the refcount there, and decreasing it
- here too would cause breakage. */
- dev = rfcomm_dev_get(id);
+ dev = rfcomm_dev_get(tty->index);
if (!dev)
return -ENODEV;
- BT_DBG("dev %p dst %pMR channel %d opened %d", dev, &dev->dst,
- dev->channel, dev->port.count);
-
- spin_lock_irqsave(&dev->port.lock, flags);
- if (++dev->port.count > 1) {
- spin_unlock_irqrestore(&dev->port.lock, flags);
- return 0;
- }
- spin_unlock_irqrestore(&dev->port.lock, flags);
-
dlc = dev->dlc;
/* Attach TTY and open DLC */
-
rfcomm_dlc_lock(dlc);
tty->driver_data = dev;
dev->port.tty = tty;
rfcomm_dlc_unlock(dlc);
set_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
+ /* install the tty_port */
+ err = tty_port_install(&dev->port, driver, tty);
+ if (err < 0)
+ goto error_no_dlc;
+
err = rfcomm_dlc_open(dlc, &dev->src, &dev->dst, dev->channel);
if (err < 0)
- return err;
+ goto error_no_dlc;
/* Wait for DLC to connect */
add_wait_queue(&dev->wait, &wait);
@@ -702,15 +714,45 @@ static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
set_current_state(TASK_RUNNING);
remove_wait_queue(&dev->wait, &wait);
- if (err == 0)
- device_move(dev->tty_dev, rfcomm_get_device(dev),
- DPM_ORDER_DEV_AFTER_PARENT);
+ if (err < 0)
+ goto error_no_connection;
+
+ device_move(dev->tty_dev, rfcomm_get_device(dev),
+ DPM_ORDER_DEV_AFTER_PARENT);
+ return 0;
+
+error_no_connection:
+ rfcomm_dlc_close(dlc, err);
+error_no_dlc:
+ clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
+ tty_port_put(&dev->port);
+ return err;
+}
+
+static int rfcomm_tty_open(struct tty_struct *tty, struct file *filp)
+{
+ struct rfcomm_dev *dev = tty->driver_data;
+ unsigned long flags;
+
+ BT_DBG("tty %p id %d", tty, tty->index);
+ BT_DBG("dev %p dst %pMR channel %d opened %d", dev, &dev->dst,
+ dev->channel, dev->port.count);
+
+ spin_lock_irqsave(&dev->port.lock, flags);
+ dev->port.count++;
+ spin_unlock_irqrestore(&dev->port.lock, flags);
+
+ /*
+ * FIXME: rfcomm should use proper flow control for
+ * received data. This hack will be unnecessary and can
+ * be removed when that's implemented
+ */
rfcomm_tty_copy_pending(dev);
rfcomm_dlc_unthrottle(dev->dlc);
- return err;
+ return 0;
}
static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
@@ -727,25 +769,11 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
spin_lock_irqsave(&dev->port.lock, flags);
if (!--dev->port.count) {
spin_unlock_irqrestore(&dev->port.lock, flags);
- if (dev->tty_dev->parent)
- device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
-
- /* Close DLC and dettach TTY */
- rfcomm_dlc_close(dev->dlc, 0);
-
- clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
-
- rfcomm_dlc_lock(dev->dlc);
- tty->driver_data = NULL;
- dev->port.tty = NULL;
- rfcomm_dlc_unlock(dev->dlc);
if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags))
tty_port_put(&dev->port);
} else
spin_unlock_irqrestore(&dev->port.lock, flags);
-
- tty_port_put(&dev->port);
}
static int rfcomm_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
@@ -1118,6 +1146,8 @@ static const struct tty_operations rfcomm_ops = {
.wait_until_sent = rfcomm_tty_wait_until_sent,
.tiocmget = rfcomm_tty_tiocmget,
.tiocmset = rfcomm_tty_tiocmset,
+ .install = rfcomm_tty_install,
+ .cleanup = rfcomm_tty_cleanup,
};
int __init rfcomm_init_ttys(void)
--
1.8.3.4
^ permalink raw reply related
* [PATCH v5 2/6] rfcomm: Remove the device from the list in the destructor
From: Gianluca Anzolin @ 2013-07-29 15:08 UTC (permalink / raw)
To: gustavo; +Cc: peter, marcel, linux-bluetooth, gregkh, jslaby, Gianluca Anzolin
In-Reply-To: <1375110493-5237-1-git-send-email-gianluca@sottospazio.it>
The current code removes the device from the device list in several
places. Do it only in the destructor instead and in the error path of
rfcomm_add_dev() if the device couldn't be initialized.
Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
net/bluetooth/rfcomm/tty.c | 27 ++++++---------------------
1 file changed, 6 insertions(+), 21 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index cd7ff37..9c0e142 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -76,13 +76,6 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig);
/* ---- Device functions ---- */
-/*
- * The reason this isn't actually a race, as you no doubt have a little voice
- * screaming at you in your head, is that the refcount should never actually
- * reach zero unless the device has already been taken off the list, in
- * rfcomm_dev_del(). And if that's not true, we'll hit the BUG() in
- * rfcomm_dev_destruct() anyway.
- */
static void rfcomm_dev_destruct(struct tty_port *port)
{
struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
@@ -90,10 +83,9 @@ static void rfcomm_dev_destruct(struct tty_port *port)
BT_DBG("dev %p dlc %p", dev, dlc);
- /* Refcount should only hit zero when called from rfcomm_dev_del()
- which will have taken us off the list. Everything else are
- refcounting bugs. */
- BUG_ON(!list_empty(&dev->list));
+ spin_lock(&rfcomm_dev_lock);
+ list_del(&dev->list);
+ spin_unlock(&rfcomm_dev_lock);
rfcomm_dlc_lock(dlc);
/* Detach DLC if it's owned by this dev */
@@ -282,7 +274,9 @@ out:
dev->id, NULL);
if (IS_ERR(dev->tty_dev)) {
err = PTR_ERR(dev->tty_dev);
+ spin_lock(&rfcomm_dev_lock);
list_del(&dev->list);
+ spin_unlock(&rfcomm_dev_lock);
goto free;
}
@@ -315,10 +309,6 @@ static void rfcomm_dev_del(struct rfcomm_dev *dev)
}
spin_unlock_irqrestore(&dev->port.lock, flags);
- spin_lock(&rfcomm_dev_lock);
- list_del_init(&dev->list);
- spin_unlock(&rfcomm_dev_lock);
-
tty_port_put(&dev->port);
}
@@ -750,13 +740,8 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
dev->port.tty = NULL;
rfcomm_dlc_unlock(dev->dlc);
- if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags)) {
- spin_lock(&rfcomm_dev_lock);
- list_del_init(&dev->list);
- spin_unlock(&rfcomm_dev_lock);
-
+ if (test_bit(RFCOMM_TTY_RELEASED, &dev->flags))
tty_port_put(&dev->port);
- }
} else
spin_unlock_irqrestore(&dev->port.lock, flags);
--
1.8.3.4
^ permalink raw reply related
* [PATCH v5 1/6] rfcomm: Take proper tty_struct references
From: Gianluca Anzolin @ 2013-07-29 15:08 UTC (permalink / raw)
To: gustavo; +Cc: peter, marcel, linux-bluetooth, gregkh, jslaby, Gianluca Anzolin
In-Reply-To: <1375110493-5237-1-git-send-email-gianluca@sottospazio.it>
In net/bluetooth/rfcomm/tty.c the struct tty_struct is used without
taking references. This may lead to a use-after-free of the rfcomm tty.
Fix this by taking references properly, using the tty_port_* helpers
when possible.
The raw assignments of dev->port.tty in rfcomm_tty_open/close are
addressed in the later commit 'rfcomm: Implement .activate, .shutdown
and .carrier_raised methods'.
Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
net/bluetooth/rfcomm/tty.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index b6e44ad..cd7ff37 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -333,10 +333,9 @@ static inline unsigned int rfcomm_room(struct rfcomm_dlc *dlc)
static void rfcomm_wfree(struct sk_buff *skb)
{
struct rfcomm_dev *dev = (void *) skb->sk;
- struct tty_struct *tty = dev->port.tty;
atomic_sub(skb->truesize, &dev->wmem_alloc);
- if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags) && tty)
- tty_wakeup(tty);
+ if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags))
+ tty_port_tty_wakeup(&dev->port);
tty_port_put(&dev->port);
}
@@ -410,6 +409,7 @@ static int rfcomm_release_dev(void __user *arg)
{
struct rfcomm_dev_req req;
struct rfcomm_dev *dev;
+ struct tty_struct *tty;
if (copy_from_user(&req, arg, sizeof(req)))
return -EFAULT;
@@ -429,8 +429,11 @@ static int rfcomm_release_dev(void __user *arg)
rfcomm_dlc_close(dev->dlc, 0);
/* Shut down TTY synchronously before freeing rfcomm_dev */
- if (dev->port.tty)
- tty_vhangup(dev->port.tty);
+ tty = tty_port_tty_get(&dev->port);
+ if (tty) {
+ tty_vhangup(tty);
+ tty_kref_put(tty);
+ }
if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
rfcomm_dev_del(dev);
@@ -563,6 +566,7 @@ static void rfcomm_dev_data_ready(struct rfcomm_dlc *dlc, struct sk_buff *skb)
static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
{
struct rfcomm_dev *dev = dlc->owner;
+ struct tty_struct *tty;
if (!dev)
return;
@@ -572,7 +576,8 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
wake_up_interruptible(&dev->wait);
if (dlc->state == BT_CLOSED) {
- if (!dev->port.tty) {
+ tty = tty_port_tty_get(&dev->port);
+ if (!tty) {
if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
/* Drop DLC lock here to avoid deadlock
* 1. rfcomm_dev_get will take rfcomm_dev_lock
@@ -591,8 +596,10 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
tty_port_put(&dev->port);
rfcomm_dlc_lock(dlc);
}
- } else
- tty_hangup(dev->port.tty);
+ } else {
+ tty_hangup(tty);
+ tty_kref_put(tty);
+ }
}
}
@@ -604,10 +611,8 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
BT_DBG("dlc %p dev %p v24_sig 0x%02x", dlc, dev, v24_sig);
- if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV)) {
- if (dev->port.tty && !C_CLOCAL(dev->port.tty))
- tty_hangup(dev->port.tty);
- }
+ if ((dev->modem_status & TIOCM_CD) && !(v24_sig & RFCOMM_V24_DV))
+ tty_port_tty_hangup(&dev->port, true);
dev->modem_status =
((v24_sig & RFCOMM_V24_RTC) ? (TIOCM_DSR | TIOCM_DTR) : 0) |
--
1.8.3.4
^ permalink raw reply related
* [PATCH v5 0/6] rfcomm: Implement rfcomm as a proper tty_port
From: Gianluca Anzolin @ 2013-07-29 15:08 UTC (permalink / raw)
To: gustavo; +Cc: peter, marcel, linux-bluetooth, gregkh, jslaby, Gianluca Anzolin
This patchset addresses an issue with the rfcomm tty driver in the
current stable kernels that manifests itself as a sudden lockup of the
whole machine or as a OOPS if we are lucky enough (I wasn't).
Triggering the problem is very easy:
1) establish a bluetooth connection with a bluetooth host
2) open the tty it provides with some program
3) turn off the bluetooth host or take it out of range
After a timeout the machine freezes.
Another way to trigger these lockups is to simply release the rfcomm
tty.
This happens beacuse the underlying tty_struct objects and tty_port
objects are freed while being used: the code doesn't take proper
references to them.
The following patches address the problem by implementing a proper
tty_port driver for rfcomm.
There are still some issues left: one relevant to flow control (which is
also missing in the current code) and another relevant to a corner case
in rfcomm_dev_state_change() that I intend to fix with a future patch.
They are commented with a FIXME.
Changes from v4:
[PATCH 3/6]: left the debug message in rfcomm_tty_open()
[PATCH 5/6]: always use !test_and_set_bit() to release the tty_port
Thank you,
Gianluca
Gianluca Anzolin (6):
rfcomm: Take proper tty_struct references
rfcomm: Remove the device from the list in the destructor
rfcomm: Move the tty initialization and cleanup out of open/close
rfcomm: Implement .activate, .shutdown and .carrier_raised methods
rfcomm: Fix the reference counting of tty_port
rfcomm: Purge the dlc->tx_queue to avoid circular dependency
net/bluetooth/rfcomm/tty.c | 271 +++++++++++++++++++++------------------------
1 file changed, 126 insertions(+), 145 deletions(-)
--
1.8.3.4
^ permalink raw reply
* [PATCH BlueZ 7/7] audio/AVCTP: Do not process browsing queue until connection completes
From: Luiz Augusto von Dentz @ 2013-07-29 14:04 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1375106679-14645-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Any request processed before the connection completes will likely fail
in avctp_browsing_send so instead delay the processing.
---
profiles/audio/avctp.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 2c1aa94..845027f 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1107,6 +1107,7 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
gpointer data)
{
struct avctp *session = data;
+ struct avctp_channel *browsing = session->browsing;
char address[18];
uint16_t imtu, omtu;
GError *gerr = NULL;
@@ -1131,18 +1132,25 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
DBG("AVCTP Browsing: connected to %s", address);
- if (session->browsing == NULL)
- session->browsing = avctp_channel_create(session, chan,
+ if (browsing == NULL) {
+ browsing = avctp_channel_create(session, chan,
avctp_destroy_browsing);
+ session->browsing = browsing;
+ }
- session->browsing->imtu = imtu;
- session->browsing->omtu = omtu;
- session->browsing->buffer = g_malloc0(MAX(imtu, omtu));
- session->browsing->watch = g_io_add_watch(session->browsing->io,
+ browsing->imtu = imtu;
+ browsing->omtu = omtu;
+ browsing->buffer = g_malloc0(MAX(imtu, omtu));
+ browsing->watch = g_io_add_watch(session->browsing->io,
G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
(GIOFunc) session_browsing_cb, session);
avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTED);
+
+ /* Process any request that was pending the connection to complete */
+ if (browsing->process_id == 0 && !g_queue_is_empty(browsing->queue))
+ browsing->process_id = g_idle_add(process_queue, browsing);
+
return;
fail:
@@ -1543,6 +1551,10 @@ int avctp_send_browsing_req(struct avctp *session,
g_queue_push_tail(browsing->queue, p);
+ /* Connection did not complete, delay process of the request */
+ if (browsing->watch == 0)
+ return 0;
+
if (browsing->process_id == 0)
browsing->process_id = g_idle_add(process_queue, browsing);
--
1.8.3.1
^ permalink raw reply related
* [PATCH BlueZ 6/7] audio/AVCTP: Fix connecting browsing channel multiple times
From: Luiz Augusto von Dentz @ 2013-07-29 14:04 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1375106679-14645-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
While accepting the connection via DEFER_SETUP a second connection can
be created by using avctp_connect_browsing which will not detect one is
in course as the state has not yet changed, this can cause memory leaks
or crashes as the following:
profiles/audio/avctp.c:avctp_confirm_cb() AVCTP: incoming connect from XX:XX:XX:XX:XX:XX
profiles/audio/avctp.c:avctp_set_state() AVCTP Browsing Connecting
profiles/audio/avctp.c:avctp_connect_browsing_cb() AVCTP Browsing: connected to XX:XX:XX:XX:XX:XX
profiles/audio/avctp.c:avctp_set_state() AVCTP Browsing Connected
profiles/audio/avctp.c:avctp_connect_browsing_cb() AVCTP Browsing: connected to XX:XX:XX:XX:XX:XX
profiles/audio/avctp.c:avctp_set_state() AVCTP Browsing Connected
profiles/audio/avctp.c:session_browsing_cb() AVCTP Browsing: disconnected
profiles/audio/avctp.c:avctp_set_state() AVCTP Connected
Invalid read of size 8
at 0x41EB28: session_browsing_cb (avctp.c:842)
by 0x32F5447E05: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5448157: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5448559: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x40A28F: main (main.c:583)
Address 0x20 is not stack'd, malloc'd or (recently) free'd
---
profiles/audio/avctp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 11e9365..2c1aa94 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1328,8 +1328,10 @@ static void avctp_browsing_confirm(struct avctp *session, GIOChannel *chan,
}
if (bt_io_accept(chan, avctp_connect_browsing_cb, session, NULL,
- &err))
+ &err)) {
+ avctp_set_state(session, AVCTP_STATE_BROWSING_CONNECTING);
return;
+ }
error("Browsing: %s", err->message);
g_error_free(err);
--
1.8.3.1
^ permalink raw reply related
* [PATCH BlueZ 5/7] audio/AVRCP: Connect browsing channel in case of GetCapabilities response
From: Luiz Augusto von Dentz @ 2013-07-29 14:04 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1375106679-14645-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
If the remote stack respond to GetCapabilities before connecting to
browsing channel connect it immediatelly as some events might depend on
it to work properly.
The spec actually recommends the browsing channel to be connected
immediatelly after the control channel:
AVRCP 1.5 - Page 23
"4.1.1 Connection establishment
...
If a browsing channel is supported by both devices it shall be
established after control channel establishment. It is recommended that
the browsing channel is established immediately after the control
channel is established and released immediately before the control
channel is released to avoid unsatisfactory latency when a browsing
command is sent."
---
profiles/audio/avrcp.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 8c39d96..9f164e4 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3195,6 +3195,13 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn,
if (pdu == NULL || pdu->params[0] != CAP_EVENTS_SUPPORTED)
return FALSE;
+ /* Connect browsing if pending */
+ if (session->browsing_timer > 0) {
+ g_source_remove(session->browsing_timer);
+ session->browsing_timer = 0;
+ avctp_connect_browsing(session->conn);
+ }
+
count = pdu->params[1];
for (; count > 0; count--) {
--
1.8.3.1
^ permalink raw reply related
* [PATCH BlueZ 4/7] audio/AVRCP: Connect browsing channel when accepting the connection
From: Luiz Augusto von Dentz @ 2013-07-29 14:04 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1375106679-14645-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
If remote stack for some reason don't connect the browsing channel
connect it after a timeout (1 sec.)
---
profiles/audio/avrcp.c | 56 ++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 8 deletions(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 32b9615..8c39d96 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -131,6 +131,8 @@
#define AVRCP_CHARSET_UTF8 106
+#define AVRCP_BROWSING_TIMEOUT 1
+
#if __BYTE_ORDER == __LITTLE_ENDIAN
struct avrcp_header {
@@ -228,6 +230,7 @@ struct avrcp {
unsigned int passthrough_id;
unsigned int control_id;
unsigned int browsing_id;
+ unsigned int browsing_timer;
uint16_t supported_events;
uint16_t registered_events;
uint8_t transaction;
@@ -3267,6 +3270,11 @@ static void destroy_browsing(void *data)
static void session_init_browsing(struct avrcp *session)
{
+ if (session->browsing_timer > 0) {
+ g_source_remove(session->browsing_timer);
+ session->browsing_timer = 0;
+ }
+
session->browsing_id = avctp_register_browsing_pdu_handler(
session->conn,
handle_browsing_pdu,
@@ -3298,6 +3306,35 @@ static struct avrcp_data *data_init(struct avrcp *session, const char *uuid)
return data;
}
+static gboolean connect_browsing(gpointer user_data)
+{
+ struct avrcp *session = user_data;
+
+ session->browsing_timer = 0;
+
+ avctp_connect_browsing(session->conn);
+
+ return FALSE;
+}
+
+static void avrcp_connect_browsing(struct avrcp *session)
+{
+ /* Immediately connect browsing channel if initiator otherwise delay
+ * it to avoid possible collisions
+ */
+ if (avctp_is_initiator(session->conn)) {
+ avctp_connect_browsing(session->conn);
+ return;
+ }
+
+ if (session->browsing_timer > 0)
+ return;
+
+ session->browsing_timer = g_timeout_add_seconds(AVRCP_BROWSING_TIMEOUT,
+ connect_browsing,
+ session);
+}
+
static void target_init(struct avrcp *session)
{
struct avrcp_server *server = session->server;
@@ -3339,10 +3376,10 @@ static void target_init(struct avrcp *session)
if (session->controller == NULL)
avrcp_get_capabilities(session);
- /* Auto-connect browsing channel only if initiator */
- if (avctp_is_initiator(session->conn) &&
- target->features & AVRCP_FEATURE_BROWSING)
- avctp_connect_browsing(session->conn);
+ if (!(target->features & AVRCP_FEATURE_BROWSING))
+ return;
+
+ avrcp_connect_browsing(session);
}
static void controller_init(struct avrcp *session)
@@ -3382,10 +3419,10 @@ static void controller_init(struct avrcp *session)
if (controller->version < 0x0104)
return;
- /* Auto-connect browsing channel only if initiator */
- if (avctp_is_initiator(session->conn) &&
- controller->features & AVRCP_FEATURE_BROWSING)
- avctp_connect_browsing(session->conn);
+ if (!(controller->features & AVRCP_FEATURE_BROWSING))
+ return;
+
+ avrcp_connect_browsing(session);
}
static void session_init_control(struct avrcp *session)
@@ -3454,6 +3491,9 @@ static void session_destroy(struct avrcp *session)
server->sessions = g_slist_remove(server->sessions, session);
+ if (session->browsing_timer > 0)
+ g_source_remove(session->browsing_timer);
+
if (session->controller != NULL)
controller_destroy(session);
--
1.8.3.1
^ permalink raw reply related
* [PATCH BlueZ 3/7] audio/AVRCP: Fix not setting browsing feature in CT record
From: Luiz Augusto von Dentz @ 2013-07-29 14:04 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1375106679-14645-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
The record already contain the additional protocol containing the
browsing channel but the feature is not set as it is supposed to.
---
profiles/audio/avrcp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index b2135c5..32b9615 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -270,7 +270,8 @@ static sdp_record_t *avrcp_ct_record(void)
uint16_t feat = ( AVRCP_FEATURE_CATEGORY_1 |
AVRCP_FEATURE_CATEGORY_2 |
AVRCP_FEATURE_CATEGORY_3 |
- AVRCP_FEATURE_CATEGORY_4 );
+ AVRCP_FEATURE_CATEGORY_4 |
+ AVRCP_FEATURE_BROWSING);
record = sdp_record_alloc();
if (!record)
--
1.8.3.1
^ permalink raw reply related
* [PATCH BlueZ 2/7] audio/AVRCP: Fix not connecting Browsing channel when supported
From: Luiz Augusto von Dentz @ 2013-07-29 14:04 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1375106679-14645-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Both controller and target services can initiate the connection but
probably only one of the services will be in CONNECTING state which may
happen to not have browsing feature set.
Currently this happens when connecting BlueZ vs BlueZ where the CT record
does not have browsing feature set so if the corresponding service start
connecting it wont connect the browsing channel due to lack of feature.
---
profiles/audio/avrcp.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 1160f68..b2135c5 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3303,7 +3303,6 @@ static void target_init(struct avrcp *session)
struct avrcp_data *target;
struct avrcp_player *player;
struct btd_service *service;
- btd_service_state_t old_state = BTD_SERVICE_STATE_UNAVAILABLE;
if (session->target != NULL)
return;
@@ -3314,10 +3313,8 @@ static void target_init(struct avrcp *session)
DBG("%p version 0x%04x", target, target->version);
service = btd_device_get_service(session->dev, AVRCP_REMOTE_UUID);
- if (service != NULL) {
- old_state = btd_service_get_state(service);
+ if (service != NULL)
btd_service_connecting_complete(service, 0);
- }
if (target->version < 0x0103)
return;
@@ -3342,7 +3339,7 @@ static void target_init(struct avrcp *session)
avrcp_get_capabilities(session);
/* Auto-connect browsing channel only if initiator */
- if (old_state == BTD_SERVICE_STATE_CONNECTING &&
+ if (avctp_is_initiator(session->conn) &&
target->features & AVRCP_FEATURE_BROWSING)
avctp_connect_browsing(session->conn);
}
@@ -3352,7 +3349,6 @@ static void controller_init(struct avrcp *session)
struct avrcp_player *player;
struct btd_service *service;
struct avrcp_data *controller;
- btd_service_state_t old_state = BTD_SERVICE_STATE_UNAVAILABLE;
if (session->controller != NULL)
return;
@@ -3366,10 +3362,8 @@ static void controller_init(struct avrcp *session)
session->supported_events |= (1 << AVRCP_EVENT_VOLUME_CHANGED);
service = btd_device_get_service(session->dev, AVRCP_TARGET_UUID);
- if (service != NULL) {
- old_state = btd_service_get_state(service);
+ if (service != NULL)
btd_service_connecting_complete(service, 0);
- }
/* Only create player if category 1 is supported */
if (!(controller->features & AVRCP_FEATURE_CATEGORY_1))
@@ -3388,7 +3382,7 @@ static void controller_init(struct avrcp *session)
return;
/* Auto-connect browsing channel only if initiator */
- if (old_state == BTD_SERVICE_STATE_CONNECTING &&
+ if (avctp_is_initiator(session->conn) &&
controller->features & AVRCP_FEATURE_BROWSING)
avctp_connect_browsing(session->conn);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH BlueZ 1/7] audio/AVRCP: Fix memory leaks
From: Luiz Augusto von Dentz @ 2013-07-29 14:04 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
When the remote stack support both AVAILABLE_PLAYER_CHANGED and
ADDRESSED_PLAYER_CHANGED the player twice are inited twice leaking
the memory allocated in the first time:
16 bytes in 1 blocks are definitely lost in loss record 45 of 236
at 0x4A06409: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x32F544D89E: g_malloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5464BF7: g_memdup (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x4247CC: avrcp_get_media_player_list_rsp (avrcp.c:2432)
by 0x41ECAB: session_browsing_cb (avctp.c:826)
by 0x32F5447E05: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5448157: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5448559: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x40A28F: main (main.c:583)
16 bytes in 1 blocks are definitely lost in loss record 46 of 236
at 0x4A082F7: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x32F544D946: g_realloc (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5466D43: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5467032: g_string_insert_len (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5435287: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x421E62: avrcp_set_browsed_player_rsp (avrcp.c:2349)
by 0x41ECAB: session_browsing_cb (avctp.c:826)
by 0x32F5447E05: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5448157: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x32F5448559: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3600.3)
by 0x40A28F: main (main.c:583)
---
profiles/audio/avrcp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index a9373f3..1160f68 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2868,7 +2868,8 @@ avrcp_parse_media_player_item(struct avrcp *session, uint8_t *operands,
player = create_ct_player(session, id);
if (player == NULL)
return NULL;
- }
+ } else if (player->features != NULL)
+ return player;
mp = player->user_data;
--
1.8.3.1
^ permalink raw reply related
* [PATCH BlueZ 5/5] scanparam: Remove unneeded header include
From: Anderson Lizardo @ 2013-07-29 13:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1375106372-2255-1-git-send-email-anderson.lizardo@openbossa.org>
From: Claudio Takahasi <claudio.takahasi@openbossa.org>
---
profiles/scanparam/scan.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index ebdb7b8..0236af6 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -29,8 +29,6 @@
#include <stdbool.h>
#include <errno.h>
-#include <bluetooth/bluetooth.h>
-
#include "lib/uuid.h"
#include "log.h"
#include "plugin.h"
--
1.7.9.5
^ permalink raw reply related
* [PATCH BlueZ 4/5] scanparam: Apply convention for "GSource id" like variable
From: Anderson Lizardo @ 2013-07-29 13:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1375106372-2255-1-git-send-email-anderson.lizardo@openbossa.org>
By convention, BlueZ code checks for "Gsource id" like variables using:
if (source_id > 0)
...
Also fix the variable type to match that returned by
g_attrib_unregister().
---
profiles/scanparam/scan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index 5b9df86..ebdb7b8 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -61,7 +61,7 @@ struct scan {
uint16_t window;
uint16_t iwhandle;
uint16_t refresh_handle;
- uint16_t refresh_cb_id;
+ guint refresh_cb_id;
};
static void write_scan_params(GAttrib *attrib, uint16_t handle)
@@ -239,7 +239,7 @@ static void scan_param_remove(struct btd_service *service)
{
struct scan *scan = btd_service_get_user_data(service);
- if (scan->attrib != NULL && scan->refresh_cb_id)
+ if (scan->attrib != NULL && scan->refresh_cb_id > 0)
g_attrib_unregister(scan->attrib, scan->refresh_cb_id);
btd_device_remove_attio_callback(scan->device, scan->attioid);
--
1.7.9.5
^ permalink raw reply related
* [PATCH BlueZ 3/5] scanparam: Fix missing check for valid GAttrib when removing device
From: Anderson Lizardo @ 2013-07-29 13:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1375106372-2255-1-git-send-email-anderson.lizardo@openbossa.org>
If device is removed while there is no connection, scan->attrib will be
NULL, because its reference is managed by attio connect/disconnect
callbacks. This means that on disconnect any resources owned by GAttrib
will be properly destroyed.
---
profiles/scanparam/scan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index 0f83f8f..5b9df86 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -239,7 +239,7 @@ static void scan_param_remove(struct btd_service *service)
{
struct scan *scan = btd_service_get_user_data(service);
- if (scan->refresh_cb_id)
+ if (scan->attrib != NULL && scan->refresh_cb_id)
g_attrib_unregister(scan->attrib, scan->refresh_cb_id);
btd_device_remove_attio_callback(scan->device, scan->attioid);
--
1.7.9.5
^ permalink raw reply related
* [PATCH BlueZ 2/5] scanparam: Remove unnecessary assignment
From: Anderson Lizardo @ 2013-07-29 13:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1375106372-2255-1-git-send-email-anderson.lizardo@openbossa.org>
"scan" will be g_free()'d on this same function, so no need to assign 0
to refresh_cb_id.
---
profiles/scanparam/scan.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index 0ad2e17..0f83f8f 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -239,10 +239,8 @@ static void scan_param_remove(struct btd_service *service)
{
struct scan *scan = btd_service_get_user_data(service);
- if (scan->refresh_cb_id) {
+ if (scan->refresh_cb_id)
g_attrib_unregister(scan->attrib, scan->refresh_cb_id);
- scan->refresh_cb_id = 0;
- }
btd_device_remove_attio_callback(scan->device, scan->attioid);
btd_device_unref(scan->device);
--
1.7.9.5
^ permalink raw reply related
* [PATCH BlueZ 1/5] scanparam: Fix handle range check on descriptor discovery
From: Anderson Lizardo @ 2013-07-29 13:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
When discovering descriptors, if there is only a single descriptor, the
start handle will be equal to the end handle. It is valid to issue the
Find Information Request on this case.
Before this fix, notification for Scan Refresh characteristic was never
enabled.
---
profiles/scanparam/scan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index f5db5ed..0ad2e17 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -158,7 +158,7 @@ static void refresh_discovered_cb(GSList *chars, guint8 status,
start = chr->value_handle + 1;
end = scan->range.end;
- if (start >= end)
+ if (start > end)
return;
scan->refresh_handle = chr->value_handle;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2] Bluetooth: Fix calling request callback more than once
From: Gustavo Padovan @ 2013-07-29 12:20 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1374952274-5829-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2013-07-27 14:11:14 -0500]:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> In certain circumstances, such as an HCI driver using __hci_cmd_sync_ev
> with HCI_EV_CMD_COMPLETE as the expected completion event there is the
> chance that hci_event_packet will call hci_req_cmd_complete twice (once
> for the explicitly looked after event and another time in the actual
> handler of cmd_complete).
>
> In the case of __hci_cmd_sync_ev this introduces a race where the first
> call wakes up the blocking __hci_cmd_sync_ev and lets it complete.
> However, by the time that a second __hci_cmd_sync_ev call is already in
> progress the second hci_req_cmd_complete call (from the previous
> operation) will wake up the blocking function prematurely and cause it
> to fail, as witnessed by the following log:
>
> [ 639.232195] hci_rx_work: hci0 Event packet
> [ 639.232201] hci_req_cmd_complete: opcode 0xfc8e status 0x00
> [ 639.232205] hci_sent_cmd_data: hci0 opcode 0xfc8e
> [ 639.232210] hci_req_sync_complete: hci0 result 0x00
> [ 639.232220] hci_cmd_complete_evt: hci0 opcode 0xfc8e
> [ 639.232225] hci_req_cmd_complete: opcode 0xfc8e status 0x00
> [ 639.232228] __hci_cmd_sync_ev: hci0 end: err 0
> [ 639.232234] __hci_cmd_sync_ev: hci0
> [ 639.232238] hci_req_add_ev: hci0 opcode 0xfc8e plen 250
> [ 639.232242] hci_prepare_cmd: skb len 253
> [ 639.232246] hci_req_run: length 1
> [ 639.232250] hci_sent_cmd_data: hci0 opcode 0xfc8e
> [ 639.232255] hci_req_sync_complete: hci0 result 0x00
> [ 639.232266] hci_cmd_work: hci0 cmd_cnt 1 cmd queued 1
> [ 639.232271] __hci_cmd_sync_ev: hci0 end: err 0
> [ 639.232276] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-61)
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> v2: Only set complete callback to NULL if it was not already so
>
> net/bluetooth/hci_core.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
Patch has been applied to bluetooth.git. Thanks.
Gustavo
^ permalink raw reply
* WG: BT Certified Module + Bluetooth
From: Cesare Magri @ 2013-07-29 12:07 UTC (permalink / raw)
To: linux-bluetooth
Dear All,
I have a question regarding the BT certification of systems when using
Bluez.
Can I use Bluez together with any HCI BT-certified module without having to
repeat the BT certification, i.e. using the BT logo on my device?
Or is the BT-certification of Bluez valid only for certain Modules? If yes
which ones?
Many thanks in advance for your reply.
Cesare
^ permalink raw reply
* Re: [PATCH 1/1] Bluetooth: Fix ACL alive for long in case of non pariable devices
From: Marcel Holtmann @ 2013-07-28 22:13 UTC (permalink / raw)
To: Syam Sidhardhan; +Cc: linux-bluetooth, Syam Sidhardhan
In-Reply-To: <1375025201-18139-1-git-send-email-syamsidhardh@gmail.com>
Hi Syam,
> For certain devices (ex: HID mouse), support for authentication,
> pairing and bonding is optional. For such devices, the ACL alive
> for too long after the L2CAP disconnection.
>
> To avoid the ACL alive for too long after L2CAP disconnection, reset the ACL
> disconnect timeout back to HCI_DISCONN_TIMEOUT during L2CAP connect.
>
> While merging the commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422
> this issue might have introduced.
>
> Signed-off-by: Sang-Ki Park <sangki79.park@samsung.com>
> Signed-off-by: Chan-yeol Park <chanyeol.park@samsung.com>
> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
> ---
> v1 -> Modified the code as per the latest code.
> v2 -> Add descriptive comment as per Marcel request.
> v3 -> Moved from l2cap_conn_ready() to l2cap_connect() inorder to fix remote
> non ssp pairing timeout. Generated the patch based on bluetooth.git tree.
>
> net/bluetooth/l2cap_core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
is the any chance you have btmon or hcidump traces that would show this kind of behavior. Adding these to the commit message might be really helpful in case we have understand again why we did this.
Regards
Marcel
^ permalink raw reply
* [PATCH 1/1] Bluetooth: Fix ACL alive for long in case of non pariable devices
From: Syam Sidhardhan @ 2013-07-28 15:26 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Syam Sidhardhan
From: Syam Sidhardhan <s.syam@samsung.com>
For certain devices (ex: HID mouse), support for authentication,
pairing and bonding is optional. For such devices, the ACL alive
for too long after the L2CAP disconnection.
To avoid the ACL alive for too long after L2CAP disconnection, reset the ACL
disconnect timeout back to HCI_DISCONN_TIMEOUT during L2CAP connect.
While merging the commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422
this issue might have introduced.
Signed-off-by: Sang-Ki Park <sangki79.park@samsung.com>
Signed-off-by: Chan-yeol Park <chanyeol.park@samsung.com>
Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
Signed-off-by: Syam Sidhardhan <s.syam@samsung.com>
---
v1 -> Modified the code as per the latest code.
v2 -> Add descriptive comment as per Marcel request.
v3 -> Moved from l2cap_conn_ready() to l2cap_connect() inorder to fix remote
non ssp pairing timeout. Generated the patch based on bluetooth.git tree.
net/bluetooth/l2cap_core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 68843a2..8179fc3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3753,6 +3753,13 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
hci_conn_hold(conn->hcon);
+ /* For certain devices (ex: HID mouse), support for authentication,
+ * pairing and bonding is optional. For such devices, inorder to avoid
+ * the ACL alive for too long after L2CAP disconnection, reset the ACL
+ * disc_timeout back to HCI_DISCONN_TIMEOUT during L2CAP connect.
+ */
+ conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
+
bacpy(&bt_sk(sk)->src, conn->src);
bacpy(&bt_sk(sk)->dst, conn->dst);
chan->psm = psm;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2] Bluetooth: Fix calling request callback more than once
From: Marcel Holtmann @ 2013-07-27 19:28 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1374952274-5829-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> In certain circumstances, such as an HCI driver using __hci_cmd_sync_ev
> with HCI_EV_CMD_COMPLETE as the expected completion event there is the
> chance that hci_event_packet will call hci_req_cmd_complete twice (once
> for the explicitly looked after event and another time in the actual
> handler of cmd_complete).
>
> In the case of __hci_cmd_sync_ev this introduces a race where the first
> call wakes up the blocking __hci_cmd_sync_ev and lets it complete.
> However, by the time that a second __hci_cmd_sync_ev call is already in
> progress the second hci_req_cmd_complete call (from the previous
> operation) will wake up the blocking function prematurely and cause it
> to fail, as witnessed by the following log:
>
> [ 639.232195] hci_rx_work: hci0 Event packet
> [ 639.232201] hci_req_cmd_complete: opcode 0xfc8e status 0x00
> [ 639.232205] hci_sent_cmd_data: hci0 opcode 0xfc8e
> [ 639.232210] hci_req_sync_complete: hci0 result 0x00
> [ 639.232220] hci_cmd_complete_evt: hci0 opcode 0xfc8e
> [ 639.232225] hci_req_cmd_complete: opcode 0xfc8e status 0x00
> [ 639.232228] __hci_cmd_sync_ev: hci0 end: err 0
> [ 639.232234] __hci_cmd_sync_ev: hci0
> [ 639.232238] hci_req_add_ev: hci0 opcode 0xfc8e plen 250
> [ 639.232242] hci_prepare_cmd: skb len 253
> [ 639.232246] hci_req_run: length 1
> [ 639.232250] hci_sent_cmd_data: hci0 opcode 0xfc8e
> [ 639.232255] hci_req_sync_complete: hci0 result 0x00
> [ 639.232266] hci_cmd_work: hci0 cmd_cnt 1 cmd queued 1
> [ 639.232271] __hci_cmd_sync_ev: hci0 end: err 0
> [ 639.232276] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-61)
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> v2: Only set complete callback to NULL if it was not already so
>
> net/bluetooth/hci_core.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* [PATCH v2] Bluetooth: Fix calling request callback more than once
From: johan.hedberg @ 2013-07-27 19:11 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <A37716C3-EFF5-4D22-B8BB-60DB36CEF31B@holtmann.org>
From: Johan Hedberg <johan.hedberg@intel.com>
In certain circumstances, such as an HCI driver using __hci_cmd_sync_ev
with HCI_EV_CMD_COMPLETE as the expected completion event there is the
chance that hci_event_packet will call hci_req_cmd_complete twice (once
for the explicitly looked after event and another time in the actual
handler of cmd_complete).
In the case of __hci_cmd_sync_ev this introduces a race where the first
call wakes up the blocking __hci_cmd_sync_ev and lets it complete.
However, by the time that a second __hci_cmd_sync_ev call is already in
progress the second hci_req_cmd_complete call (from the previous
operation) will wake up the blocking function prematurely and cause it
to fail, as witnessed by the following log:
[ 639.232195] hci_rx_work: hci0 Event packet
[ 639.232201] hci_req_cmd_complete: opcode 0xfc8e status 0x00
[ 639.232205] hci_sent_cmd_data: hci0 opcode 0xfc8e
[ 639.232210] hci_req_sync_complete: hci0 result 0x00
[ 639.232220] hci_cmd_complete_evt: hci0 opcode 0xfc8e
[ 639.232225] hci_req_cmd_complete: opcode 0xfc8e status 0x00
[ 639.232228] __hci_cmd_sync_ev: hci0 end: err 0
[ 639.232234] __hci_cmd_sync_ev: hci0
[ 639.232238] hci_req_add_ev: hci0 opcode 0xfc8e plen 250
[ 639.232242] hci_prepare_cmd: skb len 253
[ 639.232246] hci_req_run: length 1
[ 639.232250] hci_sent_cmd_data: hci0 opcode 0xfc8e
[ 639.232255] hci_req_sync_complete: hci0 result 0x00
[ 639.232266] hci_cmd_work: hci0 cmd_cnt 1 cmd queued 1
[ 639.232271] __hci_cmd_sync_ev: hci0 end: err 0
[ 639.232276] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-61)
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Only set complete callback to NULL if it was not already so
net/bluetooth/hci_core.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b821b19..5b8a8cb 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3399,8 +3399,16 @@ void hci_req_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status)
*/
if (hdev->sent_cmd) {
req_complete = bt_cb(hdev->sent_cmd)->req.complete;
- if (req_complete)
+
+ if (req_complete) {
+ /* We must set the complete callback to NULL to
+ * avoid calling the callback more than once if
+ * this function gets called again.
+ */
+ bt_cb(hdev->sent_cmd)->req.complete = NULL;
+
goto call_complete;
+ }
}
/* Remove all pending commands belonging to this request */
--
1.8.3.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox