linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable
@ 2020-06-22 23:40 Abhishek Pandit-Subedi
  2020-06-22 23:40 ` [BlueZ PATCH v5 1/6] mgmt: Add mgmt op and events for device flags Abhishek Pandit-Subedi
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-22 23:40 UTC (permalink / raw)
  To: luiz.dentz, linux-bluetooth
  Cc: alainm, marcel, chromeos-bluetooth-upstreaming, Abhishek Pandit-Subedi


Hi Luiz,

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.

Accompanying kernel change: https://patchwork.kernel.org/patch/11609999/

In order to set this flag, Bluez must call Set Device Flags with the
Remote Wakeup bit set. This change was tested with the accompanying
kernel changes on v5.4 with both manual tests and automated tests.

Here's the tests that I ran manually:
  - Pair new HID device and confirm it has Wake Allowed to True
  (default). Verify device can be woken from suspend with device.
  - Restart bluetoothd, confirm Device Flags Changed event after Add
  Device has no Remote Wakeup flag and Set Device Flags is called
  afterwards to restore it. Verify wake from suspend still works.
  - Disable Wake allowed via dbus and confirm wake from suspend no
  longer works.
  - Restart bluetoothd and confirm Wake Allowed is still false. Verify
  wake from suspend does not work.

Thanks
Abhishek

Changes in v5:
- Use device_flags mgmt op
* Decode device flags
* Refactor to use set_wake_flags and respond to device flags changed
* Add wake_override so we can keep track of user/profile configuration
  vs what is currently active
* Only call device_set_wake_support

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
* 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 (6):
  mgmt: Add mgmt op and events for device flags
  monitor: Decode device flags mgmt ops and event
  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 ++
 lib/mgmt.h              |  33 +++++++
 monitor/packet.c        |  68 +++++++++++++++
 profiles/input/device.c |   1 +
 profiles/input/hog.c    |   1 +
 src/adapter.c           |  98 +++++++++++++++++++++
 src/adapter.h           |   3 +-
 src/device.c            | 184 ++++++++++++++++++++++++++++++++++++++++
 src/device.h            |   9 ++
 10 files changed, 402 insertions(+), 1 deletion(-)

-- 
2.27.0.111.gc72c7da667-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [BlueZ PATCH v5 1/6] mgmt: Add mgmt op and events for device flags
  2020-06-22 23:40 [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable Abhishek Pandit-Subedi
@ 2020-06-22 23:40 ` Abhishek Pandit-Subedi
  2020-06-22 23:49   ` device: Allow devices to be marked as wake capable bluez.test.bot
  2020-06-22 23:40 ` [BlueZ PATCH v5 2/6] monitor: Decode device flags mgmt ops and event Abhishek Pandit-Subedi
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-22 23:40 UTC (permalink / raw)
  To: luiz.dentz, linux-bluetooth
  Cc: alainm, marcel, chromeos-bluetooth-upstreaming, Abhishek Pandit-Subedi

Add Get Device Flags, Set Device Flags and Device Flags Changed.

---

Changes in v5:
- Use device_flags mgmt op

Changes in v4: None
Changes in v3: None
Changes in v2: None

 lib/mgmt.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index fad1f3dfe..525c4dd62 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -654,6 +654,27 @@ struct mgmt_cp_set_default_runtime_config {
 	uint8_t parameters[0]; /* mgmt_tlv */
 } __packed;
 
+#define MGMT_OP_GET_DEVICE_FLAGS	0x004F
+#define MGMT_GET_DEVICE_FLAGS_SIZE	7
+struct mgmt_cp_get_device_flags {
+	struct mgmt_addr_info addr;
+} __packed;
+struct mgmt_rp_get_device_flags {
+	struct mgmt_addr_info addr;
+	uint32_t supported_flags;
+	uint32_t current_flags;
+} __packed;
+
+#define MGMT_OP_SET_DEVICE_FLAGS	0x0050
+#define MGMT_SET_DEVICE_FLAGS_SIZE	11
+struct mgmt_cp_set_device_flags {
+	struct mgmt_addr_info addr;
+	uint32_t current_flags;
+} __packed;
+struct mgmt_rp_set_device_flags {
+	struct mgmt_addr_info addr;
+} __packed;
+
 #define MGMT_ADV_MONITOR_FEATURE_MASK_OR_PATTERNS	(1 << 0)
 
 #define MGMT_OP_READ_ADV_MONITOR_FEATURES	0x0051
@@ -919,6 +940,13 @@ struct mgmt_ev_exp_feature_changed {
 	uint32_t flags;
 } __packed;
 
+#define MGMT_EV_DEVICE_FLAGS_CHANGED		0x002a
+struct mgmt_ev_device_flags_changed {
+	struct mgmt_addr_info addr;
+	uint32_t supported_flags;
+	uint32_t current_flags;
+} __packed;
+
 #define MGMT_EV_ADV_MONITOR_ADDED	0x002b
 struct mgmt_ev_adv_monitor_added {
 	uint16_t monitor_handle;
@@ -1007,6 +1035,8 @@ static const char *mgmt_op[] = {
 	"Set Experimental Feature",
 	"Read Default System Configuration",
 	"Set Default System Configuration",
+	"Get Device Flags",
+	"Set Device Flags",				/* 0x0050 */
 	"Read Advertisement Monitor Features",
 	"Add Advertisement Patterns Monitor",
 	"Remove Advertisement Monitor",
@@ -1053,6 +1083,7 @@ static const char *mgmt_ev[] = {
 	"Extended Controller Information Changed",
 	"PHY Configuration Changed",
 	"Experimental Feature Changed",
+	"Device Flags Changed",
 	"Advertisement Monitor Added",			/* 0x002b */
 	"Advertisement Monitor Removed",
 };
-- 
2.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [BlueZ PATCH v5 2/6] monitor: Decode device flags mgmt ops and event
  2020-06-22 23:40 [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable Abhishek Pandit-Subedi
  2020-06-22 23:40 ` [BlueZ PATCH v5 1/6] mgmt: Add mgmt op and events for device flags Abhishek Pandit-Subedi
@ 2020-06-22 23:40 ` Abhishek Pandit-Subedi
  2020-06-22 23:47   ` [BlueZ,v5,2/6] " bluez.test.bot
  2020-06-22 23:40 ` [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed Abhishek Pandit-Subedi
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-22 23:40 UTC (permalink / raw)
  To: luiz.dentz, linux-bluetooth
  Cc: alainm, marcel, chromeos-bluetooth-upstreaming, Abhishek Pandit-Subedi

Add support for Get Device Flags and Set Device Flags mgmt operations
and Device Flags Changed mgmt event.

Sample trace:
@ MGMT Command: Set Device Flags (0x0050) plen 11     {0x0002} [hci0]
        LE Address: CD:F3:CD:13:C5:91 (Static)
        Current Flags: 0x00000000
@ MGMT Event: Device Flags Changed (0x002a) plen 15   {0x0001} [hci0]
        LE Address: CD:F3:CD:13:C5:91 (Static)
        Supported Flags: 0x00000001
          Remote Wakeup
        Current Flags: 0x00000000
@ MGMT Event: Device Flags Changed (0x002a) plen 15   {0x0004} [hci0]
        LE Address: CD:F3:CD:13:C5:91 (Static)
        Supported Flags: 0x00000001
          Remote Wakeup
        Current Flags: 0x00000000
@ MGMT Event: Device Flags Changed (0x002a) plen 15   {0x0003} [hci0]
        LE Address: CD:F3:CD:13:C5:91 (Static)
        Supported Flags: 0x00000001
          Remote Wakeup
        Current Flags: 0x00000000
@ MGMT Event: Command Complete (0x0001) plen 10       {0x0002} [hci0]
      Set Device Flags (0x0050) plen 7
        Status: Success (0x00)
        LE Address: CD:F3:CD:13:C5:91 (Static)

---

Changes in v5:
* Decode device flags

Changes in v4: None
Changes in v3: None
Changes in v2: None

 monitor/packet.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/monitor/packet.c b/monitor/packet.c
index 3b9c06512..6280dfff0 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -100,6 +100,7 @@
 #define COLOR_UNKNOWN_EXP_FEATURE_FLAG	COLOR_WHITE_BG
 #define COLOR_UNKNOWN_ADV_FLAG		COLOR_WHITE_BG
 #define COLOR_UNKNOWN_PHY		COLOR_WHITE_BG
+#define COLOR_UNKNOWN_ADDED_DEVICE_FLAG	COLOR_WHITE_BG
 
 #define COLOR_PHY_PACKET		COLOR_BLUE
 
@@ -13099,6 +13100,54 @@ static void mgmt_set_exp_feature_rsp(const void *data, uint16_t size)
 	mgmt_print_exp_feature(data);
 }
 
+static const struct bitfield_data mgmt_added_device_flags_table[] = {
+	{ 0, "Remote Wakeup"	},
+	{ }
+};
+
+static void mgmt_print_added_device_flags(char *label, uint32_t flags)
+{
+	uint32_t mask;
+
+	print_field("%s: 0x%8.8x", label, flags);
+	mask = print_bitfield(2, flags, mgmt_added_device_flags_table);
+	if (mask)
+		print_text(COLOR_UNKNOWN_ADDED_DEVICE_FLAG,
+			   "  Unknown Flags (0x%8.8x)", mask);
+}
+
+static void mgmt_get_device_flags_cmd(const void *data, uint16_t size)
+{
+	uint8_t type = get_u8(data + 6);
+	mgmt_print_address(data, type);
+}
+
+static void mgmt_get_device_flags_rsp(const void *data, uint16_t size)
+{
+	uint8_t type = get_u8(data + 6);
+	uint32_t supported_flags = get_le32(data + 7);
+	uint32_t current_flags = get_le32(data + 11);
+
+	mgmt_print_address(data, type);
+	mgmt_print_added_device_flags("Supported Flags", supported_flags);
+	mgmt_print_added_device_flags("Current Flags", current_flags);
+}
+
+static void mgmt_set_device_flags_cmd(const void *data, uint16_t size)
+{
+	uint8_t type = get_u8(data + 6);
+	uint32_t current_flags = get_le32(data + 7);
+
+	mgmt_print_address(data, type);
+	mgmt_print_added_device_flags("Current Flags", current_flags);
+}
+
+static void mgmt_set_device_flags_rsp(const void *data, uint16_t size)
+{
+	uint8_t type = get_u8(data + 6);
+	mgmt_print_address(data, type);
+}
+
 struct mgmt_data {
 	uint16_t opcode;
 	const char *str;
@@ -13324,6 +13373,12 @@ static const struct mgmt_data mgmt_command_table[] = {
 	{ 0x004a, "Set Experimental Feature",
 				mgmt_set_exp_feature_cmd, 17, true,
 				mgmt_set_exp_feature_rsp, 20, true },
+	{ 0x004f, "Get Device Flags",
+				mgmt_get_device_flags_cmd, 7, true,
+				mgmt_get_device_flags_rsp, 15, true},
+	{ 0x0050, "Set Device Flags",
+				mgmt_set_device_flags_cmd, 11, true,
+				mgmt_set_device_flags_rsp, 7, true},
 	{ }
 };
 
@@ -13714,6 +13769,17 @@ static void mgmt_exp_feature_changed_evt(const void *data, uint16_t size)
 	mgmt_print_exp_feature(data);
 }
 
+static void mgmt_device_flags_changed_evt(const void *data, uint16_t size)
+{
+	uint8_t type = get_u8(data + 6);
+	uint32_t supported_flags = get_le32(data + 7);
+	uint32_t current_flags = get_le32(data + 11);
+
+	mgmt_print_address(data, type);
+	mgmt_print_added_device_flags("Supported Flags", supported_flags);
+	mgmt_print_added_device_flags("Current Flags", current_flags);
+}
+
 static const struct mgmt_data mgmt_event_table[] = {
 	{ 0x0001, "Command Complete",
 			mgmt_command_complete_evt, 3, false },
@@ -13793,6 +13859,8 @@ static const struct mgmt_data mgmt_event_table[] = {
 			mgmt_phy_changed_evt, 4, true },
 	{ 0x0027, "Experimental Feature Changed",
 			mgmt_exp_feature_changed_evt, 20, true },
+	{ 0x002a, "Device Flags Changed",
+			mgmt_device_flags_changed_evt, 15, true },
 	{ }
 };
 
-- 
2.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed
  2020-06-22 23:40 [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable Abhishek Pandit-Subedi
  2020-06-22 23:40 ` [BlueZ PATCH v5 1/6] mgmt: Add mgmt op and events for device flags Abhishek Pandit-Subedi
  2020-06-22 23:40 ` [BlueZ PATCH v5 2/6] monitor: Decode device flags mgmt ops and event Abhishek Pandit-Subedi
@ 2020-06-22 23:40 ` Abhishek Pandit-Subedi
  2020-06-22 23:47   ` [BlueZ,v5,3/6] " bluez.test.bot
  2020-06-23  0:10   ` [BlueZ PATCH v5 3/6] " Luiz Augusto von Dentz
  2020-06-22 23:40 ` [BlueZ PATCH v5 4/6] client: Display wake allowed property with info Abhishek Pandit-Subedi
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-22 23:40 UTC (permalink / raw)
  To: luiz.dentz, linux-bluetooth
  Cc: alainm, marcel, chromeos-bluetooth-upstreaming, 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 set device flags 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.

If a device is connecting for the first time, it will be marked
WakeAllowed if the profile supports it. On subsequent reloads of bluez,
the stored setting "WakeAllowed" will be used to override any other
setting.

---

Changes in v5:
* Refactor to use set_wake_flags and respond to device flags changed
* Add wake_override so we can keep track of user/profile configuration
  vs what is currently active

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    |   2 +
 src/adapter.c |  98 +++++++++++++++++++++++++++
 src/adapter.h |   3 +-
 src/device.c  | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.h  |   9 +++
 5 files changed, 295 insertions(+), 1 deletion(-)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index 525c4dd62..a800bcab4 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -665,6 +665,8 @@ struct mgmt_rp_get_device_flags {
 	uint32_t current_flags;
 } __packed;
 
+#define DEVICE_FLAG_REMOTE_WAKEUP	(1 << 0)
+
 #define MGMT_OP_SET_DEVICE_FLAGS	0x0050
 #define MGMT_SET_DEVICE_FLAGS_SIZE	11
 struct mgmt_cp_set_device_flags {
diff --git a/src/adapter.c b/src/adapter.c
index 9ce351893..0ab1f85a8 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -5102,6 +5102,99 @@ void adapter_auto_connect_add(struct btd_adapter *adapter,
 	adapter->connect_list = g_slist_append(adapter->connect_list, device);
 }
 
+static void set_device_wakeable_complete(uint8_t status, uint16_t length,
+					 const void *param, void *user_data)
+{
+	const struct mgmt_rp_set_device_flags *rp = param;
+	struct btd_adapter *adapter = user_data;
+	struct btd_device *dev;
+	char addr[18];
+
+	if (status != MGMT_STATUS_SUCCESS) {
+		btd_error(adapter->dev_id, "Set device flags return status: %s",
+			  mgmt_errstr(status));
+	}
+
+	if (length < sizeof(*rp)) {
+		btd_error(adapter->dev_id,
+			  "Too small Set Device Flags complete event: %d",
+			  length);
+		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 Device Flags complete for unknown device %s", addr);
+		return;
+	}
+
+	if (status != MGMT_STATUS_SUCCESS) {
+		btd_error(adapter->dev_id,
+			"Failed to configure wakeable %s (%u): %s (0x%02x)",
+			addr, rp->addr.type, mgmt_errstr(status), status);
+		return;
+	}
+
+	device_set_wake_allowed_complete(dev);
+}
+
+void adapter_set_device_wakeable(struct btd_adapter *adapter,
+				 struct btd_device *device, bool wakeable)
+{
+	struct mgmt_cp_set_device_flags cp;
+	const bdaddr_t *bdaddr;
+	uint8_t bdaddr_type;
+
+	if (!kernel_conn_control)
+		return;
+
+	bdaddr = device_get_address(device);
+	bdaddr_type = btd_device_get_bdaddr_type(device);
+
+	memset(&cp, 0, sizeof(cp));
+	bacpy(&cp.addr.bdaddr, bdaddr);
+	cp.addr.type = bdaddr_type;
+	cp.current_flags = btd_device_get_current_flags(device);
+	if (wakeable)
+		cp.current_flags |= DEVICE_FLAG_REMOTE_WAKEUP;
+	else
+		cp.current_flags &= ~DEVICE_FLAG_REMOTE_WAKEUP;
+
+	mgmt_send(adapter->mgmt, MGMT_OP_SET_DEVICE_FLAGS, adapter->dev_id,
+		  sizeof(cp), &cp, set_device_wakeable_complete, adapter, NULL);
+}
+
+static void device_flags_changed_callback(uint16_t index, uint16_t length,
+					  const void *param, void *user_data)
+{
+	const struct mgmt_ev_device_flags_changed *ev = param;
+	struct btd_adapter *adapter = user_data;
+	struct btd_device *dev;
+	char addr[18];
+
+	if (length < sizeof(*ev)) {
+		btd_error(adapter->dev_id,
+			  "Too small Device Flags Changed event: %d",
+			  length);
+		return;
+	}
+
+	ba2str(&ev->addr.bdaddr, addr);
+
+	dev = btd_adapter_find_device(adapter, &ev->addr.bdaddr, ev->addr.type);
+	if (!dev) {
+		btd_error(adapter->dev_id,
+			"Device Flags Changed for unknown device %s", addr);
+		return;
+	}
+
+	btd_device_flags_changed(dev, ev->supported_flags, ev->current_flags);
+}
+
+
 static void remove_device_complete(uint8_t status, uint16_t length,
 					const void *param, void *user_data)
 {
@@ -8544,6 +8637,11 @@ static int adapter_register(struct btd_adapter *adapter)
 							adapter, NULL);
 
 load:
+	mgmt_register(adapter->mgmt, MGMT_EV_DEVICE_FLAGS_CHANGED,
+						adapter->dev_id,
+						device_flags_changed_callback,
+						adapter, NULL);
+
 	load_config(adapter);
 	fix_storage(adapter);
 	load_drivers(adapter);
diff --git a/src/adapter.h b/src/adapter.h
index d0a5253bd..f8ac20261 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -213,6 +213,8 @@ 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);
+void adapter_set_device_wakeable(struct btd_adapter *adapter,
+				 struct btd_device *dev, bool wakeable);
 void adapter_auto_connect_add(struct btd_adapter *adapter,
 					struct btd_device *device);
 void adapter_auto_connect_remove(struct btd_adapter *adapter,
@@ -231,4 +233,3 @@ void btd_adapter_for_each_device(struct btd_adapter *adapter,
 			void *data);
 
 bool btd_le_connect_before_pairing(void);
-
diff --git a/src/device.c b/src/device.c
index 7b0eb256e..cd25111b8 100644
--- a/src/device.c
+++ b/src/device.c
@@ -177,6 +177,12 @@ struct csrk_info {
 	uint32_t counter;
 };
 
+typedef enum {
+	WAKE_FLAG_DEFAULT = 0,
+	WAKE_FLAG_ENABLED,
+	WAKE_FLAG_DISABLED,
+} wake_flag_t;
+
 struct btd_device {
 	int ref_count;
 
@@ -189,6 +195,20 @@ struct btd_device {
 	bool		le;
 	bool		pending_paired;		/* "Paired" waiting for SDP */
 	bool		svc_refreshed;
+
+	/* Manage whether this device can wake the system from suspend.
+	 * - wake_support: Requires a profile that supports wake (i.e. HID)
+	 * - wake_allowed: Is wake currently allowed?
+	 * - pending_wake_allowed - Wake flag sent via set_device_flags
+	 * - wake_override - User configured wake setting
+	 */
+	bool		wake_support;
+	bool		wake_allowed;
+	bool		pending_wake_allowed;
+	wake_flag_t	wake_override;
+
+	uint32_t	supported_flags;
+	uint32_t	current_flags;
 	GSList		*svc_callbacks;
 	GSList		*eir_uuids;
 	struct bt_ad	*ad;
@@ -415,6 +435,12 @@ static gboolean store_device_info_cb(gpointer user_data)
 	g_key_file_set_boolean(key_file, "General", "Blocked",
 							device->blocked);
 
+	if (device->wake_override != WAKE_FLAG_DEFAULT) {
+		g_key_file_set_boolean(key_file, "General", "WakeAllowed",
+				       device->wake_override ==
+					       WAKE_FLAG_ENABLED);
+	}
+
 	if (device->uuids) {
 		GSList *l;
 		int i;
@@ -1318,6 +1344,105 @@ 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;
+
+	/* If wake configuration has not been made yet, set the initial
+	 * configuration.
+	 */
+	if (device->wake_override == WAKE_FLAG_DEFAULT) {
+		device_set_wake_override(device, wake_support);
+		device_set_wake_allowed(device, wake_support);
+	}
+}
+
+bool device_get_wake_allowed(struct btd_device *device)
+{
+	return device->wake_allowed;
+}
+
+void device_set_wake_override(struct btd_device *device, bool wake_override)
+{
+	if (wake_override) {
+		device->wake_override = WAKE_FLAG_ENABLED;
+		device->current_flags |= DEVICE_FLAG_REMOTE_WAKEUP;
+	} else {
+		device->wake_override = WAKE_FLAG_DISABLED;
+		device->current_flags &= ~DEVICE_FLAG_REMOTE_WAKEUP;
+	}
+}
+
+void device_set_wake_allowed(struct btd_device *device, bool wake_allowed)
+{
+	device->pending_wake_allowed = wake_allowed;
+	adapter_set_device_wakeable(device_get_adapter(device), device,
+				    wake_allowed);
+}
+
+void device_set_wake_allowed_complete(struct btd_device *device)
+{
+	device->wake_allowed = device->pending_wake_allowed;
+	g_dbus_emit_property_changed(dbus_conn, device->path,
+					DEVICE_INTERFACE, "WakeAllowed");
+
+	store_device_info(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);
+
+	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_override(device, 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);
+}
+
+
 static gboolean disconnect_all(gpointer user_data)
 {
 	struct btd_device *device = user_data;
@@ -2790,6 +2915,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 },
 	{ }
 };
 
@@ -3038,9 +3166,11 @@ static void convert_info(struct btd_device *device, GKeyFile *key_file)
 static void load_info(struct btd_device *device, const char *local,
 			const char *peer, GKeyFile *key_file)
 {
+	GError *gerr = NULL;
 	char *str;
 	gboolean store_needed = FALSE;
 	gboolean blocked;
+	gboolean wake_allowed;
 	char **uuids;
 	int source, vendor, product, version;
 	char **techno, **t;
@@ -3152,6 +3282,17 @@ next:
 		btd_device_set_pnpid(device, source, vendor, product, version);
 	}
 
+	/* Wake allowed is only configured and stored if user changed it.
+	 * Otherwise, we enable if profile supports it. */
+	wake_allowed = g_key_file_get_boolean(key_file, "General",
+					      "WakeAllowed", &gerr);
+	if (!gerr) {
+		device_set_wake_override(device, wake_allowed);
+	} else {
+		g_error_free(gerr);
+		gerr = NULL;
+	}
+
 	if (store_needed)
 		store_device_info(device);
 }
@@ -6558,6 +6699,49 @@ void btd_device_set_pnpid(struct btd_device *device, uint16_t source,
 	store_device_info(device);
 }
 
+uint32_t btd_device_get_current_flags(struct btd_device *dev) {
+	return dev->current_flags;
+}
+
+/* This event is sent immediately after add device on all mgmt sockets.
+ * Afterwards, it is only sent to mgmt sockets other than the one which called
+ * set_device_flags.
+ */
+void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
+			      uint32_t current_flags)
+{
+	int i;
+	const uint32_t changed_flags = dev->current_flags ^ current_flags;
+	bool flag_value;
+
+	dev->supported_flags = supported_flags;
+	dev->current_flags = current_flags;
+
+	if (!changed_flags)
+		return;
+
+	if (changed_flags & DEVICE_FLAG_REMOTE_WAKEUP) {
+		flag_value = !!(current_flags & DEVICE_FLAG_REMOTE_WAKEUP);
+		dev->pending_wake_allowed = flag_value;
+
+		/* If an override exists and doesn't match the current state,
+		 * apply it. This logic will run after Add Device only and will
+		 * enable wake for previously paired devices.
+		 */
+		if (dev->wake_override != WAKE_FLAG_DEFAULT) {
+			bool wake_allowed =
+				dev->wake_override == WAKE_FLAG_ENABLED;
+			if (flag_value != wake_allowed) {
+				device_set_wake_allowed(dev, wake_allowed);
+			} else {
+				device_set_wake_allowed_complete(dev);
+			}
+		} else {
+			device_set_wake_allowed_complete(dev);
+		}
+	}
+}
+
 static void service_state_changed(struct btd_service *service,
 						btd_service_state_t old_state,
 						btd_service_state_t new_state,
diff --git a/src/device.h b/src/device.h
index 06b100499..104e3f1dd 100644
--- a/src/device.h
+++ b/src/device.h
@@ -33,6 +33,7 @@ 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 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 +140,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);
+void device_set_wake_support(struct btd_device *device, bool wake_support);
+void device_set_wake_override(struct btd_device *device, bool wake_override);
+void device_set_wake_allowed(struct btd_device *device, bool wake_allowed);
+void device_set_wake_allowed_complete(struct btd_device *device);
 
 typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
 					void *user_data);
@@ -176,5 +181,9 @@ struct btd_service *btd_device_get_service(struct btd_device *dev,
 int device_discover_services(struct btd_device *device);
 int btd_device_connect_services(struct btd_device *dev, GSList *services);
 
+uint32_t btd_device_get_current_flags(struct btd_device *dev);
+void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
+			      uint32_t current_flags);
+
 void btd_device_init(void);
 void btd_device_cleanup(void);
-- 
2.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [BlueZ PATCH v5 4/6] client: Display wake allowed property with info
  2020-06-22 23:40 [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable Abhishek Pandit-Subedi
                   ` (2 preceding siblings ...)
  2020-06-22 23:40 ` [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed Abhishek Pandit-Subedi
@ 2020-06-22 23:40 ` Abhishek Pandit-Subedi
  2020-06-22 23:40 ` [BlueZ PATCH v5 5/6] doc/device-api: Add WakeAllowed Abhishek Pandit-Subedi
  2020-06-22 23:40 ` [BlueZ PATCH v5 6/6] input: Make HID devices support wake Abhishek Pandit-Subedi
  5 siblings, 0 replies; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-22 23:40 UTC (permalink / raw)
  To: luiz.dentz, linux-bluetooth
  Cc: alainm, marcel, chromeos-bluetooth-upstreaming, Abhishek Pandit-Subedi

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

---

Changes in v5: None
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.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [BlueZ PATCH v5 5/6] doc/device-api: Add WakeAllowed
  2020-06-22 23:40 [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable Abhishek Pandit-Subedi
                   ` (3 preceding siblings ...)
  2020-06-22 23:40 ` [BlueZ PATCH v5 4/6] client: Display wake allowed property with info Abhishek Pandit-Subedi
@ 2020-06-22 23:40 ` Abhishek Pandit-Subedi
  2020-06-22 23:40 ` [BlueZ PATCH v5 6/6] input: Make HID devices support wake Abhishek Pandit-Subedi
  5 siblings, 0 replies; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-22 23:40 UTC (permalink / raw)
  To: luiz.dentz, linux-bluetooth
  Cc: alainm, marcel, chromeos-bluetooth-upstreaming, Abhishek Pandit-Subedi

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

---

Changes in v5: None
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..4e824d2de 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 from system suspend.
+
 		string Alias [readwrite]
 
 			The name alias for the remote device. The alias can
-- 
2.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [BlueZ PATCH v5 6/6] input: Make HID devices support wake
  2020-06-22 23:40 [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable Abhishek Pandit-Subedi
                   ` (4 preceding siblings ...)
  2020-06-22 23:40 ` [BlueZ PATCH v5 5/6] doc/device-api: Add WakeAllowed Abhishek Pandit-Subedi
@ 2020-06-22 23:40 ` Abhishek Pandit-Subedi
  5 siblings, 0 replies; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-22 23:40 UTC (permalink / raw)
  To: luiz.dentz, linux-bluetooth
  Cc: alainm, marcel, chromeos-bluetooth-upstreaming, Abhishek Pandit-Subedi

HID devices can wake the host from a suspended state. Mark the profiles
to support wake when they are accepted. If the device hasn't already
been configured with a Wake Allowed configuration, it will default to
yes when the profile is accepted.

---

Changes in v5:
* Only call device_set_wake_support

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 d3724ed54..2dc2ecab2 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1409,6 +1409,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 9335b7e8b..130f696a9 100644
--- a/profiles/input/hog.c
+++ b/profiles/input/hog.c
@@ -166,6 +166,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.27.0.111.gc72c7da667-goog


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* RE: [BlueZ,v5,2/6] monitor: Decode device flags mgmt ops and event
  2020-06-22 23:40 ` [BlueZ PATCH v5 2/6] monitor: Decode device flags mgmt ops and event Abhishek Pandit-Subedi
@ 2020-06-22 23:47   ` bluez.test.bot
  0 siblings, 0 replies; 12+ messages in thread
From: bluez.test.bot @ 2020-06-22 23:47 UTC (permalink / raw)
  To: linux-bluetooth, abhishekpandit

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]


This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkpatch Failed

Outputs:
WARNING:LINE_SPACING: Missing a blank line after declarations
#68: FILE: monitor/packet.c:13122:
+	uint8_t type = get_u8(data + 6);
+	mgmt_print_address(data, type);

WARNING:LINE_SPACING: Missing a blank line after declarations
#94: FILE: monitor/packet.c:13148:
+	uint8_t type = get_u8(data + 6);
+	mgmt_print_address(data, type);

- total: 0 errors, 2 warnings, 98 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.



---
Regards,
Linux Bluetooth

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [BlueZ,v5,3/6] device: Support marking a device with wake allowed
  2020-06-22 23:40 ` [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed Abhishek Pandit-Subedi
@ 2020-06-22 23:47   ` bluez.test.bot
  2020-06-23  0:10   ` [BlueZ PATCH v5 3/6] " Luiz Augusto von Dentz
  1 sibling, 0 replies; 12+ messages in thread
From: bluez.test.bot @ 2020-06-22 23:47 UTC (permalink / raw)
  To: linux-bluetooth, abhishekpandit

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]


This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkpatch Failed

Outputs:
WARNING:LONG_LINE: line over 80 characters
#66: FILE: src/adapter.c:5130:
+			"Set Device Flags complete for unknown device %s", addr);

WARNING:NEW_TYPEDEFS: do not add new typedefs
#175: FILE: src/device.c:180:
+typedef enum {

WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#351: FILE: src/device.c:3286:
+	 * Otherwise, we enable if profile supports it. */

ERROR:OPEN_BRACE: open brace '{' following function definitions go on the next line
#368: FILE: src/device.c:6702:
+uint32_t btd_device_get_current_flags(struct btd_device *dev) {

WARNING:BRACES: braces {} are not necessary for any arm of this statement
#400: FILE: src/device.c:6734:
+			if (flag_value != wake_allowed) {
[...]
+			} else {
[...]

- total: 1 errors, 4 warnings, 390 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.



---
Regards,
Linux Bluetooth

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: device: Allow devices to be marked as wake capable
  2020-06-22 23:40 ` [BlueZ PATCH v5 1/6] mgmt: Add mgmt op and events for device flags Abhishek Pandit-Subedi
@ 2020-06-22 23:49   ` bluez.test.bot
  0 siblings, 0 replies; 12+ messages in thread
From: bluez.test.bot @ 2020-06-22 23:49 UTC (permalink / raw)
  To: linux-bluetooth, abhishekpandit

[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]


This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While we are preparing for reviewing the patches, we found the following
issue/warning.

Test Result:
checkbuild Failed

Outputs:
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
ar: `u' modifier ignored since `D' is the default (see `U')
src/device.c:1347:6: error: no previous declaration for ‘device_get_wake_support’ [-Werror=missing-declarations]
 1347 | bool device_get_wake_support(struct btd_device *device)
      |      ^~~~~~~~~~~~~~~~~~~~~~~
src/device.c:1365:6: error: no previous declaration for ‘device_get_wake_allowed’ [-Werror=missing-declarations]
 1365 | bool device_get_wake_allowed(struct btd_device *device)
      |      ^~~~~~~~~~~~~~~~~~~~~~~
src/device.c: In function ‘btd_device_flags_changed’:
src/device.c:6713:6: error: unused variable ‘i’ [-Werror=unused-variable]
 6713 |  int i;
      |      ^
cc1: all warnings being treated as errors
make[1]: *** [Makefile:9223: src/bluetoothd-device.o] Error 1
make: *** [Makefile:4010: all] Error 2



---
Regards,
Linux Bluetooth

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed
  2020-06-22 23:40 ` [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed Abhishek Pandit-Subedi
  2020-06-22 23:47   ` [BlueZ,v5,3/6] " bluez.test.bot
@ 2020-06-23  0:10   ` Luiz Augusto von Dentz
  2020-06-24  3:53     ` Abhishek Pandit-Subedi
  1 sibling, 1 reply; 12+ messages in thread
From: Luiz Augusto von Dentz @ 2020-06-23  0:10 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: linux-bluetooth, Alain Michaud, Marcel Holtmann,
	ChromeOS Bluetooth Upstreaming

Hi Abhishek,

On Mon, Jun 22, 2020 at 4:41 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 set device flags 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.
>
> If a device is connecting for the first time, it will be marked
> WakeAllowed if the profile supports it. On subsequent reloads of bluez,
> the stored setting "WakeAllowed" will be used to override any other
> setting.
>
> ---
>
> Changes in v5:
> * Refactor to use set_wake_flags and respond to device flags changed
> * Add wake_override so we can keep track of user/profile configuration
>   vs what is currently active
>
> 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    |   2 +
>  src/adapter.c |  98 +++++++++++++++++++++++++++
>  src/adapter.h |   3 +-
>  src/device.c  | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/device.h  |   9 +++
>  5 files changed, 295 insertions(+), 1 deletion(-)
>
> diff --git a/lib/mgmt.h b/lib/mgmt.h
> index 525c4dd62..a800bcab4 100644
> --- a/lib/mgmt.h
> +++ b/lib/mgmt.h
> @@ -665,6 +665,8 @@ struct mgmt_rp_get_device_flags {
>         uint32_t current_flags;
>  } __packed;
>
> +#define DEVICE_FLAG_REMOTE_WAKEUP      (1 << 0)
> +
>  #define MGMT_OP_SET_DEVICE_FLAGS       0x0050
>  #define MGMT_SET_DEVICE_FLAGS_SIZE     11
>  struct mgmt_cp_set_device_flags {
> diff --git a/src/adapter.c b/src/adapter.c
> index 9ce351893..0ab1f85a8 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -5102,6 +5102,99 @@ void adapter_auto_connect_add(struct btd_adapter *adapter,
>         adapter->connect_list = g_slist_append(adapter->connect_list, device);
>  }
>
> +static void set_device_wakeable_complete(uint8_t status, uint16_t length,
> +                                        const void *param, void *user_data)
> +{
> +       const struct mgmt_rp_set_device_flags *rp = param;
> +       struct btd_adapter *adapter = user_data;
> +       struct btd_device *dev;
> +       char addr[18];
> +
> +       if (status != MGMT_STATUS_SUCCESS) {
> +               btd_error(adapter->dev_id, "Set device flags return status: %s",
> +                         mgmt_errstr(status));

Should you return here, or just move the other if statement down this
function here since it seems a little pointless to try to find the
device if the status is an error, that said perhaps we do indeed need
to notify the status on complete so we can actually report back to the
D-Bus client if it cannot be set, apparently this cannot fail now it
seems to be wrong.

> +       }
> +
> +       if (length < sizeof(*rp)) {
> +               btd_error(adapter->dev_id,
> +                         "Too small Set Device Flags complete event: %d",
> +                         length);
> +               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 Device Flags complete for unknown device %s", addr);
> +               return;
> +       }
> +
> +       if (status != MGMT_STATUS_SUCCESS) {
> +               btd_error(adapter->dev_id,
> +                       "Failed to configure wakeable %s (%u): %s (0x%02x)",
> +                       addr, rp->addr.type, mgmt_errstr(status), status);
> +               return;
> +       }
> +
> +       device_set_wake_allowed_complete(dev);
> +}
> +
> +void adapter_set_device_wakeable(struct btd_adapter *adapter,
> +                                struct btd_device *device, bool wakeable)
> +{
> +       struct mgmt_cp_set_device_flags cp;
> +       const bdaddr_t *bdaddr;
> +       uint8_t bdaddr_type;
> +
> +       if (!kernel_conn_control)
> +               return;
> +
> +       bdaddr = device_get_address(device);
> +       bdaddr_type = btd_device_get_bdaddr_type(device);
> +
> +       memset(&cp, 0, sizeof(cp));
> +       bacpy(&cp.addr.bdaddr, bdaddr);
> +       cp.addr.type = bdaddr_type;
> +       cp.current_flags = btd_device_get_current_flags(device);
> +       if (wakeable)
> +               cp.current_flags |= DEVICE_FLAG_REMOTE_WAKEUP;
> +       else
> +               cp.current_flags &= ~DEVICE_FLAG_REMOTE_WAKEUP;
> +
> +       mgmt_send(adapter->mgmt, MGMT_OP_SET_DEVICE_FLAGS, adapter->dev_id,
> +                 sizeof(cp), &cp, set_device_wakeable_complete, adapter, NULL);
> +}
> +
> +static void device_flags_changed_callback(uint16_t index, uint16_t length,
> +                                         const void *param, void *user_data)
> +{
> +       const struct mgmt_ev_device_flags_changed *ev = param;
> +       struct btd_adapter *adapter = user_data;
> +       struct btd_device *dev;
> +       char addr[18];
> +
> +       if (length < sizeof(*ev)) {
> +               btd_error(adapter->dev_id,
> +                         "Too small Device Flags Changed event: %d",
> +                         length);
> +               return;
> +       }
> +
> +       ba2str(&ev->addr.bdaddr, addr);
> +
> +       dev = btd_adapter_find_device(adapter, &ev->addr.bdaddr, ev->addr.type);
> +       if (!dev) {
> +               btd_error(adapter->dev_id,
> +                       "Device Flags Changed for unknown device %s", addr);
> +               return;
> +       }
> +
> +       btd_device_flags_changed(dev, ev->supported_flags, ev->current_flags);
> +}
> +
> +
>  static void remove_device_complete(uint8_t status, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> @@ -8544,6 +8637,11 @@ static int adapter_register(struct btd_adapter *adapter)
>                                                         adapter, NULL);
>
>  load:
> +       mgmt_register(adapter->mgmt, MGMT_EV_DEVICE_FLAGS_CHANGED,
> +                                               adapter->dev_id,
> +                                               device_flags_changed_callback,
> +                                               adapter, NULL);
> +
>         load_config(adapter);
>         fix_storage(adapter);
>         load_drivers(adapter);
> diff --git a/src/adapter.h b/src/adapter.h
> index d0a5253bd..f8ac20261 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -213,6 +213,8 @@ 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);
> +void adapter_set_device_wakeable(struct btd_adapter *adapter,
> +                                struct btd_device *dev, bool wakeable);
>  void adapter_auto_connect_add(struct btd_adapter *adapter,
>                                         struct btd_device *device);
>  void adapter_auto_connect_remove(struct btd_adapter *adapter,
> @@ -231,4 +233,3 @@ void btd_adapter_for_each_device(struct btd_adapter *adapter,
>                         void *data);
>
>  bool btd_le_connect_before_pairing(void);
> -
> diff --git a/src/device.c b/src/device.c
> index 7b0eb256e..cd25111b8 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -177,6 +177,12 @@ struct csrk_info {
>         uint32_t counter;
>  };
>
> +typedef enum {
> +       WAKE_FLAG_DEFAULT = 0,
> +       WAKE_FLAG_ENABLED,
> +       WAKE_FLAG_DISABLED,
> +} wake_flag_t;
> +
>  struct btd_device {
>         int ref_count;
>
> @@ -189,6 +195,20 @@ struct btd_device {
>         bool            le;
>         bool            pending_paired;         /* "Paired" waiting for SDP */
>         bool            svc_refreshed;
> +
> +       /* Manage whether this device can wake the system from suspend.
> +        * - wake_support: Requires a profile that supports wake (i.e. HID)
> +        * - wake_allowed: Is wake currently allowed?
> +        * - pending_wake_allowed - Wake flag sent via set_device_flags
> +        * - wake_override - User configured wake setting
> +        */
> +       bool            wake_support;
> +       bool            wake_allowed;
> +       bool            pending_wake_allowed;
> +       wake_flag_t     wake_override;
> +
> +       uint32_t        supported_flags;
> +       uint32_t        current_flags;
>         GSList          *svc_callbacks;
>         GSList          *eir_uuids;
>         struct bt_ad    *ad;
> @@ -415,6 +435,12 @@ static gboolean store_device_info_cb(gpointer user_data)
>         g_key_file_set_boolean(key_file, "General", "Blocked",
>                                                         device->blocked);
>
> +       if (device->wake_override != WAKE_FLAG_DEFAULT) {
> +               g_key_file_set_boolean(key_file, "General", "WakeAllowed",
> +                                      device->wake_override ==
> +                                              WAKE_FLAG_ENABLED);
> +       }
> +
>         if (device->uuids) {
>                 GSList *l;
>                 int i;
> @@ -1318,6 +1344,105 @@ 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;
> +
> +       /* If wake configuration has not been made yet, set the initial
> +        * configuration.
> +        */
> +       if (device->wake_override == WAKE_FLAG_DEFAULT) {
> +               device_set_wake_override(device, wake_support);
> +               device_set_wake_allowed(device, wake_support);
> +       }
> +}
> +
> +bool device_get_wake_allowed(struct btd_device *device)
> +{
> +       return device->wake_allowed;
> +}
> +
> +void device_set_wake_override(struct btd_device *device, bool wake_override)
> +{
> +       if (wake_override) {
> +               device->wake_override = WAKE_FLAG_ENABLED;
> +               device->current_flags |= DEVICE_FLAG_REMOTE_WAKEUP;
> +       } else {
> +               device->wake_override = WAKE_FLAG_DISABLED;
> +               device->current_flags &= ~DEVICE_FLAG_REMOTE_WAKEUP;
> +       }
> +}
> +
> +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed)
> +{

I guess it would be a good idea to add a check if there is already a
device->pending_wake_allowed set, since there could be multiple
clients attempting to set it, also if device->wake_allowed is already
the same there is probably no reason to call
adapter_set_device_wakeable either.

> +       device->pending_wake_allowed = wake_allowed;
> +       adapter_set_device_wakeable(device_get_adapter(device), device,
> +                                   wake_allowed);
> +}
> +
> +void device_set_wake_allowed_complete(struct btd_device *device)
> +{
> +       device->wake_allowed = device->pending_wake_allowed;
> +       g_dbus_emit_property_changed(dbus_conn, device->path,
> +                                       DEVICE_INTERFACE, "WakeAllowed");
> +
> +       store_device_info(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);
> +
> +       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_override(device, b);
> +       device_set_wake_allowed(device, b);

We should probably store the id here and only invoke
g_dbus_pendin_property* on complete that way the client is notified
even if the command fails.

> +       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);
> +}
> +
> +
>  static gboolean disconnect_all(gpointer user_data)
>  {
>         struct btd_device *device = user_data;
> @@ -2790,6 +2915,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 },
>         { }
>  };
>
> @@ -3038,9 +3166,11 @@ static void convert_info(struct btd_device *device, GKeyFile *key_file)
>  static void load_info(struct btd_device *device, const char *local,
>                         const char *peer, GKeyFile *key_file)
>  {
> +       GError *gerr = NULL;
>         char *str;
>         gboolean store_needed = FALSE;
>         gboolean blocked;
> +       gboolean wake_allowed;
>         char **uuids;
>         int source, vendor, product, version;
>         char **techno, **t;
> @@ -3152,6 +3282,17 @@ next:
>                 btd_device_set_pnpid(device, source, vendor, product, version);
>         }
>
> +       /* Wake allowed is only configured and stored if user changed it.
> +        * Otherwise, we enable if profile supports it. */
> +       wake_allowed = g_key_file_get_boolean(key_file, "General",
> +                                             "WakeAllowed", &gerr);
> +       if (!gerr) {
> +               device_set_wake_override(device, wake_allowed);
> +       } else {
> +               g_error_free(gerr);
> +               gerr = NULL;
> +       }
> +
>         if (store_needed)
>                 store_device_info(device);
>  }
> @@ -6558,6 +6699,49 @@ void btd_device_set_pnpid(struct btd_device *device, uint16_t source,
>         store_device_info(device);
>  }
>
> +uint32_t btd_device_get_current_flags(struct btd_device *dev) {
> +       return dev->current_flags;
> +}
> +
> +/* This event is sent immediately after add device on all mgmt sockets.
> + * Afterwards, it is only sent to mgmt sockets other than the one which called
> + * set_device_flags.
> + */
> +void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
> +                             uint32_t current_flags)
> +{
> +       int i;
> +       const uint32_t changed_flags = dev->current_flags ^ current_flags;
> +       bool flag_value;
> +
> +       dev->supported_flags = supported_flags;
> +       dev->current_flags = current_flags;
> +
> +       if (!changed_flags)
> +               return;
> +
> +       if (changed_flags & DEVICE_FLAG_REMOTE_WAKEUP) {
> +               flag_value = !!(current_flags & DEVICE_FLAG_REMOTE_WAKEUP);
> +               dev->pending_wake_allowed = flag_value;
> +
> +               /* If an override exists and doesn't match the current state,
> +                * apply it. This logic will run after Add Device only and will
> +                * enable wake for previously paired devices.
> +                */
> +               if (dev->wake_override != WAKE_FLAG_DEFAULT) {
> +                       bool wake_allowed =
> +                               dev->wake_override == WAKE_FLAG_ENABLED;
> +                       if (flag_value != wake_allowed) {
> +                               device_set_wake_allowed(dev, wake_allowed);
> +                       } else {
> +                               device_set_wake_allowed_complete(dev);
> +                       }
> +               } else {
> +                       device_set_wake_allowed_complete(dev);
> +               }
> +       }
> +}
> +
>  static void service_state_changed(struct btd_service *service,
>                                                 btd_service_state_t old_state,
>                                                 btd_service_state_t new_state,
> diff --git a/src/device.h b/src/device.h
> index 06b100499..104e3f1dd 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -33,6 +33,7 @@ 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 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 +140,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);
> +void device_set_wake_support(struct btd_device *device, bool wake_support);
> +void device_set_wake_override(struct btd_device *device, bool wake_override);
> +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed);
> +void device_set_wake_allowed_complete(struct btd_device *device);
>
>  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
>                                         void *user_data);
> @@ -176,5 +181,9 @@ struct btd_service *btd_device_get_service(struct btd_device *dev,
>  int device_discover_services(struct btd_device *device);
>  int btd_device_connect_services(struct btd_device *dev, GSList *services);
>
> +uint32_t btd_device_get_current_flags(struct btd_device *dev);
> +void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
> +                             uint32_t current_flags);
> +
>  void btd_device_init(void);
>  void btd_device_cleanup(void);
> --
> 2.27.0.111.gc72c7da667-goog
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed
  2020-06-23  0:10   ` [BlueZ PATCH v5 3/6] " Luiz Augusto von Dentz
@ 2020-06-24  3:53     ` Abhishek Pandit-Subedi
  0 siblings, 0 replies; 12+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-24  3:53 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: linux-bluetooth, Alain Michaud, Marcel Holtmann,
	ChromeOS Bluetooth Upstreaming

Hi Luiz,

On Mon, Jun 22, 2020 at 5:11 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Mon, Jun 22, 2020 at 4:41 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 set device flags 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.
> >
> > If a device is connecting for the first time, it will be marked
> > WakeAllowed if the profile supports it. On subsequent reloads of bluez,
> > the stored setting "WakeAllowed" will be used to override any other
> > setting.
> >
> > ---
> >
> > Changes in v5:
> > * Refactor to use set_wake_flags and respond to device flags changed
> > * Add wake_override so we can keep track of user/profile configuration
> >   vs what is currently active
> >
> > 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    |   2 +
> >  src/adapter.c |  98 +++++++++++++++++++++++++++
> >  src/adapter.h |   3 +-
> >  src/device.c  | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/device.h  |   9 +++
> >  5 files changed, 295 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/mgmt.h b/lib/mgmt.h
> > index 525c4dd62..a800bcab4 100644
> > --- a/lib/mgmt.h
> > +++ b/lib/mgmt.h
> > @@ -665,6 +665,8 @@ struct mgmt_rp_get_device_flags {
> >         uint32_t current_flags;
> >  } __packed;
> >
> > +#define DEVICE_FLAG_REMOTE_WAKEUP      (1 << 0)
> > +
> >  #define MGMT_OP_SET_DEVICE_FLAGS       0x0050
> >  #define MGMT_SET_DEVICE_FLAGS_SIZE     11
> >  struct mgmt_cp_set_device_flags {
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 9ce351893..0ab1f85a8 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -5102,6 +5102,99 @@ void adapter_auto_connect_add(struct btd_adapter *adapter,
> >         adapter->connect_list = g_slist_append(adapter->connect_list, device);
> >  }
> >
> > +static void set_device_wakeable_complete(uint8_t status, uint16_t length,
> > +                                        const void *param, void *user_data)
> > +{
> > +       const struct mgmt_rp_set_device_flags *rp = param;
> > +       struct btd_adapter *adapter = user_data;
> > +       struct btd_device *dev;
> > +       char addr[18];
> > +
> > +       if (status != MGMT_STATUS_SUCCESS) {
> > +               btd_error(adapter->dev_id, "Set device flags return status: %s",
> > +                         mgmt_errstr(status));
>
> Should you return here, or just move the other if statement down this
> function here since it seems a little pointless to try to find the
> device if the status is an error, that said perhaps we do indeed need
> to notify the status on complete so we can actually report back to the
> D-Bus client if it cannot be set, apparently this cannot fail now it
> seems to be wrong.

Whoops, I added this for debugging but forgot to remove the other one
and return here. Will just return here if MGMT status is incorrect.

>
> > +       }
> > +
> > +       if (length < sizeof(*rp)) {
> > +               btd_error(adapter->dev_id,
> > +                         "Too small Set Device Flags complete event: %d",
> > +                         length);
> > +               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 Device Flags complete for unknown device %s", addr);
> > +               return;
> > +       }
> > +
> > +       if (status != MGMT_STATUS_SUCCESS) {
> > +               btd_error(adapter->dev_id,
> > +                       "Failed to configure wakeable %s (%u): %s (0x%02x)",
> > +                       addr, rp->addr.type, mgmt_errstr(status), status);
> > +               return;
> > +       }
> > +
> > +       device_set_wake_allowed_complete(dev);
> > +}
> > +
> > +void adapter_set_device_wakeable(struct btd_adapter *adapter,
> > +                                struct btd_device *device, bool wakeable)
> > +{
> > +       struct mgmt_cp_set_device_flags cp;
> > +       const bdaddr_t *bdaddr;
> > +       uint8_t bdaddr_type;
> > +
> > +       if (!kernel_conn_control)
> > +               return;
> > +
> > +       bdaddr = device_get_address(device);
> > +       bdaddr_type = btd_device_get_bdaddr_type(device);
> > +
> > +       memset(&cp, 0, sizeof(cp));
> > +       bacpy(&cp.addr.bdaddr, bdaddr);
> > +       cp.addr.type = bdaddr_type;
> > +       cp.current_flags = btd_device_get_current_flags(device);
> > +       if (wakeable)
> > +               cp.current_flags |= DEVICE_FLAG_REMOTE_WAKEUP;
> > +       else
> > +               cp.current_flags &= ~DEVICE_FLAG_REMOTE_WAKEUP;
> > +
> > +       mgmt_send(adapter->mgmt, MGMT_OP_SET_DEVICE_FLAGS, adapter->dev_id,
> > +                 sizeof(cp), &cp, set_device_wakeable_complete, adapter, NULL);
> > +}
> > +
> > +static void device_flags_changed_callback(uint16_t index, uint16_t length,
> > +                                         const void *param, void *user_data)
> > +{
> > +       const struct mgmt_ev_device_flags_changed *ev = param;
> > +       struct btd_adapter *adapter = user_data;
> > +       struct btd_device *dev;
> > +       char addr[18];
> > +
> > +       if (length < sizeof(*ev)) {
> > +               btd_error(adapter->dev_id,
> > +                         "Too small Device Flags Changed event: %d",
> > +                         length);
> > +               return;
> > +       }
> > +
> > +       ba2str(&ev->addr.bdaddr, addr);
> > +
> > +       dev = btd_adapter_find_device(adapter, &ev->addr.bdaddr, ev->addr.type);
> > +       if (!dev) {
> > +               btd_error(adapter->dev_id,
> > +                       "Device Flags Changed for unknown device %s", addr);
> > +               return;
> > +       }
> > +
> > +       btd_device_flags_changed(dev, ev->supported_flags, ev->current_flags);
> > +}
> > +
> > +
> >  static void remove_device_complete(uint8_t status, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > @@ -8544,6 +8637,11 @@ static int adapter_register(struct btd_adapter *adapter)
> >                                                         adapter, NULL);
> >
> >  load:
> > +       mgmt_register(adapter->mgmt, MGMT_EV_DEVICE_FLAGS_CHANGED,
> > +                                               adapter->dev_id,
> > +                                               device_flags_changed_callback,
> > +                                               adapter, NULL);
> > +
> >         load_config(adapter);
> >         fix_storage(adapter);
> >         load_drivers(adapter);
> > diff --git a/src/adapter.h b/src/adapter.h
> > index d0a5253bd..f8ac20261 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -213,6 +213,8 @@ 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);
> > +void adapter_set_device_wakeable(struct btd_adapter *adapter,
> > +                                struct btd_device *dev, bool wakeable);
> >  void adapter_auto_connect_add(struct btd_adapter *adapter,
> >                                         struct btd_device *device);
> >  void adapter_auto_connect_remove(struct btd_adapter *adapter,
> > @@ -231,4 +233,3 @@ void btd_adapter_for_each_device(struct btd_adapter *adapter,
> >                         void *data);
> >
> >  bool btd_le_connect_before_pairing(void);
> > -
> > diff --git a/src/device.c b/src/device.c
> > index 7b0eb256e..cd25111b8 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -177,6 +177,12 @@ struct csrk_info {
> >         uint32_t counter;
> >  };
> >
> > +typedef enum {
> > +       WAKE_FLAG_DEFAULT = 0,
> > +       WAKE_FLAG_ENABLED,
> > +       WAKE_FLAG_DISABLED,
> > +} wake_flag_t;
> > +
> >  struct btd_device {
> >         int ref_count;
> >
> > @@ -189,6 +195,20 @@ struct btd_device {
> >         bool            le;
> >         bool            pending_paired;         /* "Paired" waiting for SDP */
> >         bool            svc_refreshed;
> > +
> > +       /* Manage whether this device can wake the system from suspend.
> > +        * - wake_support: Requires a profile that supports wake (i.e. HID)
> > +        * - wake_allowed: Is wake currently allowed?
> > +        * - pending_wake_allowed - Wake flag sent via set_device_flags
> > +        * - wake_override - User configured wake setting
> > +        */
> > +       bool            wake_support;
> > +       bool            wake_allowed;
> > +       bool            pending_wake_allowed;
> > +       wake_flag_t     wake_override;
> > +
> > +       uint32_t        supported_flags;
> > +       uint32_t        current_flags;
> >         GSList          *svc_callbacks;
> >         GSList          *eir_uuids;
> >         struct bt_ad    *ad;
> > @@ -415,6 +435,12 @@ static gboolean store_device_info_cb(gpointer user_data)
> >         g_key_file_set_boolean(key_file, "General", "Blocked",
> >                                                         device->blocked);
> >
> > +       if (device->wake_override != WAKE_FLAG_DEFAULT) {
> > +               g_key_file_set_boolean(key_file, "General", "WakeAllowed",
> > +                                      device->wake_override ==
> > +                                              WAKE_FLAG_ENABLED);
> > +       }
> > +
> >         if (device->uuids) {
> >                 GSList *l;
> >                 int i;
> > @@ -1318,6 +1344,105 @@ 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;
> > +
> > +       /* If wake configuration has not been made yet, set the initial
> > +        * configuration.
> > +        */
> > +       if (device->wake_override == WAKE_FLAG_DEFAULT) {
> > +               device_set_wake_override(device, wake_support);
> > +               device_set_wake_allowed(device, wake_support);
> > +       }
> > +}
> > +
> > +bool device_get_wake_allowed(struct btd_device *device)
> > +{
> > +       return device->wake_allowed;
> > +}
> > +
> > +void device_set_wake_override(struct btd_device *device, bool wake_override)
> > +{
> > +       if (wake_override) {
> > +               device->wake_override = WAKE_FLAG_ENABLED;
> > +               device->current_flags |= DEVICE_FLAG_REMOTE_WAKEUP;
> > +       } else {
> > +               device->wake_override = WAKE_FLAG_DISABLED;
> > +               device->current_flags &= ~DEVICE_FLAG_REMOTE_WAKEUP;
> > +       }
> > +}
> > +
> > +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed)
> > +{
>
> I guess it would be a good idea to add a check if there is already a
> device->pending_wake_allowed set, since there could be multiple
> clients attempting to set it, also if device->wake_allowed is already
> the same there is probably no reason to call
> adapter_set_device_wakeable either.

Yes, I will short-circuit when device->wake_allowed is the same. As
for pending_wake_allowed, I could use this to mark when the mgmt op is
active instead (meaning complete will just be device->wake_allowed =
!device->wake_allowed). Will be there in next revision.

>
> > +       device->pending_wake_allowed = wake_allowed;
> > +       adapter_set_device_wakeable(device_get_adapter(device), device,
> > +                                   wake_allowed);
> > +}
> > +
> > +void device_set_wake_allowed_complete(struct btd_device *device)
> > +{
> > +       device->wake_allowed = device->pending_wake_allowed;
> > +       g_dbus_emit_property_changed(dbus_conn, device->path,
> > +                                       DEVICE_INTERFACE, "WakeAllowed");
> > +
> > +       store_device_info(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);
> > +
> > +       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_override(device, b);
> > +       device_set_wake_allowed(device, b);
>
> We should probably store the id here and only invoke
> g_dbus_pendin_property* on complete that way the client is notified
> even if the command fails.

Will change for next revision.

>
> > +       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);
> > +}
> > +
> > +
> >  static gboolean disconnect_all(gpointer user_data)
> >  {
> >         struct btd_device *device = user_data;
> > @@ -2790,6 +2915,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 },
> >         { }
> >  };
> >
> > @@ -3038,9 +3166,11 @@ static void convert_info(struct btd_device *device, GKeyFile *key_file)
> >  static void load_info(struct btd_device *device, const char *local,
> >                         const char *peer, GKeyFile *key_file)
> >  {
> > +       GError *gerr = NULL;
> >         char *str;
> >         gboolean store_needed = FALSE;
> >         gboolean blocked;
> > +       gboolean wake_allowed;
> >         char **uuids;
> >         int source, vendor, product, version;
> >         char **techno, **t;
> > @@ -3152,6 +3282,17 @@ next:
> >                 btd_device_set_pnpid(device, source, vendor, product, version);
> >         }
> >
> > +       /* Wake allowed is only configured and stored if user changed it.
> > +        * Otherwise, we enable if profile supports it. */
> > +       wake_allowed = g_key_file_get_boolean(key_file, "General",
> > +                                             "WakeAllowed", &gerr);
> > +       if (!gerr) {
> > +               device_set_wake_override(device, wake_allowed);
> > +       } else {
> > +               g_error_free(gerr);
> > +               gerr = NULL;
> > +       }
> > +
> >         if (store_needed)
> >                 store_device_info(device);
> >  }
> > @@ -6558,6 +6699,49 @@ void btd_device_set_pnpid(struct btd_device *device, uint16_t source,
> >         store_device_info(device);
> >  }
> >
> > +uint32_t btd_device_get_current_flags(struct btd_device *dev) {
> > +       return dev->current_flags;
> > +}
> > +
> > +/* This event is sent immediately after add device on all mgmt sockets.
> > + * Afterwards, it is only sent to mgmt sockets other than the one which called
> > + * set_device_flags.
> > + */
> > +void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
> > +                             uint32_t current_flags)
> > +{
> > +       int i;
> > +       const uint32_t changed_flags = dev->current_flags ^ current_flags;
> > +       bool flag_value;
> > +
> > +       dev->supported_flags = supported_flags;
> > +       dev->current_flags = current_flags;
> > +
> > +       if (!changed_flags)
> > +               return;
> > +
> > +       if (changed_flags & DEVICE_FLAG_REMOTE_WAKEUP) {
> > +               flag_value = !!(current_flags & DEVICE_FLAG_REMOTE_WAKEUP);
> > +               dev->pending_wake_allowed = flag_value;
> > +
> > +               /* If an override exists and doesn't match the current state,
> > +                * apply it. This logic will run after Add Device only and will
> > +                * enable wake for previously paired devices.
> > +                */
> > +               if (dev->wake_override != WAKE_FLAG_DEFAULT) {
> > +                       bool wake_allowed =
> > +                               dev->wake_override == WAKE_FLAG_ENABLED;
> > +                       if (flag_value != wake_allowed) {
> > +                               device_set_wake_allowed(dev, wake_allowed);
> > +                       } else {
> > +                               device_set_wake_allowed_complete(dev);
> > +                       }
> > +               } else {
> > +                       device_set_wake_allowed_complete(dev);
> > +               }
> > +       }
> > +}
> > +
> >  static void service_state_changed(struct btd_service *service,
> >                                                 btd_service_state_t old_state,
> >                                                 btd_service_state_t new_state,
> > diff --git a/src/device.h b/src/device.h
> > index 06b100499..104e3f1dd 100644
> > --- a/src/device.h
> > +++ b/src/device.h
> > @@ -33,6 +33,7 @@ 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 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 +140,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);
> > +void device_set_wake_support(struct btd_device *device, bool wake_support);
> > +void device_set_wake_override(struct btd_device *device, bool wake_override);
> > +void device_set_wake_allowed(struct btd_device *device, bool wake_allowed);
> > +void device_set_wake_allowed_complete(struct btd_device *device);
> >
> >  typedef void (*disconnect_watch) (struct btd_device *device, gboolean removal,
> >                                         void *user_data);
> > @@ -176,5 +181,9 @@ struct btd_service *btd_device_get_service(struct btd_device *dev,
> >  int device_discover_services(struct btd_device *device);
> >  int btd_device_connect_services(struct btd_device *dev, GSList *services);
> >
> > +uint32_t btd_device_get_current_flags(struct btd_device *dev);
> > +void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
> > +                             uint32_t current_flags);
> > +
> >  void btd_device_init(void);
> >  void btd_device_cleanup(void);
> > --
> > 2.27.0.111.gc72c7da667-goog
> >
>
>
> --
> Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-06-24  3:54 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-22 23:40 [BlueZ PATCH v5 0/6] device: Allow devices to be marked as wake capable Abhishek Pandit-Subedi
2020-06-22 23:40 ` [BlueZ PATCH v5 1/6] mgmt: Add mgmt op and events for device flags Abhishek Pandit-Subedi
2020-06-22 23:49   ` device: Allow devices to be marked as wake capable bluez.test.bot
2020-06-22 23:40 ` [BlueZ PATCH v5 2/6] monitor: Decode device flags mgmt ops and event Abhishek Pandit-Subedi
2020-06-22 23:47   ` [BlueZ,v5,2/6] " bluez.test.bot
2020-06-22 23:40 ` [BlueZ PATCH v5 3/6] device: Support marking a device with wake allowed Abhishek Pandit-Subedi
2020-06-22 23:47   ` [BlueZ,v5,3/6] " bluez.test.bot
2020-06-23  0:10   ` [BlueZ PATCH v5 3/6] " Luiz Augusto von Dentz
2020-06-24  3:53     ` Abhishek Pandit-Subedi
2020-06-22 23:40 ` [BlueZ PATCH v5 4/6] client: Display wake allowed property with info Abhishek Pandit-Subedi
2020-06-22 23:40 ` [BlueZ PATCH v5 5/6] doc/device-api: Add WakeAllowed Abhishek Pandit-Subedi
2020-06-22 23:40 ` [BlueZ PATCH v5 6/6] input: Make HID devices support wake Abhishek Pandit-Subedi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).