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

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

[Abhishek Pandit-Subedi]

Hi Luiz and Marcel,

Please do not merge this until the accompanying kernel change is
accepted. I am sending this series to show how I plan on using the
changes to the Add Device management operation.

Pending questions I have for this series:
* Do I need to distinguish whether the kernel supports the updated Add
  Device structure? (It will probably reject it if the size isn't
  correct)
* Can we allow HID devices to default to Wake Allowed? I think its
  preferable to allow newly paired HID devices to wake the system from
  sleep without any additional changes.

--

This change accompanies a change in the kernel to allow marking devices
as wakeable so they can wake the system from suspend. Currently, only
HID devices support this and will be configured to allow this setting to
be changed via the WakeAllowed property.

In order to set this flag, Bluez must call the Add Device management
operation with the DEVICE_FLAG_WAKEABLE flag mask and value set. This
call can be made multiple times with the same address (the kernel will
add the device the first time but only update the action or flags on
subsequent calls).

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 v4:
* Newly added support in Add Device for flags
* Renamed wake_capable to wake_allowed
* Removed set_wake_capable mgmt op and updated add_device to accept
  flags to set whether a device is wakeable
* Refactored adapter_whitelist_add and adapter_auto_connect_add to call
  adapter_add_device
* Renamed WakeCapable to WakeAllowed
* Renamed WakeCapable to WakeAllowed
* Renamed device_set_profile_wake_support to just
  device_set_wake_support

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:
* 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: Update docs for Add Device
  device: Support marking a device with wake allowed
  client: Display wake allowed property with info
  doc/device-api: Add WakeAllowed
  input: Make HID devices support wake

 client/main.c           |   1 +
 doc/device-api.txt      |   5 ++
 doc/mgmt-api.txt        |  12 ++++
 lib/mgmt.h              |   3 +
 profiles/input/device.c |   1 +
 profiles/input/hog.c    |   1 +
 src/adapter.c           | 112 +++++++++++++----------------------
 src/adapter.h           |   1 +
 src/device.c            | 125 ++++++++++++++++++++++++++++++++++++++++
 src/device.h            |   8 +++
 10 files changed, 197 insertions(+), 72 deletions(-)

-- 
2.25.1.696.g5e7596f4ac-goog

[BlueZ PATCH v4 1/5] mgmt: Update docs for Add Device

[Abhishek Pandit-Subedi]

Update the docs for Add Device with information on flags mask and value.
Add information on the Wakeable flag that can be set to allow the device
to wake the system from suspend.
---

Changes in v4:
* Newly added support in Add Device for flags

Changes in v3: None
Changes in v2: None

 doc/mgmt-api.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
index 27a41f334..e99c23710 100644
--- a/doc/mgmt-api.txt
+++ b/doc/mgmt-api.txt
@@ -1997,6 +1997,8 @@ Add Device Command
 	Command Parameters:	Address (6 Octets)
 				Address_Type (1 Octet)
 				Action (1 Octet)
+				Flags Mask (1 Octet)
+				Flags Value (1 Octet)
 	Return Parameters:	Address (6 Octets)
 				Address_Type (1 Octet)
 
@@ -2014,6 +2016,9 @@ Add Device Command
 		1	Allow incoming connection
 		2	Auto-connect remote device
 
+	The following flags are supported:
+		0x1	Wakeable
+
 	With the Action 0, when the device is found, a new Device Found
 	event will be sent indicating this device is available. This
 	action is only valid for LE Public and LE Random address types.
@@ -2036,6 +2041,13 @@ Add Device Command
 	connectable setting is off. This acts as list of known trusted
 	devices.
 
+	To set flags on the device, first set the bit in the mask for the
+	flag to set and then set or clear the bit in the value to indicate
+	whether the flag should be set.
+
+	The Wakeable flag controls whether this device can wake the system
+	from suspend.
+
 	This command can be used when the controller is not powered and
 	all settings will be programmed once powered.
 
-- 
2.25.1.696.g5e7596f4ac-goog

[BlueZ PATCH v4 2/5] device: Support marking a device with wake allowed

[Abhishek Pandit-Subedi]

If a device is allowed to wake the host system from suspend, it should
be marked as wake allowed. We add support for a new property that is
sent to the kernel via flags in the add_device mgmt op. We also add the
dbus endpoint to allow the wake allowed setting to be controlled.

In order for wake allowed to be set, the profile must also support wake.
This setting isn't exposed to the user but must be set by profiles that
intend to support wake from suspend.
---

Changes in v4:
* Renamed wake_capable to wake_allowed
* Removed set_wake_capable mgmt op and updated add_device to accept
  flags to set whether a device is wakeable
* Refactored adapter_whitelist_add and adapter_auto_connect_add to call
  adapter_add_device

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    |   3 ++
 src/adapter.c | 112 ++++++++++++++++----------------------------
 src/adapter.h |   1 +
 src/device.c  | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.h  |   8 ++++
 5 files changed, 177 insertions(+), 72 deletions(-)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index 7682537fe..732d3afa8 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -395,10 +395,13 @@ struct mgmt_rp_get_clock_info {
 struct mgmt_cp_add_device {
 	struct mgmt_addr_info addr;
 	uint8_t action;
+	uint8_t flags_mask;
+	uint8_t flags_value;
 } __packed;
 struct mgmt_rp_add_device {
 	struct mgmt_addr_info addr;
 } __packed;
+#define DEVICE_FLAG_WAKEABLE		0x1
 
 #define MGMT_OP_REMOVE_DEVICE		0x0034
 struct mgmt_cp_remove_device {
diff --git a/src/adapter.c b/src/adapter.c
index 972d88772..6bfd31fb0 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -4654,7 +4654,7 @@ void adapter_connect_list_remove(struct btd_adapter *adapter,
 	trigger_passive_scanning(adapter);
 }
 
-static void add_whitelist_complete(uint8_t status, uint16_t length,
+static void add_device_complete(uint8_t status, uint16_t length,
 					const void *param, void *user_data)
 {
 	const struct mgmt_rp_add_device *rp = param;
@@ -4670,8 +4670,8 @@ static void add_whitelist_complete(uint8_t status, uint16_t length,
 
 	ba2str(&rp->addr.bdaddr, addr);
 
-	dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr,
-							rp->addr.type);
+	dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr, rp->addr.type);
+
 	if (!dev) {
 		btd_error(adapter->dev_id,
 			"Add Device complete for unknown device %s", addr);
@@ -4680,29 +4680,50 @@ static void add_whitelist_complete(uint8_t status, uint16_t length,
 
 	if (status != MGMT_STATUS_SUCCESS) {
 		btd_error(adapter->dev_id,
-					"Failed to add device %s: %s (0x%02x)",
-					addr, mgmt_errstr(status), status);
+			"Failed to add device %s (%u): %s (0x%02x)",
+			addr, rp->addr.type, mgmt_errstr(status), status);
+
+		if (rp->addr.type != BDADDR_BREDR)
+			adapter->connect_list =
+				g_slist_remove(adapter->connect_list, dev);
 		return;
 	}
 
-	DBG("%s added to kernel whitelist", addr);
+	/* Update flag results if any */
+	device_add_complete(dev);
+
+	DBG("%s (%u) added to kernel connect list", addr, rp->addr.type);
 }
 
-void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
+int adapter_add_device(struct btd_adapter *adapter, struct btd_device *device)
 {
 	struct mgmt_cp_add_device cp;
+	const bdaddr_t *bdaddr;
+	uint8_t bdaddr_type;
 
 	if (!kernel_conn_control)
-		return;
+		return 0;
+
+	bdaddr = device_get_address(device);
+	bdaddr_type = btd_device_get_bdaddr_type(device);
 
 	memset(&cp, 0, sizeof(cp));
-	bacpy(&cp.addr.bdaddr, device_get_address(dev));
-	cp.addr.type = BDADDR_BREDR;
-	cp.action = 0x01;
+	bacpy(&cp.addr.bdaddr, bdaddr);
+	cp.addr.type = bdaddr_type;
 
-	mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
-				adapter->dev_id, sizeof(cp), &cp,
-				add_whitelist_complete, adapter, NULL);
+	/* BREDR will always have action = 0x1 (allow incoming connection) */
+	cp.action = bdaddr_type == BDADDR_BREDR ? 0x1 : 0x2;
+	cp.flags_mask = device_get_flags_mask(device);
+	cp.flags_value = device_get_flags_value(device);
+
+	return mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
+			adapter->dev_id, sizeof(cp), &cp, add_device_complete,
+			adapter, NULL);
+}
+
+void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
+{
+	adapter_add_device(adapter, dev);
 }
 
 static void remove_whitelist_complete(uint8_t status, uint16_t length,
@@ -4743,76 +4764,23 @@ void adapter_whitelist_remove(struct btd_adapter *adapter, struct btd_device *de
 				remove_whitelist_complete, adapter, NULL);
 }
 
-static void add_device_complete(uint8_t status, uint16_t length,
-					const void *param, void *user_data)
-{
-	const struct mgmt_rp_add_device *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 Add Device 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,
-			"Add Device complete for unknown device %s", addr);
-		return;
-	}
-
-	if (status != MGMT_STATUS_SUCCESS) {
-		btd_error(adapter->dev_id,
-			"Failed to add device %s (%u): %s (0x%02x)",
-			addr, rp->addr.type, mgmt_errstr(status), status);
-		adapter->connect_list = g_slist_remove(adapter->connect_list,
-									dev);
-		return;
-	}
-
-	DBG("%s (%u) added to kernel connect list", addr, rp->addr.type);
-}
-
 void adapter_auto_connect_add(struct btd_adapter *adapter,
-					struct btd_device *device)
+			      struct btd_device *device)
 {
-	struct mgmt_cp_add_device cp;
-	const bdaddr_t *bdaddr;
-	uint8_t bdaddr_type;
-	unsigned int id;
-
-	if (!kernel_conn_control)
-		return;
+	uint8_t bdaddr_type = btd_device_get_bdaddr_type(device);
 
 	if (g_slist_find(adapter->connect_list, device)) {
 		DBG("ignoring already added device %s",
-						device_get_path(device));
+		    device_get_path(device));
 		return;
 	}
 
-	bdaddr = device_get_address(device);
-	bdaddr_type = btd_device_get_bdaddr_type(device);
-
 	if (bdaddr_type == BDADDR_BREDR) {
-		DBG("auto-connection feature is not avaiable for BR/EDR");
+		DBG("auto-connection feature is not available for BR/EDR");
 		return;
 	}
 
-	memset(&cp, 0, sizeof(cp));
-	bacpy(&cp.addr.bdaddr, bdaddr);
-	cp.addr.type = bdaddr_type;
-	cp.action = 0x02;
-
-	id = mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
-			adapter->dev_id, sizeof(cp), &cp, add_device_complete,
-			adapter, NULL);
-	if (id == 0)
+	if (adapter_add_device(adapter, device) == 0)
 		return;
 
 	adapter->connect_list = g_slist_append(adapter->connect_list, device);
diff --git a/src/adapter.h b/src/adapter.h
index d0a5253bd..a5a9e917a 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -213,6 +213,7 @@ int adapter_connect_list_add(struct btd_adapter *adapter,
 					struct btd_device *device);
 void adapter_connect_list_remove(struct btd_adapter *adapter,
 						struct btd_device *device);
+int adapter_add_device(struct btd_adapter* adapter, struct btd_device* dev);
 void adapter_auto_connect_add(struct btd_adapter *adapter,
 					struct btd_device *device);
 void adapter_auto_connect_remove(struct btd_adapter *adapter,
diff --git a/src/device.c b/src/device.c
index 69f98e488..c646a7fee 100644
--- a/src/device.c
+++ b/src/device.c
@@ -184,11 +184,15 @@ struct btd_device {
 	uint8_t		conn_bdaddr_type;
 	bdaddr_t	bdaddr;
 	uint8_t		bdaddr_type;
+	uint8_t		mgmt_flags_mask;   /* Flags to update on add_device */
+	uint8_t		mgmt_flags_value; /* Flag values for add_device */
 	char		*path;
 	bool		bredr;
 	bool		le;
 	bool		pending_paired;		/* "Paired" waiting for SDP */
 	bool		svc_refreshed;
+	bool		wake_support;		/* Profile supports wake */
+	bool		wake_allowed;		/* Can wake from suspend */
 	GSList		*svc_callbacks;
 	GSList		*eir_uuids;
 	struct bt_ad	*ad;
@@ -415,6 +419,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", "WakeAllowed",
+							device->wake_allowed);
+
 	if (device->uuids) {
 		GSList *l;
 		int i;
@@ -1318,6 +1325,79 @@ dev_property_advertising_data_exist(const GDBusPropertyTable *property,
 	return bt_ad_has_data(device->ad, NULL);
 }
 
+bool device_get_wake_support(struct btd_device *device)
+{
+	return device->wake_support;
+}
+
+void device_set_wake_support(struct btd_device *device, bool wake_support)
+{
+	device->wake_support = wake_support;
+}
+
+bool device_get_wake_allowed(struct btd_device *device)
+{
+	return device->wake_allowed;
+}
+
+void device_set_wake_allowed(struct btd_device *device, bool wake_allowed)
+{
+	device->mgmt_flags_mask |= DEVICE_FLAG_WAKEABLE;
+	if (wake_allowed)
+		device->mgmt_flags_value |= DEVICE_FLAG_WAKEABLE;
+	else
+		device->mgmt_flags_value &= ~DEVICE_FLAG_WAKEABLE;
+
+	adapter_add_device(device->adapter, device);
+}
+
+static gboolean
+dev_property_get_wake_allowed(const GDBusPropertyTable *property,
+			     DBusMessageIter *iter, void *data)
+{
+	struct btd_device *device = data;
+	dbus_bool_t wake_allowed =
+			device_get_wake_allowed(device) ? TRUE : FALSE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_allowed);
+
+	return TRUE;
+}
+
+static void dev_property_set_wake_allowed(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;
+	}
+
+	if (device->temporary) {
+		g_dbus_pending_property_error(id,
+					ERROR_INTERFACE ".Unsupported",
+					"Cannot set property while temporary");
+		return;
+	}
+
+	dbus_message_iter_get_basic(value, &b);
+	device_set_wake_allowed(device, b);
+	g_dbus_pending_property_success(id);
+}
+
+static gboolean dev_property_wake_allowed_exist(
+		const GDBusPropertyTable *property, void *data)
+{
+	struct btd_device *device = data;
+
+	return device_get_wake_support(device) ? TRUE : FALSE;
+}
+
 static gboolean disconnect_all(gpointer user_data)
 {
 	struct btd_device *device = user_data;
@@ -2779,6 +2859,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 },
+	{ "WakeAllowed", "b", dev_property_get_wake_allowed,
+				dev_property_set_wake_allowed,
+				dev_property_wake_allowed_exist },
 	{ }
 };
 
@@ -3030,6 +3113,7 @@ static void load_info(struct btd_device *device, const char *local,
 	char *str;
 	gboolean store_needed = FALSE;
 	gboolean blocked;
+	gboolean wake_allowed;
 	char **uuids;
 	int source, vendor, product, version;
 	char **techno, **t;
@@ -3141,6 +3225,14 @@ next:
 		btd_device_set_pnpid(device, source, vendor, product, version);
 	}
 
+	/* Mark wake allowed */
+	wake_allowed = g_key_file_get_boolean(key_file, "General",
+					      "WakeAllowed", NULL);
+	if (wake_allowed) {
+		device->mgmt_flags_mask |= DEVICE_FLAG_WAKEABLE;
+		device->mgmt_flags_value |= DEVICE_FLAG_WAKEABLE;
+	}
+
 	if (store_needed)
 		store_device_info(device);
 }
@@ -3959,6 +4051,39 @@ char *btd_device_get_storage_path(struct btd_device *device,
 				dstaddr, filename);
 }
 
+void device_add_complete(struct btd_device *device)
+{
+	bool flag = false;
+
+	/* No flags updated */
+	if (!device->mgmt_flags_mask)
+		return;
+
+	if (device->mgmt_flags_mask & DEVICE_FLAG_WAKEABLE)
+	{
+		flag = device->mgmt_flags_value & DEVICE_FLAG_WAKEABLE;
+		device->wake_allowed = flag;
+
+		store_device_info(device);
+		g_dbus_emit_property_changed(dbus_conn, device->path,
+					     DEVICE_INTERFACE, "WakeAllowed");
+	}
+
+	/* Clear mask and values for next completion */
+	device->mgmt_flags_mask = 0;
+	device->mgmt_flags_value = 0;
+}
+
+uint8_t device_get_flags_mask(struct btd_device *device)
+{
+	return device->mgmt_flags_mask;
+}
+
+uint8_t device_get_flags_value(struct btd_device *device)
+{
+	return device->mgmt_flags_value;
+}
+
 void btd_device_device_set_name(struct btd_device *device, const char *name)
 {
 	if (strncmp(name, device->name, MAX_NAME_LENGTH) == 0)
diff --git a/src/device.h b/src/device.h
index 06b100499..85e33f1cd 100644
--- a/src/device.h
+++ b/src/device.h
@@ -33,6 +33,10 @@ struct btd_device *device_create_from_storage(struct btd_adapter *adapter,
 char *btd_device_get_storage_path(struct btd_device *device,
 				const char *filename);
 
+void device_add_complete(struct btd_device *device);
+uint8_t device_get_flags_mask(struct btd_device *device);
+uint8_t device_get_flags_value(struct btd_device *device);
+
 void btd_device_device_set_name(struct btd_device *device, const char *name);
 void device_store_cached_name(struct btd_device *dev, const char *name);
 void device_get_name(struct btd_device *device, char *name, size_t len);
@@ -139,6 +143,10 @@ 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_support(struct btd_device *device);
+void device_set_wake_support(struct btd_device *device, bool wake_support);
+bool device_get_wake_allowed(struct btd_device *device);
+void device_set_wake_allowed(struct btd_device *device, bool wake_allowed);
 
 typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
 					void *user_data);
-- 
2.25.1.696.g5e7596f4ac-goog

[BlueZ PATCH v4 3/5] client: Display wake allowed property with info

[Abhishek Pandit-Subedi]

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

Changes in v4:
* Renamed WakeCapable to WakeAllowed

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 422da5593..4953f50f0 100644
--- a/client/main.c
+++ b/client/main.c
@@ -1637,6 +1637,7 @@ static void cmd_info(int argc, char *argv[])
 	print_property(proxy, "Trusted");
 	print_property(proxy, "Blocked");
 	print_property(proxy, "Connected");
+	print_property(proxy, "WakeAllowed");
 	print_property(proxy, "LegacyPairing");
 	print_uuids(proxy);
 	print_property(proxy, "Modalias");
-- 
2.25.1.696.g5e7596f4ac-goog

[BlueZ PATCH v4 4/5] doc/device-api: Add WakeAllowed

[Abhishek Pandit-Subedi]

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

Changes in v4:
* Renamed WakeCapable to WakeAllowed

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..6b265c73b 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 WakeAllowed [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.1.696.g5e7596f4ac-goog

[BlueZ PATCH v4 5/5] input: Make HID devices support wake

[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 v4:
* Renamed device_set_profile_wake_support to just
  device_set_wake_support

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 d89da2d7c..d2a4ec82e 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1402,6 +1402,7 @@ int input_device_register(struct btd_service *service)
 	}
 
 	btd_service_set_user_data(service, idev);
+	device_set_wake_support(device, true);
 
 	return 0;
 }
diff --git a/profiles/input/hog.c b/profiles/input/hog.c
index 327a1d1c3..0e4bd1c34 100644
--- a/profiles/input/hog.c
+++ b/profiles/input/hog.c
@@ -168,6 +168,7 @@ static int hog_probe(struct btd_service *service)
 		return -EINVAL;
 
 	btd_service_set_user_data(service, dev);
+	device_set_wake_support(device, true);
 	return 0;
 }
 
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [BlueZ PATCH v4 2/5] device: Support marking a device with wake allowed

[Luiz Augusto von Dentz]

Hi Abhishek,

On Thu, Mar 19, 2020 at 6:50 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> If a device is allowed to wake the host system from suspend, it should
> be marked as wake allowed. We add support for a new property that is
> sent to the kernel via flags in the add_device mgmt op. We also add the
> dbus endpoint to allow the wake allowed setting to be controlled.
>
> In order for wake allowed to be set, the profile must also support wake.
> This setting isn't exposed to the user but must be set by profiles that
> intend to support wake from suspend.
> ---
>
> Changes in v4:
> * Renamed wake_capable to wake_allowed
> * Removed set_wake_capable mgmt op and updated add_device to accept
>   flags to set whether a device is wakeable
> * Refactored adapter_whitelist_add and adapter_auto_connect_add to call
>   adapter_add_device
>
> 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    |   3 ++
>  src/adapter.c | 112 ++++++++++++++++----------------------------
>  src/adapter.h |   1 +
>  src/device.c  | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/device.h  |   8 ++++
>  5 files changed, 177 insertions(+), 72 deletions(-)
>
> diff --git a/lib/mgmt.h b/lib/mgmt.h
> index 7682537fe..732d3afa8 100644
> --- a/lib/mgmt.h
> +++ b/lib/mgmt.h
> @@ -395,10 +395,13 @@ struct mgmt_rp_get_clock_info {
>  struct mgmt_cp_add_device {
>         struct mgmt_addr_info addr;
>         uint8_t action;
> +       uint8_t flags_mask;
> +       uint8_t flags_value;
>  } __packed;
>  struct mgmt_rp_add_device {
>         struct mgmt_addr_info addr;
>  } __packed;
> +#define DEVICE_FLAG_WAKEABLE           0x1
>
>  #define MGMT_OP_REMOVE_DEVICE          0x0034
>  struct mgmt_cp_remove_device {
> diff --git a/src/adapter.c b/src/adapter.c
> index 972d88772..6bfd31fb0 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -4654,7 +4654,7 @@ void adapter_connect_list_remove(struct btd_adapter *adapter,
>         trigger_passive_scanning(adapter);
>  }
>
> -static void add_whitelist_complete(uint8_t status, uint16_t length,
> +static void add_device_complete(uint8_t status, uint16_t length,
>                                         const void *param, void *user_data)
>  {
>         const struct mgmt_rp_add_device *rp = param;
> @@ -4670,8 +4670,8 @@ static void add_whitelist_complete(uint8_t status, uint16_t length,
>
>         ba2str(&rp->addr.bdaddr, addr);
>
> -       dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr,
> -                                                       rp->addr.type);
> +       dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr, rp->addr.type);
> +
>         if (!dev) {
>                 btd_error(adapter->dev_id,
>                         "Add Device complete for unknown device %s", addr);
> @@ -4680,29 +4680,50 @@ static void add_whitelist_complete(uint8_t status, uint16_t length,
>
>         if (status != MGMT_STATUS_SUCCESS) {
>                 btd_error(adapter->dev_id,
> -                                       "Failed to add device %s: %s (0x%02x)",
> -                                       addr, mgmt_errstr(status), status);
> +                       "Failed to add device %s (%u): %s (0x%02x)",
> +                       addr, rp->addr.type, mgmt_errstr(status), status);

Don't we have to handle the case where the kernel is older and the
command fails?

> +
> +               if (rp->addr.type != BDADDR_BREDR)
> +                       adapter->connect_list =
> +                               g_slist_remove(adapter->connect_list, dev);
>                 return;
>         }
>
> -       DBG("%s added to kernel whitelist", addr);
> +       /* Update flag results if any */
> +       device_add_complete(dev);
> +
> +       DBG("%s (%u) added to kernel connect list", addr, rp->addr.type);
>  }
>
> -void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
> +int adapter_add_device(struct btd_adapter *adapter, struct btd_device *device)
>  {
>         struct mgmt_cp_add_device cp;
> +       const bdaddr_t *bdaddr;
> +       uint8_t bdaddr_type;
>
>         if (!kernel_conn_control)
> -               return;
> +               return 0;
> +
> +       bdaddr = device_get_address(device);
> +       bdaddr_type = btd_device_get_bdaddr_type(device);
>
>         memset(&cp, 0, sizeof(cp));
> -       bacpy(&cp.addr.bdaddr, device_get_address(dev));
> -       cp.addr.type = BDADDR_BREDR;
> -       cp.action = 0x01;
> +       bacpy(&cp.addr.bdaddr, bdaddr);
> +       cp.addr.type = bdaddr_type;
>
> -       mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
> -                               adapter->dev_id, sizeof(cp), &cp,
> -                               add_whitelist_complete, adapter, NULL);
> +       /* BREDR will always have action = 0x1 (allow incoming connection) */
> +       cp.action = bdaddr_type == BDADDR_BREDR ? 0x1 : 0x2;
> +       cp.flags_mask = device_get_flags_mask(device);
> +       cp.flags_value = device_get_flags_value(device);

An alternative to check if the command fails due to the kernel not
understanding the command flags perhaps we could send it based on the
revision provided we bump the revision with this change, anyway we
will need to make it work with older kernels as well. Also perhaps we
should add tests to mgmt-tester to validate this set, it should skip
the test if the kernel is too old and don't support it.

> +
> +       return mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
> +                       adapter->dev_id, sizeof(cp), &cp, add_device_complete,
> +                       adapter, NULL);
> +}
> +
> +void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
> +{
> +       adapter_add_device(adapter, dev);
>  }
>
>  static void remove_whitelist_complete(uint8_t status, uint16_t length,
> @@ -4743,76 +4764,23 @@ void adapter_whitelist_remove(struct btd_adapter *adapter, struct btd_device *de
>                                 remove_whitelist_complete, adapter, NULL);
>  }
>
> -static void add_device_complete(uint8_t status, uint16_t length,
> -                                       const void *param, void *user_data)
> -{
> -       const struct mgmt_rp_add_device *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 Add Device 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,
> -                       "Add Device complete for unknown device %s", addr);
> -               return;
> -       }
> -
> -       if (status != MGMT_STATUS_SUCCESS) {
> -               btd_error(adapter->dev_id,
> -                       "Failed to add device %s (%u): %s (0x%02x)",
> -                       addr, rp->addr.type, mgmt_errstr(status), status);
> -               adapter->connect_list = g_slist_remove(adapter->connect_list,
> -                                                                       dev);
> -               return;
> -       }
> -
> -       DBG("%s (%u) added to kernel connect list", addr, rp->addr.type);
> -}
> -
>  void adapter_auto_connect_add(struct btd_adapter *adapter,
> -                                       struct btd_device *device)
> +                             struct btd_device *device)
>  {
> -       struct mgmt_cp_add_device cp;
> -       const bdaddr_t *bdaddr;
> -       uint8_t bdaddr_type;
> -       unsigned int id;
> -
> -       if (!kernel_conn_control)
> -               return;
> +       uint8_t bdaddr_type = btd_device_get_bdaddr_type(device);
>
>         if (g_slist_find(adapter->connect_list, device)) {
>                 DBG("ignoring already added device %s",
> -                                               device_get_path(device));
> +                   device_get_path(device));
>                 return;
>         }
>
> -       bdaddr = device_get_address(device);
> -       bdaddr_type = btd_device_get_bdaddr_type(device);
> -
>         if (bdaddr_type == BDADDR_BREDR) {
> -               DBG("auto-connection feature is not avaiable for BR/EDR");
> +               DBG("auto-connection feature is not available for BR/EDR");
>                 return;
>         }
>
> -       memset(&cp, 0, sizeof(cp));
> -       bacpy(&cp.addr.bdaddr, bdaddr);
> -       cp.addr.type = bdaddr_type;
> -       cp.action = 0x02;
> -
> -       id = mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
> -                       adapter->dev_id, sizeof(cp), &cp, add_device_complete,
> -                       adapter, NULL);
> -       if (id == 0)
> +       if (adapter_add_device(adapter, device) == 0)
>                 return;
>
>         adapter->connect_list = g_slist_append(adapter->connect_list, device);
> diff --git a/src/adapter.h b/src/adapter.h
> index d0a5253bd..a5a9e917a 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -213,6 +213,7 @@ int adapter_connect_list_add(struct btd_adapter *adapter,
>                                         struct btd_device *device);
>  void adapter_connect_list_remove(struct btd_adapter *adapter,
>                                                 struct btd_device *device);
> +int adapter_add_device(struct btd_adapter* adapter, struct btd_device* dev);
>  void adapter_auto_connect_add(struct btd_adapter *adapter,
>                                         struct btd_device *device);
>  void adapter_auto_connect_remove(struct btd_adapter *adapter,
> diff --git a/src/device.c b/src/device.c
> index 69f98e488..c646a7fee 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -184,11 +184,15 @@ struct btd_device {
>         uint8_t         conn_bdaddr_type;
>         bdaddr_t        bdaddr;
>         uint8_t         bdaddr_type;
> +       uint8_t         mgmt_flags_mask;   /* Flags to update on add_device */
> +       uint8_t         mgmt_flags_value; /* Flag values for add_device */
>         char            *path;
>         bool            bredr;
>         bool            le;
>         bool            pending_paired;         /* "Paired" waiting for SDP */
>         bool            svc_refreshed;
> +       bool            wake_support;           /* Profile supports wake */
> +       bool            wake_allowed;           /* Can wake from suspend */
>         GSList          *svc_callbacks;
>         GSList          *eir_uuids;
>         struct bt_ad    *ad;
> @@ -415,6 +419,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", "WakeAllowed",
> +                                                       device->wake_allowed);
> +
>         if (device->uuids) {
>                 GSList *l;
>                 int i;
> @@ -1318,6 +1325,79 @@ dev_property_advertising_data_exist(const GDBusPropertyTable *property,
>         return bt_ad_has_data(device->ad, NULL);
>  }
>
> +bool device_get_wake_support(struct btd_device *device)
> +{
> +       return device->wake_support;
> +}
> +
> +void device_set_wake_support(struct btd_device *device, bool wake_support)
> +{
> +       device->wake_support = wake_support;
> +}
> +
> +bool device_get_wake_allowed(struct btd_device *device)
> +{
> +       return device->wake_allowed;
> +}
> +
> +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed)
> +{
> +       device->mgmt_flags_mask |= DEVICE_FLAG_WAKEABLE;
> +       if (wake_allowed)
> +               device->mgmt_flags_value |= DEVICE_FLAG_WAKEABLE;
> +       else
> +               device->mgmt_flags_value &= ~DEVICE_FLAG_WAKEABLE;
> +
> +       adapter_add_device(device->adapter, device);
> +}
> +
> +static gboolean
> +dev_property_get_wake_allowed(const GDBusPropertyTable *property,
> +                            DBusMessageIter *iter, void *data)
> +{
> +       struct btd_device *device = data;
> +       dbus_bool_t wake_allowed =
> +                       device_get_wake_allowed(device) ? TRUE : FALSE;
> +
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_allowed);
> +
> +       return TRUE;
> +}
> +
> +static void dev_property_set_wake_allowed(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;
> +       }
> +
> +       if (device->temporary) {
> +               g_dbus_pending_property_error(id,
> +                                       ERROR_INTERFACE ".Unsupported",
> +                                       "Cannot set property while temporary");
> +               return;
> +       }
> +
> +       dbus_message_iter_get_basic(value, &b);
> +       device_set_wake_allowed(device, b);
> +       g_dbus_pending_property_success(id);
> +}
> +
> +static gboolean dev_property_wake_allowed_exist(
> +               const GDBusPropertyTable *property, void *data)
> +{
> +       struct btd_device *device = data;
> +
> +       return device_get_wake_support(device) ? TRUE : FALSE;
> +}
> +
>  static gboolean disconnect_all(gpointer user_data)
>  {
>         struct btd_device *device = user_data;
> @@ -2779,6 +2859,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 },
> +       { "WakeAllowed", "b", dev_property_get_wake_allowed,
> +                               dev_property_set_wake_allowed,
> +                               dev_property_wake_allowed_exist },
>         { }
>  };
>
> @@ -3030,6 +3113,7 @@ static void load_info(struct btd_device *device, const char *local,
>         char *str;
>         gboolean store_needed = FALSE;
>         gboolean blocked;
> +       gboolean wake_allowed;
>         char **uuids;
>         int source, vendor, product, version;
>         char **techno, **t;
> @@ -3141,6 +3225,14 @@ next:
>                 btd_device_set_pnpid(device, source, vendor, product, version);
>         }
>
> +       /* Mark wake allowed */
> +       wake_allowed = g_key_file_get_boolean(key_file, "General",
> +                                             "WakeAllowed", NULL);
> +       if (wake_allowed) {
> +               device->mgmt_flags_mask |= DEVICE_FLAG_WAKEABLE;
> +               device->mgmt_flags_value |= DEVICE_FLAG_WAKEABLE;
> +       }
> +
>         if (store_needed)
>                 store_device_info(device);
>  }
> @@ -3959,6 +4051,39 @@ char *btd_device_get_storage_path(struct btd_device *device,
>                                 dstaddr, filename);
>  }
>
> +void device_add_complete(struct btd_device *device)
> +{
> +       bool flag = false;
> +
> +       /* No flags updated */
> +       if (!device->mgmt_flags_mask)
> +               return;
> +
> +       if (device->mgmt_flags_mask & DEVICE_FLAG_WAKEABLE)
> +       {
> +               flag = device->mgmt_flags_value & DEVICE_FLAG_WAKEABLE;
> +               device->wake_allowed = flag;
> +
> +               store_device_info(device);
> +               g_dbus_emit_property_changed(dbus_conn, device->path,
> +                                            DEVICE_INTERFACE, "WakeAllowed");
> +       }
> +
> +       /* Clear mask and values for next completion */
> +       device->mgmt_flags_mask = 0;
> +       device->mgmt_flags_value = 0;
> +}
> +
> +uint8_t device_get_flags_mask(struct btd_device *device)
> +{
> +       return device->mgmt_flags_mask;
> +}
> +
> +uint8_t device_get_flags_value(struct btd_device *device)
> +{
> +       return device->mgmt_flags_value;
> +}
> +
>  void btd_device_device_set_name(struct btd_device *device, const char *name)
>  {
>         if (strncmp(name, device->name, MAX_NAME_LENGTH) == 0)
> diff --git a/src/device.h b/src/device.h
> index 06b100499..85e33f1cd 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -33,6 +33,10 @@ struct btd_device *device_create_from_storage(struct btd_adapter *adapter,
>  char *btd_device_get_storage_path(struct btd_device *device,
>                                 const char *filename);
>
> +void device_add_complete(struct btd_device *device);
> +uint8_t device_get_flags_mask(struct btd_device *device);
> +uint8_t device_get_flags_value(struct btd_device *device);
> +
>  void btd_device_device_set_name(struct btd_device *device, const char *name);
>  void device_store_cached_name(struct btd_device *dev, const char *name);
>  void device_get_name(struct btd_device *device, char *name, size_t len);
> @@ -139,6 +143,10 @@ 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_support(struct btd_device *device);
> +void device_set_wake_support(struct btd_device *device, bool wake_support);
> +bool device_get_wake_allowed(struct btd_device *device);
> +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed);
>
>  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
>                                         void *user_data);
> --
> 2.25.1.696.g5e7596f4ac-goog
>


-- 
Luiz Augusto von Dentz

Re: [BlueZ PATCH v4 1/5] mgmt: Update docs for Add Device

[Luiz Augusto von Dentz]

Hi Abhishek,

On Thu, Mar 19, 2020 at 6:50 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Update the docs for Add Device with information on flags mask and value.
> Add information on the Wakeable flag that can be set to allow the device
> to wake the system from suspend.
> ---
>
> Changes in v4:
> * Newly added support in Add Device for flags
>
> Changes in v3: None
> Changes in v2: None
>
>  doc/mgmt-api.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> index 27a41f334..e99c23710 100644
> --- a/doc/mgmt-api.txt
> +++ b/doc/mgmt-api.txt
> @@ -1997,6 +1997,8 @@ Add Device Command
>         Command Parameters:     Address (6 Octets)
>                                 Address_Type (1 Octet)
>                                 Action (1 Octet)
> +                               Flags Mask (1 Octet)
> +                               Flags Value (1 Octet)
>         Return Parameters:      Address (6 Octets)
>                                 Address_Type (1 Octet)
>
> @@ -2014,6 +2016,9 @@ Add Device Command
>                 1       Allow incoming connection
>                 2       Auto-connect remote device
>
> +       The following flags are supported:
> +               0x1     Wakeable
> +

Hmm I thought we would be adding another action, adding this as a flag
is not backward compatible so the command will probably fail on older
kernels.

>         With the Action 0, when the device is found, a new Device Found
>         event will be sent indicating this device is available. This
>         action is only valid for LE Public and LE Random address types.
> @@ -2036,6 +2041,13 @@ Add Device Command
>         connectable setting is off. This acts as list of known trusted
>         devices.
>
> +       To set flags on the device, first set the bit in the mask for the
> +       flag to set and then set or clear the bit in the value to indicate
> +       whether the flag should be set.
> +
> +       The Wakeable flag controls whether this device can wake the system
> +       from suspend.
> +
>         This command can be used when the controller is not powered and
>         all settings will be programmed once powered.
>
> --
> 2.25.1.696.g5e7596f4ac-goog
>


-- 
Luiz Augusto von Dentz

Re: [BlueZ PATCH v4 2/5] device: Support marking a device with wake allowed

[Abhishek Pandit-Subedi]

On Wed, Mar 25, 2020 at 12:54 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Thu, Mar 19, 2020 at 6:50 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > If a device is allowed to wake the host system from suspend, it should
> > be marked as wake allowed. We add support for a new property that is
> > sent to the kernel via flags in the add_device mgmt op. We also add the
> > dbus endpoint to allow the wake allowed setting to be controlled.
> >
> > In order for wake allowed to be set, the profile must also support wake.
> > This setting isn't exposed to the user but must be set by profiles that
> > intend to support wake from suspend.
> > ---
> >
> > Changes in v4:
> > * Renamed wake_capable to wake_allowed
> > * Removed set_wake_capable mgmt op and updated add_device to accept
> >   flags to set whether a device is wakeable
> > * Refactored adapter_whitelist_add and adapter_auto_connect_add to call
> >   adapter_add_device
> >
> > 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    |   3 ++
> >  src/adapter.c | 112 ++++++++++++++++----------------------------
> >  src/adapter.h |   1 +
> >  src/device.c  | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/device.h  |   8 ++++
> >  5 files changed, 177 insertions(+), 72 deletions(-)
> >
> > diff --git a/lib/mgmt.h b/lib/mgmt.h
> > index 7682537fe..732d3afa8 100644
> > --- a/lib/mgmt.h
> > +++ b/lib/mgmt.h
> > @@ -395,10 +395,13 @@ struct mgmt_rp_get_clock_info {
> >  struct mgmt_cp_add_device {
> >         struct mgmt_addr_info addr;
> >         uint8_t action;
> > +       uint8_t flags_mask;
> > +       uint8_t flags_value;
> >  } __packed;
> >  struct mgmt_rp_add_device {
> >         struct mgmt_addr_info addr;
> >  } __packed;
> > +#define DEVICE_FLAG_WAKEABLE           0x1
> >
> >  #define MGMT_OP_REMOVE_DEVICE          0x0034
> >  struct mgmt_cp_remove_device {
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 972d88772..6bfd31fb0 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -4654,7 +4654,7 @@ void adapter_connect_list_remove(struct btd_adapter *adapter,
> >         trigger_passive_scanning(adapter);
> >  }
> >
> > -static void add_whitelist_complete(uint8_t status, uint16_t length,
> > +static void add_device_complete(uint8_t status, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> >         const struct mgmt_rp_add_device *rp = param;
> > @@ -4670,8 +4670,8 @@ static void add_whitelist_complete(uint8_t status, uint16_t length,
> >
> >         ba2str(&rp->addr.bdaddr, addr);
> >
> > -       dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr,
> > -                                                       rp->addr.type);
> > +       dev = btd_adapter_find_device(adapter, &rp->addr.bdaddr, rp->addr.type);
> > +
> >         if (!dev) {
> >                 btd_error(adapter->dev_id,
> >                         "Add Device complete for unknown device %s", addr);
> > @@ -4680,29 +4680,50 @@ static void add_whitelist_complete(uint8_t status, uint16_t length,
> >
> >         if (status != MGMT_STATUS_SUCCESS) {
> >                 btd_error(adapter->dev_id,
> > -                                       "Failed to add device %s: %s (0x%02x)",
> > -                                       addr, mgmt_errstr(status), status);
> > +                       "Failed to add device %s (%u): %s (0x%02x)",
> > +                       addr, rp->addr.type, mgmt_errstr(status), status);
>
> Don't we have to handle the case where the kernel is older and the
> command fails?
>
> > +
> > +               if (rp->addr.type != BDADDR_BREDR)
> > +                       adapter->connect_list =
> > +                               g_slist_remove(adapter->connect_list, dev);
> >                 return;
> >         }
> >
> > -       DBG("%s added to kernel whitelist", addr);
> > +       /* Update flag results if any */
> > +       device_add_complete(dev);
> > +
> > +       DBG("%s (%u) added to kernel connect list", addr, rp->addr.type);
> >  }
> >
> > -void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
> > +int adapter_add_device(struct btd_adapter *adapter, struct btd_device *device)
> >  {
> >         struct mgmt_cp_add_device cp;
> > +       const bdaddr_t *bdaddr;
> > +       uint8_t bdaddr_type;
> >
> >         if (!kernel_conn_control)
> > -               return;
> > +               return 0;
> > +
> > +       bdaddr = device_get_address(device);
> > +       bdaddr_type = btd_device_get_bdaddr_type(device);
> >
> >         memset(&cp, 0, sizeof(cp));
> > -       bacpy(&cp.addr.bdaddr, device_get_address(dev));
> > -       cp.addr.type = BDADDR_BREDR;
> > -       cp.action = 0x01;
> > +       bacpy(&cp.addr.bdaddr, bdaddr);
> > +       cp.addr.type = bdaddr_type;
> >
> > -       mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
> > -                               adapter->dev_id, sizeof(cp), &cp,
> > -                               add_whitelist_complete, adapter, NULL);
> > +       /* BREDR will always have action = 0x1 (allow incoming connection) */
> > +       cp.action = bdaddr_type == BDADDR_BREDR ? 0x1 : 0x2;
> > +       cp.flags_mask = device_get_flags_mask(device);
> > +       cp.flags_value = device_get_flags_value(device);
>
> An alternative to check if the command fails due to the kernel not
> understanding the command flags perhaps we could send it based on the
> revision provided we bump the revision with this change, anyway we
> will need to make it work with older kernels as well. Also perhaps we
> should add tests to mgmt-tester to validate this set, it should skip
> the test if the kernel is too old and don't support it.

I think bumping the version and adding something like
"mgmt_supports_add_device_flag" would be the way to go.
The "WakeAllowed" property's exist function can depend on that support
and the adapter_add_device can check if it's supported and use the
correctly sized structure.

Would it be better to keep two structs in mgmt.h to denote that there
are different sizes?
i.e. struct mgmt_cp_add_device_with_flags?

>
> > +
> > +       return mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
> > +                       adapter->dev_id, sizeof(cp), &cp, add_device_complete,
> > +                       adapter, NULL);
> > +}
> > +
> > +void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
> > +{
> > +       adapter_add_device(adapter, dev);
> >  }
> >
> >  static void remove_whitelist_complete(uint8_t status, uint16_t length,
> > @@ -4743,76 +4764,23 @@ void adapter_whitelist_remove(struct btd_adapter *adapter, struct btd_device *de
> >                                 remove_whitelist_complete, adapter, NULL);
> >  }
> >
> > -static void add_device_complete(uint8_t status, uint16_t length,
> > -                                       const void *param, void *user_data)
> > -{
> > -       const struct mgmt_rp_add_device *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 Add Device 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,
> > -                       "Add Device complete for unknown device %s", addr);
> > -               return;
> > -       }
> > -
> > -       if (status != MGMT_STATUS_SUCCESS) {
> > -               btd_error(adapter->dev_id,
> > -                       "Failed to add device %s (%u): %s (0x%02x)",
> > -                       addr, rp->addr.type, mgmt_errstr(status), status);
> > -               adapter->connect_list = g_slist_remove(adapter->connect_list,
> > -                                                                       dev);
> > -               return;
> > -       }
> > -
> > -       DBG("%s (%u) added to kernel connect list", addr, rp->addr.type);
> > -}
> > -
> >  void adapter_auto_connect_add(struct btd_adapter *adapter,
> > -                                       struct btd_device *device)
> > +                             struct btd_device *device)
> >  {
> > -       struct mgmt_cp_add_device cp;
> > -       const bdaddr_t *bdaddr;
> > -       uint8_t bdaddr_type;
> > -       unsigned int id;
> > -
> > -       if (!kernel_conn_control)
> > -               return;
> > +       uint8_t bdaddr_type = btd_device_get_bdaddr_type(device);
> >
> >         if (g_slist_find(adapter->connect_list, device)) {
> >                 DBG("ignoring already added device %s",
> > -                                               device_get_path(device));
> > +                   device_get_path(device));
> >                 return;
> >         }
> >
> > -       bdaddr = device_get_address(device);
> > -       bdaddr_type = btd_device_get_bdaddr_type(device);
> > -
> >         if (bdaddr_type == BDADDR_BREDR) {
> > -               DBG("auto-connection feature is not avaiable for BR/EDR");
> > +               DBG("auto-connection feature is not available for BR/EDR");
> >                 return;
> >         }
> >
> > -       memset(&cp, 0, sizeof(cp));
> > -       bacpy(&cp.addr.bdaddr, bdaddr);
> > -       cp.addr.type = bdaddr_type;
> > -       cp.action = 0x02;
> > -
> > -       id = mgmt_send(adapter->mgmt, MGMT_OP_ADD_DEVICE,
> > -                       adapter->dev_id, sizeof(cp), &cp, add_device_complete,
> > -                       adapter, NULL);
> > -       if (id == 0)
> > +       if (adapter_add_device(adapter, device) == 0)
> >                 return;
> >
> >         adapter->connect_list = g_slist_append(adapter->connect_list, device);
> > diff --git a/src/adapter.h b/src/adapter.h
> > index d0a5253bd..a5a9e917a 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -213,6 +213,7 @@ int adapter_connect_list_add(struct btd_adapter *adapter,
> >                                         struct btd_device *device);
> >  void adapter_connect_list_remove(struct btd_adapter *adapter,
> >                                                 struct btd_device *device);
> > +int adapter_add_device(struct btd_adapter* adapter, struct btd_device* dev);
> >  void adapter_auto_connect_add(struct btd_adapter *adapter,
> >                                         struct btd_device *device);
> >  void adapter_auto_connect_remove(struct btd_adapter *adapter,
> > diff --git a/src/device.c b/src/device.c
> > index 69f98e488..c646a7fee 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -184,11 +184,15 @@ struct btd_device {
> >         uint8_t         conn_bdaddr_type;
> >         bdaddr_t        bdaddr;
> >         uint8_t         bdaddr_type;
> > +       uint8_t         mgmt_flags_mask;   /* Flags to update on add_device */
> > +       uint8_t         mgmt_flags_value; /* Flag values for add_device */
> >         char            *path;
> >         bool            bredr;
> >         bool            le;
> >         bool            pending_paired;         /* "Paired" waiting for SDP */
> >         bool            svc_refreshed;
> > +       bool            wake_support;           /* Profile supports wake */
> > +       bool            wake_allowed;           /* Can wake from suspend */
> >         GSList          *svc_callbacks;
> >         GSList          *eir_uuids;
> >         struct bt_ad    *ad;
> > @@ -415,6 +419,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", "WakeAllowed",
> > +                                                       device->wake_allowed);
> > +
> >         if (device->uuids) {
> >                 GSList *l;
> >                 int i;
> > @@ -1318,6 +1325,79 @@ dev_property_advertising_data_exist(const GDBusPropertyTable *property,
> >         return bt_ad_has_data(device->ad, NULL);
> >  }
> >
> > +bool device_get_wake_support(struct btd_device *device)
> > +{
> > +       return device->wake_support;
> > +}
> > +
> > +void device_set_wake_support(struct btd_device *device, bool wake_support)
> > +{
> > +       device->wake_support = wake_support;
> > +}
> > +
> > +bool device_get_wake_allowed(struct btd_device *device)
> > +{
> > +       return device->wake_allowed;
> > +}
> > +
> > +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed)
> > +{
> > +       device->mgmt_flags_mask |= DEVICE_FLAG_WAKEABLE;
> > +       if (wake_allowed)
> > +               device->mgmt_flags_value |= DEVICE_FLAG_WAKEABLE;
> > +       else
> > +               device->mgmt_flags_value &= ~DEVICE_FLAG_WAKEABLE;
> > +
> > +       adapter_add_device(device->adapter, device);
> > +}
> > +
> > +static gboolean
> > +dev_property_get_wake_allowed(const GDBusPropertyTable *property,
> > +                            DBusMessageIter *iter, void *data)
> > +{
> > +       struct btd_device *device = data;
> > +       dbus_bool_t wake_allowed =
> > +                       device_get_wake_allowed(device) ? TRUE : FALSE;
> > +
> > +       dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &wake_allowed);
> > +
> > +       return TRUE;
> > +}
> > +
> > +static void dev_property_set_wake_allowed(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;
> > +       }
> > +
> > +       if (device->temporary) {
> > +               g_dbus_pending_property_error(id,
> > +                                       ERROR_INTERFACE ".Unsupported",
> > +                                       "Cannot set property while temporary");
> > +               return;
> > +       }
> > +
> > +       dbus_message_iter_get_basic(value, &b);
> > +       device_set_wake_allowed(device, b);
> > +       g_dbus_pending_property_success(id);
> > +}
> > +
> > +static gboolean dev_property_wake_allowed_exist(
> > +               const GDBusPropertyTable *property, void *data)
> > +{
> > +       struct btd_device *device = data;
> > +
> > +       return device_get_wake_support(device) ? TRUE : FALSE;
> > +}
> > +
> >  static gboolean disconnect_all(gpointer user_data)
> >  {
> >         struct btd_device *device = user_data;
> > @@ -2779,6 +2859,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 },
> > +       { "WakeAllowed", "b", dev_property_get_wake_allowed,
> > +                               dev_property_set_wake_allowed,
> > +                               dev_property_wake_allowed_exist },
> >         { }
> >  };
> >
> > @@ -3030,6 +3113,7 @@ static void load_info(struct btd_device *device, const char *local,
> >         char *str;
> >         gboolean store_needed = FALSE;
> >         gboolean blocked;
> > +       gboolean wake_allowed;
> >         char **uuids;
> >         int source, vendor, product, version;
> >         char **techno, **t;
> > @@ -3141,6 +3225,14 @@ next:
> >                 btd_device_set_pnpid(device, source, vendor, product, version);
> >         }
> >
> > +       /* Mark wake allowed */
> > +       wake_allowed = g_key_file_get_boolean(key_file, "General",
> > +                                             "WakeAllowed", NULL);
> > +       if (wake_allowed) {
> > +               device->mgmt_flags_mask |= DEVICE_FLAG_WAKEABLE;
> > +               device->mgmt_flags_value |= DEVICE_FLAG_WAKEABLE;
> > +       }
> > +
> >         if (store_needed)
> >                 store_device_info(device);
> >  }
> > @@ -3959,6 +4051,39 @@ char *btd_device_get_storage_path(struct btd_device *device,
> >                                 dstaddr, filename);
> >  }
> >
> > +void device_add_complete(struct btd_device *device)
> > +{
> > +       bool flag = false;
> > +
> > +       /* No flags updated */
> > +       if (!device->mgmt_flags_mask)
> > +               return;
> > +
> > +       if (device->mgmt_flags_mask & DEVICE_FLAG_WAKEABLE)
> > +       {
> > +               flag = device->mgmt_flags_value & DEVICE_FLAG_WAKEABLE;
> > +               device->wake_allowed = flag;
> > +
> > +               store_device_info(device);
> > +               g_dbus_emit_property_changed(dbus_conn, device->path,
> > +                                            DEVICE_INTERFACE, "WakeAllowed");
> > +       }
> > +
> > +       /* Clear mask and values for next completion */
> > +       device->mgmt_flags_mask = 0;
> > +       device->mgmt_flags_value = 0;
> > +}
> > +
> > +uint8_t device_get_flags_mask(struct btd_device *device)
> > +{
> > +       return device->mgmt_flags_mask;
> > +}
> > +
> > +uint8_t device_get_flags_value(struct btd_device *device)
> > +{
> > +       return device->mgmt_flags_value;
> > +}
> > +
> >  void btd_device_device_set_name(struct btd_device *device, const char *name)
> >  {
> >         if (strncmp(name, device->name, MAX_NAME_LENGTH) == 0)
> > diff --git a/src/device.h b/src/device.h
> > index 06b100499..85e33f1cd 100644
> > --- a/src/device.h
> > +++ b/src/device.h
> > @@ -33,6 +33,10 @@ struct btd_device *device_create_from_storage(struct btd_adapter *adapter,
> >  char *btd_device_get_storage_path(struct btd_device *device,
> >                                 const char *filename);
> >
> > +void device_add_complete(struct btd_device *device);
> > +uint8_t device_get_flags_mask(struct btd_device *device);
> > +uint8_t device_get_flags_value(struct btd_device *device);
> > +
> >  void btd_device_device_set_name(struct btd_device *device, const char *name);
> >  void device_store_cached_name(struct btd_device *dev, const char *name);
> >  void device_get_name(struct btd_device *device, char *name, size_t len);
> > @@ -139,6 +143,10 @@ 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_support(struct btd_device *device);
> > +void device_set_wake_support(struct btd_device *device, bool wake_support);
> > +bool device_get_wake_allowed(struct btd_device *device);
> > +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed);
> >
> >  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
> >                                         void *user_data);
> > --
> > 2.25.1.696.g5e7596f4ac-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Re: [BlueZ PATCH v4 1/5] mgmt: Update docs for Add Device

[Marcel Holtmann]

Hi Abhishek,

> Update the docs for Add Device with information on flags mask and value.
> Add information on the Wakeable flag that can be set to allow the device
> to wake the system from suspend.
> ---
> 
> Changes in v4:
> * Newly added support in Add Device for flags
> 
> Changes in v3: None
> Changes in v2: None
> 
> doc/mgmt-api.txt | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
> 
> diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> index 27a41f334..e99c23710 100644
> --- a/doc/mgmt-api.txt
> +++ b/doc/mgmt-api.txt
> @@ -1997,6 +1997,8 @@ Add Device Command
> 	Command Parameters:	Address (6 Octets)
> 				Address_Type (1 Octet)
> 				Action (1 Octet)
> +				Flags Mask (1 Octet)
> +				Flags Value (1 Octet)

so we can actually not do that. This is not backwards compatible since the current kernel version have a strict size check for the command parameter size of Add Device.

Is there a problem to just introduce action 3 as I mentioned before?

Regards

Marcel

Re: [BlueZ PATCH v4 1/5] mgmt: Update docs for Add Device

[Abhishek Pandit-Subedi]

On Wed, Mar 25, 2020 at 2:25 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > Update the docs for Add Device with information on flags mask and value.
> > Add information on the Wakeable flag that can be set to allow the device
> > to wake the system from suspend.
> > ---
> >
> > Changes in v4:
> > * Newly added support in Add Device for flags
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> > doc/mgmt-api.txt | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> > index 27a41f334..e99c23710 100644
> > --- a/doc/mgmt-api.txt
> > +++ b/doc/mgmt-api.txt
> > @@ -1997,6 +1997,8 @@ Add Device Command
> >       Command Parameters:     Address (6 Octets)
> >                               Address_Type (1 Octet)
> >                               Action (1 Octet)
> > +                             Flags Mask (1 Octet)
> > +                             Flags Value (1 Octet)
>
> so we can actually not do that. This is not backwards compatible since the current kernel version have a strict size check for the command parameter size of Add Device.
>
> Is there a problem to just introduce action 3 as I mentioned before?

How does one mark the device as not wake capable? (if 0x3 means mark
the device as wake capable, should we add 0x4 meaning mark the device
as not wake capable)
Does a device need to have been already added to be marked wake
capable? (Answer should probably be no)

If you're ok with adding new actions 0x3 and 0x4 and making them not
imply 0x1 as well (LE always sets 0x2 so overwriting the value doesn't
make sense), I think that will work.

>
> Regards
>
> Marcel
>