Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200117132754.BlueZ.1.I56a225147d9cabef55f94b57129a5e37ea79d165@changeid>
In-Reply-To: <20200117132754.BlueZ.1.I56a225147d9cabef55f94b57129a5e37ea79d165@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 17 Jan 2020 14:06:03 -0800
Message-ID: <CABBYNZK-LJRpH+NFGySdRQQjxBvsqWVBiV3wWguDnx61g_s8HQ@mail.gmail.com>
Subject: Re: [BlueZ PATCH 1/2] adapter: Add set wake capable operation
To:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Alain Michaud <alainm@chromium.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        chromeos-bluetooth-upstreaming@chromium.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Abhishek,

On Fri, Jan 17, 2020 at 1:28 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> If a device is capable of waking the host system from suspend, it should
> be marked as wake capable. We introduce a new management operation here
> to set this property and implement the API needed to call it.
>
> ---
>
>  doc/mgmt-api.txt | 19 +++++++++++++++
>  lib/mgmt.h       |  8 +++++++
>  src/adapter.c    | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/adapter.h    |  2 ++
>  src/device.c     | 15 ++++++++++++
>  src/device.h     |  2 ++
>  6 files changed, 108 insertions(+)
>
> diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> index 1e59acc54..8a73a9bb9 100644
> --- a/doc/mgmt-api.txt
> +++ b/doc/mgmt-api.txt
> @@ -3047,6 +3047,25 @@ Load Blocked Keys Command
>         Possible errors:        Invalid Parameters
>                                 Invalid Index
>
> +Set Wake Capable Command
> +===========================
> +
> +       Command Code:           0x0047
> +       Controller Index:       <controller id>
> +       Command Parameters:     Address (6 Octets)
> +                               Address_Type (1 Octet)
> +                               Wake Capable (1 Octet)
> +       Return Parameters:      Address (6 Octets)
> +                               Address_Type (1 Octet)
> +                               Wake Capable (1 Octet)
> +
> +       This command sets whether a bluetooth device is capable of waking the
> +       system from suspend. This property is used to set the event filter and
> +       LE whitelist when the system enters suspend.
> +
> +       Possible errors:        Failed
> +                               Invalid Parameters
> +                               Invalid Index

Please always have the documentation changes as a separated patch so
we can discuss these changes in a separate thread. Btw, Im missing the
D-Bus API changes in order for the application to control this
setting, we could probably consider doing this with auto-connect logic
but I assume this would need user input in order to use devices to
wake their systems, right? Anyway even if we were to do as a separate
setting if the device address is not programmed in the controller
whitelist I fail to see how it would be able to wake the system, so we
need the logic to work together with auto-connect.

For such features it probably make sense to have a cover letter for
the patch-set explaining the feature so we don't try guessing how it
could be used, also this sort of feature may have both security and
power impact, for instance if we don't have a controller capable of
resolving private addresses this may wake up the system for no reason,
in that case I would guess it is better to disable this feature for
devices that do use private addresses, also we may still have some
limitations either way because controllers have limited number of
slots in both the white list and resolving list.

Also, keep in mind the kernel has no persistent storage so we also
will need to store these settings in the storage so may make sense to
allow the command to set a list rather than just going one by one when
i.e. restarting the daemon, doing this via application is probably not
recommended as it may not be active when the daemon is starting.

>  Command Complete Event
>  ======================
> diff --git a/lib/mgmt.h b/lib/mgmt.h
> index 276445d0a..da2cce8c5 100644
> --- a/lib/mgmt.h
> +++ b/lib/mgmt.h
> @@ -597,6 +597,12 @@ struct mgmt_blocked_key_info {
>  struct mgmt_cp_set_blocked_keys {
>         uint16_t key_count;
>         struct mgmt_blocked_key_info keys[0];
> +
> +#define MGMT_OP_SET_WAKE_CAPABLE                       0x0047
> +#define MGMT_SET_WAKE_CAPABLE_SIZE                     8
> +struct mgmt_cp_set_wake_capable {
> +       struct mgmt_addr_info addr;
> +       uint8_t wake_capable;
>  } __packed;
>
>  #define MGMT_EV_CMD_COMPLETE           0x0001
> @@ -893,6 +899,8 @@ static const char *mgmt_op[] = {
>         "Set Appearance",
>         "Get PHY Configuration",
>         "Set PHY Configuration",
> +       "Set Blocked Keys",
> +       "Set Wake Capable",
>  };
>
>  static const char *mgmt_ev[] = {
> diff --git a/src/adapter.c b/src/adapter.c
> index 329c3ae0b..a35cf1349 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4685,6 +4685,68 @@ void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
>                                 add_whitelist_complete, adapter, NULL);
>  }
>
> +static void set_wake_capable_complete(uint8_t status, uint16_t length,
> +                                       const void *param, void *user_data)
> +{
> +       const struct mgmt_cp_set_wake_capable *rp = param;
> +       struct btd_adapter *adapter = user_data;
> +       struct btd_device *dev;
> +
> +       char addr[18];
> +
> +       if (length < sizeof(*rp)) {
> +               btd_error(adapter->dev_id,
> +                         "Too small Set Wake Capable complete event");
> +               return;
> +        }
> +
> +       ba2str(&rp->addr.bdaddr, addr);
> +
> +       dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr,
> +                                     rp->addr.type);
> +       if (!dev) {
> +               btd_error(adapter->dev_id,
> +                         "Set Wake Capable complete for unknown device %s",
> +                         addr);
> +               return;
> +       }
> +
> +       if (status != MGMT_STATUS_SUCCESS) {
> +               btd_error(adapter->dev_id,
> +                         "Failed to set wake capable %s(%u) = %d: %s (0x%02x)",
> +                         addr, rp->addr.type, rp->wake_capable,
> +                         mgmt_errstr(status), status);
> +               return;
> +       }
> +
> +       DBG("Set wake capable complete %s (%u)", addr, rp->addr.type);
> +}
> +
> +void adapter_set_wake_capable(struct btd_adapter* adapter,
> +                             struct btd_device* dev)
> +{
> +       struct mgmt_cp_set_wake_capable cp;
> +       char addr[18];
> +
> +       memset(&cp, 0, sizeof(cp));
> +       bacpy(&cp.addr.bdaddr, device_get_address(dev));
> +       cp.addr.type = btd_device_get_bdaddr_type(dev);
> +       cp.wake_capable = device_get_wake_capable(dev);
> +
> +       ba2strlc(&cp.addr.bdaddr, addr);
> +
> +       if (!mgmt_send(adapter->mgmt, MGMT_OP_SET_WAKE_CAPABLE, adapter->dev_id,
> +                      sizeof(cp), &cp, set_wake_capable_complete, adapter,
> +                      NULL)) {
> +               btd_warn(adapter->dev_id,
> +                        "Could not set wake capable = %u on %s (%u)",
> +                        cp.wake_capable, addr, cp.addr.type);
> +       }
> +
> +       DBG("Setting %s (%u) to wake capable = %u", addr,
> +           cp.addr.type, cp.wake_capable);
> +}
> +
>  static void remove_whitelist_complete(uint8_t status, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> diff --git a/src/adapter.h b/src/adapter.h
> index d0a5253bd..bb63db574 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -221,6 +221,8 @@ void adapter_whitelist_add(struct btd_adapter *adapter,
>                                                 struct btd_device *dev);
>  void adapter_whitelist_remove(struct btd_adapter *adapter,
>                                                 struct btd_device *dev);
> +void adapter_set_wake_capable(struct btd_adapter* adapter,
> +                             struct btd_device* dev);
>
>  void btd_adapter_set_oob_handler(struct btd_adapter *adapter,
>                                                 struct oob_handler *handler);
> diff --git a/src/device.c b/src/device.c
> index a4fe10980..221f23b09 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -189,6 +189,7 @@ struct btd_device {
>         bool            le;
>         bool            pending_paired;         /* "Paired" waiting for SDP */
>         bool            svc_refreshed;
> +       bool            wake_capable;           /* Can wake from suspend */
>         GSList          *svc_callbacks;
>         GSList          *eir_uuids;
>         struct bt_ad    *ad;
> @@ -1509,6 +1510,20 @@ void device_set_ltk_enc_size(struct btd_device *device, uint8_t enc_size)
>         bt_att_set_enc_key_size(device->att, device->ltk_enc_size);
>  }
>
> +bool device_get_wake_capable(struct btd_device *device)
> +{
> +       return device->wake_capable;
> +}
> +
> +void device_set_wake_capable(struct btd_device *device, bool wake_capable)
> +{
> +       if (device->wake_capable == wake_capable)
> +               return;
> +
> +       device->wake_capable = wake_capable;
> +       adapter_set_wake_capable(device->adapter, device);
> +}
> +
>  static void device_set_auto_connect(struct btd_device *device, gboolean enable)
>  {
>         char addr[18];
> diff --git a/src/device.h b/src/device.h
> index 06b100499..44df8e22e 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -139,6 +139,8 @@ void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
>                                                                 uint16_t value);
>  void device_load_svc_chng_ccc(struct btd_device *device, uint16_t *ccc_le,
>                                                         uint16_t *ccc_bredr);
> +bool device_get_wake_capable(struct btd_device *device);
> +void device_set_wake_capable(struct btd_device *device, bool wake_capable);
>
>  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
>                                         void *user_data);
> --
> 2.25.0.341.g760bfbb309-goog
>


-- 
Luiz Augusto von Dentz