Hi Kiran,
> For NOP command, need to cancel work scheduled on cmd_timer,
> on receiving command status or commmand complete event.
>
> Below use case might lead to race condition multiple when NOP
> commands are queued sequentially:
>
> hci_cmd_work() {
> if (atomic_read(&hdev->cmd_cnt) {
> .
> .
> .
> atomic_dec(&hdev->cmd_cnt);
> hci_send_frame(hdev,...);
> schedule_delayed_work(&hdev->cmd_timer,...);
> }
> }
>
> On receiving event for first NOP, the work scheduled on hdev->cmd_timer
> is not cancelled and second NOP is dequeued and sent to controller.
>
> While waiting for an event for second NOP command, work scheduled on
> cmd_timer for first NOP can get scheduled, resulting in sending third
> NOP command not waiting for an event for second NOP. This might cause
> issues at controller side (like memory overrun, controller going
> unresponsive) resulting in hci tx timeouts, hardware errors etc.
>
> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@xxxxxxxxx>
> ---
> net/bluetooth/hci_event.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index ea7fc09478be..14dfbdc8b81b 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3271,8 +3271,7 @@ static void hci_remote_features_evt(struct hci_dev *hdev,
> static inline void handle_cmd_cnt_and_timer(struct hci_dev *hdev,
> u16 opcode, u8 ncmd)
> {
> - if (opcode != HCI_OP_NOP)
> - cancel_delayed_work(&hdev->cmd_timer);
> + cancel_delayed_work(&hdev->cmd_timer);
>
> if (!test_bit(HCI_RESET, &hdev->flags)) {
> if (ncmd) {
so this is conflicting with the patch introducing the ncmd timeout handling.
commit de75cd0d9b2f3250d5f25846bb5632ccce6275f4
Author: Manish Mandlik <mmandlik@xxxxxxxxxx>
Date: Thu Apr 29 10:24:22 2021 -0700
Bluetooth: Add ncmd=0 recovery handling
During command status or command complete event, the controller may set
ncmd=0 indicating that it is not accepting any more commands. In such a
case, host holds off sending any more commands to the controller. If the
controller doesn't recover from such condition, host will wait forever,
until the user decides that the Bluetooth is broken and may power cycles
the Bluetooth.
This patch triggers the hardware error to reset the controller and
driver when it gets into such state as there is no other wat out.
Nowhere in your commit description you are addressing why is this the right to do.
Regards
Marcel
