Re: [PATCH v1] Bluetooth: hci_qca: Handle spurious wakeup from SoC
Hi Abhishek, > I think this code would be simplified by using a delayed_work struct > instead of a timer. And I pointed this out before that all the timers should be moved to a delayed_work. In addition such a complex support for hardware should move towards its own driver solely base on serdev. I am currently getting a bit fed up if I point these things out and the answer, please merge this now and we fix it later. Maybe I need to stop merging things and wait for a proper separate driver for this hardware. Regards Marcel
Re: [PATCH v1] Bluetooth: hci_qca: Handle spurious wakeup from SoC
Hi Venkata,
I think this code would be simplified by using a delayed_work struct
instead of a timer.
Based on your commit description:
On Sun, Nov 15, 2020 at 9:59 AM Venkata Lakshmi Narayana Gubba
wrote:
>
> Added timer to handle spurious wakeup from SoC.
> Timer is started when wake indicator is received from SoC.
> Timer is restarted when valid data is received from SoC.
> Timer is stopped when sleep indicator is received from SoC.
> SSR is triggered upon timer expiry.
>
> Signed-off-by: Venkata Lakshmi Narayana Gubba
in function qca_ibs_wake_ind: (timer started when wake indicator is
received from SoC)
queue_delayed_work(qca->workqueue, >spurious_wake,
IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS)
in function qca_ibs_sleep_ind: (Timer is stopped when sleep indicator
is received from SoC.)
cancel_delayed_work(qca->workqueue, >spurious_wake);
in qca_recv_acl_data, qca_recv_sco_data and qca_recv_event: (Timer is
restarted when valid data is received from SoC.)
if (!test_bit(QCA_IBS_DISABLED, >flags))
mod_delayed_work(qca->workqueue, >spurious_wake,
IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS)
and finally in qca_ibs_spurious_wake_timeout (originally named
hci_ibs_spurious_wake_timeout in your patch): (SSR is triggered upon
timer expiry.)
if (!test_bit(QCA_HW_ERROR_EVENT, >flags))
hci_reset_dev(hu->hdev);
That should trigger qca_hw_error so you don't need to duplicate the
crash dump triggering + waiting in multiple places (and if you get the
spurious wake bug WHILE doing SSR, it won't re-trigger the same
restart over and over).
Abhishek
> ---
> drivers/bluetooth/hci_qca.c | 99
> -
> 1 file changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5cc7b16..6953001 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -48,6 +48,7 @@
> #define IBS_WAKE_RETRANS_TIMEOUT_MS100
> #define IBS_BTSOC_TX_IDLE_TIMEOUT_MS 200
> #define IBS_HOST_TX_IDLE_TIMEOUT_MS2000
> +#define IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS 1
> #define CMD_TRANS_TIMEOUT_MS 100
> #define MEMDUMP_TIMEOUT_MS 8000
> #define IBS_DISABLE_SSR_TIMEOUT_MS (MEMDUMP_TIMEOUT_MS + 1000)
> @@ -147,7 +148,9 @@ struct qca_data {
> bool tx_vote; /* Clock must be on for TX */
> bool rx_vote; /* Clock must be on for RX */
> struct timer_list tx_idle_timer;
> + struct timer_list spurious_wake_timer;
> u32 tx_idle_delay;
> + u32 spurious_wake;
> struct timer_list wake_retrans_timer;
> u32 wake_retrans;
> struct workqueue_struct *workqueue;
> @@ -156,6 +159,7 @@ struct qca_data {
> struct work_struct ws_rx_vote_off;
> struct work_struct ws_tx_vote_off;
> struct work_struct ctrl_memdump_evt;
> + struct work_struct spurious_wake_timeout;
> struct delayed_work ctrl_memdump_timeout;
> struct qca_memdump_data *qca_memdump;
> unsigned long flags;
> @@ -229,6 +233,7 @@ static void qca_regulator_disable(struct qca_serdev
> *qcadev);
> static void qca_power_shutdown(struct hci_uart *hu);
> static int qca_power_off(struct hci_dev *hdev);
> static void qca_controller_memdump(struct work_struct *work);
> +static void qca_wq_spurious_wake_timeout(struct work_struct *work);
>
> static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
> {
> @@ -530,6 +535,15 @@ static void hci_ibs_wake_retrans_timeout(struct
> timer_list *t)
> hci_uart_tx_wakeup(hu);
> }
>
> +static void hci_ibs_spurious_wake_timeout(struct timer_list *t)
> +{
> + struct qca_data *qca = from_timer(qca, t, spurious_wake_timer);
> + struct hci_uart *hu = qca->hu;
> +
> + bt_dev_warn(hu->hdev, "hu %p spurious wake timeout in %d state", hu,
> qca->rx_ibs_state);
> +
> + queue_work(qca->workqueue, >spurious_wake_timeout);
> +}
>
> static void qca_controller_memdump_timeout(struct work_struct *work)
> {
> @@ -584,6 +598,7 @@ static int qca_open(struct hci_uart *hu)
> INIT_WORK(>ws_rx_vote_off, qca_wq_serial_rx_clock_vote_off);
> INIT_WORK(>ws_tx_vote_off, qca_wq_serial_tx_clock_vote_off);
> INIT_WORK(>ctrl_memdump_evt, qca_controller_memdump);
> + INIT_WORK(>spurious_wake_timeout, qca_wq_spurious_wake_timeout);
> INIT_DELAYED_WORK(>ctrl_memdump_timeout,
> qca_controller_memdump_timeout);
> init_waitqueue_head(>suspend_wait_q);
> @@ -615,6 +630,9 @@ static int qca_open(struct hci_uart *hu)
> timer_setup(>tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
> qca->tx_idle_delay = IBS_HOST_TX_IDLE_TIMEOUT_MS;
>
> + timer_setup(>spurious_wake_timer, hci_ibs_spurious_wake_timeout,
> 0);
> + qca->spurious_wake = IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS;
> +
> BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
>
[PATCH v1] Bluetooth: hci_qca: Handle spurious wakeup from SoC
Added timer to handle spurious wakeup from SoC.
Timer is started when wake indicator is received from SoC.
Timer is restarted when valid data is received from SoC.
Timer is stopped when sleep indicator is received from SoC.
SSR is triggered upon timer expiry.
Signed-off-by: Venkata Lakshmi Narayana Gubba
---
drivers/bluetooth/hci_qca.c | 99 -
1 file changed, 97 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5cc7b16..6953001 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -48,6 +48,7 @@
#define IBS_WAKE_RETRANS_TIMEOUT_MS100
#define IBS_BTSOC_TX_IDLE_TIMEOUT_MS 200
#define IBS_HOST_TX_IDLE_TIMEOUT_MS2000
+#define IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS 1
#define CMD_TRANS_TIMEOUT_MS 100
#define MEMDUMP_TIMEOUT_MS 8000
#define IBS_DISABLE_SSR_TIMEOUT_MS (MEMDUMP_TIMEOUT_MS + 1000)
@@ -147,7 +148,9 @@ struct qca_data {
bool tx_vote; /* Clock must be on for TX */
bool rx_vote; /* Clock must be on for RX */
struct timer_list tx_idle_timer;
+ struct timer_list spurious_wake_timer;
u32 tx_idle_delay;
+ u32 spurious_wake;
struct timer_list wake_retrans_timer;
u32 wake_retrans;
struct workqueue_struct *workqueue;
@@ -156,6 +159,7 @@ struct qca_data {
struct work_struct ws_rx_vote_off;
struct work_struct ws_tx_vote_off;
struct work_struct ctrl_memdump_evt;
+ struct work_struct spurious_wake_timeout;
struct delayed_work ctrl_memdump_timeout;
struct qca_memdump_data *qca_memdump;
unsigned long flags;
@@ -229,6 +233,7 @@ static void qca_regulator_disable(struct qca_serdev
*qcadev);
static void qca_power_shutdown(struct hci_uart *hu);
static int qca_power_off(struct hci_dev *hdev);
static void qca_controller_memdump(struct work_struct *work);
+static void qca_wq_spurious_wake_timeout(struct work_struct *work);
static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
{
@@ -530,6 +535,15 @@ static void hci_ibs_wake_retrans_timeout(struct timer_list
*t)
hci_uart_tx_wakeup(hu);
}
+static void hci_ibs_spurious_wake_timeout(struct timer_list *t)
+{
+ struct qca_data *qca = from_timer(qca, t, spurious_wake_timer);
+ struct hci_uart *hu = qca->hu;
+
+ bt_dev_warn(hu->hdev, "hu %p spurious wake timeout in %d state", hu,
qca->rx_ibs_state);
+
+ queue_work(qca->workqueue, >spurious_wake_timeout);
+}
static void qca_controller_memdump_timeout(struct work_struct *work)
{
@@ -584,6 +598,7 @@ static int qca_open(struct hci_uart *hu)
INIT_WORK(>ws_rx_vote_off, qca_wq_serial_rx_clock_vote_off);
INIT_WORK(>ws_tx_vote_off, qca_wq_serial_tx_clock_vote_off);
INIT_WORK(>ctrl_memdump_evt, qca_controller_memdump);
+ INIT_WORK(>spurious_wake_timeout, qca_wq_spurious_wake_timeout);
INIT_DELAYED_WORK(>ctrl_memdump_timeout,
qca_controller_memdump_timeout);
init_waitqueue_head(>suspend_wait_q);
@@ -615,6 +630,9 @@ static int qca_open(struct hci_uart *hu)
timer_setup(>tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
qca->tx_idle_delay = IBS_HOST_TX_IDLE_TIMEOUT_MS;
+ timer_setup(>spurious_wake_timer, hci_ibs_spurious_wake_timeout,
0);
+ qca->spurious_wake = IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS;
+
BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
qca->tx_idle_delay, qca->wake_retrans);
@@ -694,6 +712,7 @@ static int qca_close(struct hci_uart *hu)
skb_queue_purge(>rx_memdump_q);
del_timer(>tx_idle_timer);
del_timer(>wake_retrans_timer);
+ del_timer(>spurious_wake_timer);
destroy_workqueue(qca->workqueue);
qca->hu = NULL;
@@ -710,7 +729,7 @@ static int qca_close(struct hci_uart *hu)
*/
static void device_want_to_wakeup(struct hci_uart *hu)
{
- unsigned long flags;
+ unsigned long flags, wake_timeout;
struct qca_data *qca = hu->priv;
BT_DBG("hu %p want to wake up", hu);
@@ -731,6 +750,10 @@ static void device_want_to_wakeup(struct hci_uart *hu)
* receiving the wake up indicator awake rx clock.
*/
queue_work(qca->workqueue, >ws_awake_rx);
+ if (!test_bit(QCA_SSR_TRIGGERED, >flags)) {
+ wake_timeout = msecs_to_jiffies(qca->spurious_wake);
+ mod_timer(>spurious_wake_timer, jiffies +
wake_timeout);
+ }
spin_unlock_irqrestore(>hci_ibs_lock, flags);
return;
@@ -777,9 +800,11 @@ static void device_want_to_sleep(struct hci_uart *hu)
qca->rx_ibs_state = HCI_IBS_RX_ASLEEP;
/* Vote off rx clock under workqueue */
queue_work(qca->workqueue, >ws_rx_vote_off);
+
