[PATCH 0/4] Error handling in HCI request framework

[PATCH 0/4] Error handling in HCI request framework

[Andre Guedes]

Hi all,

This small patch set aims to add a better support for error handling in HCI
request framework.

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. This way the
HCI commands already queued on HCI request can be deleted. Otherwise we will
face some memory leaks issues. Discussing about this issue on IRC, we came up
with the approach implemented in this patch set.

The approach is the following: If a hci_req_add fails, we set a flag in struct
hci_request to indicate the failure. Subsequent hci_req_add calls will simply
add no HCI commands to the HCI request queue. Once hci_req_run is called, we
verify the error flag. If it is set, we delete all HCI commands already queued
and return a error code.

Regards,

Andre


Andre Guedes (4):
  Bluetooth: Check hci_req_run error code in __hci_req_sync
  Bluetooth: HCI request error handling
  Bluetooth: Make hci_req_add return void
  Bluetooth: Check req->error flag in hci_req_add

 include/net/bluetooth/hci_core.h |  5 ++++-
 net/bluetooth/hci_core.c         | 33 +++++++++++++++++++++++++++------
 2 files changed, 31 insertions(+), 7 deletions(-)

-- 
1.8.1.2

[PATCH 1/4] Bluetooth: Check hci_req_run error code 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.

This patch also changes the returning value of hci_req_run in case
the HCI request is empty.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b6d44a2..a1bbf6d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -101,7 +101,7 @@ static int __hci_req_sync(struct hci_dev *hdev,
 	func(&req, opt);
 
 	err = hci_req_run(&req, hci_req_sync_complete);
-	if (err < 0) {
+	if (err == -ENODATA) {
 		hdev->req_status = 0;
 		remove_wait_queue(&hdev->req_wait_q, &wait);
 		/* req_run will fail if the request did not add any
@@ -112,6 +112,11 @@ static int __hci_req_sync(struct hci_dev *hdev,
 		 */
 		return 0;
 	}
+	if (err < 0) {
+		hdev->req_status = HCI_REQ_CANCELED;
+		remove_wait_queue(&hdev->req_wait_q, &wait);
+		return err;
+	}
 
 	schedule_timeout(timeout);
 
@@ -2453,7 +2458,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

Re: [PATCH 1/4] Bluetooth: Check hci_req_run error code in __hci_req_sync

[Johan Hedberg]

Hi Andre,

On Wed, Mar 06, 2013, Andre Guedes wrote:
> Since hci_req_run will be returning more than one error code, we
> should check its returning value in __hci_req_sync.
> 
> This patch also changes the returning value of hci_req_run in case
> the HCI request is empty.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b6d44a2..a1bbf6d 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -101,7 +101,7 @@ static int __hci_req_sync(struct hci_dev *hdev,
>  	func(&req, opt);
>  
>  	err = hci_req_run(&req, hci_req_sync_complete);
> -	if (err < 0) {
> +	if (err == -ENODATA) {
>  		hdev->req_status = 0;
>  		remove_wait_queue(&hdev->req_wait_q, &wait);
>  		/* req_run will fail if the request did not add any
> @@ -112,6 +112,11 @@ static int __hci_req_sync(struct hci_dev *hdev,
>  		 */
>  		return 0;
>  	}
> +	if (err < 0) {
> +		hdev->req_status = HCI_REQ_CANCELED;
> +		remove_wait_queue(&hdev->req_wait_q, &wait);
> +		return err;
> +	}

It might be cleaner to just use one if-branch and something like:

	if (err <  0) {
		/* ENODATA means no commands were added, and it should
		 * not be treated as an error.
		 */
		 if (err == -ENODATA)
		 	err = 0;

		...
	}

> @@ -2453,7 +2458,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;

This should probably be in its own patch before the reset, explaining
that ENODATA is a more natural error to be returned in this case.

Johan

Re: [PATCH 1/4] Bluetooth: Check hci_req_run error code in __hci_req_sync

[Andre Guedes]

Hi Johan,

On Thu, Mar 7, 2013 at 4:11 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Andre,
>
> On Wed, Mar 06, 2013, Andre Guedes wrote:
>> Since hci_req_run will be returning more than one error code, we
>> should check its returning value in __hci_req_sync.
>>
>> This patch also changes the returning value of hci_req_run in case
>> the HCI request is empty.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>>  net/bluetooth/hci_core.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index b6d44a2..a1bbf6d 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -101,7 +101,7 @@ static int __hci_req_sync(struct hci_dev *hdev,
>>       func(&req, opt);
>>
>>       err = hci_req_run(&req, hci_req_sync_complete);
>> -     if (err < 0) {
>> +     if (err == -ENODATA) {
>>               hdev->req_status = 0;
>>               remove_wait_queue(&hdev->req_wait_q, &wait);
>>               /* req_run will fail if the request did not add any
>> @@ -112,6 +112,11 @@ static int __hci_req_sync(struct hci_dev *hdev,
>>                */
>>               return 0;
>>       }
>> +     if (err < 0) {
>> +             hdev->req_status = HCI_REQ_CANCELED;
>> +             remove_wait_queue(&hdev->req_wait_q, &wait);
>> +             return err;
>> +     }
>
> It might be cleaner to just use one if-branch and something like:
>
>         if (err <  0) {
>                 /* ENODATA means no commands were added, and it should
>                  * not be treated as an error.
>                  */
>                  if (err == -ENODATA)
>                         err = 0;
>
>                 ...
>         }

I'll do like this in v2.

>> @@ -2453,7 +2458,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;
>
> This should probably be in its own patch before the reset, explaining
> that ENODATA is a more natural error to be returned in this case.

Ok, I'll change the returning value in a separated patch.

Regards,

Andre

[PATCH 2/4] 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 set a flag in struct hci_
request to indicate the failure. Once hci_req_run is called, we
verify the error flag. If it is set, we delete all HCI commands
already queued and return a error code.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |  3 +++
 net/bluetooth/hci_core.c         | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3a9cbf2..2edac3f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1042,6 +1042,9 @@ int hci_unregister_cb(struct hci_cb *hcb);
 struct hci_request {
 	struct hci_dev		*hdev;
 	struct sk_buff_head	cmd_q;
+
+	/* This flag is set if something goes wrong during request creation */
+	bool			error;
 };
 
 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 a1bbf6d..402247a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2446,6 +2446,7 @@ void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
 {
 	skb_queue_head_init(&req->cmd_q);
 	req->hdev = hdev;
+	req->error = false;
 }
 
 int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
@@ -2460,6 +2461,15 @@ int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
 	if (skb_queue_empty(&req->cmd_q))
 		return -ENODATA;
 
+	/*
+	 * If error flag is set, remove all HCI commands queued on the HCI
+	 * request queue.
+	 */
+	if (req->error) {
+		skb_queue_purge(&req->cmd_q);
+		return -EIO;
+	}
+
 	skb = skb_peek_tail(&req->cmd_q);
 	bt_cb(skb)->req.complete = complete;
 
@@ -2533,6 +2543,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->error = true;
 		return -ENOMEM;
 	}
 
-- 
1.8.1.2

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

[Johan Hedberg]

Hi Andre,

On Wed, Mar 06, 2013, Andre Guedes wrote:
> @@ -1042,6 +1042,9 @@ int hci_unregister_cb(struct hci_cb *hcb);
>  struct hci_request {
>  	struct hci_dev		*hdev;
>  	struct sk_buff_head	cmd_q;
> +
> +	/* This flag is set if something goes wrong during request creation */
> +	bool			error;
>  };

I'd prefer if you'd use "int err". That'd allow you to record the exact
error from when it occurred and have hci_req_run return it.

>  int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
> @@ -2460,6 +2461,15 @@ int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
>  	if (skb_queue_empty(&req->cmd_q))
>  		return -ENODATA;
>  
> +	/*
> +	 * If error flag is set, remove all HCI commands queued on the HCI
> +	 * request queue.
> +	 */
> +	if (req->error) {
> +		skb_queue_purge(&req->cmd_q);
> +		return -EIO;
> +	}

I think this check belongs before the queue_empty check. If the first
command failed to be added we should properly have hci_req_run fail too.
If you do it your way e.g. hci_req_sync would just succeed even though
there was a memory allocation error.

> @@ -2533,6 +2543,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->error = true;
>  		return -ENOMEM;
>  	}

If the error is already set then I don't think hci_req_add should even
be attempting to add another command to the queue. So you should be
checking for a set error early in the function and just return if it's
set.

Johan

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

[Johan Hedberg]

Hi,

On Thu, Mar 07, 2013, Johan Hedberg wrote:
> > @@ -2533,6 +2543,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->error = true;
> >  		return -ENOMEM;
> >  	}
> 
> If the error is already set then I don't think hci_req_add should even
> be attempting to add another command to the queue. So you should be
> checking for a set error early in the function and just return if it's
> set.

Nevermind. I saw that you have this in the last patch of this set. You
could probably merge that into this patch though.

Johan

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

[Andre Guedes]

Hi Johan,

On Thu, Mar 7, 2013 at 4:19 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Thu, Mar 07, 2013, Johan Hedberg wrote:
>> > @@ -2533,6 +2543,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->error = true;
>> >             return -ENOMEM;
>> >     }
>>
>> If the error is already set then I don't think hci_req_add should even
>> be attempting to add another command to the queue. So you should be
>> checking for a set error early in the function and just return if it's
>> set.
>
> Nevermind. I saw that you have this in the last patch of this set. You
> could probably merge that into this patch though.

Since at this point hci_req_add is not returning void yet, if I merge
the last patch I will have to return a value in case req->err is set.
However, The next patch makes hci_req_add returning void so that value
will be removed anyway.

Regards,

Andre

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

[Andre Guedes]

Hi Johan,

On Thu, Mar 7, 2013 at 4:15 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Andre,
>
> On Wed, Mar 06, 2013, Andre Guedes wrote:
>> @@ -1042,6 +1042,9 @@ int hci_unregister_cb(struct hci_cb *hcb);
>>  struct hci_request {
>>       struct hci_dev          *hdev;
>>       struct sk_buff_head     cmd_q;
>> +
>> +     /* This flag is set if something goes wrong during request creation */
>> +     bool                    error;
>>  };
>
> I'd prefer if you'd use "int err". That'd allow you to record the exact
> error from when it occurred and have hci_req_run return it.

I'll change this in v2.

>>  int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
>> @@ -2460,6 +2461,15 @@ int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
>>       if (skb_queue_empty(&req->cmd_q))
>>               return -ENODATA;
>>
>> +     /*
>> +      * If error flag is set, remove all HCI commands queued on the HCI
>> +      * request queue.
>> +      */
>> +     if (req->error) {
>> +             skb_queue_purge(&req->cmd_q);
>> +             return -EIO;
>> +     }
>
> I think this check belongs before the queue_empty check. If the first
> command failed to be added we should properly have hci_req_run fail too.
> If you do it your way e.g. hci_req_sync would just succeed even though
> there was a memory allocation error.

Fair enough. I'll fix it in v2.

Regards,

Andre

[PATCH 3/4] Bluetooth: Make hci_req_add return 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 return
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 2edac3f..342a0e7 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1049,7 +1049,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 402247a..64fa390 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2533,7 +2533,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;
@@ -2544,15 +2544,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->error = true;
-		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 4/4] Bluetooth: Check req->error flag in hci_req_add

[Andre Guedes]

If req->error 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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 64fa390..e2fdc59 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2540,6 +2540,13 @@ 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 HCI request error flag is set, there is no point in queueing the
+	 * HCI command. We can simply return.
+	 */
+	if (req->error)
+		return;
+
 	skb = hci_prepare_cmd(hdev, opcode, plen, param);
 	if (!skb) {
 		BT_ERR("%s no memory for command", hdev->name);
-- 
1.8.1.2