[PATCH] Bluetooth: Cancel the hci_request timeout if it received expected event

[PATCH] Bluetooth: Cancel the hci_request timeout if it received expected event

[Tedd Ho-Jeong An]

From: Tedd Ho-Jeong An <tedd.an@intel.com>

This patch cancels the hci_request timeout work if the expected event
is recevied.

Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com>
---
 net/bluetooth/hci_event.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 39a5c8a..9d18470 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4855,6 +4855,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		struct hci_command_hdr *cmd_hdr = (void *) hdev->sent_cmd->data;
 		u16 opcode = __le16_to_cpu(cmd_hdr->opcode);
 
+		if (opcode != HCI_OP_NOP)
+			cancel_delayed_work(&hdev->cmd_timer);
+
 		hci_req_cmd_complete(hdev, opcode, 0);
 	}
 
-- 
1.9.1

Re: [PATCH] Bluetooth: Cancel the hci_request timeout if it received expected event

[Johan Hedberg]

Hi Tedd,

On Mon, Dec 22, 2014, Tedd Ho-Jeong An wrote:
> This patch cancels the hci_request timeout work if the expected event
> is recevied.

The timer you're canceling isn't hci_request specific, so I'm not sure
why you're making that reference here? You might want to provide some
more detailed explanation in the commit message.

> +		if (opcode != HCI_OP_NOP)
> +			cancel_delayed_work(&hdev->cmd_timer);

I don't think the check for HCI_OP_NOP is necessary here. The opcode is
from the original command that was sent and it can't be HCI_OP_NOP. The
other places checking for this before calling cancel_delayed_work() take
their opcode from the cmd_status/cmd_complete events where it has
special meaning (i.e. a spontaneous event generated by the controller).

Johan

Re: [PATCH] Bluetooth: Cancel the hci_request timeout if it received expected event

[An, Tedd]

Hi Johan

On 12/23/14, 10:55 AM, "Johan Hedberg" <johan.hedberg@intel.com> wrote:

>Hi Tedd,
>
>On Mon, Dec 22, 2014, Tedd Ho-Jeong An wrote:
>> This patch cancels the hci_request timeout work if the expected event
>> is recevied.
>
>The timer you're canceling isn't hci_request specific, so I'm not sure
>why you're making that reference here? You might want to provide some
>more detailed explanation in the commit message.

When the command is sent with __hci_cmd_sync_ev() and expected event is
other than Command_Complete or
Command_Status, especially, vendor specific event (0xFF), the function
returns the skb filled with
event parameters properly, but there is tx timeout error because the
cmd_timer is not canceled.

I will send out updated patch with details.


>
>> +		if (opcode !=3D HCI_OP_NOP)
>> +			cancel_delayed_work(&hdev->cmd_timer);
>
>I don't think the check for HCI_OP_NOP is necessary here. The opcode is
>from the original command that was sent and it can't be HCI_OP_NOP. The
>other places checking for this before calling cancel_delayed_work() take
>their opcode from the cmd_status/cmd_complete events where it has
>special meaning (i.e. a spontaneous event generated by the controller).

Got it.
=20
>
>Johan

Re: [PATCH] Bluetooth: Cancel the hci_request timeout if it received expected event

[Marcel Holtmann]

Hi Tedd,

>>> This patch cancels the hci_request timeout work if the expected event
>>> is recevied.
>> 
>> The timer you're canceling isn't hci_request specific, so I'm not sure
>> why you're making that reference here? You might want to provide some
>> more detailed explanation in the commit message.
> 
> When the command is sent with __hci_cmd_sync_ev() and expected event is
> other than Command_Complete or
> Command_Status, especially, vendor specific event (0xFF), the function
> returns the skb filled with
> event parameters properly, but there is tx timeout error because the
> cmd_timer is not canceled.

the cmd_timer is protecting the cmd_q and with that is doing exactly the right thing here. It is here for handling cmd_status and cmd_complete events and with that updating ncmd and HCI command flow control. So we can not just cancel the timer here.

It seems that __hci_cmd_sync_ev is actually not used at all at the moment. However it works just fine when you get the interim cmd_status as done by all HCI commands. The problem is that you are not seeing the cmd_status and thus you have problems here. But we can not just ignore the missing cmd_status. If we do, then ncmd is out of date and HCI flow control is broken at that point.

With that said, we can neither use __hci_cmd_sync nor __hci_cmd_sync_ev for Bluetooth controllers that do not obey the HCI command flow control. So sending commands via cmd_q is not an option here.

The only way I am thinking of being able to work around such behavior is by introducing separate functionality to handle it. So we might need __hci_cmd_raw_sync and __hci_cmd_raw_sync_ev that allows scheduling on raw_q and skipping cmd_complete and cmd_status processing. This however would mean that __hci_cmd_raw_sync can never return data from cmd_complete events (meaning it is actually not _sync in the sense of returning data, even while the name would suggest it).

Regards

Marcel