[Bluez,v2,3/3] policy: Reconnect audio on controller resume

Commit Message

Abhishek Pandit-Subedi Aug. 18, 2020, 11:28 p.m. UTC

During system suspend, all peer devices are disconnected. On resume, HID
devices will reconnect but audio devices stay disconnected. As a quality
of life improvement, mark audio devices that were disconnected due to
suspend and attempt to reconnect them when the controller resumes (after
a delay for better co-existence with Wi-Fi).

Signed-off-by: Abhishek Pandit-Subedi <[e-mail]>
---

Changes in v2:
- Refactored to use policy instead of connecting directly in adapter

 plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
 src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
 src/adapter.h    |  6 +++++
 src/main.c       |  1 +
 src/main.conf    |  9 +++++++
 5 files changed, 121 insertions(+), 4 deletions(-)

Comments

Luiz Augusto von Dentz Aug. 27, 2020, 6:32 a.m. UTC | #1

Hi Abhishek,

On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
<[e-mail]> wrote:
>
> During system suspend, all peer devices are disconnected. On resume, HID
> devices will reconnect but audio devices stay disconnected. As a quality
> of life improvement, mark audio devices that were disconnected due to
> suspend and attempt to reconnect them when the controller resumes (after
> a delay for better co-existence with Wi-Fi).
>
> Signed-off-by: Abhishek Pandit-Subedi <[e-mail]>
> ---
>
> Changes in v2:
> - Refactored to use policy instead of connecting directly in adapter
>
>  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
>  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
>  src/adapter.h    |  6 +++++
>  src/main.c       |  1 +
>  src/main.conf    |  9 +++++++
>  5 files changed, 121 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/policy.c b/plugins/policy.c
> index de51e58b9..b07a997b9 100644
> --- a/plugins/policy.c
> +++ b/plugins/policy.c
> @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
>  static int *reconnect_intervals = NULL;
>  static size_t reconnect_intervals_len = 0;
>
> +static const int default_reconnect_delay = 5;
> +static int resume_reconnect_delay = 5;
> +
>  static GSList *reconnects = NULL;
>
>  static unsigned int service_id = 0;
> @@ -93,6 +96,8 @@ struct policy_data {
>         uint8_t ct_retries;
>         guint tg_timer;
>         uint8_t tg_retries;
> +
> +       bool reconnect_on_resume;
>  };
>
>  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
>
>         data->sink_timer = 0;
>         data->sink_retries++;
> +       data->reconnect_on_resume = false;
>
>         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
>         if (service != NULL)
> @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
>         return FALSE;
>  }
>
> -static void policy_set_sink_timer(struct policy_data *data)
> +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
>  {
>         if (data->sink_timer > 0)
>                 g_source_remove(data->sink_timer);
>
> -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> -                                                       policy_connect_sink,
> +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
>                                                         data);
>  }
>
> +static void policy_set_sink_timer(struct policy_data *data)
> +{
> +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> +}
> +
>  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
>                                                 btd_service_state_t new_state)
>  {
> @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
>  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
>  {
>         struct reconnect_data *reconnect;
> +       struct btd_service *service;
> +       struct policy_data *data;
> +       bool do_reconnect = false;
>
>         DBG("reason %u", reason);
>
> -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> +       switch(reason) {
> +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> +       case MGMT_DEV_DISCONN_REMOTE:
> +               do_reconnect = true;
> +               break;
> +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> +               if (service && (data = policy_get_data(dev))) {
> +                       data->reconnect_on_resume = true;
> +               }

Can't we just program the timer directly here? Or would that wakeup
the system, I would imagine all timers are disabled while suspended
which means when the system resumes so does the timers which would
then trigger the reconnection logic.

> +               break;
> +       /* All other cases do not result in reconnect */
> +       default:
> +               break;
> +       }
> +
> +       if (!do_reconnect)
>                 return;
>
>         reconnect = reconnect_find(dev);
> @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
>         reconnect_set_timer(reconnect);
>  }
>
> +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> +                                       const uint8_t addr_type)
> +{
> +       struct btd_device *dev;
> +       GSList *l;
> +
> +       /* Check if any devices needed to be reconnected on resume */
> +       for (l = devices; l; l = g_slist_next(l)) {
> +               struct policy_data *data = l->data;
> +
> +               if (data->reconnect_on_resume) {
> +                       policy_set_sink_timer_internal(data,
> +                                                      resume_reconnect_delay);
> +               }
> +       }
> +}
> +
>  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
>  {
>         struct reconnect_data *reconnect;
> @@ -854,9 +901,17 @@ static int policy_init(void)
>         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
>                                                                         NULL);
>
> +       resume_reconnect_delay = g_key_file_get_integer(
> +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> +
> +       if (gerr) {
> +               g_clear_error(&gerr);
> +               resume_reconnect_delay = default_reconnect_delay;
> +       }
>  done:
>         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
>                 btd_add_disconnect_cb(disconnect_cb);
> +               btd_add_controller_resume_cb(controller_resume_cb);
>                 btd_add_conn_fail_cb(conn_fail_cb);
>         }
>
> @@ -869,6 +924,7 @@ done:
>  static void policy_exit(void)
>  {
>         btd_remove_disconnect_cb(disconnect_cb);
> +       btd_remove_controller_resume_cb(controller_resume_cb);
>         btd_remove_conn_fail_cb(conn_fail_cb);
>
>         if (reconnect_uuids)
> diff --git a/src/adapter.c b/src/adapter.c
> index 5e896a9f0..7526feb9e 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
>
>  static GSList *disconnect_list = NULL;
>  static GSList *conn_fail_list = NULL;
> +static GSList *controller_resume_list = NULL;
>
>  struct link_key_info {
>         bdaddr_t bdaddr;
> @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
>         eir_data_free(&eir_data);
>  }
>
> +static void controller_resume_notify(const uint8_t wake_reason,
> +                                       const bdaddr_t *addr,
> +                                       const uint8_t addr_type)
> +{
> +       GSList *l;
> +
> +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> +               btd_controller_resume_cb resume_cb = l->data;
> +               resume_cb(wake_reason, addr, addr_type);
> +       }
> +}
> +
> +static void controller_resume_callback(uint16_t index, uint16_t length,
> +                                      const void *param, void *user_data)
> +{
> +       const struct mgmt_ev_controller_resume *ev = param;
> +       struct btd_adapter *adapter = user_data;
> +
> +       if (length < sizeof(*ev)) {
> +               btd_error(adapter->dev_id, "Too small device resume event");
> +               return;
> +       }
> +
> +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> +
> +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> +                                ev->addr.type);
> +}
> +
> +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> +{
> +       controller_resume_list = g_slist_append(controller_resume_list, func);
> +}
> +
> +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> +{
> +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> +}
> +
>  static void device_blocked_callback(uint16_t index, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
>                                                 user_passkey_notify_callback,
>                                                 adapter, NULL);
>
> +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> +                                               adapter->dev_id,
> +                                               controller_resume_callback,
> +                                               adapter, NULL);
> +
>         set_dev_class(adapter);
>
>         set_name(adapter, btd_adapter_get_name(adapter));
> diff --git a/src/adapter.h b/src/adapter.h
> index f8ac20261..5527e4205 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
>  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
>  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
>
> +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> +                                       const bdaddr_t *addr,
> +                                       const uint8_t addr_type);
> +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);

If we can program the timer just before we suspend this is not really
necessary, but if you still thing that would be a good idea to notify
Id put it in btd_adapter_driver with a callback for suspend and
resume, that way we don't have to maintain multiple lists for each of
the callbacks.

>  struct btd_adapter *adapter_find(const bdaddr_t *sba);
>  struct btd_adapter *adapter_find_by_id(int id);
>  void adapter_foreach(adapter_cb func, gpointer user_data);
> diff --git a/src/main.c b/src/main.c
> index 2c083de67..a1c3e7e9f 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -131,6 +131,7 @@ static const char *policy_options[] = {
>         "ReconnectAttempts",
>         "ReconnectIntervals",
>         "AutoEnable",
> +       "ReconnectAudioDelay",
>         NULL
>  };
>
> diff --git a/src/main.conf b/src/main.conf
> index f41203b96..8d74a2aec 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -198,3 +198,12 @@
>  # This includes adapters present on start as well as adapters that are plugged
>  # in later on. Defaults to 'false'.
>  #AutoEnable=false
> +
> +# Audio devices that were disconnected due to suspend will be reconnected on
> +# resume. ReconnectAudioDelay determines the delay between when the controller
> +# resumes from suspend and a connection attempt is made. A longer delay can be
> +# better for co-existence with Wi-Fi.
> +# The value is in seconds.
> +# Default: 5
> +#ReconnectAudioDelay = 5
> +
> --
> 2.28.0.297.g1956fa8f8d-goog
>

Abhishek Pandit-Subedi Aug. 27, 2020, 9:08 p.m. UTC | #2

Hi Luiz,

On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Abhishek,
>
> On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> <[e-mail]> wrote:
> >
> > During system suspend, all peer devices are disconnected. On resume, HID
> > devices will reconnect but audio devices stay disconnected. As a quality
> > of life improvement, mark audio devices that were disconnected due to
> > suspend and attempt to reconnect them when the controller resumes (after
> > a delay for better co-existence with Wi-Fi).
> >
> > Signed-off-by: Abhishek Pandit-Subedi <[e-mail]>
> > ---
> >
> > Changes in v2:
> > - Refactored to use policy instead of connecting directly in adapter
> >
> >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> >  src/adapter.h    |  6 +++++
> >  src/main.c       |  1 +
> >  src/main.conf    |  9 +++++++
> >  5 files changed, 121 insertions(+), 4 deletions(-)
> >
> > diff --git a/plugins/policy.c b/plugins/policy.c
> > index de51e58b9..b07a997b9 100644
> > --- a/plugins/policy.c
> > +++ b/plugins/policy.c
> > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> >  static int *reconnect_intervals = NULL;
> >  static size_t reconnect_intervals_len = 0;
> >
> > +static const int default_reconnect_delay = 5;
> > +static int resume_reconnect_delay = 5;
> > +
> >  static GSList *reconnects = NULL;
> >
> >  static unsigned int service_id = 0;
> > @@ -93,6 +96,8 @@ struct policy_data {
> >         uint8_t ct_retries;
> >         guint tg_timer;
> >         uint8_t tg_retries;
> > +
> > +       bool reconnect_on_resume;
> >  };
> >
> >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> >
> >         data->sink_timer = 0;
> >         data->sink_retries++;
> > +       data->reconnect_on_resume = false;
> >
> >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> >         if (service != NULL)
> > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> >         return FALSE;
> >  }
> >
> > -static void policy_set_sink_timer(struct policy_data *data)
> > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> >  {
> >         if (data->sink_timer > 0)
> >                 g_source_remove(data->sink_timer);
> >
> > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > -                                                       policy_connect_sink,
> > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> >                                                         data);
> >  }
> >
> > +static void policy_set_sink_timer(struct policy_data *data)
> > +{
> > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > +}
> > +
> >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> >                                                 btd_service_state_t new_state)
> >  {
> > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> >  {
> >         struct reconnect_data *reconnect;
> > +       struct btd_service *service;
> > +       struct policy_data *data;
> > +       bool do_reconnect = false;
> >
> >         DBG("reason %u", reason);
> >
> > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > +       switch(reason) {
> > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > +       case MGMT_DEV_DISCONN_REMOTE:
> > +               do_reconnect = true;
> > +               break;
> > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > +               if (service && (data = policy_get_data(dev))) {
> > +                       data->reconnect_on_resume = true;
> > +               }
>
> Can't we just program the timer directly here? Or would that wakeup
> the system, I would imagine all timers are disabled while suspended
> which means when the system resumes so does the timers which would
> then trigger the reconnection logic.

This approach works if every resume is user initiated and keeps the
device awake long enough to reconnect to the audio device. On
ChromeOS, this isn't always the case. We have a feature called Dark
Resume that occurs when the system wakes to handle an event that
doesn't require user intervention and suspends again without ever
turning on the screen. See:
https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
A primary user for this is a periodic wake-up that checks battery
percentage and shuts down the system if it's too low.

One of the tests I ran specifically for this is a suspend stress test
with a wake time between 2-4s (i.e. suspend_stress_test -c 10
--wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
timer when disconnecting, the device would attempt a connection on one
of the wakeups and fail (since we suspended without completing the
connection).

>
> > +               break;
> > +       /* All other cases do not result in reconnect */
> > +       default:
> > +               break;
> > +       }
> > +
> > +       if (!do_reconnect)
> >                 return;
> >
> >         reconnect = reconnect_find(dev);
> > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> >         reconnect_set_timer(reconnect);
> >  }
> >
> > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > +                                       const uint8_t addr_type)
> > +{
> > +       struct btd_device *dev;
> > +       GSList *l;
> > +
> > +       /* Check if any devices needed to be reconnected on resume */
> > +       for (l = devices; l; l = g_slist_next(l)) {
> > +               struct policy_data *data = l->data;
> > +
> > +               if (data->reconnect_on_resume) {
> > +                       policy_set_sink_timer_internal(data,
> > +                                                      resume_reconnect_delay);
> > +               }
> > +       }
> > +}
> > +
> >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> >  {
> >         struct reconnect_data *reconnect;
> > @@ -854,9 +901,17 @@ static int policy_init(void)
> >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> >                                                                         NULL);
> >
> > +       resume_reconnect_delay = g_key_file_get_integer(
> > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > +
> > +       if (gerr) {
> > +               g_clear_error(&gerr);
> > +               resume_reconnect_delay = default_reconnect_delay;
> > +       }
> >  done:
> >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> >                 btd_add_disconnect_cb(disconnect_cb);
> > +               btd_add_controller_resume_cb(controller_resume_cb);
> >                 btd_add_conn_fail_cb(conn_fail_cb);
> >         }
> >
> > @@ -869,6 +924,7 @@ done:
> >  static void policy_exit(void)
> >  {
> >         btd_remove_disconnect_cb(disconnect_cb);
> > +       btd_remove_controller_resume_cb(controller_resume_cb);
> >         btd_remove_conn_fail_cb(conn_fail_cb);
> >
> >         if (reconnect_uuids)
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 5e896a9f0..7526feb9e 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> >
> >  static GSList *disconnect_list = NULL;
> >  static GSList *conn_fail_list = NULL;
> > +static GSList *controller_resume_list = NULL;
> >
> >  struct link_key_info {
> >         bdaddr_t bdaddr;
> > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> >         eir_data_free(&eir_data);
> >  }
> >
> > +static void controller_resume_notify(const uint8_t wake_reason,
> > +                                       const bdaddr_t *addr,
> > +                                       const uint8_t addr_type)
> > +{
> > +       GSList *l;
> > +
> > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > +               btd_controller_resume_cb resume_cb = l->data;
> > +               resume_cb(wake_reason, addr, addr_type);
> > +       }
> > +}
> > +
> > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > +                                      const void *param, void *user_data)
> > +{
> > +       const struct mgmt_ev_controller_resume *ev = param;
> > +       struct btd_adapter *adapter = user_data;
> > +
> > +       if (length < sizeof(*ev)) {
> > +               btd_error(adapter->dev_id, "Too small device resume event");
> > +               return;
> > +       }
> > +
> > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > +
> > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > +                                ev->addr.type);
> > +}
> > +
> > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > +{
> > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > +}
> > +
> > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > +{
> > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > +}
> > +
> >  static void device_blocked_callback(uint16_t index, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> >                                                 user_passkey_notify_callback,
> >                                                 adapter, NULL);
> >
> > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > +                                               adapter->dev_id,
> > +                                               controller_resume_callback,
> > +                                               adapter, NULL);
> > +
> >         set_dev_class(adapter);
> >
> >         set_name(adapter, btd_adapter_get_name(adapter));
> > diff --git a/src/adapter.h b/src/adapter.h
> > index f8ac20261..5527e4205 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> >
> > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > +                                       const bdaddr_t *addr,
> > +                                       const uint8_t addr_type);
> > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
>
> If we can program the timer just before we suspend this is not really
> necessary, but if you still thing that would be a good idea to notify
> Id put it in btd_adapter_driver with a callback for suspend and
> resume, that way we don't have to maintain multiple lists for each of
> the callbacks.

The adapter driver seems like a reasonable place to put this so let me
port that over.

>
> >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> >  struct btd_adapter *adapter_find_by_id(int id);
> >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > diff --git a/src/main.c b/src/main.c
> > index 2c083de67..a1c3e7e9f 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> >         "ReconnectAttempts",
> >         "ReconnectIntervals",
> >         "AutoEnable",
> > +       "ReconnectAudioDelay",
> >         NULL
> >  };
> >
> > diff --git a/src/main.conf b/src/main.conf
> > index f41203b96..8d74a2aec 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -198,3 +198,12 @@
> >  # This includes adapters present on start as well as adapters that are plugged
> >  # in later on. Defaults to 'false'.
> >  #AutoEnable=false
> > +
> > +# Audio devices that were disconnected due to suspend will be reconnected on
> > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > +# better for co-existence with Wi-Fi.
> > +# The value is in seconds.
> > +# Default: 5
> > +#ReconnectAudioDelay = 5
> > +
> > --
> > 2.28.0.297.g1956fa8f8d-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Luiz Augusto von Dentz Aug. 28, 2020, 5:08 p.m. UTC | #3

Hi Abhishek,

On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
<[e-mail]> wrote:
>
> Hi Luiz,
>
> On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Abhishek,
> >
> > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > <[e-mail]> wrote:
> > >
> > > During system suspend, all peer devices are disconnected. On resume, HID
> > > devices will reconnect but audio devices stay disconnected. As a quality
> > > of life improvement, mark audio devices that were disconnected due to
> > > suspend and attempt to reconnect them when the controller resumes (after
> > > a delay for better co-existence with Wi-Fi).
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <[e-mail]>
> > > ---
> > >
> > > Changes in v2:
> > > - Refactored to use policy instead of connecting directly in adapter
> > >
> > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > >  src/adapter.h    |  6 +++++
> > >  src/main.c       |  1 +
> > >  src/main.conf    |  9 +++++++
> > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > index de51e58b9..b07a997b9 100644
> > > --- a/plugins/policy.c
> > > +++ b/plugins/policy.c
> > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > >  static int *reconnect_intervals = NULL;
> > >  static size_t reconnect_intervals_len = 0;
> > >
> > > +static const int default_reconnect_delay = 5;
> > > +static int resume_reconnect_delay = 5;
> > > +
> > >  static GSList *reconnects = NULL;
> > >
> > >  static unsigned int service_id = 0;
> > > @@ -93,6 +96,8 @@ struct policy_data {
> > >         uint8_t ct_retries;
> > >         guint tg_timer;
> > >         uint8_t tg_retries;
> > > +
> > > +       bool reconnect_on_resume;
> > >  };
> > >
> > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > >
> > >         data->sink_timer = 0;
> > >         data->sink_retries++;
> > > +       data->reconnect_on_resume = false;
> > >
> > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > >         if (service != NULL)
> > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > >         return FALSE;
> > >  }
> > >
> > > -static void policy_set_sink_timer(struct policy_data *data)
> > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > >  {
> > >         if (data->sink_timer > 0)
> > >                 g_source_remove(data->sink_timer);
> > >
> > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > -                                                       policy_connect_sink,
> > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > >                                                         data);
> > >  }
> > >
> > > +static void policy_set_sink_timer(struct policy_data *data)
> > > +{
> > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > +}
> > > +
> > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > >                                                 btd_service_state_t new_state)
> > >  {
> > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > >  {
> > >         struct reconnect_data *reconnect;
> > > +       struct btd_service *service;
> > > +       struct policy_data *data;
> > > +       bool do_reconnect = false;
> > >
> > >         DBG("reason %u", reason);
> > >
> > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > +       switch(reason) {
> > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > +               do_reconnect = true;
> > > +               break;
> > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > +               if (service && (data = policy_get_data(dev))) {
> > > +                       data->reconnect_on_resume = true;
> > > +               }
> >
> > Can't we just program the timer directly here? Or would that wakeup
> > the system, I would imagine all timers are disabled while suspended
> > which means when the system resumes so does the timers which would
> > then trigger the reconnection logic.
>
> This approach works if every resume is user initiated and keeps the
> device awake long enough to reconnect to the audio device. On
> ChromeOS, this isn't always the case. We have a feature called Dark
> Resume that occurs when the system wakes to handle an event that
> doesn't require user intervention and suspends again without ever
> turning on the screen. See:
> https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> A primary user for this is a periodic wake-up that checks battery
> percentage and shuts down the system if it's too low.

Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
Resumes should be handled by the kernel instead of each userspace
component? I suppose there quite a few timers that may have side
effects if dark resumes actually resume them.

> One of the tests I ran specifically for this is a suspend stress test
> with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> timer when disconnecting, the device would attempt a connection on one
> of the wakeups and fail (since we suspended without completing the
> connection).

We could perhaps check if the kernel is capable of emitting
suspend/resume events and if not just program the timer, or
alternatively add another config option to indicate when the system
supports such concept of Dark Resumes.

> >
> > > +               break;
> > > +       /* All other cases do not result in reconnect */
> > > +       default:
> > > +               break;
> > > +       }
> > > +
> > > +       if (!do_reconnect)
> > >                 return;
> > >
> > >         reconnect = reconnect_find(dev);
> > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > >         reconnect_set_timer(reconnect);
> > >  }
> > >
> > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > +                                       const uint8_t addr_type)
> > > +{
> > > +       struct btd_device *dev;
> > > +       GSList *l;
> > > +
> > > +       /* Check if any devices needed to be reconnected on resume */
> > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > +               struct policy_data *data = l->data;
> > > +
> > > +               if (data->reconnect_on_resume) {
> > > +                       policy_set_sink_timer_internal(data,
> > > +                                                      resume_reconnect_delay);
> > > +               }
> > > +       }
> > > +}
> > > +
> > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > >  {
> > >         struct reconnect_data *reconnect;
> > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > >                                                                         NULL);
> > >
> > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > +
> > > +       if (gerr) {
> > > +               g_clear_error(&gerr);
> > > +               resume_reconnect_delay = default_reconnect_delay;
> > > +       }
> > >  done:
> > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > >                 btd_add_disconnect_cb(disconnect_cb);
> > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > >         }
> > >
> > > @@ -869,6 +924,7 @@ done:
> > >  static void policy_exit(void)
> > >  {
> > >         btd_remove_disconnect_cb(disconnect_cb);
> > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > >
> > >         if (reconnect_uuids)
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index 5e896a9f0..7526feb9e 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > >
> > >  static GSList *disconnect_list = NULL;
> > >  static GSList *conn_fail_list = NULL;
> > > +static GSList *controller_resume_list = NULL;
> > >
> > >  struct link_key_info {
> > >         bdaddr_t bdaddr;
> > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > >         eir_data_free(&eir_data);
> > >  }
> > >
> > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > +                                       const bdaddr_t *addr,
> > > +                                       const uint8_t addr_type)
> > > +{
> > > +       GSList *l;
> > > +
> > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > +               btd_controller_resume_cb resume_cb = l->data;
> > > +               resume_cb(wake_reason, addr, addr_type);
> > > +       }
> > > +}
> > > +
> > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > +                                      const void *param, void *user_data)
> > > +{
> > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > +       struct btd_adapter *adapter = user_data;
> > > +
> > > +       if (length < sizeof(*ev)) {
> > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > +               return;
> > > +       }
> > > +
> > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > +
> > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > +                                ev->addr.type);
> > > +}
> > > +
> > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > +{
> > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > +}
> > > +
> > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > +{
> > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > +}
> > > +
> > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > >                                         const void *param, void *user_data)
> > >  {
> > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > >                                                 user_passkey_notify_callback,
> > >                                                 adapter, NULL);
> > >
> > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > +                                               adapter->dev_id,
> > > +                                               controller_resume_callback,
> > > +                                               adapter, NULL);
> > > +
> > >         set_dev_class(adapter);
> > >
> > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > diff --git a/src/adapter.h b/src/adapter.h
> > > index f8ac20261..5527e4205 100644
> > > --- a/src/adapter.h
> > > +++ b/src/adapter.h
> > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > >
> > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > +                                       const bdaddr_t *addr,
> > > +                                       const uint8_t addr_type);
> > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> >
> > If we can program the timer just before we suspend this is not really
> > necessary, but if you still thing that would be a good idea to notify
> > Id put it in btd_adapter_driver with a callback for suspend and
> > resume, that way we don't have to maintain multiple lists for each of
> > the callbacks.
>
> The adapter driver seems like a reasonable place to put this so let me
> port that over.
>
> >
> > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > >  struct btd_adapter *adapter_find_by_id(int id);
> > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > diff --git a/src/main.c b/src/main.c
> > > index 2c083de67..a1c3e7e9f 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > >         "ReconnectAttempts",
> > >         "ReconnectIntervals",
> > >         "AutoEnable",
> > > +       "ReconnectAudioDelay",
> > >         NULL
> > >  };
> > >
> > > diff --git a/src/main.conf b/src/main.conf
> > > index f41203b96..8d74a2aec 100644
> > > --- a/src/main.conf
> > > +++ b/src/main.conf
> > > @@ -198,3 +198,12 @@
> > >  # This includes adapters present on start as well as adapters that are plugged
> > >  # in later on. Defaults to 'false'.
> > >  #AutoEnable=false
> > > +
> > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > +# better for co-existence with Wi-Fi.
> > > +# The value is in seconds.
> > > +# Default: 5
> > > +#ReconnectAudioDelay = 5
> > > +
> > > --
> > > 2.28.0.297.g1956fa8f8d-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz

Abhishek Pandit-Subedi Aug. 28, 2020, 5:31 p.m. UTC | #4

Hi Luiz,

On Fri, Aug 28, 2020 at 10:09 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Abhishek,
>
> On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
> <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > > <[e-mail]> wrote:
> > > >
> > > > During system suspend, all peer devices are disconnected. On resume, HID
> > > > devices will reconnect but audio devices stay disconnected. As a quality
> > > > of life improvement, mark audio devices that were disconnected due to
> > > > suspend and attempt to reconnect them when the controller resumes (after
> > > > a delay for better co-existence with Wi-Fi).
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Refactored to use policy instead of connecting directly in adapter
> > > >
> > > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > > >  src/adapter.h    |  6 +++++
> > > >  src/main.c       |  1 +
> > > >  src/main.conf    |  9 +++++++
> > > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > > index de51e58b9..b07a997b9 100644
> > > > --- a/plugins/policy.c
> > > > +++ b/plugins/policy.c
> > > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > > >  static int *reconnect_intervals = NULL;
> > > >  static size_t reconnect_intervals_len = 0;
> > > >
> > > > +static const int default_reconnect_delay = 5;
> > > > +static int resume_reconnect_delay = 5;
> > > > +
> > > >  static GSList *reconnects = NULL;
> > > >
> > > >  static unsigned int service_id = 0;
> > > > @@ -93,6 +96,8 @@ struct policy_data {
> > > >         uint8_t ct_retries;
> > > >         guint tg_timer;
> > > >         uint8_t tg_retries;
> > > > +
> > > > +       bool reconnect_on_resume;
> > > >  };
> > > >
> > > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > >
> > > >         data->sink_timer = 0;
> > > >         data->sink_retries++;
> > > > +       data->reconnect_on_resume = false;
> > > >
> > > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > > >         if (service != NULL)
> > > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > >         return FALSE;
> > > >  }
> > > >
> > > > -static void policy_set_sink_timer(struct policy_data *data)
> > > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > > >  {
> > > >         if (data->sink_timer > 0)
> > > >                 g_source_remove(data->sink_timer);
> > > >
> > > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > > -                                                       policy_connect_sink,
> > > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > > >                                                         data);
> > > >  }
> > > >
> > > > +static void policy_set_sink_timer(struct policy_data *data)
> > > > +{
> > > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > > +}
> > > > +
> > > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > > >                                                 btd_service_state_t new_state)
> > > >  {
> > > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > >  {
> > > >         struct reconnect_data *reconnect;
> > > > +       struct btd_service *service;
> > > > +       struct policy_data *data;
> > > > +       bool do_reconnect = false;
> > > >
> > > >         DBG("reason %u", reason);
> > > >
> > > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > > +       switch(reason) {
> > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > > +               do_reconnect = true;
> > > > +               break;
> > > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > > +               if (service && (data = policy_get_data(dev))) {
> > > > +                       data->reconnect_on_resume = true;
> > > > +               }
> > >
> > > Can't we just program the timer directly here? Or would that wakeup
> > > the system, I would imagine all timers are disabled while suspended
> > > which means when the system resumes so does the timers which would
> > > then trigger the reconnection logic.
> >
> > This approach works if every resume is user initiated and keeps the
> > device awake long enough to reconnect to the audio device. On
> > ChromeOS, this isn't always the case. We have a feature called Dark
> > Resume that occurs when the system wakes to handle an event that
> > doesn't require user intervention and suspends again without ever
> > turning on the screen. See:
> > https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> > A primary user for this is a periodic wake-up that checks battery
> > percentage and shuts down the system if it's too low.
>
> Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
> Resumes should be handled by the kernel instead of each userspace
> component? I suppose there quite a few timers that may have side
> effects if dark resumes actually resume them.
>

I'm not aware of the history around it but I found another page with
more context of use cases (like actually letting a user space
application run as would happen on a smartphone):
https://www.chromium.org/chromium-os/chromiumos-design-docs/lucid-sleep

> > One of the tests I ran specifically for this is a suspend stress test
> > with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> > --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> > timer when disconnecting, the device would attempt a connection on one
> > of the wakeups and fail (since we suspended without completing the
> > connection).
>
> We could perhaps check if the kernel is capable of emitting
> suspend/resume events and if not just program the timer, or
> alternatively add another config option to indicate when the system
> supports such concept of Dark Resumes.
>

How does the following sound:

* On disconnect, I always queue the reconnect but with the resume
delay set instead of the interval[0] value. I also set the
resume_reconnect flag
* On every resume event (or dark resume event), I reset the timer and
add resume_delay seconds if resume_reconnect is set
* The first time the reconnect_timeout runs, I clear the resume_reconnect flag

The delay will only be reapplied if the kernel is capable of emitting
suspend/resume events and/or dark resume events.

> > >
> > > > +               break;
> > > > +       /* All other cases do not result in reconnect */
> > > > +       default:
> > > > +               break;
> > > > +       }
> > > > +
> > > > +       if (!do_reconnect)
> > > >                 return;
> > > >
> > > >         reconnect = reconnect_find(dev);
> > > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > >         reconnect_set_timer(reconnect);
> > > >  }
> > > >
> > > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > > +                                       const uint8_t addr_type)
> > > > +{
> > > > +       struct btd_device *dev;
> > > > +       GSList *l;
> > > > +
> > > > +       /* Check if any devices needed to be reconnected on resume */
> > > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > > +               struct policy_data *data = l->data;
> > > > +
> > > > +               if (data->reconnect_on_resume) {
> > > > +                       policy_set_sink_timer_internal(data,
> > > > +                                                      resume_reconnect_delay);
> > > > +               }
> > > > +       }
> > > > +}
> > > > +
> > > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > > >  {
> > > >         struct reconnect_data *reconnect;
> > > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > > >                                                                         NULL);
> > > >
> > > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > > +
> > > > +       if (gerr) {
> > > > +               g_clear_error(&gerr);
> > > > +               resume_reconnect_delay = default_reconnect_delay;
> > > > +       }
> > > >  done:
> > > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > > >                 btd_add_disconnect_cb(disconnect_cb);
> > > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > > >         }
> > > >
> > > > @@ -869,6 +924,7 @@ done:
> > > >  static void policy_exit(void)
> > > >  {
> > > >         btd_remove_disconnect_cb(disconnect_cb);
> > > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > > >
> > > >         if (reconnect_uuids)
> > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > index 5e896a9f0..7526feb9e 100644
> > > > --- a/src/adapter.c
> > > > +++ b/src/adapter.c
> > > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > > >
> > > >  static GSList *disconnect_list = NULL;
> > > >  static GSList *conn_fail_list = NULL;
> > > > +static GSList *controller_resume_list = NULL;
> > > >
> > > >  struct link_key_info {
> > > >         bdaddr_t bdaddr;
> > > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > > >         eir_data_free(&eir_data);
> > > >  }
> > > >
> > > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > > +                                       const bdaddr_t *addr,
> > > > +                                       const uint8_t addr_type)
> > > > +{
> > > > +       GSList *l;
> > > > +
> > > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > > +               btd_controller_resume_cb resume_cb = l->data;
> > > > +               resume_cb(wake_reason, addr, addr_type);
> > > > +       }
> > > > +}
> > > > +
> > > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > > +                                      const void *param, void *user_data)
> > > > +{
> > > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > > +       struct btd_adapter *adapter = user_data;
> > > > +
> > > > +       if (length < sizeof(*ev)) {
> > > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > > +               return;
> > > > +       }
> > > > +
> > > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > > +
> > > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > > +                                ev->addr.type);
> > > > +}
> > > > +
> > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > > +{
> > > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > > +}
> > > > +
> > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > > +{
> > > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > > +}
> > > > +
> > > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > > >                                         const void *param, void *user_data)
> > > >  {
> > > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > > >                                                 user_passkey_notify_callback,
> > > >                                                 adapter, NULL);
> > > >
> > > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > > +                                               adapter->dev_id,
> > > > +                                               controller_resume_callback,
> > > > +                                               adapter, NULL);
> > > > +
> > > >         set_dev_class(adapter);
> > > >
> > > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > index f8ac20261..5527e4205 100644
> > > > --- a/src/adapter.h
> > > > +++ b/src/adapter.h
> > > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > > >
> > > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > > +                                       const bdaddr_t *addr,
> > > > +                                       const uint8_t addr_type);
> > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> > >
> > > If we can program the timer just before we suspend this is not really
> > > necessary, but if you still thing that would be a good idea to notify
> > > Id put it in btd_adapter_driver with a callback for suspend and
> > > resume, that way we don't have to maintain multiple lists for each of
> > > the callbacks.
> >
> > The adapter driver seems like a reasonable place to put this so let me
> > port that over.
> >
> > >
> > > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > > >  struct btd_adapter *adapter_find_by_id(int id);
> > > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > > diff --git a/src/main.c b/src/main.c
> > > > index 2c083de67..a1c3e7e9f 100644
> > > > --- a/src/main.c
> > > > +++ b/src/main.c
> > > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > > >         "ReconnectAttempts",
> > > >         "ReconnectIntervals",
> > > >         "AutoEnable",
> > > > +       "ReconnectAudioDelay",
> > > >         NULL
> > > >  };
> > > >
> > > > diff --git a/src/main.conf b/src/main.conf
> > > > index f41203b96..8d74a2aec 100644
> > > > --- a/src/main.conf
> > > > +++ b/src/main.conf
> > > > @@ -198,3 +198,12 @@
> > > >  # This includes adapters present on start as well as adapters that are plugged
> > > >  # in later on. Defaults to 'false'.
> > > >  #AutoEnable=false
> > > > +
> > > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > > +# better for co-existence with Wi-Fi.
> > > > +# The value is in seconds.
> > > > +# Default: 5
> > > > +#ReconnectAudioDelay = 5
> > > > +
> > > > --
> > > > 2.28.0.297.g1956fa8f8d-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Abhishek

Luiz Augusto von Dentz Aug. 28, 2020, 5:56 p.m. UTC | #5

Hi Abhishek,

On Fri, Aug 28, 2020 at 10:31 AM Abhishek Pandit-Subedi
<[e-mail]> wrote:
>
> Hi Luiz,
>
> On Fri, Aug 28, 2020 at 10:09 AM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Abhishek,
> >
> > On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
> > <[e-mail]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Abhishek,
> > > >
> > > > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > > > <[e-mail]> wrote:
> > > > >
> > > > > During system suspend, all peer devices are disconnected. On resume, HID
> > > > > devices will reconnect but audio devices stay disconnected. As a quality
> > > > > of life improvement, mark audio devices that were disconnected due to
> > > > > suspend and attempt to reconnect them when the controller resumes (after
> > > > > a delay for better co-existence with Wi-Fi).
> > > > >
> > > > > Signed-off-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Refactored to use policy instead of connecting directly in adapter
> > > > >
> > > > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > > > >  src/adapter.h    |  6 +++++
> > > > >  src/main.c       |  1 +
> > > > >  src/main.conf    |  9 +++++++
> > > > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > > > index de51e58b9..b07a997b9 100644
> > > > > --- a/plugins/policy.c
> > > > > +++ b/plugins/policy.c
> > > > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > > > >  static int *reconnect_intervals = NULL;
> > > > >  static size_t reconnect_intervals_len = 0;
> > > > >
> > > > > +static const int default_reconnect_delay = 5;
> > > > > +static int resume_reconnect_delay = 5;
> > > > > +
> > > > >  static GSList *reconnects = NULL;
> > > > >
> > > > >  static unsigned int service_id = 0;
> > > > > @@ -93,6 +96,8 @@ struct policy_data {
> > > > >         uint8_t ct_retries;
> > > > >         guint tg_timer;
> > > > >         uint8_t tg_retries;
> > > > > +
> > > > > +       bool reconnect_on_resume;
> > > > >  };
> > > > >
> > > > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > >
> > > > >         data->sink_timer = 0;
> > > > >         data->sink_retries++;
> > > > > +       data->reconnect_on_resume = false;
> > > > >
> > > > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > > > >         if (service != NULL)
> > > > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > >         return FALSE;
> > > > >  }
> > > > >
> > > > > -static void policy_set_sink_timer(struct policy_data *data)
> > > > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > > > >  {
> > > > >         if (data->sink_timer > 0)
> > > > >                 g_source_remove(data->sink_timer);
> > > > >
> > > > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > > > -                                                       policy_connect_sink,
> > > > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > > > >                                                         data);
> > > > >  }
> > > > >
> > > > > +static void policy_set_sink_timer(struct policy_data *data)
> > > > > +{
> > > > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > > > +}
> > > > > +
> > > > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > > > >                                                 btd_service_state_t new_state)
> > > > >  {
> > > > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > > > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > >  {
> > > > >         struct reconnect_data *reconnect;
> > > > > +       struct btd_service *service;
> > > > > +       struct policy_data *data;
> > > > > +       bool do_reconnect = false;
> > > > >
> > > > >         DBG("reason %u", reason);
> > > > >
> > > > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > > > +       switch(reason) {
> > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > > > +               do_reconnect = true;
> > > > > +               break;
> > > > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > > > +               if (service && (data = policy_get_data(dev))) {
> > > > > +                       data->reconnect_on_resume = true;
> > > > > +               }
> > > >
> > > > Can't we just program the timer directly here? Or would that wakeup
> > > > the system, I would imagine all timers are disabled while suspended
> > > > which means when the system resumes so does the timers which would
> > > > then trigger the reconnection logic.
> > >
> > > This approach works if every resume is user initiated and keeps the
> > > device awake long enough to reconnect to the audio device. On
> > > ChromeOS, this isn't always the case. We have a feature called Dark
> > > Resume that occurs when the system wakes to handle an event that
> > > doesn't require user intervention and suspends again without ever
> > > turning on the screen. See:
> > > https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> > > A primary user for this is a periodic wake-up that checks battery
> > > percentage and shuts down the system if it's too low.
> >
> > Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
> > Resumes should be handled by the kernel instead of each userspace
> > component? I suppose there quite a few timers that may have side
> > effects if dark resumes actually resume them.
> >
>
> I'm not aware of the history around it but I found another page with
> more context of use cases (like actually letting a user space
> application run as would happen on a smartphone):
> https://www.chromium.org/chromium-os/chromiumos-design-docs/lucid-sleep
>
> > > One of the tests I ran specifically for this is a suspend stress test
> > > with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> > > --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> > > timer when disconnecting, the device would attempt a connection on one
> > > of the wakeups and fail (since we suspended without completing the
> > > connection).
> >
> > We could perhaps check if the kernel is capable of emitting
> > suspend/resume events and if not just program the timer, or
> > alternatively add another config option to indicate when the system
> > supports such concept of Dark Resumes.
> >
>
> How does the following sound:
>
> * On disconnect, I always queue the reconnect but with the resume
> delay set instead of the interval[0] value. I also set the
> resume_reconnect flag
> * On every resume event (or dark resume event), I reset the timer and
> add resume_delay seconds if resume_reconnect is set
> * The first time the reconnect_timeout runs, I clear the resume_reconnect flag
>
> The delay will only be reapplied if the kernel is capable of emitting
> suspend/resume events and/or dark resume events.

That should work as well, does full resumes need to reset the timer
though? Or we don't have any means to identify the resume type? Well
either way I guess the difference would be quite small since the
timeout is the same just when it is programmed (on suspend vs on
resume).

> > > >
> > > > > +               break;
> > > > > +       /* All other cases do not result in reconnect */
> > > > > +       default:
> > > > > +               break;
> > > > > +       }
> > > > > +
> > > > > +       if (!do_reconnect)
> > > > >                 return;
> > > > >
> > > > >         reconnect = reconnect_find(dev);
> > > > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > >         reconnect_set_timer(reconnect);
> > > > >  }
> > > > >
> > > > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > > > +                                       const uint8_t addr_type)
> > > > > +{
> > > > > +       struct btd_device *dev;
> > > > > +       GSList *l;
> > > > > +
> > > > > +       /* Check if any devices needed to be reconnected on resume */
> > > > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > > > +               struct policy_data *data = l->data;
> > > > > +
> > > > > +               if (data->reconnect_on_resume) {
> > > > > +                       policy_set_sink_timer_internal(data,
> > > > > +                                                      resume_reconnect_delay);
> > > > > +               }
> > > > > +       }
> > > > > +}
> > > > > +
> > > > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > > > >  {
> > > > >         struct reconnect_data *reconnect;
> > > > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > > > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > > > >                                                                         NULL);
> > > > >
> > > > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > > > +
> > > > > +       if (gerr) {
> > > > > +               g_clear_error(&gerr);
> > > > > +               resume_reconnect_delay = default_reconnect_delay;
> > > > > +       }
> > > > >  done:
> > > > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > > > >                 btd_add_disconnect_cb(disconnect_cb);
> > > > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > > > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > > > >         }
> > > > >
> > > > > @@ -869,6 +924,7 @@ done:
> > > > >  static void policy_exit(void)
> > > > >  {
> > > > >         btd_remove_disconnect_cb(disconnect_cb);
> > > > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > > > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > > > >
> > > > >         if (reconnect_uuids)
> > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > index 5e896a9f0..7526feb9e 100644
> > > > > --- a/src/adapter.c
> > > > > +++ b/src/adapter.c
> > > > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > > > >
> > > > >  static GSList *disconnect_list = NULL;
> > > > >  static GSList *conn_fail_list = NULL;
> > > > > +static GSList *controller_resume_list = NULL;
> > > > >
> > > > >  struct link_key_info {
> > > > >         bdaddr_t bdaddr;
> > > > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > > > >         eir_data_free(&eir_data);
> > > > >  }
> > > > >
> > > > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > > > +                                       const bdaddr_t *addr,
> > > > > +                                       const uint8_t addr_type)
> > > > > +{
> > > > > +       GSList *l;
> > > > > +
> > > > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > > > +               btd_controller_resume_cb resume_cb = l->data;
> > > > > +               resume_cb(wake_reason, addr, addr_type);
> > > > > +       }
> > > > > +}
> > > > > +
> > > > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > > > +                                      const void *param, void *user_data)
> > > > > +{
> > > > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > > > +       struct btd_adapter *adapter = user_data;
> > > > > +
> > > > > +       if (length < sizeof(*ev)) {
> > > > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > > > +
> > > > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > > > +                                ev->addr.type);
> > > > > +}
> > > > > +
> > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > > > +{
> > > > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > > > +}
> > > > > +
> > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > > > +{
> > > > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > > > +}
> > > > > +
> > > > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > > > >                                         const void *param, void *user_data)
> > > > >  {
> > > > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > > > >                                                 user_passkey_notify_callback,
> > > > >                                                 adapter, NULL);
> > > > >
> > > > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > > > +                                               adapter->dev_id,
> > > > > +                                               controller_resume_callback,
> > > > > +                                               adapter, NULL);
> > > > > +
> > > > >         set_dev_class(adapter);
> > > > >
> > > > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > > index f8ac20261..5527e4205 100644
> > > > > --- a/src/adapter.h
> > > > > +++ b/src/adapter.h
> > > > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > > > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > > > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > > > >
> > > > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > > > +                                       const bdaddr_t *addr,
> > > > > +                                       const uint8_t addr_type);
> > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> > > >
> > > > If we can program the timer just before we suspend this is not really
> > > > necessary, but if you still thing that would be a good idea to notify
> > > > Id put it in btd_adapter_driver with a callback for suspend and
> > > > resume, that way we don't have to maintain multiple lists for each of
> > > > the callbacks.
> > >
> > > The adapter driver seems like a reasonable place to put this so let me
> > > port that over.
> > >
> > > >
> > > > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > > > >  struct btd_adapter *adapter_find_by_id(int id);
> > > > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > > > diff --git a/src/main.c b/src/main.c
> > > > > index 2c083de67..a1c3e7e9f 100644
> > > > > --- a/src/main.c
> > > > > +++ b/src/main.c
> > > > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > > > >         "ReconnectAttempts",
> > > > >         "ReconnectIntervals",
> > > > >         "AutoEnable",
> > > > > +       "ReconnectAudioDelay",
> > > > >         NULL
> > > > >  };
> > > > >
> > > > > diff --git a/src/main.conf b/src/main.conf
> > > > > index f41203b96..8d74a2aec 100644
> > > > > --- a/src/main.conf
> > > > > +++ b/src/main.conf
> > > > > @@ -198,3 +198,12 @@
> > > > >  # This includes adapters present on start as well as adapters that are plugged
> > > > >  # in later on. Defaults to 'false'.
> > > > >  #AutoEnable=false
> > > > > +
> > > > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > > > +# better for co-existence with Wi-Fi.
> > > > > +# The value is in seconds.
> > > > > +# Default: 5
> > > > > +#ReconnectAudioDelay = 5
> > > > > +
> > > > > --
> > > > > 2.28.0.297.g1956fa8f8d-goog
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Abhishek

Abhishek Pandit-Subedi Aug. 28, 2020, 11:40 p.m. UTC | #6

Hi Luiz,

On Fri, Aug 28, 2020 at 10:56 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Abhishek,
>
> On Fri, Aug 28, 2020 at 10:31 AM Abhishek Pandit-Subedi
> <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > On Fri, Aug 28, 2020 at 10:09 AM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> > > > <[e-mail]> wrote:
> > > > >
> > > > > Hi Abhishek,
> > > > >
> > > > > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > During system suspend, all peer devices are disconnected. On resume, HID
> > > > > > devices will reconnect but audio devices stay disconnected. As a quality
> > > > > > of life improvement, mark audio devices that were disconnected due to
> > > > > > suspend and attempt to reconnect them when the controller resumes (after
> > > > > > a delay for better co-existence with Wi-Fi).
> > > > > >
> > > > > > Signed-off-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Refactored to use policy instead of connecting directly in adapter
> > > > > >
> > > > > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > > > > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > > > > >  src/adapter.h    |  6 +++++
> > > > > >  src/main.c       |  1 +
> > > > > >  src/main.conf    |  9 +++++++
> > > > > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > > > > index de51e58b9..b07a997b9 100644
> > > > > > --- a/plugins/policy.c
> > > > > > +++ b/plugins/policy.c
> > > > > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > > > > >  static int *reconnect_intervals = NULL;
> > > > > >  static size_t reconnect_intervals_len = 0;
> > > > > >
> > > > > > +static const int default_reconnect_delay = 5;
> > > > > > +static int resume_reconnect_delay = 5;
> > > > > > +
> > > > > >  static GSList *reconnects = NULL;
> > > > > >
> > > > > >  static unsigned int service_id = 0;
> > > > > > @@ -93,6 +96,8 @@ struct policy_data {
> > > > > >         uint8_t ct_retries;
> > > > > >         guint tg_timer;
> > > > > >         uint8_t tg_retries;
> > > > > > +
> > > > > > +       bool reconnect_on_resume;
> > > > > >  };
> > > > > >
> > > > > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > > > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > > >
> > > > > >         data->sink_timer = 0;
> > > > > >         data->sink_retries++;
> > > > > > +       data->reconnect_on_resume = false;
> > > > > >
> > > > > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > > > > >         if (service != NULL)
> > > > > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > > >         return FALSE;
> > > > > >  }
> > > > > >
> > > > > > -static void policy_set_sink_timer(struct policy_data *data)
> > > > > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > > > > >  {
> > > > > >         if (data->sink_timer > 0)
> > > > > >                 g_source_remove(data->sink_timer);
> > > > > >
> > > > > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > > > > -                                                       policy_connect_sink,
> > > > > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > > > > >                                                         data);
> > > > > >  }
> > > > > >
> > > > > > +static void policy_set_sink_timer(struct policy_data *data)
> > > > > > +{
> > > > > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > > > > +}
> > > > > > +
> > > > > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > > > > >                                                 btd_service_state_t new_state)
> > > > > >  {
> > > > > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > > > > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > > >  {
> > > > > >         struct reconnect_data *reconnect;
> > > > > > +       struct btd_service *service;
> > > > > > +       struct policy_data *data;
> > > > > > +       bool do_reconnect = false;
> > > > > >
> > > > > >         DBG("reason %u", reason);
> > > > > >
> > > > > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > > > > +       switch(reason) {
> > > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > > > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > > > > +               do_reconnect = true;
> > > > > > +               break;
> > > > > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > > > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > > > > +               if (service && (data = policy_get_data(dev))) {
> > > > > > +                       data->reconnect_on_resume = true;
> > > > > > +               }
> > > > >
> > > > > Can't we just program the timer directly here? Or would that wakeup
> > > > > the system, I would imagine all timers are disabled while suspended
> > > > > which means when the system resumes so does the timers which would
> > > > > then trigger the reconnection logic.
> > > >
> > > > This approach works if every resume is user initiated and keeps the
> > > > device awake long enough to reconnect to the audio device. On
> > > > ChromeOS, this isn't always the case. We have a feature called Dark
> > > > Resume that occurs when the system wakes to handle an event that
> > > > doesn't require user intervention and suspends again without ever
> > > > turning on the screen. See:
> > > > https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> > > > A primary user for this is a periodic wake-up that checks battery
> > > > percentage and shuts down the system if it's too low.
> > >
> > > Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
> > > Resumes should be handled by the kernel instead of each userspace
> > > component? I suppose there quite a few timers that may have side
> > > effects if dark resumes actually resume them.
> > >
> >
> > I'm not aware of the history around it but I found another page with
> > more context of use cases (like actually letting a user space
> > application run as would happen on a smartphone):
> > https://www.chromium.org/chromium-os/chromiumos-design-docs/lucid-sleep
> >
> > > > One of the tests I ran specifically for this is a suspend stress test
> > > > with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> > > > --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> > > > timer when disconnecting, the device would attempt a connection on one
> > > > of the wakeups and fail (since we suspended without completing the
> > > > connection).
> > >
> > > We could perhaps check if the kernel is capable of emitting
> > > suspend/resume events and if not just program the timer, or
> > > alternatively add another config option to indicate when the system
> > > supports such concept of Dark Resumes.
> > >
> >
> > How does the following sound:
> >
> > * On disconnect, I always queue the reconnect but with the resume
> > delay set instead of the interval[0] value. I also set the
> > resume_reconnect flag
> > * On every resume event (or dark resume event), I reset the timer and
> > add resume_delay seconds if resume_reconnect is set
> > * The first time the reconnect_timeout runs, I clear the resume_reconnect flag
> >
> > The delay will only be reapplied if the kernel is capable of emitting
> > suspend/resume events and/or dark resume events.
>
> That should work as well, does full resumes need to reset the timer
> though? Or we don't have any means to identify the resume type? Well
> either way I guess the difference would be quite small since the
> timeout is the same just when it is programmed (on suspend vs on
> resume).

The timer exists to avoid trying to connect at the same time wi-fi may
be trying to associate.

By the way, I had a chance to test all this today and setting the
reconnect timer on disconnect causes the connection to sometimes occur
before the resume event has a chance to run. Depending on the exact
timing, this will likely break the desired dark resume behavior.
I tried `suspend_stress_test --wake_min=1 --wake_max=1 -c 5` and had
the headset reconnect on the 3rd suspend (instead of only at the end)
when using a ResumeDelay of 2.

So I propose the following behavior instead:
* On disconnect, queue a reconnect if reason != SUSPEND. If reason ==
SUSPEND, set reconnect->on_resume = true
* If we support dark resume events (i.e. chromium), set the timer if
resume reason == full resume and reconnect->on_resume = true (this
will probably be a CHROMIUM patch that we carry in our repositories)
* If we support kernel events (and not dark resume), set the timer if
reconnect->on_resume = true (this will be the upstream version)

I will update the resume delay to 2 and test further with some older
Chromebooks. If this reduces the chance of success of BT reconnecting,
we can come back and update this later.

>
> > > > >
> > > > > > +               break;
> > > > > > +       /* All other cases do not result in reconnect */
> > > > > > +       default:
> > > > > > +               break;
> > > > > > +       }
> > > > > > +
> > > > > > +       if (!do_reconnect)
> > > > > >                 return;
> > > > > >
> > > > > >         reconnect = reconnect_find(dev);
> > > > > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > > >         reconnect_set_timer(reconnect);
> > > > > >  }
> > > > > >
> > > > > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > > > > +                                       const uint8_t addr_type)
> > > > > > +{
> > > > > > +       struct btd_device *dev;
> > > > > > +       GSList *l;
> > > > > > +
> > > > > > +       /* Check if any devices needed to be reconnected on resume */
> > > > > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > > > > +               struct policy_data *data = l->data;
> > > > > > +
> > > > > > +               if (data->reconnect_on_resume) {
> > > > > > +                       policy_set_sink_timer_internal(data,
> > > > > > +                                                      resume_reconnect_delay);
> > > > > > +               }
> > > > > > +       }
> > > > > > +}
> > > > > > +
> > > > > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > > > > >  {
> > > > > >         struct reconnect_data *reconnect;
> > > > > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > > > > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > > > > >                                                                         NULL);
> > > > > >
> > > > > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > > > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > > > > +
> > > > > > +       if (gerr) {
> > > > > > +               g_clear_error(&gerr);
> > > > > > +               resume_reconnect_delay = default_reconnect_delay;
> > > > > > +       }
> > > > > >  done:
> > > > > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > > > > >                 btd_add_disconnect_cb(disconnect_cb);
> > > > > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > > > > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > > > > >         }
> > > > > >
> > > > > > @@ -869,6 +924,7 @@ done:
> > > > > >  static void policy_exit(void)
> > > > > >  {
> > > > > >         btd_remove_disconnect_cb(disconnect_cb);
> > > > > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > > > > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > > > > >
> > > > > >         if (reconnect_uuids)
> > > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > > index 5e896a9f0..7526feb9e 100644
> > > > > > --- a/src/adapter.c
> > > > > > +++ b/src/adapter.c
> > > > > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > > > > >
> > > > > >  static GSList *disconnect_list = NULL;
> > > > > >  static GSList *conn_fail_list = NULL;
> > > > > > +static GSList *controller_resume_list = NULL;
> > > > > >
> > > > > >  struct link_key_info {
> > > > > >         bdaddr_t bdaddr;
> > > > > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > > > > >         eir_data_free(&eir_data);
> > > > > >  }
> > > > > >
> > > > > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > > > > +                                       const bdaddr_t *addr,
> > > > > > +                                       const uint8_t addr_type)
> > > > > > +{
> > > > > > +       GSList *l;
> > > > > > +
> > > > > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > > > > +               btd_controller_resume_cb resume_cb = l->data;
> > > > > > +               resume_cb(wake_reason, addr, addr_type);
> > > > > > +       }
> > > > > > +}
> > > > > > +
> > > > > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > > > > +                                      const void *param, void *user_data)
> > > > > > +{
> > > > > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > > > > +       struct btd_adapter *adapter = user_data;
> > > > > > +
> > > > > > +       if (length < sizeof(*ev)) {
> > > > > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > > > > +               return;
> > > > > > +       }
> > > > > > +
> > > > > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > > > > +
> > > > > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > > > > +                                ev->addr.type);
> > > > > > +}
> > > > > > +
> > > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > > > > +{
> > > > > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > > > > +}
> > > > > > +
> > > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > > > > +{
> > > > > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > > > > +}
> > > > > > +
> > > > > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > > > > >                                         const void *param, void *user_data)
> > > > > >  {
> > > > > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > > > > >                                                 user_passkey_notify_callback,
> > > > > >                                                 adapter, NULL);
> > > > > >
> > > > > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > > > > +                                               adapter->dev_id,
> > > > > > +                                               controller_resume_callback,
> > > > > > +                                               adapter, NULL);
> > > > > > +
> > > > > >         set_dev_class(adapter);
> > > > > >
> > > > > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > > > index f8ac20261..5527e4205 100644
> > > > > > --- a/src/adapter.h
> > > > > > +++ b/src/adapter.h
> > > > > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > > > > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > > > > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > > > > >
> > > > > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > > > > +                                       const bdaddr_t *addr,
> > > > > > +                                       const uint8_t addr_type);
> > > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> > > > >
> > > > > If we can program the timer just before we suspend this is not really
> > > > > necessary, but if you still thing that would be a good idea to notify
> > > > > Id put it in btd_adapter_driver with a callback for suspend and
> > > > > resume, that way we don't have to maintain multiple lists for each of
> > > > > the callbacks.
> > > >
> > > > The adapter driver seems like a reasonable place to put this so let me
> > > > port that over.
> > > >
> > > > >
> > > > > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > > > > >  struct btd_adapter *adapter_find_by_id(int id);
> > > > > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > > > > diff --git a/src/main.c b/src/main.c
> > > > > > index 2c083de67..a1c3e7e9f 100644
> > > > > > --- a/src/main.c
> > > > > > +++ b/src/main.c
> > > > > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > > > > >         "ReconnectAttempts",
> > > > > >         "ReconnectIntervals",
> > > > > >         "AutoEnable",
> > > > > > +       "ReconnectAudioDelay",
> > > > > >         NULL
> > > > > >  };
> > > > > >
> > > > > > diff --git a/src/main.conf b/src/main.conf
> > > > > > index f41203b96..8d74a2aec 100644
> > > > > > --- a/src/main.conf
> > > > > > +++ b/src/main.conf
> > > > > > @@ -198,3 +198,12 @@
> > > > > >  # This includes adapters present on start as well as adapters that are plugged
> > > > > >  # in later on. Defaults to 'false'.
> > > > > >  #AutoEnable=false
> > > > > > +
> > > > > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > > > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > > > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > > > > +# better for co-existence with Wi-Fi.
> > > > > > +# The value is in seconds.
> > > > > > +# Default: 5
> > > > > > +#ReconnectAudioDelay = 5
> > > > > > +
> > > > > > --
> > > > > > 2.28.0.297.g1956fa8f8d-goog
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Abhishek
>
>
>
> --
> Luiz Augusto von Dentz

Patch

diff --git a/plugins/policy.c b/plugins/policy.c
index de51e58b9..b07a997b9 100644
--- a/plugins/policy.c
+++ b/plugins/policy.c
@@ -75,6 +75,9 @@  static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
 static int *reconnect_intervals = NULL;
 static size_t reconnect_intervals_len = 0;
 
+static const int default_reconnect_delay = 5;
+static int resume_reconnect_delay = 5;
+
 static GSList *reconnects = NULL;
 
 static unsigned int service_id = 0;
@@ -93,6 +96,8 @@  struct policy_data {
 	uint8_t ct_retries;
 	guint tg_timer;
 	uint8_t tg_retries;
+
+	bool reconnect_on_resume;
 };
 
 static struct reconnect_data *reconnect_find(struct btd_device *dev)
@@ -214,6 +219,7 @@  static gboolean policy_connect_sink(gpointer user_data)
 
 	data->sink_timer = 0;
 	data->sink_retries++;
+	data->reconnect_on_resume = false;
 
 	service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
 	if (service != NULL)
@@ -222,16 +228,20 @@  static gboolean policy_connect_sink(gpointer user_data)
 	return FALSE;
 }
 
-static void policy_set_sink_timer(struct policy_data *data)
+static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
 {
 	if (data->sink_timer > 0)
 		g_source_remove(data->sink_timer);
 
-	data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
-							policy_connect_sink,
+	data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
 							data);
 }
 
+static void policy_set_sink_timer(struct policy_data *data)
+{
+	policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
+}
+
 static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
 						btd_service_state_t new_state)
 {
@@ -743,10 +753,30 @@  static void reconnect_set_timer(struct reconnect_data *reconnect)
 static void disconnect_cb(struct btd_device *dev, uint8_t reason)
 {
 	struct reconnect_data *reconnect;
+	struct btd_service *service;
+	struct policy_data *data;
+	bool do_reconnect = false;
 
 	DBG("reason %u", reason);
 
-	if (reason != MGMT_DEV_DISCONN_TIMEOUT)
+	switch(reason) {
+	case MGMT_DEV_DISCONN_LOCAL_HOST:
+	case MGMT_DEV_DISCONN_REMOTE:
+		do_reconnect = true;
+		break;
+	/* Disconnect due to suspend will queue reconnect on resume for a2dp */
+	case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
+		service = btd_device_get_service(dev, A2DP_SINK_UUID);
+		if (service && (data = policy_get_data(dev))) {
+			data->reconnect_on_resume = true;
+		}
+		break;
+	/* All other cases do not result in reconnect */
+	default:
+		break;
+	}
+
+	if (!do_reconnect)
 		return;
 
 	reconnect = reconnect_find(dev);
@@ -761,6 +791,23 @@  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
 	reconnect_set_timer(reconnect);
 }
 
+static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
+					const uint8_t addr_type)
+{
+	struct btd_device *dev;
+	GSList *l;
+
+	/* Check if any devices needed to be reconnected on resume */
+	for (l = devices; l; l = g_slist_next(l)) {
+		struct policy_data *data = l->data;
+
+		if (data->reconnect_on_resume) {
+			policy_set_sink_timer_internal(data,
+						       resume_reconnect_delay);
+		}
+	}
+}
+
 static void conn_fail_cb(struct btd_device *dev, uint8_t status)
 {
 	struct reconnect_data *reconnect;
@@ -854,9 +901,17 @@  static int policy_init(void)
 	auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
 									NULL);
 
+	resume_reconnect_delay = g_key_file_get_integer(
+			conf, "Policy", "ReconnectAudioDelay", &gerr);
+
+	if (gerr) {
+		g_clear_error(&gerr);
+		resume_reconnect_delay = default_reconnect_delay;
+	}
 done:
 	if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
 		btd_add_disconnect_cb(disconnect_cb);
+		btd_add_controller_resume_cb(controller_resume_cb);
 		btd_add_conn_fail_cb(conn_fail_cb);
 	}
 
@@ -869,6 +924,7 @@  done:
 static void policy_exit(void)
 {
 	btd_remove_disconnect_cb(disconnect_cb);
+	btd_remove_controller_resume_cb(controller_resume_cb);
 	btd_remove_conn_fail_cb(conn_fail_cb);
 
 	if (reconnect_uuids)
diff --git a/src/adapter.c b/src/adapter.c
index 5e896a9f0..7526feb9e 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -139,6 +139,7 @@  static GSList *adapter_drivers = NULL;
 
 static GSList *disconnect_list = NULL;
 static GSList *conn_fail_list = NULL;
+static GSList *controller_resume_list = NULL;
 
 struct link_key_info {
 	bdaddr_t bdaddr;
@@ -8766,6 +8767,45 @@  static void connected_callback(uint16_t index, uint16_t length,
 	eir_data_free(&eir_data);
 }
 
+static void controller_resume_notify(const uint8_t wake_reason,
+					const bdaddr_t *addr,
+					const uint8_t addr_type)
+{
+	GSList *l;
+
+	for (l = controller_resume_list; l; l = g_slist_next(l)) {
+		btd_controller_resume_cb resume_cb = l->data;
+		resume_cb(wake_reason, addr, addr_type);
+	}
+}
+
+static void controller_resume_callback(uint16_t index, uint16_t length,
+				       const void *param, void *user_data)
+{
+	const struct mgmt_ev_controller_resume *ev = param;
+	struct btd_adapter *adapter = user_data;
+
+	if (length < sizeof(*ev)) {
+		btd_error(adapter->dev_id, "Too small device resume event");
+		return;
+	}
+
+	DBG("Controller resume with wake event 0x%x", ev->wake_reason);
+
+	controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
+				 ev->addr.type);
+}
+
+void btd_add_controller_resume_cb(btd_controller_resume_cb func)
+{
+	controller_resume_list = g_slist_append(controller_resume_list, func);
+}
+
+void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
+{
+	controller_resume_list = g_slist_remove(controller_resume_list, func);
+}
+
 static void device_blocked_callback(uint16_t index, uint16_t length,
 					const void *param, void *user_data)
 {
@@ -9389,6 +9429,11 @@  static void read_info_complete(uint8_t status, uint16_t length,
 						user_passkey_notify_callback,
 						adapter, NULL);
 
+	mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
+						adapter->dev_id,
+						controller_resume_callback,
+						adapter, NULL);
+
 	set_dev_class(adapter);
 
 	set_name(adapter, btd_adapter_get_name(adapter));
diff --git a/src/adapter.h b/src/adapter.h
index f8ac20261..5527e4205 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -67,6 +67,12 @@  typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
 void btd_add_conn_fail_cb(btd_conn_fail_cb func);
 void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
 
+typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
+					const bdaddr_t *addr,
+					const uint8_t addr_type);
+void btd_add_controller_resume_cb(btd_controller_resume_cb func);
+void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
+
 struct btd_adapter *adapter_find(const bdaddr_t *sba);
 struct btd_adapter *adapter_find_by_id(int id);
 void adapter_foreach(adapter_cb func, gpointer user_data);
diff --git a/src/main.c b/src/main.c
index 2c083de67..a1c3e7e9f 100644
--- a/src/main.c
+++ b/src/main.c
@@ -131,6 +131,7 @@  static const char *policy_options[] = {
 	"ReconnectAttempts",
 	"ReconnectIntervals",
 	"AutoEnable",
+	"ReconnectAudioDelay",
 	NULL
 };
 
diff --git a/src/main.conf b/src/main.conf
index f41203b96..8d74a2aec 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -198,3 +198,12 @@ 
 # This includes adapters present on start as well as adapters that are plugged
 # in later on. Defaults to 'false'.
 #AutoEnable=false
+
+# Audio devices that were disconnected due to suspend will be reconnected on
+# resume. ReconnectAudioDelay determines the delay between when the controller
+# resumes from suspend and a connection attempt is made. A longer delay can be
+# better for co-existence with Wi-Fi.
+# The value is in seconds.
+# Default: 5
+#ReconnectAudioDelay = 5
+