| Message ID | 20231226113110.3923962-1-neeraj.sanjaykale@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Bluetooth: btnxpuart: Resolve TX timeout error in power save stress test | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=812886 ---Test result--- Test Summary: CheckPatch PASS 0.63 seconds GitLint PASS 0.31 seconds SubjectPrefix PASS 0.12 seconds BuildKernel PASS 28.90 seconds CheckAllWarning PASS 31.18 seconds CheckSparse PASS 37.96 seconds CheckSmatch PASS 100.66 seconds BuildKernel32 PASS 27.66 seconds TestRunnerSetup PASS 442.29 seconds TestRunner_l2cap-tester PASS 23.08 seconds TestRunner_iso-tester PASS 46.32 seconds TestRunner_bnep-tester PASS 7.15 seconds TestRunner_mgmt-tester PASS 165.35 seconds TestRunner_rfcomm-tester PASS 10.98 seconds TestRunner_sco-tester PASS 14.47 seconds TestRunner_ioctl-tester PASS 12.20 seconds TestRunner_mesh-tester PASS 8.82 seconds TestRunner_smp-tester PASS 9.70 seconds TestRunner_userchan-tester PASS 7.32 seconds IncrementalBuild PASS 26.95 seconds --- Regards, Linux Bluetooth
Hi Neeraj, kernel test robot noticed the following build errors: [auto build test ERROR on bluetooth/master] [also build test ERROR on bluetooth-next/master linus/master v6.7-rc7 next-20231222] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Neeraj-Sanjay-Kale/Bluetooth-btnxpuart-Resolve-TX-timeout-error-in-power-save-stress-test/20231226-193718 base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master patch link: https://lore.kernel.org/r/20231226113110.3923962-1-neeraj.sanjaykale%40nxp.com patch subject: [PATCH v2] Bluetooth: btnxpuart: Resolve TX timeout error in power save stress test config: arm-randconfig-003-20231227 (https://download.01.org/0day-ci/archive/20231227/202312271021.m0of6rmb-lkp@intel.com/config) compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project d3ef86708241a3bee902615c190dead1638c4e09) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231227/202312271021.m0of6rmb-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312271021.m0of6rmb-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/bluetooth/btnxpuart.c:356:4: error: call to undeclared function 'usleep'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 356 | usleep(2000); /* Allow chip to detect UART-break and enter sleep */ | ^ 1 error generated. vim +/usleep +356 drivers/bluetooth/btnxpuart.c 333 334 static void ps_control(struct hci_dev *hdev, u8 ps_state) 335 { 336 struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); 337 struct ps_data *psdata = &nxpdev->psdata; 338 int status; 339 340 if (psdata->ps_state == ps_state || 341 !test_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state)) 342 return; 343 344 mutex_lock(&psdata->ps_lock); 345 switch (psdata->cur_h2c_wakeupmode) { 346 case WAKEUP_METHOD_DTR: 347 if (ps_state == PS_STATE_AWAKE) 348 status = serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0); 349 else 350 status = serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR); 351 break; 352 case WAKEUP_METHOD_BREAK: 353 default: 354 if (ps_state == PS_STATE_AWAKE) { 355 status = serdev_device_break_ctl(nxpdev->serdev, 0); > 356 usleep(2000); /* Allow chip to detect UART-break and enter sleep */ 357 } else { 358 status = serdev_device_break_ctl(nxpdev->serdev, -1); 359 } 360 bt_dev_dbg(hdev, "Set UART break: %s, status=%d", 361 str_on_off(ps_state == PS_STATE_SLEEP), status); 362 break; 363 } 364 if (!status) 365 psdata->ps_state = ps_state; 366 mutex_unlock(&psdata->ps_lock); 367 368 if (ps_state == PS_STATE_AWAKE) 369 btnxpuart_tx_wakeup(nxpdev); 370 } 371
Hi Neeraj, kernel test robot noticed the following build errors: [auto build test ERROR on bluetooth/master] [also build test ERROR on bluetooth-next/master linus/master v6.7-rc7 next-20231222] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Neeraj-Sanjay-Kale/Bluetooth-btnxpuart-Resolve-TX-timeout-error-in-power-save-stress-test/20231226-193718 base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master patch link: https://lore.kernel.org/r/20231226113110.3923962-1-neeraj.sanjaykale%40nxp.com patch subject: [PATCH v2] Bluetooth: btnxpuart: Resolve TX timeout error in power save stress test config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20231227/202312271920.D4X4fO6I-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231227/202312271920.D4X4fO6I-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202312271920.D4X4fO6I-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/bluetooth/btnxpuart.c: In function 'ps_control': >> drivers/bluetooth/btnxpuart.c:356:25: error: implicit declaration of function 'usleep'; did you mean 'fsleep'? [-Werror=implicit-function-declaration] 356 | usleep(2000); /* Allow chip to detect UART-break and enter sleep */ | ^~~~~~ | fsleep cc1: some warnings being treated as errors vim +356 drivers/bluetooth/btnxpuart.c 333 334 static void ps_control(struct hci_dev *hdev, u8 ps_state) 335 { 336 struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev); 337 struct ps_data *psdata = &nxpdev->psdata; 338 int status; 339 340 if (psdata->ps_state == ps_state || 341 !test_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state)) 342 return; 343 344 mutex_lock(&psdata->ps_lock); 345 switch (psdata->cur_h2c_wakeupmode) { 346 case WAKEUP_METHOD_DTR: 347 if (ps_state == PS_STATE_AWAKE) 348 status = serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0); 349 else 350 status = serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR); 351 break; 352 case WAKEUP_METHOD_BREAK: 353 default: 354 if (ps_state == PS_STATE_AWAKE) { 355 status = serdev_device_break_ctl(nxpdev->serdev, 0); > 356 usleep(2000); /* Allow chip to detect UART-break and enter sleep */ 357 } else { 358 status = serdev_device_break_ctl(nxpdev->serdev, -1); 359 } 360 bt_dev_dbg(hdev, "Set UART break: %s, status=%d", 361 str_on_off(ps_state == PS_STATE_SLEEP), status); 362 break; 363 } 364 if (!status) 365 psdata->ps_state = ps_state; 366 mutex_unlock(&psdata->ps_lock); 367 368 if (ps_state == PS_STATE_AWAKE) 369 btnxpuart_tx_wakeup(nxpdev); 370 } 371
diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c index b7e66b7ac570..c6ef5878abe5 100644 --- a/drivers/bluetooth/btnxpuart.c +++ b/drivers/bluetooth/btnxpuart.c @@ -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) @@ -346,16 +351,20 @@ static void ps_control(struct hci_dev *hdev, u8 ps_state) break; case WAKEUP_METHOD_BREAK: default: - if (ps_state == PS_STATE_AWAKE) + if (ps_state == PS_STATE_AWAKE) { status = serdev_device_break_ctl(nxpdev->serdev, 0); - else + usleep(2000); /* Allow chip to detect UART-break and enter sleep */ + } else { status = serdev_device_break_ctl(nxpdev->serdev, -1); + } 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 +400,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; + + mutex_lock(&psdata->ps_lock); + ps_state = psdata->ps_state; + mutex_unlock(&psdata->ps_lock); - if (psdata->ps_state != PS_STATE_AWAKE) { + 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 +1188,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 +1202,11 @@ static void btnxpuart_tx_work(struct work_struct *work) struct sk_buff *skb; int len; + if (ps_wakeup(nxpdev)) { + schedule_work(&nxpdev->tx_work); + return; + } + while ((skb = nxp_dequeue(nxpdev))) { len = serdev_device_write_buf(serdev, skb->data, skb->len); hdev->stat.byte_tx += len;
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> --- v2: Add 2msec delay after enabling UART break, cancel ps_work when ps_timer is restarted. --- drivers/bluetooth/btnxpuart.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)