Bluetooth: Cancel the hci_request timeout if it received expected event
Subsystems (2)
PATCH 1/1 Bluetooth: Cancel the hci_request timeout if it received expected event
From: Tedd Ho-Jeong An <[email protected]>
This patch cancels the hci_request timeout work if the expected event is recevied.
--- net/bluetooth/hci_event.c | 3 +++ 1 file changed, 3 insertions(+)
net/bluetooth/hci_event.c +3 -0
@@ -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 ); }
Signature · 1 line
1.9.1
Hi Tedd,
Quoted 2 lines · Tedd Ho-Jeong An
This patch cancels the
hci_requesttimeout 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.
net/bluetooth/hci_event.c void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb) · line 4859
Quoted 2 lines
net/bluetooth/hci_event.c +2
-0+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
Hi Johan
Quoted 9 lines · Johan Hedberg
Hi Tedd,
Quoted 2 lines · Tedd Ho-Jeong An
This patch cancels the
hci_requesttimeout work if the expected event is recevied.The timer you're canceling isn't
hci_requestspecific, 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 orCommand_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.
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ if (opcode !=3D HCI_OP_NOP) + cancel_delayed_work(&hdev->cmd_timer);I don't think the check for
HCI_OP_NOPis necessary here. The opcode is from the original command that was sent and it can't beHCI_OP_NOP. The other places checking for this before callingcancel_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
Quoted 2 lines
Johan
Marcel Holtmann
maintainerHi Tedd,
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
This patch cancels the
hci_requesttimeout work if the expected event is recevied.The timer you're canceling isn't
hci_requestspecific, 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.
Quoted 6 lines
When the command is sent with
__hci_cmd_sync_ev()and expected event is other thanCommand_CompleteorCommand_Status, especially, vendor specific event (0xFF), the function returns the skb filled with
event parameters properly, but there is tx timeout error because thecmd_timeris 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