[PATCH v3 1/6] Bluetooth: Fix __hci_req

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v3 1/6] Bluetooth: Fix __hci_req_sync
@ 2013-03-08 14:20 Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 2/6] Bluetooth: Return ENODATA in hci_req_run Andre Guedes
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Andre Guedes @ 2013-03-08 14:20 UTC (permalink / raw)
  To: linux-bluetooth

If hci_req_run returns error, we erroneously leave the current
process in TASK_INTERRUPTABLE state. If we leave the process in
TASK_INTERRUPTABLE and it is preempted, this process will never
be scheduled again.

This patch fixes this issue by moving the preparation for scheduling
(add to waitqueue and set process state) to just after the hci_req_run
call.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
Acked-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b6d44a2..a3e3c96 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -95,15 +95,11 @@ static int __hci_req_sync(struct hci_dev *hdev,
 
 	hdev->req_status = HCI_REQ_PEND;
 
-	add_wait_queue(&hdev->req_wait_q, &wait);
-	set_current_state(TASK_INTERRUPTIBLE);
-
 	func(&req, opt);
 
 	err = hci_req_run(&req, hci_req_sync_complete);
 	if (err < 0) {
 		hdev->req_status = 0;
-		remove_wait_queue(&hdev->req_wait_q, &wait);
 		/* req_run will fail if the request did not add any
 		 * commands to the queue, something that can happen when
 		 * a request with conditionals doesn't trigger any
@@ -113,6 +109,9 @@ static int __hci_req_sync(struct hci_dev *hdev,
 		return 0;
 	}
 
+	add_wait_queue(&hdev->req_wait_q, &wait);
+	set_current_state(TASK_INTERRUPTIBLE);
+
 	schedule_timeout(timeout);
 
 	remove_wait_queue(&hdev->req_wait_q, &wait);
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 2/6] Bluetooth: Return ENODATA in hci_req_run
  2013-03-08 14:20 [PATCH v3 1/6] Bluetooth: Fix __hci_req_sync Andre Guedes
@ 2013-03-08 14:20 ` Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 3/6] Bluetooth: Check hci_req_run returning value in __hci_req_sync Andre Guedes
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Andre Guedes @ 2013-03-08 14:20 UTC (permalink / raw)
  To: linux-bluetooth

In case the HCI request queue is empty, hci_req_run should return
ENODATA instead of EINVAL. This way, hci_req_run returns a more
meaningful error value.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
Acked-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index a3e3c96..5f625e0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2452,7 +2452,7 @@ int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
 
 	/* Do not allow empty requests */
 	if (skb_queue_empty(&req->cmd_q))
-		return -EINVAL;
+		return -ENODATA;
 
 	skb = skb_peek_tail(&req->cmd_q);
 	bt_cb(skb)->req.complete = complete;
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 3/6] Bluetooth: Check hci_req_run returning value in __hci_req_sync
  2013-03-08 14:20 [PATCH v3 1/6] Bluetooth: Fix __hci_req_sync Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 2/6] Bluetooth: Return ENODATA in hci_req_run Andre Guedes
@ 2013-03-08 14:20 ` Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 4/6] Bluetooth: HCI request error handling Andre Guedes
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Andre Guedes @ 2013-03-08 14:20 UTC (permalink / raw)
  To: linux-bluetooth

Since hci_req_run will be returning more than one error code, we
should check its returning value in __hci_req_sync.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
Acked-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5f625e0..dc76dcf 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -100,13 +100,16 @@ static int __hci_req_sync(struct hci_dev *hdev,
 	err = hci_req_run(&req, hci_req_sync_complete);
 	if (err < 0) {
 		hdev->req_status = 0;
-		/* req_run will fail if the request did not add any
-		 * commands to the queue, something that can happen when
-		 * a request with conditionals doesn't trigger any
-		 * commands to be sent. This is normal behavior and
-		 * should not trigger an error return.
+
+		/* ENODATA means the HCI request command queue is empty.
+		 * This can happen when a request with conditionals doesn't
+		 * trigger any commands to be sent. This is normal behavior
+		 * and should not trigger an error return.
 		 */
-		return 0;
+		if (err == -ENODATA)
+			return 0;
+
+		return err;
 	}
 
 	add_wait_queue(&hdev->req_wait_q, &wait);
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 4/6] Bluetooth: HCI request error handling
  2013-03-08 14:20 [PATCH v3 1/6] Bluetooth: Fix __hci_req_sync Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 2/6] Bluetooth: Return ENODATA in hci_req_run Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 3/6] Bluetooth: Check hci_req_run returning value in __hci_req_sync Andre Guedes
@ 2013-03-08 14:20 ` Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 5/6] Bluetooth: Make hci_req_add returning void Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 6/6] Bluetooth: Check req->err in hci_req_add Andre Guedes
  4 siblings, 0 replies; 7+ messages in thread
From: Andre Guedes @ 2013-03-08 14:20 UTC (permalink / raw)
  To: linux-bluetooth

When we are building a HCI request with more the one HCI command
and one of the hci_req_add calls fail, we should have some cleanup
routine so the HCI commands already queued on HCI request can be
deleted. Otherwise, we will face some memory leaks issues.

This patch implements the HCI request error handling which is the
following: If a hci_req_add fails, we save the error code in hci_
request. Once hci_req_run is called, we verify the error field. If
it is different from zero, we delete all HCI commands already queued
and return the error code.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
Acked-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  5 +++++
 net/bluetooth/hci_core.c         | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3a9cbf2..332ee50 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1042,6 +1042,11 @@ int hci_unregister_cb(struct hci_cb *hcb);
 struct hci_request {
 	struct hci_dev		*hdev;
 	struct sk_buff_head	cmd_q;
+
+	/* If something goes wrong when building the HCI request, the error
+	 * value is stored in this field.
+	 */
+	int			err;
 };
 
 void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dc76dcf..fa72aff 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2443,6 +2443,7 @@ void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
 {
 	skb_queue_head_init(&req->cmd_q);
 	req->hdev = hdev;
+	req->err = 0;
 }
 
 int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
@@ -2453,6 +2454,14 @@ int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
 
 	BT_DBG("length %u", skb_queue_len(&req->cmd_q));
 
+	/* If an error occured during request building, remove all HCI
+	 * commands queued on the HCI request queue.
+	 */
+	if (req->err) {
+		skb_queue_purge(&req->cmd_q);
+		return req->err;
+	}
+
 	/* Do not allow empty requests */
 	if (skb_queue_empty(&req->cmd_q))
 		return -ENODATA;
@@ -2529,7 +2538,9 @@ int hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param)
 
 	skb = hci_prepare_cmd(hdev, opcode, plen, param);
 	if (!skb) {
-		BT_ERR("%s no memory for command", hdev->name);
+		BT_ERR("%s no memory for command (opcode 0x%4.4x)",
+		       hdev->name, opcode);
+		req->err = -ENOMEM;
 		return -ENOMEM;
 	}
 
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 5/6] Bluetooth: Make hci_req_add returning void
  2013-03-08 14:20 [PATCH v3 1/6] Bluetooth: Fix __hci_req_sync Andre Guedes
                   ` (2 preceding siblings ...)
  2013-03-08 14:20 ` [PATCH v3 4/6] Bluetooth: HCI request error handling Andre Guedes
@ 2013-03-08 14:20 ` Andre Guedes
  2013-03-08 14:20 ` [PATCH v3 6/6] Bluetooth: Check req->err in hci_req_add Andre Guedes
  4 siblings, 0 replies; 7+ messages in thread
From: Andre Guedes @ 2013-03-08 14:20 UTC (permalink / raw)
  To: linux-bluetooth

Since no one checks the returning value of hci_req_add and HCI
request errors are now handled in hci_req_run, we can make hci_
req_add returning void.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
Acked-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h | 2 +-
 net/bluetooth/hci_core.c         | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 332ee50..d6c3256 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1051,7 +1051,7 @@ struct hci_request {
 
 void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
 int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
-int hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param);
+void hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param);
 void hci_req_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status);
 void hci_req_cmd_status(struct hci_dev *hdev, u16 opcode, u8 status);
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fa72aff..f14c41f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2529,7 +2529,7 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
 }
 
 /* Queue a command to an asynchronous HCI request */
-int hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param)
+void hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param)
 {
 	struct hci_dev *hdev = req->hdev;
 	struct sk_buff *skb;
@@ -2541,15 +2541,13 @@ int hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param)
 		BT_ERR("%s no memory for command (opcode 0x%4.4x)",
 		       hdev->name, opcode);
 		req->err = -ENOMEM;
-		return -ENOMEM;
+		return;
 	}
 
 	if (skb_queue_empty(&req->cmd_q))
 		bt_cb(skb)->req.start = true;
 
 	skb_queue_tail(&req->cmd_q, skb);
-
-	return 0;
 }
 
 /* Get data from the previously sent command */
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 6/6] Bluetooth: Check req->err in hci_req_add
  2013-03-08 14:20 [PATCH v3 1/6] Bluetooth: Fix __hci_req_sync Andre Guedes
                   ` (3 preceding siblings ...)
  2013-03-08 14:20 ` [PATCH v3 5/6] Bluetooth: Make hci_req_add returning void Andre Guedes
@ 2013-03-08 14:20 ` Andre Guedes
  2013-03-09 20:13   ` Gustavo Padovan
  4 siblings, 1 reply; 7+ messages in thread
From: Andre Guedes @ 2013-03-08 14:20 UTC (permalink / raw)
  To: linux-bluetooth

If req->err is set, there is no point in queueing the HCI command
in HCI request command queue since it won't be sent anyway.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
Acked-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f14c41f..a49c445 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2536,6 +2536,12 @@ void hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param)
 
 	BT_DBG("%s opcode 0x%4.4x plen %d", hdev->name, opcode, plen);
 
+	/* If an error occured during request building, there is no point in
+	 * queueing the HCI command. We can simply return.
+	 */
+	if (req->err)
+		return;
+
 	skb = hci_prepare_cmd(hdev, opcode, plen, param);
 	if (!skb) {
 		BT_ERR("%s no memory for command (opcode 0x%4.4x)",
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 6/6] Bluetooth: Check req->err in hci_req_add
  2013-03-08 14:20 ` [PATCH v3 6/6] Bluetooth: Check req->err in hci_req_add Andre Guedes
@ 2013-03-09 20:13   ` Gustavo Padovan
  0 siblings, 0 replies; 7+ messages in thread
From: Gustavo Padovan @ 2013-03-09 20:13 UTC (permalink / raw)
  To: Andre Guedes; +Cc: linux-bluetooth

Hi Andre,

* Andre Guedes <andre.guedes@openbossa.org> [2013-03-08 11:20:18 -0300]:

> If req->err is set, there is no point in queueing the HCI command
> in HCI request command queue since it won't be sent anyway.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> Acked-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_core.c | 6 ++++++
>  1 file changed, 6 insertions(+)

All patches have been applied to bluetooth-next. Thanks.

	Gustavo

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-03-09 20:13 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-08 14:20 [PATCH v3 1/6] Bluetooth: Fix __hci_req_sync Andre Guedes
2013-03-08 14:20 ` [PATCH v3 2/6] Bluetooth: Return ENODATA in hci_req_run Andre Guedes
2013-03-08 14:20 ` [PATCH v3 3/6] Bluetooth: Check hci_req_run returning value in __hci_req_sync Andre Guedes
2013-03-08 14:20 ` [PATCH v3 4/6] Bluetooth: HCI request error handling Andre Guedes
2013-03-08 14:20 ` [PATCH v3 5/6] Bluetooth: Make hci_req_add returning void Andre Guedes
2013-03-08 14:20 ` [PATCH v3 6/6] Bluetooth: Check req->err in hci_req_add Andre Guedes
2013-03-09 20:13   ` Gustavo Padovan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.