bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe

RFC PATCH Superseded Roger Quadros 2 participants 6 messages Apr 23, 2009 – Apr 27, 2009

Subsystems (2)

Bluetooth drivers Bluetooth

lore

Share & tools

lore

Roger Quadros

author

2009-04-23 11:31 UTC #0 lore raw

RFC PATCH 1/1 bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe

From: Roger Quadros <[email protected]>
Date: Thu, 23 Apr 2009 14:05:19 +0300
Subject: [PATCH] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe

Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.

It eliminates the following error on a pre-emptible kernel.

[  204.358032] Unable to handle kernel NULL pointer dereference at
virtual address 0000000c
[  204.370697] pgd = c0004000
[  204.373443] [0000000c] *pgd=00000000
[  204.378601] Internal error: Oops: 17 [#1] PREEMPT
[  204.383361] Modules linked in: vfat fat rfcomm sco l2cap sd_mod
scsi_mod iphb pvr2d drm omaplfb ps
[  204.438537] CPU: 0    Not tainted  (2.6.28-maemo2 #1)
[  204.443664] PC is at klist_put+0x2c/0xb4
[  204.447601] LR is at klist_put+0x18/0xb4
[  204.451568] pc : [<c0270f08>]    lr : [<c0270ef4>]    psr: a0000113
[  204.451568] sp : cf1b3f10  ip : cf1b3f10  fp : cf1b3f2c
[  204.463104] r10: 00000000  r9 : 00000000  r8 : bf08029c
[  204.468353] r7 : c7869200  r6 : cfbe2690  r5 : c78692c8  r4 :
00000001
[  204.474945] r3 : 00000001  r2 : cf1b2000  r1 : 00000001  r0 :
00000000
[  204.481506] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[  204.488861] Control: 10c5387d  Table: 887fc018  DAC: 00000017
[  204.494628] Process btdelconn (pid: 515, stack limit = 0xcf1b22e0)

Signed-off-by Roger Quadros <[email protected]>

---
 include/net/bluetooth/hci_core.h |    3 ++-
 net/bluetooth/hci_sysfs.c        |   37 ++++++++++++++++---------------------
 2 files changed, 18 insertions(+), 22 deletions(-)

include/net/bluetooth/hci_core.h +2 -1

index 30a5be8..988e8cb 100644
@@ -180,7 +180,8 @@ struct hci_conn {
 	struct timer_list disc_timer;
 	struct timer_list idle_timer;
 
-	struct work_struct work;
+	struct work_struct work_add;
+	struct work_struct work_del;
 
 	struct device	dev;

net/bluetooth/hci_sysfs.c +16 ~~-21~~

index 1a1f916..8cddc1c 100644
@@ -9,8 +9,7 @@
 struct class *bt_class = NULL;
 EXPORT_SYMBOL_GPL(bt_class);
 
-static struct workqueue_struct *btaddconn;
-static struct workqueue_struct *btdelconn;
+static struct workqueue_struct *btworkq;
 
 static inline char *link_typetostr(int type)
 {

@@ -88,9 +87,10 @@ static struct device_type bt_link = {
 
 static void add_conn(struct work_struct *work)
 {
-	struct hci_conn *conn = container_of(work, struct hci_conn, work);
+	struct hci_conn *conn = container_of(work, struct hci_conn, work_add);
 
-	flush_workqueue(btdelconn);
+	/* ensure previous add/del is complete */
+	flush_workqueue(btworkq);
 
 	if (device_add(&conn->dev) < 0) {
 		BT_ERR("Failed to register connection device");

@@ -114,9 +114,9 @@ void hci_conn_add_sysfs(struct hci_conn *conn)
 
 	device_initialize(&conn->dev);
 
-	INIT_WORK(&conn->work, add_conn);
+	INIT_WORK(&conn->work_add, add_conn);
 
-	queue_work(btaddconn, &conn->work);
+	queue_work(btworkq, &conn->work_add);
 }
 
 /*

@@ -131,9 +131,12 @@ static int __match_tty(struct device *dev, void *data)
 
 static void del_conn(struct work_struct *work)
 {
-	struct hci_conn *conn = container_of(work, struct hci_conn, work);
+	struct hci_conn *conn = container_of(work, struct hci_conn, work_del);
 	struct hci_dev *hdev = conn->hdev;
 
+	/* ensure previous add/del is complete */
+	flush_workqueue(btworkq);
+
 	while (1) {
 		struct device *dev;

@@ -156,9 +159,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
 	if (!device_is_registered(&conn->dev))
 		return;
 
-	INIT_WORK(&conn->work, del_conn);
+	INIT_WORK(&conn->work_del, del_conn);
 
-	queue_work(btdelconn, &conn->work);
+	queue_work(btworkq, &conn->work_del);
 }
 
 static inline char *host_typetostr(int type)

@@ -435,20 +438,13 @@ void hci_unregister_sysfs(struct hci_dev *hdev)
 
 int __init bt_sysfs_init(void)
 {
-	btaddconn = create_singlethread_workqueue("btaddconn");
-	if (!btaddconn)
-		return -ENOMEM;
-
-	btdelconn = create_singlethread_workqueue("btdelconn");
-	if (!btdelconn) {
-		destroy_workqueue(btaddconn);
+	btworkq = create_singlethread_workqueue("btworkq");
+	if (!btworkq)
 		return -ENOMEM;
-	}
 
 	bt_class = class_create(THIS_MODULE, "bluetooth");
 	if (IS_ERR(bt_class)) {
-		destroy_workqueue(btdelconn);
-		destroy_workqueue(btaddconn);
+		destroy_workqueue(btworkq);
 		return PTR_ERR(bt_class);
 	}

@@ -457,8 +453,7 @@ int __init bt_sysfs_init(void)
 
 void bt_sysfs_cleanup(void)
 {
-	destroy_workqueue(btaddconn);
-	destroy_workqueue(btdelconn);
+	destroy_workqueue(btworkq);
 
 	class_destroy(bt_class);
 }

Signature · 1 line

1.5.6.3

Marcel Holtmann

maintainer

2009-04-23 11:42 UTC #1 lore raw

Hi Roger,

Quoted 6 lines

Subject: [PATCH] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe
Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.
It eliminates the following error on a pre-emptible kernel.

patch looks good to me. Can you just please replace the work queue name with "bluetooth" instead if "btworkq". Looks more saner to me.

Regards

Marcel

Roger Quadros

author

2009-04-23 11:50 UTC #2 lore raw

PATCH v2 1/1 bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe

Quoted 14 lines · Marcel Holtmann

Hi Roger,
Quoted 6 lines
Subject: [PATCH] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe
Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.
It eliminates the following error on a pre-emptible kernel.
patch looks good to me. Can you just please replace the work queue name with "bluetooth" instead if "btworkq". Looks more saner to me.
Regards

Quoted 7 lines

Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Marcel,

I've renamed "btworkq" to "bluetooth" as per your suggestion.
Patch is below.

cheers!
-roger

From: Roger Quadros <[email protected]>
Date: Thu, 23 Apr 2009 14:05:19 +0300
Subject: [PATCH v2] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe

Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.

It eliminates the following error on a pre-emptible kernel.

[  204.358032] Unable to handle kernel NULL pointer dereference at
virtual address 0000000c
[  204.370697] pgd = c0004000
[  204.373443] [0000000c] *pgd=00000000
[  204.378601] Internal error: Oops: 17 [#1] PREEMPT
[  204.383361] Modules linked in: vfat fat rfcomm sco l2cap sd_mod
scsi_mod iphb pvr2d drm omaplfb ps
[  204.438537] CPU: 0    Not tainted  (2.6.28-maemo2 #1)
[  204.443664] PC is at klist_put+0x2c/0xb4
[  204.447601] LR is at klist_put+0x18/0xb4
[  204.451568] pc : [<c0270f08>]    lr : [<c0270ef4>]    psr: a0000113
[  204.451568] sp : cf1b3f10  ip : cf1b3f10  fp : cf1b3f2c
[  204.463104] r10: 00000000  r9 : 00000000  r8 : bf08029c
[  204.468353] r7 : c7869200  r6 : cfbe2690  r5 : c78692c8  r4 :
00000001
[  204.474945] r3 : 00000001  r2 : cf1b2000  r1 : 00000001  r0 :
00000000
[  204.481506] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[  204.488861] Control: 10c5387d  Table: 887fc018  DAC: 00000017
[  204.494628] Process btdelconn (pid: 515, stack limit = 0xcf1b22e0)

Signed-off-by Roger Quadros <[email protected]>

---
 include/net/bluetooth/hci_core.h |    3 ++-
 net/bluetooth/hci_sysfs.c        |   37 ++++++++++++++++---------------------
 2 files changed, 18 insertions(+), 22 deletions(-)

include/net/bluetooth/hci_core.h +2 -1

index 30a5be8..988e8cb 100644
@@ -180,7 +180,8 @@ struct hci_conn {
 	struct timer_list disc_timer;
 	struct timer_list idle_timer;
 
-	struct work_struct work;
+	struct work_struct work_add;
+	struct work_struct work_del;
 
 	struct device	dev;

net/bluetooth/hci_sysfs.c +16 ~~-21~~

index 1a1f916..338686e 100644
@@ -9,8 +9,7 @@
 struct class *bt_class = NULL;
 EXPORT_SYMBOL_GPL(bt_class);
 
-static struct workqueue_struct *btaddconn;
-static struct workqueue_struct *btdelconn;
+static struct workqueue_struct *bluetooth;
 
 static inline char *link_typetostr(int type)
 {

@@ -88,9 +87,10 @@ static struct device_type bt_link = {
 
 static void add_conn(struct work_struct *work)
 {
-	struct hci_conn *conn = container_of(work, struct hci_conn, work);
+	struct hci_conn *conn = container_of(work, struct hci_conn, work_add);
 
-	flush_workqueue(btdelconn);
+	/* ensure previous add/del is complete */
+	flush_workqueue(bluetooth);
 
 	if (device_add(&conn->dev) < 0) {
 		BT_ERR("Failed to register connection device");

@@ -114,9 +114,9 @@ void hci_conn_add_sysfs(struct hci_conn *conn)
 
 	device_initialize(&conn->dev);
 
-	INIT_WORK(&conn->work, add_conn);
+	INIT_WORK(&conn->work_add, add_conn);
 
-	queue_work(btaddconn, &conn->work);
+	queue_work(bluetooth, &conn->work_add);
 }
 
 /*

@@ -131,9 +131,12 @@ static int __match_tty(struct device *dev, void *data)
 
 static void del_conn(struct work_struct *work)
 {
-	struct hci_conn *conn = container_of(work, struct hci_conn, work);
+	struct hci_conn *conn = container_of(work, struct hci_conn, work_del);
 	struct hci_dev *hdev = conn->hdev;
 
+	/* ensure previous add/del is complete */
+	flush_workqueue(bluetooth);
+
 	while (1) {
 		struct device *dev;

@@ -156,9 +159,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn)
 	if (!device_is_registered(&conn->dev))
 		return;
 
-	INIT_WORK(&conn->work, del_conn);
+	INIT_WORK(&conn->work_del, del_conn);
 
-	queue_work(btdelconn, &conn->work);
+	queue_work(bluetooth, &conn->work_del);
 }
 
 static inline char *host_typetostr(int type)

@@ -435,20 +438,13 @@ void hci_unregister_sysfs(struct hci_dev *hdev)
 
 int __init bt_sysfs_init(void)
 {
-	btaddconn = create_singlethread_workqueue("btaddconn");
-	if (!btaddconn)
-		return -ENOMEM;
-
-	btdelconn = create_singlethread_workqueue("btdelconn");
-	if (!btdelconn) {
-		destroy_workqueue(btaddconn);
+	bluetooth = create_singlethread_workqueue("bluetooth");
+	if (!bluetooth)
 		return -ENOMEM;
-	}
 
 	bt_class = class_create(THIS_MODULE, "bluetooth");
 	if (IS_ERR(bt_class)) {
-		destroy_workqueue(btdelconn);
-		destroy_workqueue(btaddconn);
+		destroy_workqueue(bluetooth);
 		return PTR_ERR(bt_class);
 	}

@@ -457,8 +453,7 @@ int __init bt_sysfs_init(void)
 
 void bt_sysfs_cleanup(void)
 {
-	destroy_workqueue(btaddconn);
-	destroy_workqueue(btdelconn);
+	destroy_workqueue(bluetooth);
 
 	class_destroy(bt_class);
 }

Signature · 1 line

1.5.6.3

Roger Quadros

author

2009-04-27 09:16 UTC #3 lore raw

Quoted 62 lines · ext Roger Quadros

Quoted 7 lines · Marcel Holtmann
Hi Roger,
Quoted 5 lines
Subject: [PATCH] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe
Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.
Quoted 14 lines
Quoted 1 line
It eliminates the following error on a pre-emptible kernel.
patch looks good to me. Can you just please replace the work queue name with "bluetooth" instead if "btworkq". Looks more saner to me.
Regards
Marcel
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hi Marcel,
I've renamed "btworkq" to "bluetooth" as per your suggestion.
Patch is below.
cheers!
-roger
From: Roger Quadros <[email protected]>
Date: Thu, 23 Apr 2009 14:05:19 +0300
Subject: [PATCH v2] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe
Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.
It eliminates the following error on a pre-emptible kernel.
[  204.358032] Unable to handle kernel NULL pointer dereference at
virtual address 0000000c
[  204.370697] pgd = c0004000
[  204.373443] [0000000c] *pgd=00000000
[  204.378601] Internal error: Oops: 17 [#1] PREEMPT
[  204.383361] Modules linked in: vfat fat rfcomm sco l2cap sd_mod
scsi_mod iphb pvr2d drm omaplfb ps
[  204.438537] CPU: 0    Not tainted  (2.6.28-maemo2 #1)
[  204.443664] PC is at klist_put+0x2c/0xb4
[  204.447601] LR is at klist_put+0x18/0xb4
[  204.451568] pc : [<c0270f08>]    lr : [<c0270ef4>]    psr: a0000113
[  204.451568] sp : cf1b3f10  ip : cf1b3f10  fp : cf1b3f2c
[  204.463104] r10: 00000000  r9 : 00000000  r8 : bf08029c
[  204.468353] r7 : c7869200  r6 : cfbe2690  r5 : c78692c8  r4 :
00000001
[  204.474945] r3 : 00000001  r2 : cf1b2000  r1 : 00000001  r0 :
00000000
[  204.481506] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[  204.488861] Control: 10c5387d  Table: 887fc018  DAC: 00000017
[  204.494628] Process btdelconn (pid: 515, stack limit = 0xcf1b22e0)

Quoted 7 lines

Signed-off-by Roger Quadros <[email protected]>

---
 include/net/bluetooth/hci_core.h |    3 ++-
 net/bluetooth/hci_sysfs.c        |   37 
++++++++++++++++---------------------
 2 files changed, 18 insertions(+), 22 deletions(-)

hi Marcel,

What is the fate of this patch?

regards,
-roger

Roger Quadros

author

2009-04-27 09:17 UTC #4 lore raw

Quoted 63 lines · Roger Quadros

Quoted 62 lines · ext Roger Quadros
Quoted 7 lines · Marcel Holtmann
Hi Roger,
Quoted 5 lines
Subject: [PATCH] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe
Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.
Quoted 14 lines
Quoted 1 line
It eliminates the following error on a pre-emptible kernel.
patch looks good to me. Can you just please replace the work queue name with "bluetooth" instead if "btworkq". Looks more saner to me.
Regards
Marcel
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hi Marcel,
I've renamed "btworkq" to "bluetooth" as per your suggestion.
Patch is below.
cheers!
-roger
From: Roger Quadros <[email protected]>
Date: Thu, 23 Apr 2009 14:05:19 +0300
Subject: [PATCH v2] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe
Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.
It eliminates the following error on a pre-emptible kernel.
[  204.358032] Unable to handle kernel NULL pointer dereference at
virtual address 0000000c
[  204.370697] pgd = c0004000
[  204.373443] [0000000c] *pgd=00000000
[  204.378601] Internal error: Oops: 17 [#1] PREEMPT
[  204.383361] Modules linked in: vfat fat rfcomm sco l2cap sd_mod
scsi_mod iphb pvr2d drm omaplfb ps
[  204.438537] CPU: 0    Not tainted  (2.6.28-maemo2 #1)
[  204.443664] PC is at klist_put+0x2c/0xb4
[  204.447601] LR is at klist_put+0x18/0xb4
[  204.451568] pc : [<c0270f08>]    lr : [<c0270ef4>]    psr: a0000113
[  204.451568] sp : cf1b3f10  ip : cf1b3f10  fp : cf1b3f2c
[  204.463104] r10: 00000000  r9 : 00000000  r8 : bf08029c
[  204.468353] r7 : c7869200  r6 : cfbe2690  r5 : c78692c8  r4 :
00000001
[  204.474945] r3 : 00000001  r2 : cf1b2000  r1 : 00000001  r0 :
00000000
[  204.481506] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[  204.488861] Control: 10c5387d  Table: 887fc018  DAC: 00000017
[  204.494628] Process btdelconn (pid: 515, stack limit = 0xcf1b22e0)

Quoted 8 lines

Quoted 7 lines

Signed-off-by Roger Quadros <[email protected]>

---
 include/net/bluetooth/hci_core.h |    3 ++-
 net/bluetooth/hci_sysfs.c        |   37 
++++++++++++++++---------------------
 2 files changed, 18 insertions(+), 22 deletions(-)

hi Marcel,

What is the fate of this patch?

regards,
-roger

Marcel Holtmann

maintainer

2009-04-27 12:06 UTC #5 lore raw

In reply to Roger Quadros

Hi Roger,

Quoted 40 lines

Quoted 32 lines

From: Roger Quadros <[email protected]>
Date: Thu, 23 Apr 2009 14:05:19 +0300
Subject: [PATCH v2] bluetooth: hci_sysfs: Ensure sysfs add/del is pre-empt safe

Uses different work_struct variables for add_conn() and del_conn(). Uses single work queue instead of two for adding and deleting connections.

It eliminates the following error on a pre-emptible kernel.

[  204.358032] Unable to handle kernel NULL pointer dereference at
virtual address 0000000c
[  204.370697] pgd = c0004000
[  204.373443] [0000000c] *pgd=00000000
[  204.378601] Internal error: Oops: 17 [#1] PREEMPT
[  204.383361] Modules linked in: vfat fat rfcomm sco l2cap sd_mod
scsi_mod iphb pvr2d drm omaplfb ps
[  204.438537] CPU: 0    Not tainted  (2.6.28-maemo2 #1)
[  204.443664] PC is at klist_put+0x2c/0xb4
[  204.447601] LR is at klist_put+0x18/0xb4
[  204.451568] pc : [<c0270f08>]    lr : [<c0270ef4>]    psr: a0000113
[  204.451568] sp : cf1b3f10  ip : cf1b3f10  fp : cf1b3f2c
[  204.463104] r10: 00000000  r9 : 00000000  r8 : bf08029c
[  204.468353] r7 : c7869200  r6 : cfbe2690  r5 : c78692c8  r4 :
00000001
[  204.474945] r3 : 00000001  r2 : cf1b2000  r1 : 00000001  r0 :
00000000
[  204.481506] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[  204.488861] Control: 10c5387d  Table: 887fc018  DAC: 00000017
[  204.494628] Process btdelconn (pid: 515, stack limit = 0xcf1b22e0)

Quoted 7 lines

Signed-off-by Roger Quadros <[email protected]>

---
 include/net/bluetooth/hci_core.h |    3 ++-
 net/bluetooth/hci_sysfs.c        |   37 
++++++++++++++++---------------------
 2 files changed, 18 insertions(+), 22 deletions(-)

Quoted 1 line

What is the fate of this patch?

the patch is part of bluetooth-testing.git and as soon as I have debugged another issue (and hopefully fixed), I will send it to David Miller for inclusion.

Regards

Marcel