Bluetooth: btnxpuart: Resolve TX timeout error in power save stress test
authorNeeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
Wed, 27 Dec 2023 13:29:27 +0000 (18:59 +0530)
committerLuiz Augusto von Dentz <luiz.von.dentz@intel.com>
Wed, 6 Mar 2024 22:22:36 +0000 (17:22 -0500)
This fixes the tx timeout issue seen while running a stress test on
btnxpuart for couple of hours, such that the interval between two HCI
commands coincide with the power save timeout value of 2 seconds.

Test procedure using bash script:
<load btnxpuart.ko>
hciconfig hci0 up
//Enable Power Save feature
hcitool -i hci0 cmd 3f 23 02 00 00
while (true)
do
    hciconfig hci0 leadv
    sleep 2
    hciconfig hci0 noleadv
    sleep 2
done

Error log, after adding few more debug prints:
Bluetooth: btnxpuart_queue_skb(): 01 0A 20 01 00
Bluetooth: hci0: Set UART break: on, status=0
Bluetooth: hci0: btnxpuart_tx_wakeup() tx_work scheduled
Bluetooth: hci0: btnxpuart_tx_work() dequeue: 01 0A 20 01 00
Can't set advertise mode on hci0: Connection timed out (110)
Bluetooth: hci0: command 0x200a tx timeout

When the power save mechanism turns on UART break, and btnxpuart_tx_work()
is scheduled simultaneously, psdata->ps_state is read as PS_STATE_AWAKE,
which prevents the psdata->work from being scheduled, which is responsible
to turn OFF UART break.

This issue is fixed by adding a ps_lock mutex around UART break on/off as
well as around ps_state read/write.
btnxpuart_tx_wakeup() will now read updated ps_state value. If ps_state is
PS_STATE_SLEEP, it will first schedule psdata->work, and then it will
reschedule itself once UART break has been turned off and ps_state is
PS_STATE_AWAKE.

Tested above script for 50,000 iterations and TX timeout error was not
observed anymore.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
drivers/bluetooth/btnxpuart.c

index 1d592ac413d1ff6e9cdabe3d6b461b42c6c95dba..55b6e3dcd4ecf4612bce4d7d1ff8d4700491b1fd 100644 (file)
@@ -126,6 +126,7 @@ struct ps_data {
        struct hci_dev *hdev;
        struct work_struct work;
        struct timer_list ps_timer;
+       struct mutex ps_lock;
 };
 
 struct wakeup_cmd_payload {
@@ -317,6 +318,9 @@ static void ps_start_timer(struct btnxpuart_dev *nxpdev)
 
        if (psdata->cur_psmode == PS_MODE_ENABLE)
                mod_timer(&psdata->ps_timer, jiffies + msecs_to_jiffies(psdata->h2c_ps_interval));
+
+       if (psdata->ps_state == PS_STATE_AWAKE && psdata->ps_cmd == PS_CMD_ENTER_PS)
+               cancel_work_sync(&psdata->work);
 }
 
 static void ps_cancel_timer(struct btnxpuart_dev *nxpdev)
@@ -337,6 +341,7 @@ static void ps_control(struct hci_dev *hdev, u8 ps_state)
            !test_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state))
                return;
 
+       mutex_lock(&psdata->ps_lock);
        switch (psdata->cur_h2c_wakeupmode) {
        case WAKEUP_METHOD_DTR:
                if (ps_state == PS_STATE_AWAKE)
@@ -350,12 +355,15 @@ static void ps_control(struct hci_dev *hdev, u8 ps_state)
                        status = serdev_device_break_ctl(nxpdev->serdev, 0);
                else
                        status = serdev_device_break_ctl(nxpdev->serdev, -1);
+               msleep(20); /* Allow chip to detect UART-break and enter sleep */
                bt_dev_dbg(hdev, "Set UART break: %s, status=%d",
                           str_on_off(ps_state == PS_STATE_SLEEP), status);
                break;
        }
        if (!status)
                psdata->ps_state = ps_state;
+       mutex_unlock(&psdata->ps_lock);
+
        if (ps_state == PS_STATE_AWAKE)
                btnxpuart_tx_wakeup(nxpdev);
 }
@@ -391,17 +399,25 @@ static void ps_setup(struct hci_dev *hdev)
 
        psdata->hdev = hdev;
        INIT_WORK(&psdata->work, ps_work_func);
+       mutex_init(&psdata->ps_lock);
        timer_setup(&psdata->ps_timer, ps_timeout_func, 0);
 }
 
-static void ps_wakeup(struct btnxpuart_dev *nxpdev)
+static bool ps_wakeup(struct btnxpuart_dev *nxpdev)
 {
        struct ps_data *psdata = &nxpdev->psdata;
+       u8 ps_state;
 
-       if (psdata->ps_state != PS_STATE_AWAKE) {
+       mutex_lock(&psdata->ps_lock);
+       ps_state = psdata->ps_state;
+       mutex_unlock(&psdata->ps_lock);
+
+       if (ps_state != PS_STATE_AWAKE) {
                psdata->ps_cmd = PS_CMD_EXIT_PS;
                schedule_work(&psdata->work);
+               return true;
        }
+       return false;
 }
 
 static int send_ps_cmd(struct hci_dev *hdev, void *data)
@@ -1171,7 +1187,6 @@ static struct sk_buff *nxp_dequeue(void *data)
 {
        struct btnxpuart_dev *nxpdev = (struct btnxpuart_dev *)data;
 
-       ps_wakeup(nxpdev);
        ps_start_timer(nxpdev);
        return skb_dequeue(&nxpdev->txq);
 }
@@ -1186,6 +1201,9 @@ static void btnxpuart_tx_work(struct work_struct *work)
        struct sk_buff *skb;
        int len;
 
+       if (ps_wakeup(nxpdev))
+               return;
+
        while ((skb = nxp_dequeue(nxpdev))) {
                len = serdev_device_write_buf(serdev, skb->data, skb->len);
                hdev->stat.byte_tx += len;