From: Tedd Ho-Jeong An <[email protected]>
This patch cancels the hci_request timeout work if the expected event
is recevied.
Signed-off-by: Tedd Ho-Jeong An <[email protected]>
---
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
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
Hi Johan
On 12/23/14, 10:55 AM, "Johan Hedberg" <[email protected]> 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
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