[PATCH 1/2] Bluetooth: Return proper error codes on rfcomm tty init

[PATCH 1/2] Bluetooth: Return proper error codes on rfcomm tty init

[David Herrmann]

Forward error codes from tty core to the rfcomm_init caller instead of using
generic -1 errors.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 net/bluetooth/rfcomm/tty.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index c258796..2b753a3 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -1155,9 +1155,11 @@ static const struct tty_operations rfcomm_ops = {
 
 int __init rfcomm_init_ttys(void)
 {
+	int error;
+
 	rfcomm_tty_driver = alloc_tty_driver(RFCOMM_TTY_PORTS);
 	if (!rfcomm_tty_driver)
-		return -1;
+		return -ENOMEM;
 
 	rfcomm_tty_driver->owner	= THIS_MODULE;
 	rfcomm_tty_driver->driver_name	= "rfcomm";
@@ -1172,10 +1174,11 @@ int __init rfcomm_init_ttys(void)
 	rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON;
 	tty_set_operations(rfcomm_tty_driver, &rfcomm_ops);
 
-	if (tty_register_driver(rfcomm_tty_driver)) {
+	error = tty_register_driver(rfcomm_tty_driver);
+	if (error) {
 		BT_ERR("Can't register RFCOMM TTY driver");
 		put_tty_driver(rfcomm_tty_driver);
-		return -1;
+		return error;
 	}
 
 	BT_INFO("RFCOMM TTY layer initialized");
-- 
1.7.7

[PATCH 2/2] Bluetooth: Replace rfcomm tty tasklet by workqueue

[David Herrmann]

Remove old tasklets and replace by workqueue. To avoid reentrancy (which
tasklets always avoid) we use the system_nrt_wq.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 net/bluetooth/rfcomm/tty.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 2b753a3..947f1b3 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -34,6 +34,7 @@
 #include <linux/capability.h>
 #include <linux/slab.h>
 #include <linux/skbuff.h>
+#include <linux/workqueue.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -65,7 +66,7 @@ struct rfcomm_dev {
 	struct rfcomm_dlc	*dlc;
 	struct tty_struct	*tty;
 	wait_queue_head_t       wait;
-	struct tasklet_struct   wakeup_task;
+	struct work_struct	wakeup_task;
 
 	struct device		*tty_dev;
 
@@ -81,7 +82,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);
 static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig);
 
-static void rfcomm_tty_wakeup(unsigned long arg);
+static void rfcomm_tty_wakeup(struct work_struct *work);
 
 /* ---- Device functions ---- */
 static void rfcomm_dev_destruct(struct rfcomm_dev *dev)
@@ -257,7 +258,7 @@ static int rfcomm_dev_add(struct rfcomm_dev_req *req, struct rfcomm_dlc *dlc)
 	atomic_set(&dev->opened, 0);
 
 	init_waitqueue_head(&dev->wait);
-	tasklet_init(&dev->wakeup_task, rfcomm_tty_wakeup, (unsigned long) dev);
+	INIT_WORK(&dev->wakeup_task, rfcomm_tty_wakeup);
 
 	skb_queue_head_init(&dev->pending);
 
@@ -351,7 +352,7 @@ static void rfcomm_wfree(struct sk_buff *skb)
 	struct rfcomm_dev *dev = (void *) skb->sk;
 	atomic_sub(skb->truesize, &dev->wmem_alloc);
 	if (test_bit(RFCOMM_TTY_ATTACHED, &dev->flags))
-		tasklet_schedule(&dev->wakeup_task);
+		queue_work(system_nrt_wq, &dev->wakeup_task);
 	rfcomm_dev_put(dev);
 }
 
@@ -635,9 +636,10 @@ static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
 }
 
 /* ---- TTY functions ---- */
-static void rfcomm_tty_wakeup(unsigned long arg)
+static void rfcomm_tty_wakeup(struct work_struct *work)
 {
-	struct rfcomm_dev *dev = (void *) arg;
+	struct rfcomm_dev *dev = container_of(work, struct rfcomm_dev,
+								wakeup_task);
 	struct tty_struct *tty = dev->tty;
 	if (!tty)
 		return;
@@ -762,7 +764,7 @@ static void rfcomm_tty_close(struct tty_struct *tty, struct file *filp)
 		rfcomm_dlc_close(dev->dlc, 0);
 
 		clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
-		tasklet_kill(&dev->wakeup_task);
+		cancel_work_sync(&dev->wakeup_task);
 
 		rfcomm_dlc_lock(dev->dlc);
 		tty->driver_data = NULL;
-- 
1.7.7

Re: [PATCH 1/2] Bluetooth: Return proper error codes on rfcomm tty init

[Anderson Briglia]

Hi David,

On Mon, Oct 24, 2011 at 3:30 PM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> Forward error codes from tty core to the rfcomm_init caller instead of using
> generic -1 errors.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
>  net/bluetooth/rfcomm/tty.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> index c258796..2b753a3 100644
> --- a/net/bluetooth/rfcomm/tty.c
> +++ b/net/bluetooth/rfcomm/tty.c
> @@ -1155,9 +1155,11 @@ static const struct tty_operations rfcomm_ops = {
>
>  int __init rfcomm_init_ttys(void)
>  {
> +       int error;
> +
>        rfcomm_tty_driver = alloc_tty_driver(RFCOMM_TTY_PORTS);
>        if (!rfcomm_tty_driver)
> -               return -1;
> +               return -ENOMEM;
>
>        rfcomm_tty_driver->owner        = THIS_MODULE;
>        rfcomm_tty_driver->driver_name  = "rfcomm";
> @@ -1172,10 +1174,11 @@ int __init rfcomm_init_ttys(void)
>        rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON;
>        tty_set_operations(rfcomm_tty_driver, &rfcomm_ops);
>
> -       if (tty_register_driver(rfcomm_tty_driver)) {
> +       error = tty_register_driver(rfcomm_tty_driver);
> +       if (error) {
>                BT_ERR("Can't register RFCOMM TTY driver");
>                put_tty_driver(rfcomm_tty_driver);
> -               return -1;
> +               return error;

Since you are defining a new variable (error), how about use it on
other error paths and add a "goto out" and have just one return path?

Regards,

Anderson Briglia

>        }
>
>        BT_INFO("RFCOMM TTY layer initialized");
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
INdT - Instituto Nokia de tecnologia
+55 92 2126 1122
+55 92 8423 3183

Re: [PATCH 1/2] Bluetooth: Return proper error codes on rfcomm tty init

[David Herrmann]

On Mon, Oct 24, 2011 at 3:57 PM, Anderson Briglia
<anderson.briglia@openbossa.org> wrote:
> Hi David,
>
> On Mon, Oct 24, 2011 at 3:30 PM, David Herrmann
> <dh.herrmann@googlemail.com> wrote:
>> Forward error codes from tty core to the rfcomm_init caller instead of using
>> generic -1 errors.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
>> ---
>>  net/bluetooth/rfcomm/tty.c |    9 ++++++---
>>  1 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
>> index c258796..2b753a3 100644
>> --- a/net/bluetooth/rfcomm/tty.c
>> +++ b/net/bluetooth/rfcomm/tty.c
>> @@ -1155,9 +1155,11 @@ static const struct tty_operations rfcomm_ops = {
>>
>>  int __init rfcomm_init_ttys(void)
>>  {
>> +       int error;
>> +
>>        rfcomm_tty_driver = alloc_tty_driver(RFCOMM_TTY_PORTS);
>>        if (!rfcomm_tty_driver)
>> -               return -1;
>> +               return -ENOMEM;
>>
>>        rfcomm_tty_driver->owner        = THIS_MODULE;
>>        rfcomm_tty_driver->driver_name  = "rfcomm";
>> @@ -1172,10 +1174,11 @@ int __init rfcomm_init_ttys(void)
>>        rfcomm_tty_driver->init_termios.c_lflag &= ~ICANON;
>>        tty_set_operations(rfcomm_tty_driver, &rfcomm_ops);
>>
>> -       if (tty_register_driver(rfcomm_tty_driver)) {
>> +       error = tty_register_driver(rfcomm_tty_driver);
>> +       if (error) {
>>                BT_ERR("Can't register RFCOMM TTY driver");
>>                put_tty_driver(rfcomm_tty_driver);
>> -               return -1;
>> +               return error;
>
> Since you are defining a new variable (error), how about use it on
> other error paths and add a "goto out" and have just one return path?

Isn't it quite common style to have the first error path return
directly? And then the other error paths use goto. However, there is
only one additional error path left so I don't see any way to make
this look nicer.

With more than 2 error paths I agree, a goto would be more readable,
but I cannot see this making this function look nicer. Anyway, if you
guys want, I can resend with goto's.

> Regards,
>
> Anderson Briglia

Regards
David

Re: [PATCH 2/2] Bluetooth: Replace rfcomm tty tasklet by workqueue

[Gustavo Padovan]

Hi David,

* David Herrmann <dh.herrmann@googlemail.com> [2011-10-24 15:30:58 +0200]:

> Remove old tasklets and replace by workqueue. To avoid reentrancy (which
> tasklets always avoid) we use the system_nrt_wq.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
>  net/bluetooth/rfcomm/tty.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)

Both patches have been applied, thanks.

	Gustavo