[PATCH] bluetooth : move children of connection device to NULL before connection down

[PATCH] bluetooth : move children of connection device to NULL before connection down

[Dave Young]

The rfcomm tty device will possibly retain even when conn is down,
and sysfs doesn't support zombie device moving, so this patch
move the tty device before conn device is destroyed.

For the bug refered please see :
http://lkml.org/lkml/2007/12/28/87

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
 net/bluetooth/hci_sysfs.c  |   17 +++++++++++++++++
 net/bluetooth/rfcomm/tty.c |    3 ++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff -upr linux/net/bluetooth/hci_sysfs.c linux.new/net/bluetooth/hci_sysfs.c
--- linux/net/bluetooth/hci_sysfs.c	2008-01-21 11:29:34.000000000 +0800
+++ linux.new/net/bluetooth/hci_sysfs.c	2008-01-21 11:33:46.000000000 +0800
@@ -316,9 +316,26 @@ void hci_conn_add_sysfs(struct hci_conn 
 	schedule_work(&conn->work);
 }
 
+static int __match_tty(struct device *dev, void *data)
+{
+	/* The rfcomm tty device will possibly retain even when conn
+	 * is down, and sysfs doesn't support move zombie device,
+	 * so we should move the device before conn device is destroyed.
+	 * Due to the only child device of hci_conn dev is rfcomm
+	 * tty_dev, here just return 1
+	 */
+	return 1;
+}
+
 static void del_conn(struct work_struct *work)
 {
+	struct device *dev;
 	struct hci_conn *conn = container_of(work, struct hci_conn, work);
+
+	while (dev = device_find_child(&conn->dev, NULL, __match_tty)) {
+		device_move(dev, NULL);
+		put_device(dev);
+	}
 	device_del(&conn->dev);
 	put_device(&conn->dev);
 }
diff -upr linux/net/bluetooth/rfcomm/tty.c linux.new/net/bluetooth/rfcomm/tty.c
--- linux/net/bluetooth/rfcomm/tty.c	2008-01-21 11:30:44.000000000 +0800
+++ linux.new/net/bluetooth/rfcomm/tty.c	2008-01-21 11:32:23.000000000 +0800
@@ -696,7 +696,8 @@ static void rfcomm_tty_close(struct tty_
 	BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc, dev->opened);
 
 	if (--dev->opened == 0) {
-		device_move(dev->tty_dev, NULL);
+		if (dev->tty_dev->parent)
+			device_move(dev->tty_dev, NULL);
 
 		/* Close DLC and dettach TTY */
 		rfcomm_dlc_close(dev->dlc, 0);

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[Dave Young]

On Mon, Jan 21, 2008 at 12:49:13PM +0800, Dave Young wrote:
> The rfcomm tty device will possibly retain even when conn is down,
> and sysfs doesn't support zombie device moving, so this patch
> move the tty device before conn device is destroyed.
> 
> For the bug refered please see :
> http://lkml.org/lkml/2007/12/28/87
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 
> 
> ---
>  net/bluetooth/hci_sysfs.c  |   17 +++++++++++++++++
>  net/bluetooth/rfcomm/tty.c |    3 ++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff -upr linux/net/bluetooth/hci_sysfs.c linux.new/net/bluetooth/hci_sysfs.c
> --- linux/net/bluetooth/hci_sysfs.c	2008-01-21 11:29:34.000000000 +0800
> +++ linux.new/net/bluetooth/hci_sysfs.c	2008-01-21 11:33:46.000000000 +0800
> @@ -316,9 +316,26 @@ void hci_conn_add_sysfs(struct hci_conn 
>  	schedule_work(&conn->work);
>  }
>  
> +static int __match_tty(struct device *dev, void *data)
> +{
> +	/* The rfcomm tty device will possibly retain even when conn
> +	 * is down, and sysfs doesn't support move zombie device,
> +	 * so we should move the device before conn device is destroyed.
> +	 * Due to the only child device of hci_conn dev is rfcomm
> +	 * tty_dev, here just return 1
> +	 */
> +	return 1;
> +}
> +
>  static void del_conn(struct work_struct *work)
>  {
> +	struct device *dev;
>  	struct hci_conn *conn = container_of(work, struct hci_conn, work);
> +
> +	while (dev = device_find_child(&conn->dev, NULL, __match_tty)) {
> +		device_move(dev, NULL);
> +		put_device(dev);
> +	}
>  	device_del(&conn->dev);
>  	put_device(&conn->dev);
>  }
> diff -upr linux/net/bluetooth/rfcomm/tty.c linux.new/net/bluetooth/rfcomm/tty.c
> --- linux/net/bluetooth/rfcomm/tty.c	2008-01-21 11:30:44.000000000 +0800
> +++ linux.new/net/bluetooth/rfcomm/tty.c	2008-01-21 11:32:23.000000000 +0800
> @@ -696,7 +696,8 @@ static void rfcomm_tty_close(struct tty_
>  	BT_DBG("tty %p dev %p dlc %p opened %d", tty, dev, dev->dlc, dev->opened);
>  
>  	if (--dev->opened == 0) {
> -		device_move(dev->tty_dev, NULL);
> +		if (dev->tty_dev->parent)
> +			device_move(dev->tty_dev, NULL);
>  
>  		/* Close DLC and dettach TTY */
>  		rfcomm_dlc_close(dev->dlc, 0);

Add people missed in cc-list.

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[David Miller]

From: Dave Young <hidave.darkstar@gmail.com>
Date: Mon, 21 Jan 2008 12:54:01 +0800

> Add people missed in cc-list.

Thanks Dave for your continued efforts on Bluetooth bugs like this.

Marcel, are you going to review/ACK/integrate/push-upstream/whatever
any of these Bluetooth patches?

It hasn't been getting much love from you as of late, you are one of
the listed maintainers, and I don't want to lose any of Dave's
valuable bug fixing work.

Or should I just handle it all directly?

Thanks.

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[Marcel Holtmann]

Hi Dave,

> > Add people missed in cc-list.
> 
> Thanks Dave for your continued efforts on Bluetooth bugs like this.
> 
> Marcel, are you going to review/ACK/integrate/push-upstream/whatever
> any of these Bluetooth patches?
> 
> It hasn't been getting much love from you as of late, you are one of
> the listed maintainers, and I don't want to lose any of Dave's
> valuable bug fixing work.

I will be fully back in business next week. Just got stuck in a project
that needed 200% of my time to get it going.

> Or should I just handle it all directly?

I followed the list only a little bit, but from what I have seen is that
Dave is doing a great job in tracking all issues down to the real cause.

I had a look at his last patch and after review, I agree that this is a
possible solution. I only have two nitpicks about the coding style. So
in del_conn the struct device declaration should be made after the
struct hci_conn assignment from the container and I would put an extra
empty line before the devel_del, put_device block. Nitpicks only.

Right now I can't think of any side effects by this patch. Actually I
only see an improvement with this patch. So please take it directly and
starting with next week, I gonna make sure that they are handled again
properly by me.

Regards

Marcel

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[David Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Tue, 22 Jan 2008 07:18:16 +0100

> Right now I can't think of any side effects by this patch. Actually I
> only see an improvement with this patch. So please take it directly and
> starting with next week, I gonna make sure that they are handled again
> properly by me.

Excellent, I'll do that.

Thanks for the feedback.

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[Dave Young]

On Jan 22, 2008 2:18 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Dave,
>
> > > Add people missed in cc-list.
> >
> > Thanks Dave for your continued efforts on Bluetooth bugs like this.
> >
> > Marcel, are you going to review/ACK/integrate/push-upstream/whatever
> > any of these Bluetooth patches?
> >
> > It hasn't been getting much love from you as of late, you are one of
> > the listed maintainers, and I don't want to lose any of Dave's
> > valuable bug fixing work.

Thanks.

>
> I will be fully back in business next week. Just got stuck in a project
> that needed 200% of my time to get it going.
>
> > Or should I just handle it all directly?
>
> I followed the list only a little bit, but from what I have seen is that
> Dave is doing a great job in tracking all issues down to the real cause.

Thanks.

>
> I had a look at his last patch and after review, I agree that this is a
> possible solution. I only have two nitpicks about the coding style. So
> in del_conn the struct device declaration should be made after the
> struct hci_conn assignment from the container and I would put an extra
> empty line before the devel_del, put_device block. Nitpicks only.

Marcel, could you tell something more about your coding style?
I would like to submit patches about bluetooth according to your sytle
later If I have.

Maybe you could put it on the bluez web site or anywhere.

>
> Right now I can't think of any side effects by this patch. Actually I
> only see an improvement with this patch. So please take it directly and
> starting with next week, I gonna make sure that they are handled again
> properly by me.
>
> Regards
>
> Marcel
>
>
>

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[Marcel Holtmann]

Hi Dave,

> could you tell something more about your coding style?
> I would like to submit patches about bluetooth according to your sytle
> later If I have.
> 
> Maybe you could put it on the bluez web site or anywhere.

it follows closely the kernel coding style as layout within the kernel
documentation. However there are some minor style things, that I am
going to enforce from time to time. Like having the container_of or
get_user_data calls at the top of the variable declaration. This has
never formalized as far as I recall, but makes from my point of view the
code clearer and easier to read.

Some other times I like an extra empty line to more visual separate
different code blocks. For this some people might agree with me others
might disagree. It is fully a personal more liking one way over the
other.

When it comes to indentation and placement of braces etc. I is 100% the
kernel coding style and nothing else. If not, then it needs fixing and
is an oversight from the old days.

Regards

Marcel

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[Andrew Morton]

> On Tue, 22 Jan 2008 07:18:16 +0100 Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Dave,
> 
> > > Add people missed in cc-list.
> > 
> > Thanks Dave for your continued efforts on Bluetooth bugs like this.
> > 
> > Marcel, are you going to review/ACK/integrate/push-upstream/whatever
> > any of these Bluetooth patches?
> > 
> > It hasn't been getting much love from you as of late, you are one of
> > the listed maintainers, and I don't want to lose any of Dave's
> > valuable bug fixing work.
> 
> I will be fully back in business next week. Just got stuck in a project
> that needed 200% of my time to get it going.
>

These patches in -mm:

bluetooth-hidp_process_hid_control-remove-unnecessary-parameter-dealing.patch
bluetooth-uninlining.patch
drivers-bluetooth-bpa10xc-fix-memleak.patch
drivers-bluetooth-btsdioc-fix-double-free.patch
bluetooth-blacklist-another-broadcom-bcm2035-device.patch
bluetooth-rfcomm-tty_close-before-destruct.patch
hci_ldisc-fix-null-pointer-deref.patch

could benefit from some attention please.

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[Dave Young]

On Wed, Jan 23, 2008 at 02:06:29PM -0800, Andrew Morton wrote:
> > On Tue, 22 Jan 2008 07:18:16 +0100 Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Dave,
> > 
> > > > Add people missed in cc-list.
> > > 
> > > Thanks Dave for your continued efforts on Bluetooth bugs like this.
> > > 
> > > Marcel, are you going to review/ACK/integrate/push-upstream/whatever
> > > any of these Bluetooth patches?
> > > 
> > > It hasn't been getting much love from you as of late, you are one of
> > > the listed maintainers, and I don't want to lose any of Dave's
> > > valuable bug fixing work.
> > 
> > I will be fully back in business next week. Just got stuck in a project
> > that needed 200% of my time to get it going.
> >
> 
> These patches in -mm:
> 
> bluetooth-hidp_process_hid_control-remove-unnecessary-parameter-dealing.patch
> bluetooth-uninlining.patch
> drivers-bluetooth-bpa10xc-fix-memleak.patch
> drivers-bluetooth-btsdioc-fix-double-free.patch
> bluetooth-blacklist-another-broadcom-bcm2035-device.patch
> bluetooth-rfcomm-tty_close-before-destruct.patch
> hci_ldisc-fix-null-pointer-deref.patch
> 
> could benefit from some attention please.

Hi, andrew

For the patch bluetooth-rfcomm-tty_close-before-destruct.patch I have to rethinkabout it.

1. The subject is not correct, should be rfcomm-tty-destroy-before-tty_close.
2. Don't know what I was thinking that time, could you replace it with the following better one? Sorry for that.

---
rfcomm dev could be deleted in tty_hangup, so we must not call rfcomm_dev_del again to prevent from destroying rfcomm dev before tty close.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
 net/bluetooth/rfcomm/tty.c |    2 ++
 1 file changed, 2 insertions(+)

diff -upr a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
--- a/net/bluetooth/rfcomm/tty.c	2008-01-24 09:03:59.000000000 +0800
+++ b/net/bluetooth/rfcomm/tty.c	2008-01-24 09:03:59.000000000 +0800
@@ -429,6 +429,8 @@ static int rfcomm_release_dev(void __use
 	if (dev->tty)
 		tty_vhangup(dev->tty);
 
+	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+		rfcomm_dev_del(dev);
 	rfcomm_dev_del(dev);
 	rfcomm_dev_put(dev);
 	return 0;

Re: [PATCH] bluetooth : move children of connection device to NULL before connection down

[Dave Young]

On Thu, Jan 24, 2008 at 09:19:26AM +0800, Dave Young wrote:
> On Wed, Jan 23, 2008 at 02:06:29PM -0800, Andrew Morton wrote:
> > > On Tue, 22 Jan 2008 07:18:16 +0100 Marcel Holtmann <marcel@holtmann.org> wrote:
> > > Hi Dave,
> > > 
> > > > > Add people missed in cc-list.
> > > > 
> > > > Thanks Dave for your continued efforts on Bluetooth bugs like this.
> > > > 
> > > > Marcel, are you going to review/ACK/integrate/push-upstream/whatever
> > > > any of these Bluetooth patches?
> > > > 
> > > > It hasn't been getting much love from you as of late, you are one of
> > > > the listed maintainers, and I don't want to lose any of Dave's
> > > > valuable bug fixing work.
> > > 
> > > I will be fully back in business next week. Just got stuck in a project
> > > that needed 200% of my time to get it going.
> > >
> > 
> > These patches in -mm:
> > 
> > bluetooth-hidp_process_hid_control-remove-unnecessary-parameter-dealing.patch
> > bluetooth-uninlining.patch
> > drivers-bluetooth-bpa10xc-fix-memleak.patch
> > drivers-bluetooth-btsdioc-fix-double-free.patch
> > bluetooth-blacklist-another-broadcom-bcm2035-device.patch
> > bluetooth-rfcomm-tty_close-before-destruct.patch
> > hci_ldisc-fix-null-pointer-deref.patch
> > 
> > could benefit from some attention please.
> 
> Hi, andrew
> 
> For the patch bluetooth-rfcomm-tty_close-before-destruct.patch I have to rethinkabout it.
> 
> 1. The subject is not correct, should be rfcomm-tty-destroy-before-tty_close.
> 2. Don't know what I was thinking that time, could you replace it with the following better one? Sorry for that.
> 
> ---
> rfcomm dev could be deleted in tty_hangup, so we must not call rfcomm_dev_del again to prevent from destroying rfcomm dev before tty close.
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 
> 
> ---
>  net/bluetooth/rfcomm/tty.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -upr a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
> --- a/net/bluetooth/rfcomm/tty.c	2008-01-24 09:03:59.000000000 +0800
> +++ b/net/bluetooth/rfcomm/tty.c	2008-01-24 09:03:59.000000000 +0800
> @@ -429,6 +429,8 @@ static int rfcomm_release_dev(void __use
>  	if (dev->tty)
>  		tty_vhangup(dev->tty);
>  
> +	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
> +		rfcomm_dev_del(dev);
>  	rfcomm_dev_del(dev);
	~~~~~~~~~~~~~~~~~
>  	rfcomm_dev_put(dev);
>  	return 0;

Please ignore the previous silly one, now resubmit :
----------

rfcomm dev could be deleted in tty_hangup, so we must not call rfcomm_dev_del again to prevent from destroying rfcomm dev before tty close.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
 net/bluetooth/rfcomm/tty.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -upr a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
--- a/net/bluetooth/rfcomm/tty.c	2008-01-24 09:21:56.000000000 +0800
+++ b/net/bluetooth/rfcomm/tty.c	2008-01-24 09:21:56.000000000 +0800
@@ -429,7 +429,8 @@ static int rfcomm_release_dev(void __use
 	if (dev->tty)
 		tty_vhangup(dev->tty);
 
-	rfcomm_dev_del(dev);
+	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+		rfcomm_dev_del(dev);
 	rfcomm_dev_put(dev);
 	return 0;
 }