[PATCH v2 0/6] Error handling in HCI request framework

[PATCH v2 0/6] Error handling in HCI request framework

[Andre Guedes]

Hi all,

This v2 is based on Johan's suggestions about the previous patch set.
The main comments were:
- Replace EINVAL by ENODATA in a separated patch.
- Save the exact error code in struct hci_request instead of having just a
  flag to indicate an error has occurred.
- Check req->err before before empty queue check.

I also added a new patch (Patch 1) to fix a problem found in __hci_req_sync
while I was working on this patch set.

Regards,

Andre

Andre Guedes (6):
  Bluetooth: Fix __hci_req_sync
  Bluetooth: Return ENODATA in hci_req_run
  Bluetooth: Check hci_req_run returning value in __hci_req_sync
  Bluetooth: HCI request error handling
  Bluetooth: Make hci_req_add returning void
  Bluetooth: Check req->err in hci_req_add

 include/net/bluetooth/hci_core.h |  7 +++++-
 net/bluetooth/hci_core.c         | 46 +++++++++++++++++++++++++++-------------
 2 files changed, 37 insertions(+), 16 deletions(-)

-- 
1.8.1.2

[PATCH v2 1/6] Bluetooth: Fix __hci_req_sync

[Andre Guedes]

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>
---
 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

[PATCH v2 2/6] Bluetooth: Return ENODATA in hci_req_run

[Andre Guedes]

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>
---
 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

[PATCH v2 3/6] Bluetooth: Check hci_req_run returning value in __hci_req_sync

[Andre Guedes]

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>
---
 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

[PATCH v2 4/6] Bluetooth: HCI request error handling

[Andre Guedes]

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>
---
 include/net/bluetooth/hci_core.h |  5 +++++
 net/bluetooth/hci_core.c         | 10 ++++++++++
 2 files changed, 15 insertions(+)

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..397adc6 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;
@@ -2530,6 +2539,7 @@ 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);
+		req->err = -ENOMEM;
 		return -ENOMEM;
 	}
 
-- 
1.8.1.2

Re: [PATCH v2 4/6] Bluetooth: HCI request error handling

[Johan Hedberg]

Hi Andre,

On Thu, Mar 07, 2013, Andre Guedes wrote:
> +	if (req->err) {
> +		skb_queue_purge(&req->cmd_q);
> +		return req->err;
> +	}

Don't we usually use if (err < 0) instead of if (err) for error checks?

>  	skb = hci_prepare_cmd(hdev, opcode, plen, param);
>  	if (!skb) {
>  		BT_ERR("%s no memory for command", hdev->name);
> +		req->err = -ENOMEM;
>  		return -ENOMEM;
>  	}

I think it'd be good to add at least the opcode to the BT_ERR log above
since otherwise the information of exactly which command in the request
caused an error is lost.

Johan

[PATCH v2 5/6] Bluetooth: Make hci_req_add returning void

[Andre Guedes]

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>
---
 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 397adc6..1103d9f 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;
@@ -2540,15 +2540,13 @@ int hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param)
 	if (!skb) {
 		BT_ERR("%s no memory for command", hdev->name);
 		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

[PATCH v2 6/6] Bluetooth: Check req->err in hci_req_add

[Andre Guedes]

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>
---
 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 1103d9f..2f07d88 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", hdev->name);
-- 
1.8.1.2

Re: [PATCH v2 6/6] Bluetooth: Check req->err in hci_req_add

[Johan Hedberg]

Hi Andre,

On Thu, Mar 07, 2013, Andre Guedes wrote:
> +	if (req->err)
> +		return;

Same thing here with the error check, I think if (err < 0) is more
consistent.

Johan

Re: [PATCH v2 6/6] Bluetooth: Check req->err in hci_req_add

[Andrei Emeltchenko]

Hi Johan,

On Fri, Mar 08, 2013 at 09:50:09AM +0200, Johan Hedberg wrote:
> Hi Andre,
> 
> On Thu, Mar 07, 2013, Andre Guedes wrote:
> > +	if (req->err)
> > +		return;
> 
> Same thing here with the error check, I think if (err < 0) is more
> consistent.

Using "if (err)" seems to be OK for kernel and looks shorter.

Best regards 
Andrei Emeltchenko 

Re: [PATCH v2 6/6] Bluetooth: Check req->err in hci_req_add

[Johan Hedberg]

Hi Andrei,

On Fri, Mar 08, 2013, Andrei Emeltchenko wrote:
> On Fri, Mar 08, 2013 at 09:50:09AM +0200, Johan Hedberg wrote:
> > Hi Andre,
> > 
> > On Thu, Mar 07, 2013, Andre Guedes wrote:
> > > +	if (req->err)
> > > +		return;
> > 
> > Same thing here with the error check, I think if (err < 0) is more
> > consistent.
> 
> Using "if (err)" seems to be OK for kernel and looks shorter.

It certainly seems to be more common than in user space, but I wouldn't say
it's the most common form:

jh@x220:linux{master}$ git grep 'if (err)' net/bluetooth/|wc -l
34
jh@x220:linux{master}$ git grep 'if (err < 0)' net/bluetooth/|wc -l
89
jh@x220:linux{master}$ 

Johan

Re: [PATCH v2 6/6] Bluetooth: Check req->err in hci_req_add

[Johan Hedberg]

Hi,

On Fri, Mar 08, 2013, Johan Hedberg wrote:
> On Fri, Mar 08, 2013, Andrei Emeltchenko wrote:
> > On Fri, Mar 08, 2013 at 09:50:09AM +0200, Johan Hedberg wrote:
> > > Hi Andre,
> > > 
> > > On Thu, Mar 07, 2013, Andre Guedes wrote:
> > > > +	if (req->err)
> > > > +		return;
> > > 
> > > Same thing here with the error check, I think if (err < 0) is more
> > > consistent.
> > 
> > Using "if (err)" seems to be OK for kernel and looks shorter.
> 
> It certainly seems to be more common than in user space, but I wouldn't say
> it's the most common form:
> 
> jh@x220:linux{master}$ git grep 'if (err)' net/bluetooth/|wc -l
> 34
> jh@x220:linux{master}$ git grep 'if (err < 0)' net/bluetooth/|wc -l
> 89
> jh@x220:linux{master}$ 

Looking elsewhere than net/bluetooth/ in the kernel tree the balance
seems to be the other way around though, so I wont insist on this.

Andre, if you do the fix to BT_ERR() you can also add my acks to all
patches in your v3 of this set.

Acked-by: Johan Hedberg <johan.hedberg@intel.com>

Johan

Re: [PATCH v2 6/6] Bluetooth: Check req->err in hci_req_add

[Andre Guedes]

Hi Johan,

On Fri, Mar 8, 2013 at 8:34 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Fri, Mar 08, 2013, Johan Hedberg wrote:
>> On Fri, Mar 08, 2013, Andrei Emeltchenko wrote:
>> > On Fri, Mar 08, 2013 at 09:50:09AM +0200, Johan Hedberg wrote:
>> > > Hi Andre,
>> > >
>> > > On Thu, Mar 07, 2013, Andre Guedes wrote:
>> > > > +       if (req->err)
>> > > > +               return;
>> > >
>> > > Same thing here with the error check, I think if (err < 0) is more
>> > > consistent.
>> >
>> > Using "if (err)" seems to be OK for kernel and looks shorter.
>>
>> It certainly seems to be more common than in user space, but I wouldn't say
>> it's the most common form:
>>
>> jh@x220:linux{master}$ git grep 'if (err)' net/bluetooth/|wc -l
>> 34
>> jh@x220:linux{master}$ git grep 'if (err < 0)' net/bluetooth/|wc -l
>> 89
>> jh@x220:linux{master}$
>
> Looking elsewhere than net/bluetooth/ in the kernel tree the balance
> seems to be the other way around though, so I wont insist on this.
>
> Andre, if you do the fix to BT_ERR() you can also add my acks to all
> patches in your v3 of this set.
>
> Acked-by: Johan Hedberg <johan.hedberg@intel.com>

Ok, then I'll add the opcode to BT_ERR(), add yours acks and send the v3.

Regards,

Andre