[BlueZ PATCH v3 0/5] device: Allow devices to be marked as wake capable

[BlueZ PATCH v3 0/5] device: Allow devices to be marked as wake capable

[Abhishek Pandit-Subedi]

Hi bluez maintainers,

This change accompanies changes in the kernel to mark HID devices as
wake capable so they can wake the system from suspend. The
implementation depends on the Set Wake Capable management operation. It
is currently a separate management operation but it may be added as an
extension to an exiting operand like add_device (need some feedback
regarding this).

Per request on the last patch, I've moved docs/mgmt-api.txt into its own
patch so we can continue discussions on it.

This change was tested with appropriate kernel changes on v4.19
(verified that HID devices were being marked as wake capable in the
kernel).

Thanks
Abhishek

Changes in v3:
* Added profile_wake_support and made wake_capable dependent on it
* Added documentation for WakeCapable
* Mark HID device to support wake from suspend

Changes in v2:
* Separated docs/mgmt-api.txt into its own patch
* Added dbus api "WakeCapable" to set value
* Update device_set_wake_capable to be called by
  adapter_set_wake_capable_complete so we can emit property changed
* Newly added to show whether device is wake capable
* Removed automatically setting wake capable for HID devices

Abhishek Pandit-Subedi (5):
  mgmt: Add docs for Set Wake Capable
  device: Allow device to be marked as wake capable
  client: Display wake capable property with info
  doc/device-api: Add WakeCapable
  input: Make HID devices wake capable

 client/main.c           |   1 +
 doc/device-api.txt      |   5 ++
 doc/mgmt-api.txt        |  19 +++++++
 lib/mgmt.h              |   9 ++++
 profiles/input/device.c |   1 +
 profiles/input/hog.c    |   1 +
 src/adapter.c           |  65 ++++++++++++++++++++++
 src/adapter.h           |   2 +
 src/device.c            | 116 ++++++++++++++++++++++++++++++++++++++++
 src/device.h            |   5 ++
 10 files changed, 224 insertions(+)

-- 
2.25.0.341.g760bfbb309-goog

[BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Abhishek Pandit-Subedi]

Add docs for new management operation to mark a device as wake capable.

---

Changes in v3: None
Changes in v2:
* Separated docs/mgmt-api.txt into its own patch

 doc/mgmt-api.txt | 19 +++++++++++++++++++
 1 file changed, 19 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
 
 Command Complete Event
 ======================
-- 
2.25.0.341.g760bfbb309-goog

[BlueZ PATCH v3 2/5] device: Allow device to be marked as wake capable

[Abhishek Pandit-Subedi]

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. We also
add the dbus endpoint to allow the wake capable setting to be
controlled.

---

Changes in v3:
* Added profile_wake_support and made wake_capable dependent on it

Changes in v2:
* Added dbus api "WakeCapable" to set value
* Update device_set_wake_capable to be called by
  adapter_set_wake_capable_complete so we can emit property changed

 lib/mgmt.h    |   9 ++++
 src/adapter.c |  65 ++++++++++++++++++++++++++++
 src/adapter.h |   2 +
 src/device.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.h  |   5 +++
 5 files changed, 197 insertions(+)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index 276445d0a..cf19dd086 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -599,6 +599,13 @@ struct mgmt_cp_set_blocked_keys {
 	struct mgmt_blocked_key_info keys[0];
 } __packed;
 
+#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_enable;
+} __packed;
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	uint16_t opcode;
@@ -893,6 +900,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..294a9c9e4 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4685,6 +4685,71 @@ 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_enable,
+			  mgmt_errstr(status), status);
+		return;
+	}
+
+	device_set_wake_capable(dev, rp->wake_enable);
+
+	DBG("Set wake capable complete %s (%u)", addr, rp->addr.type);
+}
+
+void adapter_set_wake_capable(struct btd_adapter* adapter,
+			      struct btd_device* dev,
+			      bool wake_enable)
+{
+	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_enable = device_get_profile_wake_support(dev) && wake_enable;
+
+	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_enable, addr, cp.addr.type);
+	}
+
+	DBG("Setting %s (%u) to wake capable = %u", addr,
+	    cp.addr.type, cp.wake_enable);
+}
+
 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..e990279ed 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, bool wake_enable);
 
 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..6b152bb19 100644
--- a/src/device.c
+++ b/src/device.c
@@ -189,6 +189,8 @@ struct btd_device {
 	bool		le;
 	bool		pending_paired;		/* "Paired" waiting for SDP */
 	bool		svc_refreshed;
+	bool		profile_wake_support;	/* Profile supports wake */
+	bool		wake_capable;		/* Can wake from suspend */
 	GSList		*svc_callbacks;
 	GSList		*eir_uuids;
 	struct bt_ad	*ad;
@@ -415,6 +417,9 @@ static gboolean store_device_info_cb(gpointer user_data)
 	g_key_file_set_boolean(key_file, "General", "Blocked",
 							device->blocked);
 
+	g_key_file_set_boolean(key_file, "General", "WakeCapable",
+							device->wake_capable);
+
 	if (device->uuids) {
 		GSList *l;
 		int i;
@@ -1318,6 +1323,80 @@ dev_property_advertising_data_exist(const GDBusPropertyTable *property,
 	return bt_ad_has_data(device->ad, NULL);
 }
 
+static gboolean
+dev_property_get_wake_capable(const GDBusPropertyTable *property,
+			     DBusMessageIter *iter, void *data)
+{
+	struct btd_device *device = data;
+	dbus_bool_t wake_capable =
+			device_get_wake_capable(device) ? TRUE : FALSE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_capable);
+
+	return TRUE;
+}
+
+static void dev_property_set_wake_capable(const GDBusPropertyTable *property,
+					 DBusMessageIter *value,
+					 GDBusPendingPropertySet id, void *data)
+{
+	struct btd_device *device = data;
+	dbus_bool_t b;
+
+	if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN) {
+		g_dbus_pending_property_error(id,
+					ERROR_INTERFACE ".InvalidArguments",
+					"Invalid arguments in method call");
+		return;
+	}
+
+	dbus_message_iter_get_basic(value, &b);
+
+	adapter_set_wake_capable(device->adapter, device, b == TRUE);
+	g_dbus_pending_property_success(id);
+}
+
+static gboolean
+dev_property_get_wake_capable(const GDBusPropertyTable *property,
+			     DBusMessageIter *iter, void *data)
+{
+	struct btd_device *device = data;
+	dbus_bool_t wake_capable =
+			device_get_wake_capable(device) ? TRUE : FALSE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_capable);
+
+	return TRUE;
+}
+
+static void dev_property_set_wake_capable(const GDBusPropertyTable *property,
+					 DBusMessageIter *value,
+					 GDBusPendingPropertySet id, void *data)
+{
+	struct btd_device *device = data;
+	dbus_bool_t b;
+
+	if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN) {
+		g_dbus_pending_property_error(id,
+					ERROR_INTERFACE ".InvalidArguments",
+					"Invalid arguments in method call");
+		return;
+	}
+
+	dbus_message_iter_get_basic(value, &b);
+
+	adapter_set_wake_capable(device->adapter, device, b == TRUE);
+	g_dbus_pending_property_success(id);
+}
+
+static gboolean dev_property_wake_capable_exist(
+		const GDBusPropertyTable *property, void *data)
+{
+	struct btd_device *device = data;
+
+	return device_get_profile_wake_support(device) ? TRUE : FALSE;
+}
+
 static gboolean disconnect_all(gpointer user_data)
 {
 	struct btd_device *device = user_data;
@@ -1509,6 +1588,34 @@ 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_profile_wake_support(struct btd_device *device)
+{
+	return device->profile_wake_support;
+}
+
+void device_set_profile_wake_support(struct btd_device *device,
+				     bool wake_support)
+{
+	device->profile_wake_support = wake_support;
+	/* WakeCapable is only visible when wake_support is true */
+	g_dbus_emit_property_changed(dbus_conn, device->path, DEVICE_INTERFACE,
+				     "WakeCapable");
+}
+
+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)
+{
+	device->wake_capable = wake_capable;
+
+	store_device_info(device);
+	g_dbus_emit_property_changed(dbus_conn, device->path, DEVICE_INTERFACE,
+				     "WakeCapable");
+}
+
 static void device_set_auto_connect(struct btd_device *device, gboolean enable)
 {
 	char addr[18];
@@ -2779,6 +2886,9 @@ static const GDBusPropertyTable device_properties[] = {
 	{ "AdvertisingData", "a{yv}", dev_property_get_advertising_data,
 				NULL, dev_property_advertising_data_exist,
 				G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
+	{ "WakeCapable", "b", dev_property_get_wake_capable,
+				dev_property_set_wake_capable,
+				dev_property_wake_capable_exist },
 	{ }
 };
 
@@ -3030,6 +3140,7 @@ static void load_info(struct btd_device *device, const char *local,
 	char *str;
 	gboolean store_needed = FALSE;
 	gboolean blocked;
+	gboolean wake_capable;
 	char **uuids;
 	int source, vendor, product, version;
 	char **techno, **t;
@@ -3141,6 +3252,11 @@ next:
 		btd_device_set_pnpid(device, source, vendor, product, version);
 	}
 
+	/* Mark wake capable */
+	wake_capable = g_key_file_get_boolean(key_file, "General",
+					      "WakeCapable", NULL);
+	adapter_set_wake_capable(device->adapter, device, wake_capable == TRUE);
+
 	if (store_needed)
 		store_device_info(device);
 }
diff --git a/src/device.h b/src/device.h
index 06b100499..43f633e38 100644
--- a/src/device.h
+++ b/src/device.h
@@ -139,6 +139,11 @@ 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_profile_wake_support(struct btd_device *device);
+void device_set_profile_wake_support(struct btd_device *device,
+				     bool wake_support);
+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

[BlueZ PATCH v3 3/5] client: Display wake capable property with info

[Abhishek Pandit-Subedi]

Display whether the device is configured as wake capable when queried
with cmd_info.

---

Changes in v3: None
Changes in v2:
* Newly added to show whether device is wake capable
* Removed automatically setting wake capable for HID devices

 client/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/client/main.c b/client/main.c
index 8bd0bac9e..5c53fe08d 100644
--- a/client/main.c
+++ b/client/main.c
@@ -1609,6 +1609,7 @@ static void cmd_info(int argc, char *argv[])
 	print_property(proxy, "Trusted");
 	print_property(proxy, "Blocked");
 	print_property(proxy, "Connected");
+	print_property(proxy, "WakeCapable");
 	print_property(proxy, "LegacyPairing");
 	print_uuids(proxy);
 	print_property(proxy, "Modalias");
-- 
2.25.0.341.g760bfbb309-goog

[BlueZ PATCH v3 4/5] doc/device-api: Add WakeCapable

[Abhishek Pandit-Subedi]

Add documentation for WakeCapable, which allows a device to wake the
system from suspend.

---

Changes in v3:
* Added documentation for WakeCapable

Changes in v2: None

 doc/device-api.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/device-api.txt b/doc/device-api.txt
index 65d8fee37..492a7f8c7 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -189,6 +189,11 @@ Properties	string Address [readonly]
 			drivers will also be removed and no new ones will
 			be probed as long as the device is blocked.
 
+		boolean WakeCapable [readwrite]
+
+			If set to true this device will be allowed to wake the
+			host processor from system suspend.
+
 		string Alias [readwrite]
 
 			The name alias for the remote device. The alias can
-- 
2.25.0.341.g760bfbb309-goog

[BlueZ PATCH v3 5/5] input: Make HID devices wake capable

[Abhishek Pandit-Subedi]

HID devices can wake the host from a suspended state. Mark the profiles
to support wake when they are accepted.

---

Changes in v3:
* Mark HID device to support wake from suspend

Changes in v2: None

 profiles/input/device.c | 1 +
 profiles/input/hog.c    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 2cb3811c8..e5865d098 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1381,6 +1381,7 @@ int input_device_register(struct btd_service *service)
 	}
 
 	btd_service_set_user_data(service, idev);
+	device_set_profile_wake_support(device, true);
 
 	return 0;
 }
diff --git a/profiles/input/hog.c b/profiles/input/hog.c
index 83c017dcb..227facf97 100644
--- a/profiles/input/hog.c
+++ b/profiles/input/hog.c
@@ -159,6 +159,7 @@ static int hog_probe(struct btd_service *service)
 		return -EINVAL;
 
 	btd_service_set_user_data(service, dev);
+	device_set_profile_wake_support(device, true);
 	return 0;
 }
 
-- 
2.25.0.341.g760bfbb309-goog

Re: [BlueZ PATCH v3 2/5] device: Allow device to be marked as wake capable

[Abhishek Pandit-Subedi]

Please ignore the first version of this -- it has a duplicated
function entry (I made a mistake while doing merges between branches
and must have forgotten to compile check right before sending).

Will resend just this email with the fix.

On Mon, Jan 27, 2020 at 6:05 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. We also
> add the dbus endpoint to allow the wake capable setting to be
> controlled.
>
> ---
>
> Changes in v3:
> * Added profile_wake_support and made wake_capable dependent on it
>
> Changes in v2:
> * Added dbus api "WakeCapable" to set value
> * Update device_set_wake_capable to be called by
>   adapter_set_wake_capable_complete so we can emit property changed
>
>  lib/mgmt.h    |   9 ++++
>  src/adapter.c |  65 ++++++++++++++++++++++++++++
>  src/adapter.h |   2 +
>  src/device.c  | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/device.h  |   5 +++
>  5 files changed, 197 insertions(+)
>
> diff --git a/lib/mgmt.h b/lib/mgmt.h
> index 276445d0a..cf19dd086 100644
> --- a/lib/mgmt.h
> +++ b/lib/mgmt.h
> @@ -599,6 +599,13 @@ struct mgmt_cp_set_blocked_keys {
>         struct mgmt_blocked_key_info keys[0];
>  } __packed;
>
> +#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_enable;
> +} __packed;
> +
>  #define MGMT_EV_CMD_COMPLETE           0x0001
>  struct mgmt_ev_cmd_complete {
>         uint16_t opcode;
> @@ -893,6 +900,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..294a9c9e4 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4685,6 +4685,71 @@ 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_enable,
> +                         mgmt_errstr(status), status);
> +               return;
> +       }
> +
> +       device_set_wake_capable(dev, rp->wake_enable);
> +
> +       DBG("Set wake capable complete %s (%u)", addr, rp->addr.type);
> +}
> +
> +void adapter_set_wake_capable(struct btd_adapter* adapter,
> +                             struct btd_device* dev,
> +                             bool wake_enable)
> +{
> +       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_enable = device_get_profile_wake_support(dev) && wake_enable;
> +
> +       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_enable, addr, cp.addr.type);
> +       }
> +
> +       DBG("Setting %s (%u) to wake capable = %u", addr,
> +           cp.addr.type, cp.wake_enable);
> +}
> +
>  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..e990279ed 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, bool wake_enable);
>
>  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..6b152bb19 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -189,6 +189,8 @@ struct btd_device {
>         bool            le;
>         bool            pending_paired;         /* "Paired" waiting for SDP */
>         bool            svc_refreshed;
> +       bool            profile_wake_support;   /* Profile supports wake */
> +       bool            wake_capable;           /* Can wake from suspend */
>         GSList          *svc_callbacks;
>         GSList          *eir_uuids;
>         struct bt_ad    *ad;
> @@ -415,6 +417,9 @@ static gboolean store_device_info_cb(gpointer user_data)
>         g_key_file_set_boolean(key_file, "General", "Blocked",
>                                                         device->blocked);
>
> +       g_key_file_set_boolean(key_file, "General", "WakeCapable",
> +                                                       device->wake_capable);
> +
>         if (device->uuids) {
>                 GSList *l;
>                 int i;
> @@ -1318,6 +1323,80 @@ dev_property_advertising_data_exist(const GDBusPropertyTable *property,
>         return bt_ad_has_data(device->ad, NULL);
>  }
>
> +static gboolean
> +dev_property_get_wake_capable(const GDBusPropertyTable *property,
> +                            DBusMessageIter *iter, void *data)
> +{
> +       struct btd_device *device = data;
> +       dbus_bool_t wake_capable =
> +                       device_get_wake_capable(device) ? TRUE : FALSE;
> +
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_capable);
> +
> +       return TRUE;
> +}
> +
> +static void dev_property_set_wake_capable(const GDBusPropertyTable *property,
> +                                        DBusMessageIter *value,
> +                                        GDBusPendingPropertySet id, void *data)
> +{
> +       struct btd_device *device = data;
> +       dbus_bool_t b;
> +
> +       if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN) {
> +               g_dbus_pending_property_error(id,
> +                                       ERROR_INTERFACE ".InvalidArguments",
> +                                       "Invalid arguments in method call");
> +               return;
> +       }
> +
> +       dbus_message_iter_get_basic(value, &b);
> +
> +       adapter_set_wake_capable(device->adapter, device, b == TRUE);
> +       g_dbus_pending_property_success(id);
> +}
> +
> +static gboolean
> +dev_property_get_wake_capable(const GDBusPropertyTable *property,
> +                            DBusMessageIter *iter, void *data)
> +{
> +       struct btd_device *device = data;
> +       dbus_bool_t wake_capable =
> +                       device_get_wake_capable(device) ? TRUE : FALSE;
> +
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_capable);
> +
> +       return TRUE;
> +}
> +
> +static void dev_property_set_wake_capable(const GDBusPropertyTable *property,
> +                                        DBusMessageIter *value,
> +                                        GDBusPendingPropertySet id, void *data)
> +{
> +       struct btd_device *device = data;
> +       dbus_bool_t b;
> +
> +       if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN) {
> +               g_dbus_pending_property_error(id,
> +                                       ERROR_INTERFACE ".InvalidArguments",
> +                                       "Invalid arguments in method call");
> +               return;
> +       }
> +
> +       dbus_message_iter_get_basic(value, &b);
> +
> +       adapter_set_wake_capable(device->adapter, device, b == TRUE);
> +       g_dbus_pending_property_success(id);
> +}
> +
> +static gboolean dev_property_wake_capable_exist(
> +               const GDBusPropertyTable *property, void *data)
> +{
> +       struct btd_device *device = data;
> +
> +       return device_get_profile_wake_support(device) ? TRUE : FALSE;
> +}
> +
>  static gboolean disconnect_all(gpointer user_data)
>  {
>         struct btd_device *device = user_data;
> @@ -1509,6 +1588,34 @@ 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_profile_wake_support(struct btd_device *device)
> +{
> +       return device->profile_wake_support;
> +}
> +
> +void device_set_profile_wake_support(struct btd_device *device,
> +                                    bool wake_support)
> +{
> +       device->profile_wake_support = wake_support;
> +       /* WakeCapable is only visible when wake_support is true */
> +       g_dbus_emit_property_changed(dbus_conn, device->path, DEVICE_INTERFACE,
> +                                    "WakeCapable");
> +}
> +
> +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)
> +{
> +       device->wake_capable = wake_capable;
> +
> +       store_device_info(device);
> +       g_dbus_emit_property_changed(dbus_conn, device->path, DEVICE_INTERFACE,
> +                                    "WakeCapable");
> +}
> +
>  static void device_set_auto_connect(struct btd_device *device, gboolean enable)
>  {
>         char addr[18];
> @@ -2779,6 +2886,9 @@ static const GDBusPropertyTable device_properties[] = {
>         { "AdvertisingData", "a{yv}", dev_property_get_advertising_data,
>                                 NULL, dev_property_advertising_data_exist,
>                                 G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
> +       { "WakeCapable", "b", dev_property_get_wake_capable,
> +                               dev_property_set_wake_capable,
> +                               dev_property_wake_capable_exist },
>         { }
>  };
>
> @@ -3030,6 +3140,7 @@ static void load_info(struct btd_device *device, const char *local,
>         char *str;
>         gboolean store_needed = FALSE;
>         gboolean blocked;
> +       gboolean wake_capable;
>         char **uuids;
>         int source, vendor, product, version;
>         char **techno, **t;
> @@ -3141,6 +3252,11 @@ next:
>                 btd_device_set_pnpid(device, source, vendor, product, version);
>         }
>
> +       /* Mark wake capable */
> +       wake_capable = g_key_file_get_boolean(key_file, "General",
> +                                             "WakeCapable", NULL);
> +       adapter_set_wake_capable(device->adapter, device, wake_capable == TRUE);
> +
>         if (store_needed)
>                 store_device_info(device);
>  }
> diff --git a/src/device.h b/src/device.h
> index 06b100499..43f633e38 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -139,6 +139,11 @@ 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_profile_wake_support(struct btd_device *device);
> +void device_set_profile_wake_support(struct btd_device *device,
> +                                    bool wake_support);
> +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
>

Re: [BlueZ PATCH v3 0/5] device: Allow devices to be marked as wake capable

[Luiz Augusto von Dentz]

Hi Abhishek,

On Mon, Jan 27, 2020 at 6:05 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
>
> Hi bluez maintainers,
>
> This change accompanies changes in the kernel to mark HID devices as
> wake capable so they can wake the system from suspend. The
> implementation depends on the Set Wake Capable management operation. It
> is currently a separate management operation but it may be added as an
> extension to an exiting operand like add_device (need some feedback
> regarding this).
>
> Per request on the last patch, I've moved docs/mgmt-api.txt into its own
> patch so we can continue discussions on it.
>
> This change was tested with appropriate kernel changes on v4.19
> (verified that HID devices were being marked as wake capable in the
> kernel).
>
> Thanks
> Abhishek
>
> Changes in v3:
> * Added profile_wake_support and made wake_capable dependent on it
> * Added documentation for WakeCapable
> * Mark HID device to support wake from suspend
>
> Changes in v2:
> * Separated docs/mgmt-api.txt into its own patch
> * Added dbus api "WakeCapable" to set value
> * Update device_set_wake_capable to be called by
>   adapter_set_wake_capable_complete so we can emit property changed
> * Newly added to show whether device is wake capable
> * Removed automatically setting wake capable for HID devices
>
> Abhishek Pandit-Subedi (5):
>   mgmt: Add docs for Set Wake Capable
>   device: Allow device to be marked as wake capable
>   client: Display wake capable property with info
>   doc/device-api: Add WakeCapable
>   input: Make HID devices wake capable
>
>  client/main.c           |   1 +
>  doc/device-api.txt      |   5 ++
>  doc/mgmt-api.txt        |  19 +++++++
>  lib/mgmt.h              |   9 ++++
>  profiles/input/device.c |   1 +
>  profiles/input/hog.c    |   1 +
>  src/adapter.c           |  65 ++++++++++++++++++++++
>  src/adapter.h           |   2 +
>  src/device.c            | 116 ++++++++++++++++++++++++++++++++++++++++
>  src/device.h            |   5 ++
>  10 files changed, 224 insertions(+)
>
> --
> 2.25.0.341.g760bfbb309-goog

Other than the small comments I had this set is quite good, does the
kernel support has already been merged?

-- 
Luiz Augusto von Dentz

Re: [BlueZ PATCH v3 0/5] device: Allow devices to be marked as wake capable

[Abhishek Pandit-Subedi]

Hi Luiz,

It has not yet been merged in the kernel so please avoid merging for
the time being.

We also need to discuss what the management API for setting the wake
capable needs to be called as well (I think we want to create a
generic Set Device Params sort of management api).

Abhishek

On Tue, Jan 28, 2020 at 12:31 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Mon, Jan 27, 2020 at 6:05 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> >
> > Hi bluez maintainers,
> >
> > This change accompanies changes in the kernel to mark HID devices as
> > wake capable so they can wake the system from suspend. The
> > implementation depends on the Set Wake Capable management operation. It
> > is currently a separate management operation but it may be added as an
> > extension to an exiting operand like add_device (need some feedback
> > regarding this).
> >
> > Per request on the last patch, I've moved docs/mgmt-api.txt into its own
> > patch so we can continue discussions on it.
> >
> > This change was tested with appropriate kernel changes on v4.19
> > (verified that HID devices were being marked as wake capable in the
> > kernel).
> >
> > Thanks
> > Abhishek
> >
> > Changes in v3:
> > * Added profile_wake_support and made wake_capable dependent on it
> > * Added documentation for WakeCapable
> > * Mark HID device to support wake from suspend
> >
> > Changes in v2:
> > * Separated docs/mgmt-api.txt into its own patch
> > * Added dbus api "WakeCapable" to set value
> > * Update device_set_wake_capable to be called by
> >   adapter_set_wake_capable_complete so we can emit property changed
> > * Newly added to show whether device is wake capable
> > * Removed automatically setting wake capable for HID devices
> >
> > Abhishek Pandit-Subedi (5):
> >   mgmt: Add docs for Set Wake Capable
> >   device: Allow device to be marked as wake capable
> >   client: Display wake capable property with info
> >   doc/device-api: Add WakeCapable
> >   input: Make HID devices wake capable
> >
> >  client/main.c           |   1 +
> >  doc/device-api.txt      |   5 ++
> >  doc/mgmt-api.txt        |  19 +++++++
> >  lib/mgmt.h              |   9 ++++
> >  profiles/input/device.c |   1 +
> >  profiles/input/hog.c    |   1 +
> >  src/adapter.c           |  65 ++++++++++++++++++++++
> >  src/adapter.h           |   2 +
> >  src/device.c            | 116 ++++++++++++++++++++++++++++++++++++++++
> >  src/device.h            |   5 ++
> >  10 files changed, 224 insertions(+)
> >
> > --
> > 2.25.0.341.g760bfbb309-goog
>
> Other than the small comments I had this set is quite good, does the
> kernel support has already been merged?
>
> --
> Luiz Augusto von Dentz

Re: [BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Marcel Holtmann]

Hi Abhishek,

> Add docs for new management operation to mark a device as wake capable.
> 
> ---
> 
> Changes in v3: None
> Changes in v2:
> * Separated docs/mgmt-api.txt into its own patch
> 
> doc/mgmt-api.txt | 19 +++++++++++++++++++
> 1 file changed, 19 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
> 

my current thinking goes into this API addition:

--- a/doc/mgmt-api.txt
+++ b/doc/mgmt-api.txt
@@ -2003,6 +2003,7 @@ Add Device Command
                0       Background scan for device
                1       Allow incoming connection
                2       Auto-connect remote device
+               3       Allow incoming connection to wake up the system
 
        With the Action 0, when the device is found, a new Device Found
        event will be sent indicating this device is available. This
@@ -2018,6 +2019,9 @@ Add Device Command
        and if successful a Device Connected event will be sent. This
        action is only valid for LE Public and LE Random address types.
 
+       With the Action 3, the device is allowed to connect the same way
+       as with Action 1, but allows to wake up the system from suspend.
+
        When a device is blocked using Block Device command, then it is
        valid to add the device here, but all actions will be ignored
        until the device is unblocked.

Since we are really talking about incoming connections, then the Add Device command is the one that handles this. Giving a device the option to wake us up that is not set up for incoming connections makes no sense. We can discuss if certain LE advertising packets should wake us up, but that is a total different API in my book.

Regards

Marcel

Re: [BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Abhishek Pandit-Subedi]

Hi Marcel,

On Tue, Jan 28, 2020 at 8:33 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > Add docs for new management operation to mark a device as wake capable.
> >
> > ---
> >
> > Changes in v3: None
> > Changes in v2:
> > * Separated docs/mgmt-api.txt into its own patch
> >
> > doc/mgmt-api.txt | 19 +++++++++++++++++++
> > 1 file changed, 19 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
> >
>
> my current thinking goes into this API addition:
>
> --- a/doc/mgmt-api.txt
> +++ b/doc/mgmt-api.txt
> @@ -2003,6 +2003,7 @@ Add Device Command
>                 0       Background scan for device
>                 1       Allow incoming connection
>                 2       Auto-connect remote device
> +               3       Allow incoming connection to wake up the system
>
>         With the Action 0, when the device is found, a new Device Found
>         event will be sent indicating this device is available. This
> @@ -2018,6 +2019,9 @@ Add Device Command
>         and if successful a Device Connected event will be sent. This
>         action is only valid for LE Public and LE Random address types.
>
> +       With the Action 3, the device is allowed to connect the same way
> +       as with Action 1, but allows to wake up the system from suspend.
> +
>         When a device is blocked using Block Device command, then it is
>         valid to add the device here, but all actions will be ignored
>         until the device is unblocked.
>
> Since we are really talking about incoming connections, then the Add Device command is the one that handles this. Giving a device the option to wake us up that is not set up for incoming connections makes no sense. We can discuss if certain LE advertising packets should wake us up, but that is a total different API in my book.

I originally tried implementing this with the Add Device api as you
suggested in the RFC email back in November (when we first talked
about this). I had trouble figuring out when/where in bluez to
actually send the Add Device command.

Whether a device supports wake-up is a profile level setting (i.e. HID
only so far). As far as I can tell, Add Device is called before the
profile connection is established. Bluez has two apis that call
MGMT_OP_ADD_DEVICE:
* adapter_auto_connect_add (this seems to be LE)
* adapter_whitelist_add (this seems to be BR/EDR)

Both seem to be called BEFORE the registered profiles have a chance to
accept the connection.

>
> Regards
>
> Marcel
>

Re: [BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Marcel Holtmann]

Hi Abhishek,

>>> Add docs for new management operation to mark a device as wake capable.
>>> 
>>> ---
>>> 
>>> Changes in v3: None
>>> Changes in v2:
>>> * Separated docs/mgmt-api.txt into its own patch
>>> 
>>> doc/mgmt-api.txt | 19 +++++++++++++++++++
>>> 1 file changed, 19 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
>>> 
>> 
>> my current thinking goes into this API addition:
>> 
>> --- a/doc/mgmt-api.txt
>> +++ b/doc/mgmt-api.txt
>> @@ -2003,6 +2003,7 @@ Add Device Command
>>                0       Background scan for device
>>                1       Allow incoming connection
>>                2       Auto-connect remote device
>> +               3       Allow incoming connection to wake up the system
>> 
>>        With the Action 0, when the device is found, a new Device Found
>>        event will be sent indicating this device is available. This
>> @@ -2018,6 +2019,9 @@ Add Device Command
>>        and if successful a Device Connected event will be sent. This
>>        action is only valid for LE Public and LE Random address types.
>> 
>> +       With the Action 3, the device is allowed to connect the same way
>> +       as with Action 1, but allows to wake up the system from suspend.
>> +
>>        When a device is blocked using Block Device command, then it is
>>        valid to add the device here, but all actions will be ignored
>>        until the device is unblocked.
>> 
>> Since we are really talking about incoming connections, then the Add Device command is the one that handles this. Giving a device the option to wake us up that is not set up for incoming connections makes no sense. We can discuss if certain LE advertising packets should wake us up, but that is a total different API in my book.
> 
> I originally tried implementing this with the Add Device api as you
> suggested in the RFC email back in November (when we first talked
> about this). I had trouble figuring out when/where in bluez to
> actually send the Add Device command.
> 
> Whether a device supports wake-up is a profile level setting (i.e. HID
> only so far). As far as I can tell, Add Device is called before the
> profile connection is established. Bluez has two apis that call
> MGMT_OP_ADD_DEVICE:
> * adapter_auto_connect_add (this seems to be LE)
> * adapter_whitelist_add (this seems to be BR/EDR)
> 
> Both seem to be called BEFORE the registered profiles have a chance to
> accept the connection.

this is something for Luiz or Johan to comment on. Maybe our code is not as good as I was thinking in this regard. Or maybe this is actually legacy code that I am trying to rid of by requiring a mgmt API revision that has Add Device support.

In general once we bonded with a device, we should be able to assign certain properties to it that are kept persistently.

Regards

Marcel

Re: [BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Abhishek Pandit-Subedi]

On Wed, Jan 29, 2020 at 10:06 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> >>> Add docs for new management operation to mark a device as wake capable.
> >>>
> >>> ---
> >>>
> >>> Changes in v3: None
> >>> Changes in v2:
> >>> * Separated docs/mgmt-api.txt into its own patch
> >>>
> >>> doc/mgmt-api.txt | 19 +++++++++++++++++++
> >>> 1 file changed, 19 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
> >>>
> >>
> >> my current thinking goes into this API addition:
> >>
> >> --- a/doc/mgmt-api.txt
> >> +++ b/doc/mgmt-api.txt
> >> @@ -2003,6 +2003,7 @@ Add Device Command
> >>                0       Background scan for device
> >>                1       Allow incoming connection
> >>                2       Auto-connect remote device
> >> +               3       Allow incoming connection to wake up the system
> >>
> >>        With the Action 0, when the device is found, a new Device Found
> >>        event will be sent indicating this device is available. This
> >> @@ -2018,6 +2019,9 @@ Add Device Command
> >>        and if successful a Device Connected event will be sent. This
> >>        action is only valid for LE Public and LE Random address types.
> >>
> >> +       With the Action 3, the device is allowed to connect the same way
> >> +       as with Action 1, but allows to wake up the system from suspend.
> >> +
> >>        When a device is blocked using Block Device command, then it is
> >>        valid to add the device here, but all actions will be ignored
> >>        until the device is unblocked.
> >>
> >> Since we are really talking about incoming connections, then the Add Device command is the one that handles this. Giving a device the option to wake us up that is not set up for incoming connections makes no sense. We can discuss if certain LE advertising packets should wake us up, but that is a total different API in my book.
> >
> > I originally tried implementing this with the Add Device api as you
> > suggested in the RFC email back in November (when we first talked
> > about this). I had trouble figuring out when/where in bluez to
> > actually send the Add Device command.
> >
> > Whether a device supports wake-up is a profile level setting (i.e. HID
> > only so far). As far as I can tell, Add Device is called before the
> > profile connection is established. Bluez has two apis that call
> > MGMT_OP_ADD_DEVICE:
> > * adapter_auto_connect_add (this seems to be LE)
> > * adapter_whitelist_add (this seems to be BR/EDR)
> >
> > Both seem to be called BEFORE the registered profiles have a chance to
> > accept the connection.
>
> this is something for Luiz or Johan to comment on. Maybe our code is not as good as I was thinking in this regard. Or maybe this is actually legacy code that I am trying to rid of by requiring a mgmt API revision that has Add Device support.
>
> In general once we bonded with a device, we should be able to assign certain properties to it that are kept persistently.
>

It looks like add_device would work if I opted to use it as an
"update_device_params" type of function (I think I can use it in the
same location I currently use adapter_set_wake_capable; I would just
check that the device has already been added first).

You would still need to make a separate call from the original
add_device so your original criticism of requiring another mgmt call
for every param being set is still there. I could refactor the action
parameter to accept flags (i.e. 0x3 = Set connection parameters) and
then add a uint16_t flags parameter (i.e. 1 << 0: Allow wakeup from
incoming connection).

What do you think?

> Regards
>
> Marcel
>

Re: [BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Marcel Holtmann]

Hi Abhishek,

>>>>> Add docs for new management operation to mark a device as wake capable.
>>>>> 
>>>>> ---
>>>>> 
>>>>> Changes in v3: None
>>>>> Changes in v2:
>>>>> * Separated docs/mgmt-api.txt into its own patch
>>>>> 
>>>>> doc/mgmt-api.txt | 19 +++++++++++++++++++
>>>>> 1 file changed, 19 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
>>>>> 
>>>> 
>>>> my current thinking goes into this API addition:
>>>> 
>>>> --- a/doc/mgmt-api.txt
>>>> +++ b/doc/mgmt-api.txt
>>>> @@ -2003,6 +2003,7 @@ Add Device Command
>>>>               0       Background scan for device
>>>>               1       Allow incoming connection
>>>>               2       Auto-connect remote device
>>>> +               3       Allow incoming connection to wake up the system
>>>> 
>>>>       With the Action 0, when the device is found, a new Device Found
>>>>       event will be sent indicating this device is available. This
>>>> @@ -2018,6 +2019,9 @@ Add Device Command
>>>>       and if successful a Device Connected event will be sent. This
>>>>       action is only valid for LE Public and LE Random address types.
>>>> 
>>>> +       With the Action 3, the device is allowed to connect the same way
>>>> +       as with Action 1, but allows to wake up the system from suspend.
>>>> +
>>>>       When a device is blocked using Block Device command, then it is
>>>>       valid to add the device here, but all actions will be ignored
>>>>       until the device is unblocked.
>>>> 
>>>> Since we are really talking about incoming connections, then the Add Device command is the one that handles this. Giving a device the option to wake us up that is not set up for incoming connections makes no sense. We can discuss if certain LE advertising packets should wake us up, but that is a total different API in my book.
>>> 
>>> I originally tried implementing this with the Add Device api as you
>>> suggested in the RFC email back in November (when we first talked
>>> about this). I had trouble figuring out when/where in bluez to
>>> actually send the Add Device command.
>>> 
>>> Whether a device supports wake-up is a profile level setting (i.e. HID
>>> only so far). As far as I can tell, Add Device is called before the
>>> profile connection is established. Bluez has two apis that call
>>> MGMT_OP_ADD_DEVICE:
>>> * adapter_auto_connect_add (this seems to be LE)
>>> * adapter_whitelist_add (this seems to be BR/EDR)
>>> 
>>> Both seem to be called BEFORE the registered profiles have a chance to
>>> accept the connection.
>> 
>> this is something for Luiz or Johan to comment on. Maybe our code is not as good as I was thinking in this regard. Or maybe this is actually legacy code that I am trying to rid of by requiring a mgmt API revision that has Add Device support.
>> 
>> In general once we bonded with a device, we should be able to assign certain properties to it that are kept persistently.
>> 
> 
> It looks like add_device would work if I opted to use it as an
> "update_device_params" type of function (I think I can use it in the
> same location I currently use adapter_set_wake_capable; I would just
> check that the device has already been added first).
> 
> You would still need to make a separate call from the original
> add_device so your original criticism of requiring another mgmt call
> for every param being set is still there. I could refactor the action
> parameter to accept flags (i.e. 0x3 = Set connection parameters) and
> then add a uint16_t flags parameter (i.e. 1 << 0: Allow wakeup from
> incoming connection).
> 
> What do you think?

I like to give Johan and Luiz some time to look at userspace code and see where this fits best.

My proposal would be to ignore the API question for a bit. Keep the mgmt command you have for testing for now. Switching over to a different command is done within an hour once we have the internals solid.

As I commented in my review, I would store the BR/EDR ones in the wakeable list and the LE ones as a flag or parameter in the conn params list. My real concern is the complexity your patch set adds. We really need to streamline this and make things simpler. The whitelist update worries me a lot. That code path is already rather complicated and we should not add to it.

For me it looks like you designed this based on the API that mgmt exposes (aka your first patch in the patch set). That leads you on a path that I feel is too complicated. So I would do this complete opposite and figure out the tasks and what information of wakeable or not you need at what point and where are they best places for BR/EDR and LE in hci_dev.

As an example, the whole complexity of disconnecting all devices and disabling page scan and scanning etc. is something that we can get merged quickly. If I assume an empty list of wakeable devices, then I would still disconnect all devices, disable page scan and scanning and also suspend all advertising and discovery.

I fully realize that this is a lot of work, but we need to get this support done right. I already foresee that you might want to have a keep advertising while suspend flag to advertise some sort of non-connectable beacon (like a find me hint). And at that point I don’t want to have to rethink the whole code path again.

Also please don’t be shy and tell if we did things totally stupid. We can change existing code as long as mgmt API stays backwards compatible. I am a big fan of fixing things to make code simpler and make maintenance easier.

Regards

Marcel

Re: [BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Abhishek Pandit-Subedi]

Hi all,

Reviving this thread to talk about how we should handle "set wake
capable".  For a more general implementation, we could rename this to
"Set Connection Params". It could be extended for other connection
parameters in the future.

i.e.
enum conn_params_type {
  SET_WAKE_CAPABLE,
};

struct conn_param {
  uint8_t type;
  uint8_t value;
};

struct set_conn_params {
  bdaddr_t addr;
  uint8_t count;
  struct conn_param values[];
};

Thoughts?

Abhishek









On Wed, Jan 29, 2020 at 11:03 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> >>>>> Add docs for new management operation to mark a device as wake capable.
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> Changes in v3: None
> >>>>> Changes in v2:
> >>>>> * Separated docs/mgmt-api.txt into its own patch
> >>>>>
> >>>>> doc/mgmt-api.txt | 19 +++++++++++++++++++
> >>>>> 1 file changed, 19 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
> >>>>>
> >>>>
> >>>> my current thinking goes into this API addition:
> >>>>
> >>>> --- a/doc/mgmt-api.txt
> >>>> +++ b/doc/mgmt-api.txt
> >>>> @@ -2003,6 +2003,7 @@ Add Device Command
> >>>>               0       Background scan for device
> >>>>               1       Allow incoming connection
> >>>>               2       Auto-connect remote device
> >>>> +               3       Allow incoming connection to wake up the system
> >>>>
> >>>>       With the Action 0, when the device is found, a new Device Found
> >>>>       event will be sent indicating this device is available. This
> >>>> @@ -2018,6 +2019,9 @@ Add Device Command
> >>>>       and if successful a Device Connected event will be sent. This
> >>>>       action is only valid for LE Public and LE Random address types.
> >>>>
> >>>> +       With the Action 3, the device is allowed to connect the same way
> >>>> +       as with Action 1, but allows to wake up the system from suspend.
> >>>> +
> >>>>       When a device is blocked using Block Device command, then it is
> >>>>       valid to add the device here, but all actions will be ignored
> >>>>       until the device is unblocked.
> >>>>
> >>>> Since we are really talking about incoming connections, then the Add Device command is the one that handles this. Giving a device the option to wake us up that is not set up for incoming connections makes no sense. We can discuss if certain LE advertising packets should wake us up, but that is a total different API in my book.
> >>>
> >>> I originally tried implementing this with the Add Device api as you
> >>> suggested in the RFC email back in November (when we first talked
> >>> about this). I had trouble figuring out when/where in bluez to
> >>> actually send the Add Device command.
> >>>
> >>> Whether a device supports wake-up is a profile level setting (i.e. HID
> >>> only so far). As far as I can tell, Add Device is called before the
> >>> profile connection is established. Bluez has two apis that call
> >>> MGMT_OP_ADD_DEVICE:
> >>> * adapter_auto_connect_add (this seems to be LE)
> >>> * adapter_whitelist_add (this seems to be BR/EDR)
> >>>
> >>> Both seem to be called BEFORE the registered profiles have a chance to
> >>> accept the connection.
> >>
> >> this is something for Luiz or Johan to comment on. Maybe our code is not as good as I was thinking in this regard. Or maybe this is actually legacy code that I am trying to rid of by requiring a mgmt API revision that has Add Device support.
> >>
> >> In general once we bonded with a device, we should be able to assign certain properties to it that are kept persistently.
> >>
> >
> > It looks like add_device would work if I opted to use it as an
> > "update_device_params" type of function (I think I can use it in the
> > same location I currently use adapter_set_wake_capable; I would just
> > check that the device has already been added first).
> >
> > You would still need to make a separate call from the original
> > add_device so your original criticism of requiring another mgmt call
> > for every param being set is still there. I could refactor the action
> > parameter to accept flags (i.e. 0x3 = Set connection parameters) and
> > then add a uint16_t flags parameter (i.e. 1 << 0: Allow wakeup from
> > incoming connection).
> >
> > What do you think?
>
> I like to give Johan and Luiz some time to look at userspace code and see where this fits best.
>
> My proposal would be to ignore the API question for a bit. Keep the mgmt command you have for testing for now. Switching over to a different command is done within an hour once we have the internals solid.
>
> As I commented in my review, I would store the BR/EDR ones in the wakeable list and the LE ones as a flag or parameter in the conn params list. My real concern is the complexity your patch set adds. We really need to streamline this and make things simpler. The whitelist update worries me a lot. That code path is already rather complicated and we should not add to it.
>
> For me it looks like you designed this based on the API that mgmt exposes (aka your first patch in the patch set). That leads you on a path that I feel is too complicated. So I would do this complete opposite and figure out the tasks and what information of wakeable or not you need at what point and where are they best places for BR/EDR and LE in hci_dev.
>
> As an example, the whole complexity of disconnecting all devices and disabling page scan and scanning etc. is something that we can get merged quickly. If I assume an empty list of wakeable devices, then I would still disconnect all devices, disable page scan and scanning and also suspend all advertising and discovery.
>
> I fully realize that this is a lot of work, but we need to get this support done right. I already foresee that you might want to have a keep advertising while suspend flag to advertise some sort of non-connectable beacon (like a find me hint). And at that point I don’t want to have to rethink the whole code path again.
>
> Also please don’t be shy and tell if we did things totally stupid. We can change existing code as long as mgmt API stays backwards compatible. I am a big fan of fixing things to make code simpler and make maintenance easier.
>
> Regards
>
> Marcel
>

Re: [BlueZ PATCH v3 1/5] mgmt: Add docs for Set Wake Capable

[Abhishek Pandit-Subedi]

+Luiz and +Johan -- I think I had you in the cc: before but, as Marcel
said in earlier email, what are your thoughts on where this belongs in
userspace?

I'm reluctant to plop this in "Add Device" because that happens before
profile connection and that's when we determine whether the connection
should be wake capable.

Here's the contents of my last email:
 Hi all,

 Reviving this thread to talk about how we should handle "set wake
 capable".  For a more general implementation, we could rename this to
 "Set Connection Params". It could be extended for other connection
 parameters in the future.

 i.e.
 enum conn_params_type {
   SET_WAKE_CAPABLE,
 };

 struct conn_param {
   uint8_t type;
   uint8_t value;
 };

 struct set_conn_params {
   bdaddr_t addr;
   uint8_t count;
   struct conn_param values[];
 };

 Thoughts?

 Abhishek

>
>
>
>
>
> On Wed, Jan 29, 2020 at 11:03 AM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Abhishek,
> >
> > >>>>> Add docs for new management operation to mark a device as wake capable.
> > >>>>>
> > >>>>> ---
> > >>>>>
> > >>>>> Changes in v3: None
> > >>>>> Changes in v2:
> > >>>>> * Separated docs/mgmt-api.txt into its own patch
> > >>>>>
> > >>>>> doc/mgmt-api.txt | 19 +++++++++++++++++++
> > >>>>> 1 file changed, 19 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
> > >>>>>
> > >>>>
> > >>>> my current thinking goes into this API addition:
> > >>>>
> > >>>> --- a/doc/mgmt-api.txt
> > >>>> +++ b/doc/mgmt-api.txt
> > >>>> @@ -2003,6 +2003,7 @@ Add Device Command
> > >>>>               0       Background scan for device
> > >>>>               1       Allow incoming connection
> > >>>>               2       Auto-connect remote device
> > >>>> +               3       Allow incoming connection to wake up the system
> > >>>>
> > >>>>       With the Action 0, when the device is found, a new Device Found
> > >>>>       event will be sent indicating this device is available. This
> > >>>> @@ -2018,6 +2019,9 @@ Add Device Command
> > >>>>       and if successful a Device Connected event will be sent. This
> > >>>>       action is only valid for LE Public and LE Random address types.
> > >>>>
> > >>>> +       With the Action 3, the device is allowed to connect the same way
> > >>>> +       as with Action 1, but allows to wake up the system from suspend.
> > >>>> +
> > >>>>       When a device is blocked using Block Device command, then it is
> > >>>>       valid to add the device here, but all actions will be ignored
> > >>>>       until the device is unblocked.
> > >>>>
> > >>>> Since we are really talking about incoming connections, then the Add Device command is the one that handles this. Giving a device the option to wake us up that is not set up for incoming connections makes no sense. We can discuss if certain LE advertising packets should wake us up, but that is a total different API in my book.
> > >>>
> > >>> I originally tried implementing this with the Add Device api as you
> > >>> suggested in the RFC email back in November (when we first talked
> > >>> about this). I had trouble figuring out when/where in bluez to
> > >>> actually send the Add Device command.
> > >>>
> > >>> Whether a device supports wake-up is a profile level setting (i.e. HID
> > >>> only so far). As far as I can tell, Add Device is called before the
> > >>> profile connection is established. Bluez has two apis that call
> > >>> MGMT_OP_ADD_DEVICE:
> > >>> * adapter_auto_connect_add (this seems to be LE)
> > >>> * adapter_whitelist_add (this seems to be BR/EDR)
> > >>>
> > >>> Both seem to be called BEFORE the registered profiles have a chance to
> > >>> accept the connection.
> > >>
> > >> this is something for Luiz or Johan to comment on. Maybe our code is not as good as I was thinking in this regard. Or maybe this is actually legacy code that I am trying to rid of by requiring a mgmt API revision that has Add Device support.
> > >>
> > >> In general once we bonded with a device, we should be able to assign certain properties to it that are kept persistently.
> > >>
> > >
> > > It looks like add_device would work if I opted to use it as an
> > > "update_device_params" type of function (I think I can use it in the
> > > same location I currently use adapter_set_wake_capable; I would just
> > > check that the device has already been added first).
> > >
> > > You would still need to make a separate call from the original
> > > add_device so your original criticism of requiring another mgmt call
> > > for every param being set is still there. I could refactor the action
> > > parameter to accept flags (i.e. 0x3 = Set connection parameters) and
> > > then add a uint16_t flags parameter (i.e. 1 << 0: Allow wakeup from
> > > incoming connection).
> > >
> > > What do you think?
> >
> > I like to give Johan and Luiz some time to look at userspace code and see where this fits best.
> >
> > My proposal would be to ignore the API question for a bit. Keep the mgmt command you have for testing for now. Switching over to a different command is done within an hour once we have the internals solid.
> >
> > As I commented in my review, I would store the BR/EDR ones in the wakeable list and the LE ones as a flag or parameter in the conn params list. My real concern is the complexity your patch set adds. We really need to streamline this and make things simpler. The whitelist update worries me a lot. That code path is already rather complicated and we should not add to it.
> >
> > For me it looks like you designed this based on the API that mgmt exposes (aka your first patch in the patch set). That leads you on a path that I feel is too complicated. So I would do this complete opposite and figure out the tasks and what information of wakeable or not you need at what point and where are they best places for BR/EDR and LE in hci_dev.
> >
> > As an example, the whole complexity of disconnecting all devices and disabling page scan and scanning etc. is something that we can get merged quickly. If I assume an empty list of wakeable devices, then I would still disconnect all devices, disable page scan and scanning and also suspend all advertising and discovery.
> >
> > I fully realize that this is a lot of work, but we need to get this support done right. I already foresee that you might want to have a keep advertising while suspend flag to advertise some sort of non-connectable beacon (like a find me hint). And at that point I don’t want to have to rethink the whole code path again.
> >
> > Also please don’t be shy and tell if we did things totally stupid. We can change existing code as long as mgmt API stays backwards compatible. I am a big fan of fixing things to make code simpler and make maintenance easier.
> >
> > Regards
> >
> > Marcel
> >