[PATCH BlueZ v4 1/6] adapter: Keep track of whether the adapter is rfkill'ed

[PATCH BlueZ v4 1/6] adapter: Keep track of whether the adapter is rfkill'ed

[Bastien Nocera]

Instead of only replying to D-Bus requests with an error saying the
adapter is blocked, keep track of the rfkill being enabled or disabled
so we know the rfkill state of the adapter at all times.
---
 src/adapter.c | 25 +++++++++++++++++++++++--
 src/adapter.h |  1 +
 src/rfkill.c  |  8 ++++++--
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index b453e86a0..641db67f9 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -250,6 +250,7 @@ struct btd_adapter {
 	uint32_t dev_class;		/* controller class of device */
 	char *name;			/* controller device name */
 	char *short_name;		/* controller short name */
+	bool blocked;			/* whether rfkill is enabled */
 	uint32_t supported_settings;	/* controller supported settings */
 	uint32_t pending_settings;	/* pending controller settings */
 	uint32_t current_settings;	/* current controller settings */
@@ -654,6 +655,8 @@ static void set_mode_complete(uint8_t status, uint16_t length,
 	if (status != MGMT_STATUS_SUCCESS) {
 		btd_error(adapter->dev_id, "Failed to set mode: %s (0x%02x)",
 						mgmt_errstr(status), status);
+		if (status == MGMT_STATUS_RFKILLED)
+			adapter->blocked = true;
 		adapter->pending_settings &= ~data->setting;
 		return;
 	}
@@ -2914,10 +2917,12 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
 		btd_error(adapter->dev_id, "Failed to set mode: %s (0x%02x)",
 						mgmt_errstr(status), status);
 
-		if (status == MGMT_STATUS_RFKILLED)
+		if (status == MGMT_STATUS_RFKILLED) {
 			dbus_err = ERROR_INTERFACE ".Blocked";
-		else
+			adapter->blocked = true;
+		} else {
 			dbus_err = ERROR_INTERFACE ".Failed";
+		}
 
 		g_dbus_pending_property_error(data->id, dbus_err,
 							mgmt_errstr(status));
@@ -7548,6 +7553,12 @@ int btd_cancel_authorization(guint id)
 
 int btd_adapter_restore_powered(struct btd_adapter *adapter)
 {
+	if (adapter->blocked) {
+		adapter->blocked = false;
+	        g_dbus_emit_property_changed(dbus_conn, adapter->path,
+					ADAPTER_INTERFACE, "PowerState");
+	}
+
 	if (btd_adapter_get_powered(adapter))
 		return 0;
 
@@ -7556,6 +7567,16 @@ int btd_adapter_restore_powered(struct btd_adapter *adapter)
 	return 0;
 }
 
+int btd_adapter_set_blocked(struct btd_adapter *adapter)
+{
+	if (!adapter->blocked) {
+		adapter->blocked = true;
+	        g_dbus_emit_property_changed(dbus_conn, adapter->path,
+					ADAPTER_INTERFACE, "PowerState");
+	}
+	return 0;
+}
+
 void btd_adapter_register_pin_cb(struct btd_adapter *adapter,
 							btd_adapter_pin_cb_t cb)
 {
diff --git a/src/adapter.h b/src/adapter.h
index b09044edd..332c0b239 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -143,6 +143,7 @@ guint btd_request_authorization_cable_configured(const bdaddr_t *src, const bdad
 int btd_cancel_authorization(guint id);
 
 int btd_adapter_restore_powered(struct btd_adapter *adapter);
+int btd_adapter_set_blocked(struct btd_adapter *adapter);
 
 typedef ssize_t (*btd_adapter_pin_cb_t) (struct btd_adapter *adapter,
 			struct btd_device *dev, char *out, bool *display,
diff --git a/src/rfkill.c b/src/rfkill.c
index 2099c5ac5..93f8e0e12 100644
--- a/src/rfkill.c
+++ b/src/rfkill.c
@@ -61,6 +61,7 @@ static gboolean rfkill_event(GIOChannel *chan,
 	struct rfkill_event event = { 0 };
 	struct btd_adapter *adapter;
 	char sysname[PATH_MAX];
+	bool blocked = false;
 	ssize_t len;
 	int fd, id;
 
@@ -84,7 +85,7 @@ static gboolean rfkill_event(GIOChannel *chan,
 						event.soft, event.hard);
 
 	if (event.soft || event.hard)
-		return TRUE;
+		blocked = true;
 
 	if (event.op != RFKILL_OP_CHANGE)
 		return TRUE;
@@ -122,7 +123,10 @@ static gboolean rfkill_event(GIOChannel *chan,
 
 	DBG("RFKILL unblock for hci%d", id);
 
-	btd_adapter_restore_powered(adapter);
+	if (blocked)
+		btd_adapter_set_blocked(adapter);
+	else
+		btd_adapter_restore_powered(adapter);
 
 	return TRUE;
 }
-- 
2.37.2

[PATCH BlueZ v4 2/6] adapter: Implement PowerState property

[Bastien Nocera]

This property should allow any program to show whether an adapter is in
the process of being turned on.

As turning on an adapter isn't instantaneous, it's important that the UI
reflects the transitional state of the adapter's power, and doesn't
assume the device is already turned on but not yet working, or still off
despite having requested for it to be turned on, in both cases making
the UI feel unresponsive.

This can also not be implemented in front-ends directly as, then,
the status of an adapter wouldn't be reflected correctly in the Settings
window if it's turned on in the system menu. Implementing it in the
front-ends would also preclude from having feedback about the state of
the adapter when bluetoothd is the one powering up the adapter after the
rfkill was unblocked.

See https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/121
and the original https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/5773
---
 src/adapter.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/src/adapter.c b/src/adapter.c
index 641db67f9..e295ef247 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -239,6 +239,12 @@ struct btd_adapter_pin_cb_iter {
 	/* When the iterator reaches the end, it is NULL and attempt is 0 */
 };
 
+enum {
+	ADAPTER_POWER_STATE_TARGET_NONE = 0,
+	ADAPTER_POWER_STATE_TARGET_OFF,
+	ADAPTER_POWER_STATE_TARGET_ON
+};
+
 struct btd_adapter {
 	int ref_count;
 
@@ -253,6 +259,7 @@ struct btd_adapter {
 	bool blocked;			/* whether rfkill is enabled */
 	uint32_t supported_settings;	/* controller supported settings */
 	uint32_t pending_settings;	/* pending controller settings */
+	uint32_t power_state_target;	/* the target power state */
 	uint32_t current_settings;	/* current controller settings */
 
 	char *path;			/* adapter object path */
@@ -580,6 +587,8 @@ static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
 	if (changed_mask & MGMT_SETTING_POWERED) {
 	        g_dbus_emit_property_changed(dbus_conn, adapter->path,
 					ADAPTER_INTERFACE, "Powered");
+	        g_dbus_emit_property_changed(dbus_conn, adapter->path,
+					ADAPTER_INTERFACE, "PowerState");
 
 		if (adapter->current_settings & MGMT_SETTING_POWERED) {
 			adapter_start(adapter);
@@ -619,6 +628,16 @@ static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
 	}
 }
 
+static void reset_power_state_target(struct btd_adapter *adapter, uint8_t value)
+{
+	if ((value &&
+	     adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_ON) ||
+	    (!value &&
+	     adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_OFF)) {
+		adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
+	}
+}
+
 static void new_settings_callback(uint16_t index, uint16_t length,
 					const void *param, void *user_data)
 {
@@ -636,6 +655,9 @@ static void new_settings_callback(uint16_t index, uint16_t length,
 	if (settings == adapter->current_settings)
 		return;
 
+	if ((adapter->current_settings ^ settings) & MGMT_SETTING_POWERED)
+		reset_power_state_target(adapter, settings & MGMT_SETTING_POWERED ? 0x01 : 0x00);
+
 	DBG("Settings: 0x%08x", settings);
 
 	settings_changed(adapter, settings);
@@ -644,6 +666,7 @@ static void new_settings_callback(uint16_t index, uint16_t length,
 struct set_mode_data {
 	struct btd_adapter *adapter;
 	uint32_t setting;
+	uint8_t value;
 };
 
 static void set_mode_complete(uint8_t status, uint16_t length,
@@ -658,6 +681,8 @@ static void set_mode_complete(uint8_t status, uint16_t length,
 		if (status == MGMT_STATUS_RFKILLED)
 			adapter->blocked = true;
 		adapter->pending_settings &= ~data->setting;
+		if (data->setting & MGMT_SETTING_POWERED)
+			reset_power_state_target(adapter, data->value);
 		return;
 	}
 
@@ -695,6 +720,11 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
 	switch (opcode) {
 	case MGMT_OP_SET_POWERED:
 		setting = MGMT_SETTING_POWERED;
+		adapter->power_state_target = mode ?
+			ADAPTER_POWER_STATE_TARGET_ON :
+			ADAPTER_POWER_STATE_TARGET_OFF;
+		g_dbus_emit_property_changed(dbus_conn, adapter->path,
+						ADAPTER_INTERFACE, "PowerState");
 		break;
 	case MGMT_OP_SET_CONNECTABLE:
 		setting = MGMT_SETTING_CONNECTABLE;
@@ -715,6 +745,7 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
 	data = g_new0(struct set_mode_data, 1);
 	data->adapter = adapter;
 	data->setting = setting;
+	data->value = mode;
 
 	if (mgmt_send(adapter->mgmt, opcode,
 				adapter->dev_id, sizeof(cp), &cp,
@@ -722,8 +753,13 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
 		adapter->pending_settings |= setting;
 		return true;
 	}
-
 	g_free(data);
+	if (setting == MGMT_SETTING_POWERED) {
+		/* cancel the earlier setting */
+		adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
+		g_dbus_emit_property_changed(dbus_conn, adapter->path,
+						ADAPTER_INTERFACE, "PowerState");
+	}
 	btd_error(adapter->dev_id, "Failed to set mode for index %u",
 							adapter->dev_id);
 
@@ -2901,6 +2937,7 @@ struct property_set_data {
 	struct btd_adapter *adapter;
 	uint32_t setting;
 	GDBusPendingPropertySet id;
+	uint8_t value;
 };
 
 static void property_set_mode_complete(uint8_t status, uint16_t length,
@@ -2928,6 +2965,8 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
 							mgmt_errstr(status));
 
 		adapter->pending_settings &= ~data->setting;
+		if (data->setting & MGMT_SETTING_POWERED)
+			reset_power_state_target(adapter, data->value);
 		return;
 	}
 
@@ -3051,6 +3090,16 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
 	data->adapter = adapter;
 	data->setting = setting;
 	data->id = id;
+	data->setting = setting;
+	data->value = mode;
+
+	if (setting == MGMT_SETTING_POWERED) {
+		adapter->power_state_target = mode ?
+			ADAPTER_POWER_STATE_TARGET_ON :
+			ADAPTER_POWER_STATE_TARGET_OFF;
+		g_dbus_emit_property_changed(dbus_conn, adapter->path,
+						ADAPTER_INTERFACE, "PowerState");
+	}
 
 	if (mgmt_send(adapter->mgmt, opcode, adapter->dev_id, len, param,
 			property_set_mode_complete, data, g_free) > 0) {
@@ -3059,6 +3108,12 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
 	}
 
 	g_free(data);
+	if (setting == MGMT_SETTING_POWERED) {
+		/* cancel the earlier setting */
+		adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
+		g_dbus_emit_property_changed(dbus_conn, adapter->path,
+						ADAPTER_INTERFACE, "PowerState");
+	}
 
 failed:
 	btd_error(adapter->dev_id, "Failed to set mode for index %u",
@@ -3090,6 +3145,31 @@ static void property_set_powered(const GDBusPropertyTable *property,
 	property_set_mode(adapter, MGMT_SETTING_POWERED, iter, id);
 }
 
+static gboolean property_get_power_state(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *user_data)
+{
+	struct btd_adapter *adapter = user_data;
+	const char *str;
+
+	if (adapter->blocked) {
+		str = "off-blocked";
+	} else if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_NONE) {
+		if (adapter->current_settings & MGMT_SETTING_POWERED)
+			str = "on";
+		else
+			str = "off";
+	} else {
+		if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_ON)
+			str = "off-enabling";
+		else
+			str = "on-disabling";
+	}
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &str);
+
+	return TRUE;
+}
+
 static gboolean property_get_discoverable(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *user_data)
 {
@@ -3728,6 +3808,8 @@ static const GDBusPropertyTable adapter_properties[] = {
 	{ "Alias", "s", property_get_alias, property_set_alias },
 	{ "Class", "u", property_get_class },
 	{ "Powered", "b", property_get_powered, property_set_powered },
+	{ "PowerState", "s", property_get_power_state, NULL, NULL,
+			     G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
 	{ "Discoverable", "b", property_get_discoverable,
 					property_set_discoverable },
 	{ "DiscoverableTimeout", "u", property_get_discoverable_timeout,
@@ -5534,6 +5616,8 @@ static void adapter_start(struct btd_adapter *adapter)
 {
 	g_dbus_emit_property_changed(dbus_conn, adapter->path,
 						ADAPTER_INTERFACE, "Powered");
+	g_dbus_emit_property_changed(dbus_conn, adapter->path,
+						ADAPTER_INTERFACE, "PowerState");
 
 	DBG("adapter %s has been enabled", adapter->path);
 
@@ -7277,6 +7361,8 @@ static void adapter_stop(struct btd_adapter *adapter)
 
 	g_dbus_emit_property_changed(dbus_conn, adapter->path,
 						ADAPTER_INTERFACE, "Powered");
+	g_dbus_emit_property_changed(dbus_conn, adapter->path,
+						ADAPTER_INTERFACE, "PowerState");
 
 	DBG("adapter %s has been disabled", adapter->path);
 }
-- 
2.37.2

[PATCH BlueZ v4 3/6] client: Print the PowerState property

[Bastien Nocera]

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

diff --git a/client/main.c b/client/main.c
index 19139d15b..ddd97c23c 100644
--- a/client/main.c
+++ b/client/main.c
@@ -981,6 +981,7 @@ static void cmd_show(int argc, char *argv[])
 	print_property(adapter->proxy, "Alias");
 	print_property(adapter->proxy, "Class");
 	print_property(adapter->proxy, "Powered");
+	print_property(adapter->proxy, "PowerState");
 	print_property(adapter->proxy, "Discoverable");
 	print_property(adapter->proxy, "DiscoverableTimeout");
 	print_property(adapter->proxy, "Pairable");
-- 
2.37.2

[PATCH BlueZ v4 4/6] adapter-api: Add PowerState property documentation

[Bastien Nocera]

---
 doc/adapter-api.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index 48466ab75..28e53a105 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -269,6 +269,21 @@ Properties	string Address [readonly]
 			restart or unplugging of the adapter it will reset
 			back to false.
 
+		string PowerState [readonly]
+
+			The power state of an adapter.
+
+			The power state will show whether the adapter is
+			turning off, or turning on, as well as being on
+			or off.
+
+			Possible values:
+				"on" - powered on
+				"off" - powered off
+				"off-enabling" - transitioning from "off" to "on"
+				"on-disabling" - transitioning from "on" to "off"
+				"off-blocked" - blocked by rfkill
+
 		boolean Discoverable [readwrite]
 
 			Switch an adapter to discoverable or non-discoverable
-- 
2.37.2

[PATCH BlueZ v4 5/6] adapter: Fix typo in function name

[Bastien Nocera]

---
 src/adapter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index e295ef247..11a21ca5c 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3455,7 +3455,7 @@ static gboolean property_get_experimental(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
-static gboolean property_experimental_exits(const GDBusPropertyTable *property,
+static gboolean property_experimental_exists(const GDBusPropertyTable *property,
 								void *data)
 {
 	struct btd_adapter *adapter = data;
@@ -3823,7 +3823,7 @@ static const GDBusPropertyTable adapter_properties[] = {
 					property_exists_modalias },
 	{ "Roles", "as", property_get_roles },
 	{ "ExperimentalFeatures", "as", property_get_experimental, NULL,
-					property_experimental_exits },
+					property_experimental_exists },
 	{ }
 };
 
-- 
2.37.2

[PATCH BlueZ v4 6/6] adapter: Remove experimental flag for PowerState

[Bastien Nocera]

Now that the feature has been tested, that the API is deemed adequate
and the reliability sufficient.
---
 src/adapter.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index 11a21ca5c..2a4a0a977 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3808,8 +3808,7 @@ static const GDBusPropertyTable adapter_properties[] = {
 	{ "Alias", "s", property_get_alias, property_set_alias },
 	{ "Class", "u", property_get_class },
 	{ "Powered", "b", property_get_powered, property_set_powered },
-	{ "PowerState", "s", property_get_power_state, NULL, NULL,
-			     G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
+	{ "PowerState", "s", property_get_power_state },
 	{ "Discoverable", "b", property_get_discoverable,
 					property_set_discoverable },
 	{ "DiscoverableTimeout", "u", property_get_discoverable_timeout,
-- 
2.37.2

Re: [PATCH BlueZ v4 6/6] adapter: Remove experimental flag for PowerState

[Bastien Nocera]

You'll want to give a miss to this one in the first pass ;)

On Wed, 2022-08-31 at 11:19 +0200, Bastien Nocera wrote:
> Now that the feature has been tested, that the API is deemed adequate
> and the reliability sufficient.
> ---
>  src/adapter.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/adapter.c b/src/adapter.c
> index 11a21ca5c..2a4a0a977 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -3808,8 +3808,7 @@ static const GDBusPropertyTable
> adapter_properties[] = {
>         { "Alias", "s", property_get_alias, property_set_alias },
>         { "Class", "u", property_get_class },
>         { "Powered", "b", property_get_powered, property_set_powered
> },
> -       { "PowerState", "s", property_get_power_state, NULL, NULL,
> -                            G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
> +       { "PowerState", "s", property_get_power_state },
>         { "Discoverable", "b", property_get_discoverable,
>                                         property_set_discoverable },
>         { "DiscoverableTimeout", "u",
> property_get_discoverable_timeout,

RE: [BlueZ,v4,1/6] adapter: Keep track of whether the adapter is rfkill'ed

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 4502 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.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=672764

---Test result---

Test Summary:
CheckPatch                    FAIL      9.12 seconds
GitLint                       PASS      6.06 seconds
Prep - Setup ELL              PASS      31.33 seconds
Build - Prep                  PASS      0.88 seconds
Build - Configure             PASS      10.02 seconds
Build - Make                  PASS      941.21 seconds
Make Check                    PASS      12.27 seconds
Make Check w/Valgrind         PASS      338.64 seconds
Make Distcheck                PASS      277.34 seconds
Build w/ext ELL - Configure   PASS      10.07 seconds
Build w/ext ELL - Make        PASS      97.46 seconds
Incremental Build w/ patches  PASS      702.02 seconds
Scan Build                    PASS      736.92 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ,v4,1/6] adapter: Keep track of whether the adapter is rfkill'ed
ERROR:CODE_INDENT: code indent should use tabs where possible
#95: FILE: src/adapter.c:7558:
+^I        g_dbus_emit_property_changed(dbus_conn, adapter->path,$

ERROR:CODE_INDENT: code indent should use tabs where possible
#110: FILE: src/adapter.c:7574:
+^I        g_dbus_emit_property_changed(dbus_conn, adapter->path,$

/github/workspace/src/12960589.patch total: 2 errors, 0 warnings, 90 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.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

/github/workspace/src/12960589.patch has style problems, please review.

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

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

[BlueZ,v4,2/6] adapter: Implement PowerState property
ERROR:CODE_INDENT: code indent should use tabs where possible
#96: FILE: src/adapter.c:590:
+^I        g_dbus_emit_property_changed(dbus_conn, adapter->path,$

WARNING:LONG_LINE: line length of 97 exceeds 80 columns
#123: FILE: src/adapter.c:659:
+		reset_power_state_target(adapter, settings & MGMT_SETTING_POWERED ? 0x01 : 0x00);

WARNING:LONG_LINE_STRING: line length of 81 exceeds 80 columns
#153: FILE: src/adapter.c:727:
+						ADAPTER_INTERFACE, "PowerState");

WARNING:LONG_LINE_STRING: line length of 81 exceeds 80 columns
#175: FILE: src/adapter.c:761:
+						ADAPTER_INTERFACE, "PowerState");

WARNING:LONG_LINE_STRING: line length of 81 exceeds 80 columns
#209: FILE: src/adapter.c:3101:
+						ADAPTER_INTERFACE, "PowerState");

WARNING:LONG_LINE_STRING: line length of 81 exceeds 80 columns
#222: FILE: src/adapter.c:3115:
+						ADAPTER_INTERFACE, "PowerState");

WARNING:LONG_LINE: line length of 84 exceeds 80 columns
#239: FILE: src/adapter.c:3156:
+	} else if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_NONE) {

WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#245: FILE: src/adapter.c:3162:
+		if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_ON)

WARNING:LONG_LINE_STRING: line length of 81 exceeds 80 columns
#273: FILE: src/adapter.c:5620:
+						ADAPTER_INTERFACE, "PowerState");

WARNING:LONG_LINE_STRING: line length of 81 exceeds 80 columns
#282: FILE: src/adapter.c:7365:
+						ADAPTER_INTERFACE, "PowerState");

/github/workspace/src/12960593.patch total: 1 errors, 9 warnings, 197 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.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

/github/workspace/src/12960593.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG 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

Re: [PATCH BlueZ v4 1/6] adapter: Keep track of whether the adapter is rfkill'ed

[Bastien Nocera]

On Wed, 2022-08-31 at 11:19 +0200, Bastien Nocera wrote:
> @@ -7548,6 +7553,12 @@ int btd_cancel_authorization(guint id)
>  
>  int btd_adapter_restore_powered(struct btd_adapter *adapter)
>  {
> +       if (adapter->blocked) {
> +               adapter->blocked = false;
> +               g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                       ADAPTER_INTERFACE, "PowerState");

This should have been in the next patch.

> +       }
> +
>         if (btd_adapter_get_powered(adapter))
>                 return 0;
>  
> @@ -7556,6 +7567,16 @@ int btd_adapter_restore_powered(struct btd_adapter *adapter)
>         return 0;
>  }
>  
> +int btd_adapter_set_blocked(struct btd_adapter *adapter)
> +{
> +       if (!adapter->blocked) {
> +               adapter->blocked = true;
> +               g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                       ADAPTER_INTERFACE, "PowerState");

Ditto.

Re: [BlueZ,v4,1/6] adapter: Keep track of whether the adapter is rfkill'ed

[Bastien Nocera]

On Wed, 2022-08-31 at 03:23 -0700, bluez.test.bot@gmail.com wrote:
> 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.
> This is a CI test results with your patch series:
> PW
> Link:https://patchwork.kernel.org/project/bluetooth/list/?series=6727
> 64
> 
> ---Test result---
> 
> Test Summary:
> CheckPatch                    FAIL      9.12 seconds

All those should be fixed in v5.

Re: [PATCH BlueZ v4 2/6] adapter: Implement PowerState property

[Luiz Augusto von Dentz]

Hi Bastien,

On Wed, Aug 31, 2022 at 2:32 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> This property should allow any program to show whether an adapter is in
> the process of being turned on.
>
> As turning on an adapter isn't instantaneous, it's important that the UI
> reflects the transitional state of the adapter's power, and doesn't
> assume the device is already turned on but not yet working, or still off
> despite having requested for it to be turned on, in both cases making
> the UI feel unresponsive.
>
> This can also not be implemented in front-ends directly as, then,
> the status of an adapter wouldn't be reflected correctly in the Settings
> window if it's turned on in the system menu. Implementing it in the
> front-ends would also preclude from having feedback about the state of
> the adapter when bluetoothd is the one powering up the adapter after the
> rfkill was unblocked.
>
> See https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/121
> and the original https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/5773
> ---
>  src/adapter.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 641db67f9..e295ef247 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -239,6 +239,12 @@ struct btd_adapter_pin_cb_iter {
>         /* When the iterator reaches the end, it is NULL and attempt is 0 */
>  };
>
> +enum {
> +       ADAPTER_POWER_STATE_TARGET_NONE = 0,
> +       ADAPTER_POWER_STATE_TARGET_OFF,
> +       ADAPTER_POWER_STATE_TARGET_ON
> +};

Lets take out the TARGET portion and have all the states PowerState
can assume including the transitioning ones.

> +
>  struct btd_adapter {
>         int ref_count;
>
> @@ -253,6 +259,7 @@ struct btd_adapter {
>         bool blocked;                   /* whether rfkill is enabled */
>         uint32_t supported_settings;    /* controller supported settings */
>         uint32_t pending_settings;      /* pending controller settings */
> +       uint32_t power_state_target;    /* the target power state */

Let's have the value stored as the enum here so it reflects directly
the values PowerState property can assume.

>         uint32_t current_settings;      /* current controller settings */
>
>         char *path;                     /* adapter object path */
> @@ -580,6 +587,8 @@ static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
>         if (changed_mask & MGMT_SETTING_POWERED) {
>                 g_dbus_emit_property_changed(dbus_conn, adapter->path,
>                                         ADAPTER_INTERFACE, "Powered");
> +               g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                       ADAPTER_INTERFACE, "PowerState");
>
>                 if (adapter->current_settings & MGMT_SETTING_POWERED) {
>                         adapter_start(adapter);
> @@ -619,6 +628,16 @@ static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
>         }
>  }
>
> +static void reset_power_state_target(struct btd_adapter *adapter, uint8_t value)
> +{
> +       if ((value &&
> +            adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_ON) ||
> +           (!value &&
> +            adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_OFF)) {
> +               adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
> +       }
> +}

Id suggest we add DBG statement to make it easier to debug, and rework
a little bit so it takes care of updating the property when the state
changes:

static void adapter_set_power_state(struct btd_adapter *adapter, enum value)
{
    if (adapter->power_state == value)
        return;

    DBG("%s", adapter_power_state_str(value));
...update...

    g_dbus_emit_property_changed(dbus_conn, adapter->path,
                                               ADAPTER_INTERFACE, "PowerState");
}

>  static void new_settings_callback(uint16_t index, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> @@ -636,6 +655,9 @@ static void new_settings_callback(uint16_t index, uint16_t length,
>         if (settings == adapter->current_settings)
>                 return;
>
> +       if ((adapter->current_settings ^ settings) & MGMT_SETTING_POWERED)
> +               reset_power_state_target(adapter, settings & MGMT_SETTING_POWERED ? 0x01 : 0x00);
> +
>         DBG("Settings: 0x%08x", settings);
>
>         settings_changed(adapter, settings);
> @@ -644,6 +666,7 @@ static void new_settings_callback(uint16_t index, uint16_t length,
>  struct set_mode_data {
>         struct btd_adapter *adapter;
>         uint32_t setting;
> +       uint8_t value;
>  };
>
>  static void set_mode_complete(uint8_t status, uint16_t length,
> @@ -658,6 +681,8 @@ static void set_mode_complete(uint8_t status, uint16_t length,
>                 if (status == MGMT_STATUS_RFKILLED)
>                         adapter->blocked = true;
>                 adapter->pending_settings &= ~data->setting;
> +               if (data->setting & MGMT_SETTING_POWERED)
> +                       reset_power_state_target(adapter, data->value);
>                 return;
>         }
>
> @@ -695,6 +720,11 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
>         switch (opcode) {
>         case MGMT_OP_SET_POWERED:
>                 setting = MGMT_SETTING_POWERED;
> +               adapter->power_state_target = mode ?
> +                       ADAPTER_POWER_STATE_TARGET_ON :
> +                       ADAPTER_POWER_STATE_TARGET_OFF;
> +               g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                               ADAPTER_INTERFACE, "PowerState");
>                 break;
>         case MGMT_OP_SET_CONNECTABLE:
>                 setting = MGMT_SETTING_CONNECTABLE;
> @@ -715,6 +745,7 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
>         data = g_new0(struct set_mode_data, 1);
>         data->adapter = adapter;
>         data->setting = setting;
> +       data->value = mode;
>
>         if (mgmt_send(adapter->mgmt, opcode,
>                                 adapter->dev_id, sizeof(cp), &cp,
> @@ -722,8 +753,13 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
>                 adapter->pending_settings |= setting;
>                 return true;
>         }
> -
>         g_free(data);
> +       if (setting == MGMT_SETTING_POWERED) {
> +               /* cancel the earlier setting */
> +               adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
> +               g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                               ADAPTER_INTERFACE, "PowerState");
> +       }
>         btd_error(adapter->dev_id, "Failed to set mode for index %u",
>                                                         adapter->dev_id);
>
> @@ -2901,6 +2937,7 @@ struct property_set_data {
>         struct btd_adapter *adapter;
>         uint32_t setting;
>         GDBusPendingPropertySet id;
> +       uint8_t value;
>  };
>
>  static void property_set_mode_complete(uint8_t status, uint16_t length,
> @@ -2928,6 +2965,8 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
>                                                         mgmt_errstr(status));
>
>                 adapter->pending_settings &= ~data->setting;
> +               if (data->setting & MGMT_SETTING_POWERED)
> +                       reset_power_state_target(adapter, data->value);
>                 return;
>         }
>
> @@ -3051,6 +3090,16 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>         data->adapter = adapter;
>         data->setting = setting;
>         data->id = id;
> +       data->setting = setting;
> +       data->value = mode;
> +
> +       if (setting == MGMT_SETTING_POWERED) {
> +               adapter->power_state_target = mode ?
> +                       ADAPTER_POWER_STATE_TARGET_ON :
> +                       ADAPTER_POWER_STATE_TARGET_OFF;
> +               g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                               ADAPTER_INTERFACE, "PowerState");
> +       }
>
>         if (mgmt_send(adapter->mgmt, opcode, adapter->dev_id, len, param,
>                         property_set_mode_complete, data, g_free) > 0) {
> @@ -3059,6 +3108,12 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>         }
>
>         g_free(data);
> +       if (setting == MGMT_SETTING_POWERED) {
> +               /* cancel the earlier setting */
> +               adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
> +               g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                               ADAPTER_INTERFACE, "PowerState");
> +       }
>
>  failed:
>         btd_error(adapter->dev_id, "Failed to set mode for index %u",
> @@ -3090,6 +3145,31 @@ static void property_set_powered(const GDBusPropertyTable *property,
>         property_set_mode(adapter, MGMT_SETTING_POWERED, iter, id);
>  }
>
> +static gboolean property_get_power_state(const GDBusPropertyTable *property,
> +                                       DBusMessageIter *iter, void *user_data)
> +{
> +       struct btd_adapter *adapter = user_data;
> +       const char *str;
> +
> +       if (adapter->blocked) {
> +               str = "off-blocked";
> +       } else if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_NONE) {
> +               if (adapter->current_settings & MGMT_SETTING_POWERED)
> +                       str = "on";
> +               else
> +                       str = "off";
> +       } else {
> +               if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_ON)
> +                       str = "off-enabling";
> +               else
> +                       str = "on-disabling";
> +       }
> +
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &str);

With the suggestion above this should be just a matter of:

    const char *str = adapter_power_state_str(adapter->power_state);

    dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &str);

So any updates to the enum, etc, are reflected directly as well since
we will need to update adapter_power_state_str.

> +       return TRUE;
> +}
> +
>  static gboolean property_get_discoverable(const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *user_data)
>  {
> @@ -3728,6 +3808,8 @@ static const GDBusPropertyTable adapter_properties[] = {
>         { "Alias", "s", property_get_alias, property_set_alias },
>         { "Class", "u", property_get_class },
>         { "Powered", "b", property_get_powered, property_set_powered },
> +       { "PowerState", "s", property_get_power_state, NULL, NULL,
> +                            G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
>         { "Discoverable", "b", property_get_discoverable,
>                                         property_set_discoverable },
>         { "DiscoverableTimeout", "u", property_get_discoverable_timeout,
> @@ -5534,6 +5616,8 @@ static void adapter_start(struct btd_adapter *adapter)
>  {
>         g_dbus_emit_property_changed(dbus_conn, adapter->path,
>                                                 ADAPTER_INTERFACE, "Powered");
> +       g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                               ADAPTER_INTERFACE, "PowerState");
>
>         DBG("adapter %s has been enabled", adapter->path);
>
> @@ -7277,6 +7361,8 @@ static void adapter_stop(struct btd_adapter *adapter)
>
>         g_dbus_emit_property_changed(dbus_conn, adapter->path,
>                                                 ADAPTER_INTERFACE, "Powered");
> +       g_dbus_emit_property_changed(dbus_conn, adapter->path,
> +                                               ADAPTER_INTERFACE, "PowerState");
>
>         DBG("adapter %s has been disabled", adapter->path);
>  }
> --
> 2.37.2
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v4 2/6] adapter: Implement PowerState property

[Bastien Nocera]

On Wed, 2022-08-31 at 13:03 -0700, Luiz Augusto von Dentz wrote:
> > +enum {
> > +       ADAPTER_POWER_STATE_TARGET_NONE = 0,
> > +       ADAPTER_POWER_STATE_TARGET_OFF,
> > +       ADAPTER_POWER_STATE_TARGET_ON
> > +};
> 
> Lets take out the TARGET portion and have all the states PowerState
> can assume including the transitioning ones.

Done, with the rest of the changes, in v5.

I've also fixed the initial rfkill state if the adapter is blocked
before bluetoothd is started.