Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210826085906.BlueZ.v1.1.Iae4b26a8036d47ca4d0db470f2bb23247f6cac7d@changeid>
In-Reply-To: <20210826085906.BlueZ.v1.1.Iae4b26a8036d47ca4d0db470f2bb23247f6cac7d@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 9 Sep 2021 16:08:44 -0700
Message-ID: <CABBYNZJsOLTk6iO1stc=6SBBdDms6xi-=j2s8hXLGVUnqqFm-w@mail.gmail.com>
Subject: Re: [BlueZ PATCH v1] adv_monitor: Clear any running DeviceLost timers
 on power down
To:     Manish Mandlik <mmandlik@google.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Miao-chen Chou <mcchou@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Manish,

On Thu, Aug 26, 2021 at 8:59 AM Manish Mandlik <mmandlik@google.com> wrote:
>
> This patch clears any running Adv Monitor DeviceLost timers on bt power
> down. It'll also invoke DeviceLost event if the device is already found
> and is being tracked for the DeviceLost event.
>
> Verified this by adding a monitor using bluetoothctl and confirming that
> the DeviceLost event is getting triggered for already found device in
> case of bt power down.
>
> Reviewed-by: mcchou@google.com
> Signed-off-by: Manish Mandlik <mmandlik@google.com>

We don't use signed-off-by on userspace.

> ---
>
>  src/adapter.c     |  8 ++++++++
>  src/adv_monitor.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/adv_monitor.h |  2 ++
>  3 files changed, 62 insertions(+)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 84bc5a1b0..3af7d1f1a 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -2872,6 +2872,13 @@ static void clear_discoverable(struct btd_adapter *adapter)
>         set_mode(adapter, MGMT_OP_SET_CONNECTABLE, 0x00);
>  }
>
> +static void adv_monitor_notify_power_down(struct btd_adapter *adapter)
> +{
> +       /* Notify Adv Monitor about power down */
> +       if (adapter->adv_monitor_manager)
> +               btd_adv_monitor_notify_power_down(adapter->adv_monitor_manager);
> +}

I guess we could just check for NULL inside btd_adv_monitor*, also I
would rename it to btd_adv_monitor_power_down.

>  static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>                                                 DBusMessageIter *value,
>                                                 GDBusPendingPropertySet id)
> @@ -2912,6 +2919,7 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>                 len = sizeof(mode);
>
>                 if (!mode) {
> +                       adv_monitor_notify_power_down(adapter);
>                         clear_discoverable(adapter);
>                         remove_temporary_devices(adapter);
>                 }
> diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> index 715ac5904..59f307ae9 100644
> --- a/src/adv_monitor.c
> +++ b/src/adv_monitor.c
> @@ -2011,3 +2011,55 @@ static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
>                                             NULL);
>         }
>  }
> +
> +/* Clears running DeviceLost timer for a given device */
> +static void clear_device_lost_timer(void *data, void *user_data)
> +{
> +       struct adv_monitor_device *dev = data;
> +       struct adv_monitor *monitor = NULL;
> +
> +       if (dev->lost_timer) {
> +               timeout_remove(dev->lost_timer);
> +               dev->lost_timer = 0;
> +
> +               monitor = dev->monitor;
> +
> +               DBG("Clearing device lost timer for device %p. "
> +                               "Calling DeviceLost() on Adv Monitor of "
> +                               "owner %s at path %s", dev->device,
> +                               monitor->app->owner, monitor->path);

The function name should already give the information about clearing
device lost timer so I'm not why you have to be so verbose about this
action.

> +               g_dbus_proxy_method_call(monitor->proxy, "DeviceLost",
> +                               report_device_state_setup,
> +                               NULL, dev->device, NULL);
> +       }
> +}
> +
> +/* Clears running DeviceLost timers from each monitor */
> +static void clear_lost_timers_from_monitor(void *data, void *user_data)
> +{
> +       struct adv_monitor *monitor = data;
> +
> +       queue_foreach(monitor->devices, clear_device_lost_timer, NULL);
> +}
> +
> +/* Clears running DeviceLost timers from each app */
> +static void clear_lost_timers_from_app(void *data, void *user_data)
> +{
> +       struct adv_monitor_app *app = data;
> +
> +       queue_foreach(app->monitors, clear_lost_timers_from_monitor, NULL);
> +}
> +
> +/* Handles bt power down scenario */
> +void btd_adv_monitor_notify_power_down(struct btd_adv_monitor_manager *manager)
> +{
> +       if (!manager) {
> +               error("Unexpected NULL btd_adv_monitor_manager object upon "
> +                               "power down");
> +               return;
> +       }

So you are doing the manager pointer check so it is even more a reason
to not have a wrapper around it.

> +       /* Clear any running DeviceLost timers in case of power down */
> +       queue_foreach(manager->apps, clear_lost_timers_from_app, NULL);
> +}
> diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> index 2b4f68abf..20da012d4 100644
> --- a/src/adv_monitor.h
> +++ b/src/adv_monitor.h
> @@ -38,4 +38,6 @@ void btd_adv_monitor_notify_monitors(struct btd_adv_monitor_manager *manager,
>  void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
>                                    struct btd_device *device);
>
> +void btd_adv_monitor_notify_power_down(struct btd_adv_monitor_manager *manager);
> +
>  #endif /* __ADV_MONITOR_H */
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>


-- 
Luiz Augusto von Dentz