[v2] Bluetooth: btnxpuart: Resolve TX timeout error in power save stress test
Commit Message
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(-)
Comments
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
@@ -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;