[PATCH 0/4] asus-ec-sensors: add support for board families

[PATCH 0/4] asus-ec-sensors: add support for board families

[Eugene Shalygin]

Users provided information for boards from AMD-400 and sTRX40 families
and demonstrated that sensor addresses differ from those for the AMD-500
family. Also the AMD-400 family board uses the global ACPI lock instead
of a dedicated mutex to guard access to the hardware. 

This patchset implements required changes to support other board
families:
 - per-family sensor definitions
 - options to choose hardware/state guard mutex: an AML mutex, the
	 global ACPI lock, and a regular mutex in case no ACPI lock required.

These changes are used to add support for the PRIME X470-PRO board.

Eugene Shalygin (4):
  hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data
  hwmon: (asus-ec-sensors) implement locking via the ACPI global lock
  hwmon: (asus-ec-sensors) add support for board families
  hwmon: (asus-ec-sensors) add PRIME X470-PRO board

 drivers/hwmon/asus-ec-sensors.c | 431 +++++++++++++++++++++++---------
 1 file changed, 310 insertions(+), 121 deletions(-)

-- 
2.35.1

[PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

We need to keep some more information about the current board than just
the sensors set, and with more boards to add the dmi id array grows
quickly. Our probe code is always the same so let's switch to a custom
test code and a custom board info array. That allows us to omit board
vendor string (ASUS uses two strings that differ in case) in the board
info and use case-insensitive comparison, and also do not duplicate
sensor definitions for such board variants as " (WI-FI)" when sensors
are identical to the base variant.

Also saves a quarter of the module size by replacing big dmi_system_id
structs with smaller ones.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 209 ++++++++++++++++++--------------
 1 file changed, 119 insertions(+), 90 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index b5cf0136360c..7e28fc62f717 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -54,8 +54,7 @@ static char *mutex_path_override;
 /* ACPI mutex for locking access to the EC for the firmware */
 #define ASUS_HW_ACCESS_MUTEX_ASMX	"\\AMW0.ASMX"
 
-/* There are two variants of the vendor spelling */
-#define VENDOR_ASUS_UPPER_CASE	"ASUSTeK COMPUTER INC."
+#define MAX_IDENTICAL_BOARD_VARIATIONS	2
 
 typedef union {
 	u32 value;
@@ -164,68 +163,88 @@ static const struct ec_sensor_info known_ec_sensors[] = {
 	(SENSOR_TEMP_CHIPSET | SENSOR_TEMP_CPU | SENSOR_TEMP_MB)
 #define SENSOR_SET_TEMP_WATER (SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT)
 
-#define DMI_EXACT_MATCH_BOARD(vendor, name, sensors) {                         \
-	.matches = {                                                           \
-		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor),                     \
-		DMI_EXACT_MATCH(DMI_BOARD_NAME, name),                         \
-	},                                                                     \
-	.driver_data = (void *)(sensors), \
-}
+struct ec_board_info {
+	const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
+	unsigned long sensors;
+};
 
-static const struct dmi_system_id asus_ec_dmi_table[] __initconst = {
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "PRIME X570-PRO",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "Pro WS X570-ACE",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
-		SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII DARK HERO",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
-		SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII FORMULA",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG CROSSHAIR VIII HERO",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
-		SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
-		SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII HERO (WI-FI)",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
-		SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
-		SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
-			      "ROG CROSSHAIR VIII IMPACT",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-E GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-I GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_VRM_HS |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-E GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR |
-		SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET |
-		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-F GAMING",
-		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET),
-	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-I GAMING",
-		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
-		SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
+static const struct ec_board_info board_info[] __initconst = {
+	{
+		.board_names = {"PRIME X570-PRO"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
+			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+	},
+	{
+		.board_names = {"Pro WS X570-ACE"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR |
+			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
+			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {
+			"ROG CROSSHAIR VIII HERO",
+			"ROG CROSSHAIR VIII HERO (WI-FI)",
+		},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR |
+			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
+			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
+			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG STRIX B550-E GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CPU_OPT,
+	},
+	{
+		.board_names = {"ROG STRIX B550-I GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG STRIX X570-E GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
+	{
+		.board_names = {"ROG STRIX X570-F GAMING"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+	},
+	{
+		.board_names = {"ROG STRIX X570-I GAMING"},
+		.sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
+			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
+			SENSOR_IN_CPU_CORE,
+	},
 	{}
 };
 
@@ -235,7 +254,7 @@ struct ec_sensor {
 };
 
 struct ec_sensors_data {
-	unsigned long board_sensors;
+	struct ec_board_info board_info;
 	struct ec_sensor *sensors;
 	/* EC registers to read from */
 	u16 *registers;
@@ -245,8 +264,6 @@ struct ec_sensors_data {
 	/* in jiffies */
 	unsigned long last_updated;
 	acpi_handle aml_mutex;
-	/* number of board EC sensors */
-	u8 nr_sensors;
 	/*
 	 * number of EC registers to read
 	 * (sensor might span more than 1 register)
@@ -281,12 +298,17 @@ get_sensor_info(const struct ec_sensors_data *state, int index)
 	return &known_ec_sensors[state->sensors[index].info_index];
 }
 
+static int sensor_count(const struct ec_board_info *board)
+{
+	return hweight_long(board->sensors);
+}
+
 static int find_ec_sensor_index(const struct ec_sensors_data *ec,
 				enum hwmon_sensor_types type, int channel)
 {
 	unsigned int i;
 
-	for (i = 0; i < ec->nr_sensors; i++) {
+	for (i = 0; i < sensor_count(&ec->board_info); i++) {
 		if (get_sensor_info(ec, i)->type == type) {
 			if (channel == 0)
 				return i;
@@ -301,11 +323,6 @@ static int __init bank_compare(const void *a, const void *b)
 	return *((const s8 *)a) - *((const s8 *)b);
 }
 
-static int __init board_sensors_count(unsigned long sensors)
-{
-	return hweight_long(sensors);
-}
-
 static void __init setup_sensor_data(struct ec_sensors_data *ec)
 {
 	struct ec_sensor *s = ec->sensors;
@@ -316,8 +333,8 @@ static void __init setup_sensor_data(struct ec_sensors_data *ec)
 	ec->nr_banks = 0;
 	ec->nr_registers = 0;
 
-	for_each_set_bit(i, &ec->board_sensors,
-			  BITS_PER_TYPE(ec->board_sensors)) {
+	for_each_set_bit(i, &ec->board_info.sensors,
+			 BITS_PER_TYPE(ec->board_info.sensors)) {
 		s->info_index = i;
 		s->cached_value = 0;
 		ec->nr_registers +=
@@ -343,7 +360,7 @@ static void __init fill_ec_registers(struct ec_sensors_data *ec)
 	const struct ec_sensor_info *si;
 	unsigned int i, j, register_idx = 0;
 
-	for (i = 0; i < ec->nr_sensors; ++i) {
+	for (i = 0; i < sensor_count(&ec->board_info); ++i) {
 		si = get_sensor_info(ec, i);
 		for (j = 0; j < si->addr.components.size; ++j, ++register_idx) {
 			ec->registers[register_idx] =
@@ -457,9 +474,10 @@ static inline s32 get_sensor_value(const struct ec_sensor_info *si, u8 *data)
 static void update_sensor_values(struct ec_sensors_data *ec, u8 *data)
 {
 	const struct ec_sensor_info *si;
-	struct ec_sensor *s;
+	struct ec_sensor *s, *sensor_end;
 
-	for (s = ec->sensors; s != ec->sensors + ec->nr_sensors; s++) {
+	sensor_end = ec->sensors + sensor_count(&ec->board_info);
+	for (s = ec->sensors; s != sensor_end; s++) {
 		si = &known_ec_sensors[s->info_index];
 		s->cached_value = get_sensor_value(si, data);
 		data += si->addr.components.size;
@@ -597,12 +615,24 @@ static struct hwmon_chip_info asus_ec_chip_info = {
 	.ops = &asus_ec_hwmon_ops,
 };
 
-static unsigned long __init get_board_sensors(void)
+static const struct ec_board_info * __init get_board_info(void)
 {
-	const struct dmi_system_id *dmi_entry =
-		dmi_first_match(asus_ec_dmi_table);
+	const char *dmi_board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
+	const char *dmi_board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	const struct ec_board_info *board;
+
+	if (!dmi_board_vendor || !dmi_board_name ||
+	    strcasecmp(dmi_board_vendor, "ASUSTeK COMPUTER INC."))
+		return NULL;
+
+	for (board = board_info; board->sensors; board++) {
+		if (match_string(board->board_names,
+				 MAX_IDENTICAL_BOARD_VARIATIONS,
+				 dmi_board_name) >= 0)
+			return board;
+	}
 
-	return dmi_entry ? (unsigned long)dmi_entry->driver_data : 0;
+	return NULL;
 }
 
 static int __init asus_ec_probe(struct platform_device *pdev)
@@ -610,17 +640,17 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	const struct hwmon_channel_info **ptr_asus_ec_ci;
 	int nr_count[hwmon_max] = { 0 }, nr_types = 0;
 	struct hwmon_channel_info *asus_ec_hwmon_chan;
+	const struct ec_board_info *pboard_info;
 	const struct hwmon_chip_info *chip_info;
 	struct device *dev = &pdev->dev;
 	struct ec_sensors_data *ec_data;
 	const struct ec_sensor_info *si;
 	enum hwmon_sensor_types type;
-	unsigned long board_sensors;
 	struct device *hwdev;
 	unsigned int i;
 
-	board_sensors = get_board_sensors();
-	if (!board_sensors)
+	pboard_info = get_board_info();
+	if (!pboard_info)
 		return -ENODEV;
 
 	ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data),
@@ -629,9 +659,8 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dev_set_drvdata(dev, ec_data);
-	ec_data->board_sensors = board_sensors;
-	ec_data->nr_sensors = board_sensors_count(ec_data->board_sensors);
-	ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors,
+	ec_data->board_info = *pboard_info;
+	ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info),
 					sizeof(struct ec_sensor), GFP_KERNEL);
 
 	setup_sensor_data(ec_data);
@@ -647,7 +676,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 
 	ec_data->aml_mutex = asus_hw_access_mutex(dev);
 
-	for (i = 0; i < ec_data->nr_sensors; ++i) {
+	for (i = 0; i < sensor_count(&ec_data->board_info); ++i) {
 		si = get_sensor_info(ec_data, i);
 		if (!nr_count[si->type])
 			++nr_types;
@@ -681,7 +710,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	}
 
 	dev_info(dev, "board has %d EC sensors that span %d registers",
-		 ec_data->nr_sensors, ec_data->nr_registers);
+		 sensor_count(&ec_data->board_info), ec_data->nr_registers);
 
 	hwdev = devm_hwmon_device_register_with_info(dev, "asusec",
 						     ec_data, chip_info, NULL);
@@ -703,8 +732,8 @@ static struct platform_driver asus_ec_sensors_platform_driver = {
 	},
 };
 
-MODULE_DEVICE_TABLE(dmi, asus_ec_dmi_table);
 module_platform_driver_probe(asus_ec_sensors_platform_driver, asus_ec_probe);
+MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
 
 module_param_named(mutex_path, mutex_path_override, charp, 0);
 MODULE_PARM_DESC(mutex_path,
-- 
2.35.1

[PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

For some board models ASUS uses the global ACPI lock to guard access to
the hardware, so do we.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 143 +++++++++++++++++++++++++-------
 1 file changed, 115 insertions(+), 28 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index 7e28fc62f717..34841eeb800f 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -56,6 +56,9 @@ static char *mutex_path_override;
 
 #define MAX_IDENTICAL_BOARD_VARIATIONS	2
 
+/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
+#define ACPI_GLOBAL_LOCK_PSEUDO_PATH	":GLOBAL_LOCK"
+
 typedef union {
 	u32 value;
 	struct {
@@ -166,6 +169,14 @@ static const struct ec_sensor_info known_ec_sensors[] = {
 struct ec_board_info {
 	const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
 	unsigned long sensors;
+	/*
+	 * Defines which mutex to use for guarding access to the state and the
+	 * hardware. Can be either a full path to an AML mutex or the
+	 * pseudo-path ACPI_GLOBAL_LOCK_PSEUDO_PATH to use the global ACPI lock,
+	 * or left empty to use a regular mutex object, in which case access to
+	 * the hardware is not guarded.
+	 */
+	const char *mutex_path;
 };
 
 static const struct ec_board_info board_info[] __initconst = {
@@ -173,12 +184,14 @@ static const struct ec_board_info board_info[] __initconst = {
 		.board_names = {"PRIME X570-PRO"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"Pro WS X570-ACE"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
@@ -187,6 +200,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
@@ -194,6 +208,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {
@@ -206,6 +221,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
 			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
@@ -213,12 +229,14 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX B550-E GAMING"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CPU_OPT,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX B550-I GAMING"},
@@ -226,6 +244,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX X570-E GAMING"},
@@ -233,17 +252,20 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX X570-F GAMING"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{
 		.board_names = {"ROG STRIX X570-I GAMING"},
 		.sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
+		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
 	},
 	{}
 };
@@ -253,6 +275,57 @@ struct ec_sensor {
 	s32 cached_value;
 };
 
+struct lock_data {
+	union {
+		acpi_handle aml;
+		u32 global_lock_handle;
+		struct mutex regular;
+	} mutex;
+	int (*lock)(struct lock_data *data);
+	int (*unlock)(struct lock_data *data);
+};
+
+/*
+ * The next function pairs implement options for locking access to the
+ * state and the EC
+ */
+static int lock_via_acpi_mutex(struct lock_data *data)
+{
+	/*
+	 * ASUS DSDT does not specify that access to the EC has to be guarded,
+	 * but firmware does access it via ACPI
+	 */
+	return acpi_acquire_mutex(data->mutex.aml, NULL,
+				  ACPI_LOCK_DELAY_MS);
+}
+
+static int unlock_acpi_mutex(struct lock_data *data)
+{
+	return acpi_release_mutex(data->mutex.aml, NULL);
+}
+
+static int lock_via_global_acpi_lock(struct lock_data *data)
+{
+	return acpi_acquire_global_lock(ACPI_LOCK_DELAY_MS,
+					&data->mutex.global_lock_handle);
+}
+
+static int unlock_global_acpi_lock(struct lock_data *data)
+{
+	return acpi_release_global_lock(data->mutex.global_lock_handle);
+}
+
+static int lock_via_mutex(struct lock_data *data)
+{
+	return mutex_trylock(&data->mutex.regular) ? 0 : -EBUSY;
+}
+
+static int unlock_mutex(struct lock_data *data)
+{
+	mutex_unlock(&data->mutex.regular);
+	return 0;
+}
+
 struct ec_sensors_data {
 	struct ec_board_info board_info;
 	struct ec_sensor *sensors;
@@ -263,7 +336,9 @@ struct ec_sensors_data {
 	u8 banks[ASUS_EC_MAX_BANK + 1];
 	/* in jiffies */
 	unsigned long last_updated;
-	acpi_handle aml_mutex;
+	struct lock_data lock_data;
+	/* number of board EC sensors */
+	u8 nr_sensors;
 	/*
 	 * number of EC registers to read
 	 * (sensor might span more than 1 register)
@@ -370,23 +445,36 @@ static void __init fill_ec_registers(struct ec_sensors_data *ec)
 	}
 }
 
-static acpi_handle __init asus_hw_access_mutex(struct device *dev)
+static int __init setup_lock_data(struct device *dev)
 {
 	const char *mutex_path;
-	acpi_handle res;
 	int status;
+	struct ec_sensors_data *state = dev_get_drvdata(dev);
 
 	mutex_path = mutex_path_override ?
-		mutex_path_override : ASUS_HW_ACCESS_MUTEX_ASMX;
-
-	status = acpi_get_handle(NULL, (acpi_string)mutex_path, &res);
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev,
-			"Could not get hardware access guard mutex '%s': error %d",
-			mutex_path, status);
-		return NULL;
+		mutex_path_override : state->board_info.mutex_path;
+
+	if (!mutex_path || !strlen(mutex_path)) {
+		mutex_init(&state->lock_data.mutex.regular);
+		state->lock_data.lock = lock_via_mutex;
+		state->lock_data.unlock = unlock_mutex;
+	} else if (!strcmp(mutex_path, ACPI_GLOBAL_LOCK_PSEUDO_PATH)) {
+		state->lock_data.lock = lock_via_global_acpi_lock;
+		state->lock_data.unlock = unlock_global_acpi_lock;
+	} else {
+		status = acpi_get_handle(NULL, (acpi_string)mutex_path,
+					 &state->lock_data.mutex.aml);
+		if (ACPI_FAILURE(status)) {
+			dev_err(dev,
+				"Failed to get hardware access guard AML mutex"
+				"'%s': error %d",
+				mutex_path, status);
+			return -ENOENT;
+		}
+		state->lock_data.lock = lock_via_acpi_mutex;
+		state->lock_data.unlock = unlock_acpi_mutex;
 	}
-	return res;
+	return 0;
 }
 
 static int asus_ec_bank_switch(u8 bank, u8 *old)
@@ -417,7 +505,8 @@ static int asus_ec_block_read(const struct device *dev,
 	if (prev_bank) {
 		/* oops... somebody else is working with the EC too */
 		dev_warn(dev,
-			"Concurrent access to the ACPI EC detected.\nRace condition possible.");
+			"Concurrent access to the ACPI EC detected.\n"
+			"Race condition possible.");
 	}
 
 	/* read registers minimizing bank switches. */
@@ -489,15 +578,9 @@ static int update_ec_sensors(const struct device *dev,
 {
 	int status;
 
-	/*
-	 * ASUS DSDT does not specify that access to the EC has to be guarded,
-	 * but firmware does access it via ACPI
-	 */
-	if (ACPI_FAILURE(acpi_acquire_mutex(ec->aml_mutex, NULL,
-					    ACPI_LOCK_DELAY_MS))) {
-		dev_err(dev, "Failed to acquire AML mutex");
-		status = -EBUSY;
-		goto cleanup;
+	if (ec->lock_data.lock(&ec->lock_data)) {
+		dev_warn(dev, "Failed to acquire mutex");
+		return -EBUSY;
 	}
 
 	status = asus_ec_block_read(dev, ec);
@@ -505,10 +588,10 @@ static int update_ec_sensors(const struct device *dev,
 	if (!status) {
 		update_sensor_values(ec, ec->read_buffer);
 	}
-	if (ACPI_FAILURE(acpi_release_mutex(ec->aml_mutex, NULL))) {
-		dev_err(dev, "Failed to release AML mutex");
-	}
-cleanup:
+
+	if (ec->lock_data.unlock(&ec->lock_data))
+		dev_err(dev, "Failed to release mutex");
+
 	return status;
 }
 
@@ -648,6 +731,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	enum hwmon_sensor_types type;
 	struct device *hwdev;
 	unsigned int i;
+	int status;
 
 	pboard_info = get_board_info();
 	if (!pboard_info)
@@ -663,6 +747,11 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 	ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info),
 					sizeof(struct ec_sensor), GFP_KERNEL);
 
+	status = setup_lock_data(dev);
+	if (status) {
+		dev_err(dev, "Failed to setup state/EC locking: %d", status);
+		return status;
+	}
 	setup_sensor_data(ec_data);
 	ec_data->registers = devm_kcalloc(dev, ec_data->nr_registers,
 					  sizeof(u16), GFP_KERNEL);
@@ -674,8 +763,6 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 
 	fill_ec_registers(ec_data);
 
-	ec_data->aml_mutex = asus_hw_access_mutex(dev);
-
 	for (i = 0; i < sensor_count(&ec_data->board_info); ++i) {
 		si = get_sensor_info(ec_data, i);
 		if (!nr_count[si->type])
-- 
2.35.1

[PATCH 3/4] hwmon: (asus-ec-sensors) add support for board families

[Eugene Shalygin]

DSDT code for AMD 400-series chipset shows that sensor addresses differ
for this generation from those for the AMD 500-series boards.

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 43 +++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index 34841eeb800f..b4060ed1f0fa 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -135,8 +135,12 @@ enum ec_sensors {
 #define SENSOR_TEMP_WATER_IN BIT(ec_sensor_temp_water_in)
 #define SENSOR_TEMP_WATER_OUT BIT(ec_sensor_temp_water_out)
 
+enum board_family {
+	family_amd_500_series,
+};
+
 /* All the known sensors for ASUS EC controllers */
-static const struct ec_sensor_info known_ec_sensors[] = {
+static const struct ec_sensor_info sensors_family_amd_500[] = {
 	[ec_sensor_temp_chipset] =
 		EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a),
 	[ec_sensor_temp_cpu] = EC_SENSOR("CPU", hwmon_temp, 1, 0x00, 0x3b),
@@ -177,6 +181,7 @@ struct ec_board_info {
 	 * the hardware is not guarded.
 	 */
 	const char *mutex_path;
+	enum board_family family;
 };
 
 static const struct ec_board_info board_info[] __initconst = {
@@ -185,6 +190,7 @@ static const struct ec_board_info board_info[] __initconst = {
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"Pro WS X570-ACE"},
@@ -192,6 +198,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
@@ -201,6 +208,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
@@ -209,6 +217,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
 			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {
@@ -222,6 +231,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
@@ -230,6 +240,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX B550-E GAMING"},
@@ -237,6 +248,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
 			SENSOR_FAN_CPU_OPT,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX B550-I GAMING"},
@@ -245,6 +257,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX X570-E GAMING"},
@@ -253,12 +266,14 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX X570-F GAMING"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
 			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{
 		.board_names = {"ROG STRIX X570-I GAMING"},
@@ -266,6 +281,7 @@ static const struct ec_board_info board_info[] __initconst = {
 			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
 			SENSOR_IN_CPU_CORE,
 		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
+		.family = family_amd_500_series,
 	},
 	{}
 };
@@ -328,6 +344,7 @@ static int unlock_mutex(struct lock_data *data)
 
 struct ec_sensors_data {
 	struct ec_board_info board_info;
+	const struct ec_sensor_info *sensors_info;
 	struct ec_sensor *sensors;
 	/* EC registers to read from */
 	u16 *registers;
@@ -370,7 +387,7 @@ static bool is_sensor_data_signed(const struct ec_sensor_info *si)
 static const struct ec_sensor_info *
 get_sensor_info(const struct ec_sensors_data *state, int index)
 {
-	return &known_ec_sensors[state->sensors[index].info_index];
+	return state->sensors_info + state->sensors[index].info_index;
 }
 
 static int sensor_count(const struct ec_board_info *board)
@@ -413,9 +430,9 @@ static void __init setup_sensor_data(struct ec_sensors_data *ec)
 		s->info_index = i;
 		s->cached_value = 0;
 		ec->nr_registers +=
-			known_ec_sensors[s->info_index].addr.components.size;
+			ec->sensors_info[s->info_index].addr.components.size;
 		bank_found = false;
-		bank = known_ec_sensors[s->info_index].addr.components.bank;
+		bank = ec->sensors_info[s->info_index].addr.components.bank;
 		for (j = 0; j < ec->nr_banks; j++) {
 			if (ec->banks[j] == bank) {
 				bank_found = true;
@@ -566,8 +583,9 @@ static void update_sensor_values(struct ec_sensors_data *ec, u8 *data)
 	struct ec_sensor *s, *sensor_end;
 
 	sensor_end = ec->sensors + sensor_count(&ec->board_info);
+
 	for (s = ec->sensors; s != sensor_end; s++) {
-		si = &known_ec_sensors[s->info_index];
+		si = ec->sensors_info + s->info_index;
 		s->cached_value = get_sensor_value(si, data);
 		data += si->addr.components.size;
 	}
@@ -744,14 +762,25 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, ec_data);
 	ec_data->board_info = *pboard_info;
-	ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info),
-					sizeof(struct ec_sensor), GFP_KERNEL);
+	switch (ec_data->board_info.family) {
+	case family_amd_500_series:
+		ec_data->sensors_info = sensors_family_amd_500;
+		break;
+	default:
+		dev_err(dev, "Unknown board family: %d",
+			ec_data->board_info.family);
+		return -EINVAL;
+	}
 
 	status = setup_lock_data(dev);
 	if (status) {
 		dev_err(dev, "Failed to setup state/EC locking: %d", status);
 		return status;
 	}
+
+	ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info),
+					sizeof(struct ec_sensor), GFP_KERNEL);
+
 	setup_sensor_data(ec_data);
 	ec_data->registers = devm_kcalloc(dev, ec_data->nr_registers,
 					  sizeof(u16), GFP_KERNEL);
-- 
2.35.1

[PATCH 4/4] hwmon: (asus-ec-sensors) add PRIME X470-PRO board

[Eugene Shalygin]

This board is supposed to be handled by the asus-wmi-sensors driver,
but due to a buggy WMI implementation the driver and the official ASUS
software make the BIOS hang together with fan controls.

This driver complements values provided by the SIO chip and does not
freeze the BIOS, as tested by a user [2].

[1] https://github.com/electrified/asus-wmi-sensors/blob/master/README.md
[2] https://github.com/zeule/asus-ec-sensors/issues/12

Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
---
 drivers/hwmon/asus-ec-sensors.c | 44 +++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
index b4060ed1f0fa..ce0e1d97b60b 100644
--- a/drivers/hwmon/asus-ec-sensors.c
+++ b/drivers/hwmon/asus-ec-sensors.c
@@ -136,10 +136,41 @@ enum ec_sensors {
 #define SENSOR_TEMP_WATER_OUT BIT(ec_sensor_temp_water_out)
 
 enum board_family {
+	family_amd_400_series,
 	family_amd_500_series,
 };
 
 /* All the known sensors for ASUS EC controllers */
+static const struct ec_sensor_info sensors_family_amd_400[] = {
+	[ec_sensor_temp_chipset] =
+		EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a),
+	[ec_sensor_temp_cpu] =
+		EC_SENSOR("CPU", hwmon_temp, 1, 0x00, 0x3b),
+	[ec_sensor_temp_mb] =
+		EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x3c),
+	[ec_sensor_temp_t_sensor] =
+		EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x3d),
+	[ec_sensor_temp_vrm] =
+		EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x3e),
+	[ec_sensor_in_cpu_core] =
+		EC_SENSOR("CPU Core", hwmon_in, 2, 0x00, 0xa2),
+	[ec_sensor_fan_cpu_opt] =
+		EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xbc),
+	[ec_sensor_fan_vrm_hs] =
+		EC_SENSOR("VRM HS", hwmon_fan, 2, 0x00, 0xb2),
+	[ec_sensor_fan_chipset] =
+		/* no chipset fans in this generation */
+		EC_SENSOR("Chipset", hwmon_fan, 0, 0x00, 0x00),
+	[ec_sensor_fan_water_flow] =
+		EC_SENSOR("Water_Flow", hwmon_fan, 2, 0x00, 0xb4),
+	[ec_sensor_curr_cpu] =
+		EC_SENSOR("CPU", hwmon_curr, 1, 0x00, 0xf4),
+	[ec_sensor_temp_water_in] =
+		EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x0d),
+	[ec_sensor_temp_water_out] =
+		EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x0b),
+};
+
 static const struct ec_sensor_info sensors_family_amd_500[] = {
 	[ec_sensor_temp_chipset] =
 		EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a),
@@ -185,6 +216,15 @@ struct ec_board_info {
 };
 
 static const struct ec_board_info board_info[] __initconst = {
+	{
+		.board_names = {"PRIME X470-PRO"},
+		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
+			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
+			SENSOR_FAN_CPU_OPT |
+			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
+		.mutex_path = ACPI_GLOBAL_LOCK_PSEUDO_PATH,
+		.family = family_amd_400_series,
+	},
 	{
 		.board_names = {"PRIME X570-PRO"},
 		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
@@ -762,7 +802,11 @@ static int __init asus_ec_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, ec_data);
 	ec_data->board_info = *pboard_info;
+
 	switch (ec_data->board_info.family) {
+	case family_amd_400_series:
+		ec_data->sensors_info = sensors_family_amd_400;
+		break;
 	case family_amd_500_series:
 		ec_data->sensors_info = sensors_family_amd_500;
 		break;
-- 
2.35.1

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

On Sun, 27 Mar 2022 at 14:15, Eugene Shalygin <eugene.shalygin@gmail.com> wrote:
> +       /* number of board EC sensors */
> +       u8 nr_sensors;

Re-added by mistake, will be fixed in the next patch version.

> +               if (ACPI_FAILURE(status)) {
> +                       dev_err(dev,
> +                               "Failed to get hardware access guard AML mutex"
> +                               "'%s': error %d",
> +                               mutex_path, status);

Can't choose between various options to fix this broken string
literal. Would be thankful, Günter, for a suggestion.

Best regards,
Eugene

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Guenter Roeck]

On 3/27/22 09:21, Eugene Shalygin wrote:
> On Sun, 27 Mar 2022 at 14:15, Eugene Shalygin <eugene.shalygin@gmail.com> wrote:
>> +       /* number of board EC sensors */
>> +       u8 nr_sensors;
> 
> Re-added by mistake, will be fixed in the next patch version.
> 
>> +               if (ACPI_FAILURE(status)) {
>> +                       dev_err(dev,
>> +                               "Failed to get hardware access guard AML mutex"
>> +                               "'%s': error %d",
>> +                               mutex_path, status);
> 
> Can't choose between various options to fix this broken string
> literal. Would be thankful, Günter, for a suggestion.
> 

What is the problem other than that the string is split across
multiple lines ? That can easily be fixed by not splitting it,
so you'll have to be more specific.

Guenter

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

On Sun, 27 Mar 2022 at 20:04, Guenter Roeck <linux@roeck-us.net> wrote:

> What is the problem other than that the string is split across
> multiple lines ? That can easily be fixed by not splitting it,
> so you'll have to be more specific.

Yes, the problem is that it is split. When merged it exceeds the 80-th
column and creates problems for editors instructed to format text on
paste. And I would not like to neither split the string onto two debug
messages, nor shorten the debug message itself. Maybe there is an
elegant solution I'm not aware of?

Thanks,
Eugene

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Guenter Roeck]

On 3/27/22 11:51, Eugene Shalygin wrote:
> On Sun, 27 Mar 2022 at 20:04, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> What is the problem other than that the string is split across
>> multiple lines ? That can easily be fixed by not splitting it,
>> so you'll have to be more specific.
> 
> Yes, the problem is that it is split. When merged it exceeds the 80-th
> column and creates problems for editors instructed to format text on
> paste. And I would not like to neither split the string onto two debug
> messages, nor shorten the debug message itself. Maybe there is an
> elegant solution I'm not aware of?
> 

First, you can go up to 100 columns nowadays. Second, the column
limit is waived for strings because it is more important to not
split them. If you _still_ want to stick with 80 columns, sorry,
no, I don't have a solution. Your problem is with the editor,
not with kernel formatting rules.

Guenter

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

> First, you can go up to 100 columns nowadays. Second, the column
> limit is waived for strings because it is more important to not
> split them. If you _still_ want to stick with 80 columns, sorry,
> no, I don't have a solution. Your problem is with the editor,
> not with kernel formatting rules.

Thank you, Günter, 100 is better than 80 and the string fits. I
wonder, why is the .clang-format file not updated and still says the
limit is 80?

Eugene

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Nathan Chancellor]

On Mon, Mar 28, 2022 at 04:44:24PM +0200, Eugene Shalygin wrote:
> > First, you can go up to 100 columns nowadays. Second, the column
> > limit is waived for strings because it is more important to not
> > split them. If you _still_ want to stick with 80 columns, sorry,
> > no, I don't have a solution. Your problem is with the editor,
> > not with kernel formatting rules.
> 
> Thank you, Günter, 100 is better than 80 and the string fits. I
> wonder, why is the .clang-format file not updated and still says the
> limit is 80?

Because the documentation still says that 80 is preferred:

https://kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings

"The preferred limit on the length of a single line is 80 columns.

Statements longer than 80 columns should be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and does
not hide information."

There have been a few different times that people have tried to update
the .clang-format file, which ultimately leads back to that paragraph in
the documentation.

https://lore.kernel.org/r/20200610125147.2782142-1-christian.brauner@ubuntu.com/
https://lore.kernel.org/r/03d462504887401ffbcdb58a392ad01923a2be7b.camel@perches.com/

A somewhat recent patch to try and update the documentation to match
checkpatch was posted but did not really go anywhere:

https://lore.kernel.org/r/d7130556-a8a4-76c0-0fde-b6b1439efda6@infradead.org/

Cheers,
Nathan

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

Thank you, Nathan!

Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Guenter Roeck]

On 3/27/22 05:14, Eugene Shalygin wrote:
> We need to keep some more information about the current board than just
> the sensors set, and with more boards to add the dmi id array grows
> quickly. Our probe code is always the same so let's switch to a custom
> test code and a custom board info array. That allows us to omit board
> vendor string (ASUS uses two strings that differ in case) in the board
> info and use case-insensitive comparison, and also do not duplicate
> sensor definitions for such board variants as " (WI-FI)" when sensors
> are identical to the base variant.
> 
> Also saves a quarter of the module size by replacing big dmi_system_id
> structs with smaller ones.
> 
> Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
> ---
>   drivers/hwmon/asus-ec-sensors.c | 209 ++++++++++++++++++--------------
>   1 file changed, 119 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
> index b5cf0136360c..7e28fc62f717 100644
> --- a/drivers/hwmon/asus-ec-sensors.c
> +++ b/drivers/hwmon/asus-ec-sensors.c
> @@ -54,8 +54,7 @@ static char *mutex_path_override;
>   /* ACPI mutex for locking access to the EC for the firmware */
>   #define ASUS_HW_ACCESS_MUTEX_ASMX	"\\AMW0.ASMX"
>   
> -/* There are two variants of the vendor spelling */
> -#define VENDOR_ASUS_UPPER_CASE	"ASUSTeK COMPUTER INC."
> +#define MAX_IDENTICAL_BOARD_VARIATIONS	2
>   
>   typedef union {
>   	u32 value;
> @@ -164,68 +163,88 @@ static const struct ec_sensor_info known_ec_sensors[] = {
>   	(SENSOR_TEMP_CHIPSET | SENSOR_TEMP_CPU | SENSOR_TEMP_MB)
>   #define SENSOR_SET_TEMP_WATER (SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT)
>   
> -#define DMI_EXACT_MATCH_BOARD(vendor, name, sensors) {                         \
> -	.matches = {                                                           \
> -		DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor),                     \
> -		DMI_EXACT_MATCH(DMI_BOARD_NAME, name),                         \
> -	},                                                                     \
> -	.driver_data = (void *)(sensors), \
> -}
> +struct ec_board_info {
> +	const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
> +	unsigned long sensors;
> +};
>   
> -static const struct dmi_system_id asus_ec_dmi_table[] __initconst = {
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "PRIME X570-PRO",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
> -		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "Pro WS X570-ACE",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
> -		SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
> -			      "ROG CROSSHAIR VIII DARK HERO",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
> -		SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
> -		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
> -			      "ROG CROSSHAIR VIII FORMULA",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
> -		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG CROSSHAIR VIII HERO",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
> -		SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
> -		SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
> -			      "ROG CROSSHAIR VIII HERO (WI-FI)",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
> -		SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
> -		SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE,
> -			      "ROG CROSSHAIR VIII IMPACT",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET |
> -		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-E GAMING",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> -		SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-I GAMING",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> -		SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_FAN_VRM_HS |
> -		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-E GAMING",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> -		SENSOR_TEMP_T_SENSOR |
> -		SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET |
> -		SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-F GAMING",
> -		SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> -		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET),
> -	DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-I GAMING",
> -		SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
> -		SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE),
> +static const struct ec_board_info board_info[] __initconst = {
> +	{
> +		.board_names = {"PRIME X570-PRO"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
> +			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
> +	},
> +	{
> +		.board_names = {"Pro WS X570-ACE"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
> +			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
> +			SENSOR_IN_CPU_CORE,
> +	},
> +	{
> +		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR |
> +			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
> +			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
> +			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
> +	},
> +	{
> +		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
> +			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
> +			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
> +	},
> +	{
> +		.board_names = {
> +			"ROG CROSSHAIR VIII HERO",
> +			"ROG CROSSHAIR VIII HERO (WI-FI)",
> +		},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR |
> +			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
> +			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
> +			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
> +			SENSOR_IN_CPU_CORE,
> +	},
> +	{
> +		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
> +			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
> +			SENSOR_IN_CPU_CORE,
> +	},
> +	{
> +		.board_names = {"ROG STRIX B550-E GAMING"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
> +			SENSOR_FAN_CPU_OPT,
> +	},
> +	{
> +		.board_names = {"ROG STRIX B550-I GAMING"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
> +			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
> +			SENSOR_IN_CPU_CORE,
> +	},
> +	{
> +		.board_names = {"ROG STRIX X570-E GAMING"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
> +			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
> +			SENSOR_IN_CPU_CORE,
> +	},
> +	{
> +		.board_names = {"ROG STRIX X570-F GAMING"},
> +		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
> +			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
> +	},
> +	{
> +		.board_names = {"ROG STRIX X570-I GAMING"},
> +		.sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
> +			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
> +			SENSOR_IN_CPU_CORE,
> +	},
>   	{}
>   };
>   
> @@ -235,7 +254,7 @@ struct ec_sensor {
>   };
>   
>   struct ec_sensors_data {
> -	unsigned long board_sensors;
> +	struct ec_board_info board_info;

Please explain why this needs to be the entire structure and not
just a pointer to it.

>   	struct ec_sensor *sensors;
>   	/* EC registers to read from */
>   	u16 *registers;
> @@ -245,8 +264,6 @@ struct ec_sensors_data {
>   	/* in jiffies */
>   	unsigned long last_updated;
>   	acpi_handle aml_mutex;
> -	/* number of board EC sensors */
> -	u8 nr_sensors;
>   	/*
>   	 * number of EC registers to read
>   	 * (sensor might span more than 1 register)
> @@ -281,12 +298,17 @@ get_sensor_info(const struct ec_sensors_data *state, int index)
>   	return &known_ec_sensors[state->sensors[index].info_index];
>   }
>   
> +static int sensor_count(const struct ec_board_info *board)
> +{
> +	return hweight_long(board->sensors);
> +}

This function is called several times. Does it really make sense, or is it
necessary, to re-calculate the number of sensors over and over again
instead of keeping it in ec->nr_sensors as before ? What are the benefits ?
Unless there is a good explanation I see that as unrelated and unnecessary
change.

> +
>   static int find_ec_sensor_index(const struct ec_sensors_data *ec,
>   				enum hwmon_sensor_types type, int channel)
>   {
>   	unsigned int i;
>   
> -	for (i = 0; i < ec->nr_sensors; i++) {
> +	for (i = 0; i < sensor_count(&ec->board_info); i++) {
>   		if (get_sensor_info(ec, i)->type == type) {
>   			if (channel == 0)
>   				return i;
> @@ -301,11 +323,6 @@ static int __init bank_compare(const void *a, const void *b)
>   	return *((const s8 *)a) - *((const s8 *)b);
>   }
>   
> -static int __init board_sensors_count(unsigned long sensors)
> -{
> -	return hweight_long(sensors);
> -}
> -
>   static void __init setup_sensor_data(struct ec_sensors_data *ec)
>   {
>   	struct ec_sensor *s = ec->sensors;
> @@ -316,8 +333,8 @@ static void __init setup_sensor_data(struct ec_sensors_data *ec)
>   	ec->nr_banks = 0;
>   	ec->nr_registers = 0;
>   
> -	for_each_set_bit(i, &ec->board_sensors,
> -			  BITS_PER_TYPE(ec->board_sensors)) {
> +	for_each_set_bit(i, &ec->board_info.sensors,
> +			 BITS_PER_TYPE(ec->board_info.sensors)) {
>   		s->info_index = i;
>   		s->cached_value = 0;
>   		ec->nr_registers +=
> @@ -343,7 +360,7 @@ static void __init fill_ec_registers(struct ec_sensors_data *ec)
>   	const struct ec_sensor_info *si;
>   	unsigned int i, j, register_idx = 0;
>   
> -	for (i = 0; i < ec->nr_sensors; ++i) {
> +	for (i = 0; i < sensor_count(&ec->board_info); ++i) {
>   		si = get_sensor_info(ec, i);
>   		for (j = 0; j < si->addr.components.size; ++j, ++register_idx) {
>   			ec->registers[register_idx] =
> @@ -457,9 +474,10 @@ static inline s32 get_sensor_value(const struct ec_sensor_info *si, u8 *data)
>   static void update_sensor_values(struct ec_sensors_data *ec, u8 *data)
>   {
>   	const struct ec_sensor_info *si;
> -	struct ec_sensor *s;
> +	struct ec_sensor *s, *sensor_end;
>   
> -	for (s = ec->sensors; s != ec->sensors + ec->nr_sensors; s++) {
> +	sensor_end = ec->sensors + sensor_count(&ec->board_info);
> +	for (s = ec->sensors; s != sensor_end; s++) {
>   		si = &known_ec_sensors[s->info_index];
>   		s->cached_value = get_sensor_value(si, data);
>   		data += si->addr.components.size;
> @@ -597,12 +615,24 @@ static struct hwmon_chip_info asus_ec_chip_info = {
>   	.ops = &asus_ec_hwmon_ops,
>   };
>   
> -static unsigned long __init get_board_sensors(void)
> +static const struct ec_board_info * __init get_board_info(void)
>   {
> -	const struct dmi_system_id *dmi_entry =
> -		dmi_first_match(asus_ec_dmi_table);
> +	const char *dmi_board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
> +	const char *dmi_board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +	const struct ec_board_info *board;
> +
> +	if (!dmi_board_vendor || !dmi_board_name ||
> +	    strcasecmp(dmi_board_vendor, "ASUSTeK COMPUTER INC."))
> +		return NULL;
> +
> +	for (board = board_info; board->sensors; board++) {
> +		if (match_string(board->board_names,
> +				 MAX_IDENTICAL_BOARD_VARIATIONS,
> +				 dmi_board_name) >= 0)
> +			return board;
> +	}
>   
> -	return dmi_entry ? (unsigned long)dmi_entry->driver_data : 0;
> +	return NULL;
>   }
>   
>   static int __init asus_ec_probe(struct platform_device *pdev)
> @@ -610,17 +640,17 @@ static int __init asus_ec_probe(struct platform_device *pdev)
>   	const struct hwmon_channel_info **ptr_asus_ec_ci;
>   	int nr_count[hwmon_max] = { 0 }, nr_types = 0;
>   	struct hwmon_channel_info *asus_ec_hwmon_chan;
> +	const struct ec_board_info *pboard_info;
>   	const struct hwmon_chip_info *chip_info;
>   	struct device *dev = &pdev->dev;
>   	struct ec_sensors_data *ec_data;
>   	const struct ec_sensor_info *si;
>   	enum hwmon_sensor_types type;
> -	unsigned long board_sensors;
>   	struct device *hwdev;
>   	unsigned int i;
>   
> -	board_sensors = get_board_sensors();
> -	if (!board_sensors)
> +	pboard_info = get_board_info();
> +	if (!pboard_info)
>   		return -ENODEV;
>   
>   	ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data),
> @@ -629,9 +659,8 @@ static int __init asus_ec_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	dev_set_drvdata(dev, ec_data);
> -	ec_data->board_sensors = board_sensors;
> -	ec_data->nr_sensors = board_sensors_count(ec_data->board_sensors);
> -	ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors,
> +	ec_data->board_info = *pboard_info;
> +	ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info),
>   					sizeof(struct ec_sensor), GFP_KERNEL);
>   
>   	setup_sensor_data(ec_data);
> @@ -647,7 +676,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
>   
>   	ec_data->aml_mutex = asus_hw_access_mutex(dev);
>   
> -	for (i = 0; i < ec_data->nr_sensors; ++i) {
> +	for (i = 0; i < sensor_count(&ec_data->board_info); ++i) {
>   		si = get_sensor_info(ec_data, i);
>   		if (!nr_count[si->type])
>   			++nr_types;
> @@ -681,7 +710,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
>   	}
>   
>   	dev_info(dev, "board has %d EC sensors that span %d registers",
> -		 ec_data->nr_sensors, ec_data->nr_registers);
> +		 sensor_count(&ec_data->board_info), ec_data->nr_registers);
>   
>   	hwdev = devm_hwmon_device_register_with_info(dev, "asusec",
>   						     ec_data, chip_info, NULL);
> @@ -703,8 +732,8 @@ static struct platform_driver asus_ec_sensors_platform_driver = {
>   	},
>   };
>   
> -MODULE_DEVICE_TABLE(dmi, asus_ec_dmi_table);
>   module_platform_driver_probe(asus_ec_sensors_platform_driver, asus_ec_probe);
> +MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);

Why is MODULE_DEVICE_TABLE moved ?

>   
>   module_param_named(mutex_path, mutex_path_override, charp, 0);
>   MODULE_PARM_DESC(mutex_path,

Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

On Tue, 29 Mar 2022 at 15:44, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> >   struct ec_sensors_data {
> > -     unsigned long board_sensors;
> > +     struct ec_board_info board_info;
>
> Please explain why this needs to be the entire structure and not
> just a pointer to it.

I marked the board_info array as __initconst assuming that this large
array will be unloaded from memory after the init phase, while we keep
only a single element. Is that assumption incorrect?

> > +static int sensor_count(const struct ec_board_info *board)
> > +{
> > +     return hweight_long(board->sensors);
> > +}
>
> This function is called several times. Does it really make sense, or is it
> necessary, to re-calculate the number of sensors over and over again
> instead of keeping it in ec->nr_sensors as before ? What are the benefits ?
> Unless there is a good explanation I see that as unrelated and unnecessary
> change.

This had something to do with data deduplication. However, I need the
count value only for looping over the sensor array, thus I can as well
add an invalid element to the end of the array. I rushed to submit
this driver to replace the wmi one, and it still has an artifact for
the WMI code I'd like to get rid of eventually, which is the read
buffer and the registers array. This will remove all the nr_ variables
and two dynamically allocated arrays. I will understand, of course, if
you ask to submit that refactoring separately.

> > -MODULE_DEVICE_TABLE(dmi, asus_ec_dmi_table);
> >   module_platform_driver_probe(asus_ec_sensors_platform_driver, asus_ec_probe);
> > +MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
>
> Why is MODULE_DEVICE_TABLE moved ?
Accidentally, probably. Thank you, will be corrected.


Thanks,
Eugene

Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Guenter Roeck]

On 3/29/22 12:22, Eugene Shalygin wrote:
> On Tue, 29 Mar 2022 at 15:44, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>>    struct ec_sensors_data {
>>> -     unsigned long board_sensors;
>>> +     struct ec_board_info board_info;
>>
>> Please explain why this needs to be the entire structure and not
>> just a pointer to it.
> 
> I marked the board_info array as __initconst assuming that this large
> array will be unloaded from memory after the init phase, while we keep
> only a single element. Is that assumption incorrect?
> 

What happens if you build the driver into the kernel and then instantiate
and de-instantiate it multiple times ?

>>> +static int sensor_count(const struct ec_board_info *board)
>>> +{
>>> +     return hweight_long(board->sensors);
>>> +}
>>
>> This function is called several times. Does it really make sense, or is it
>> necessary, to re-calculate the number of sensors over and over again
>> instead of keeping it in ec->nr_sensors as before ? What are the benefits ?
>> Unless there is a good explanation I see that as unrelated and unnecessary
>> change.
> 
> This had something to do with data deduplication. However, I need the
> count value only for looping over the sensor array, thus I can as well
> add an invalid element to the end of the array. I rushed to submit
> this driver to replace the wmi one, and it still has an artifact for
> the WMI code I'd like to get rid of eventually, which is the read
> buffer and the registers array. This will remove all the nr_ variables
> and two dynamically allocated arrays. I will understand, of course, if
> you ask to submit that refactoring separately.
> 

The rule of "one logical change per patch" still applies. If you start
intermixing parts of future clean-up efforts into current patches, you'll
see a very unhappy maintainer - especially since this change makes up
a significant part of this patch, complicates review significantly,
and makes me wonder if other unrelated changes are included that I don't
see right now due to all the noise.

Besides, at least in this patch, I don't buy the "deduplication" argument.
Keeping a single additional variable in a data structure is much simpler
and straightforward than calling hweight_long() several times. I'd call
that "complification".

Guenter

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Guenter Roeck]

On 3/27/22 05:14, Eugene Shalygin wrote:
> For some board models ASUS uses the global ACPI lock to guard access to
> the hardware, so do we.
> 
> Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com>
> ---
>   drivers/hwmon/asus-ec-sensors.c | 143 +++++++++++++++++++++++++-------
>   1 file changed, 115 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c
> index 7e28fc62f717..34841eeb800f 100644
> --- a/drivers/hwmon/asus-ec-sensors.c
> +++ b/drivers/hwmon/asus-ec-sensors.c
> @@ -56,6 +56,9 @@ static char *mutex_path_override;
>   
>   #define MAX_IDENTICAL_BOARD_VARIATIONS	2
>   
> +/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
> +#define ACPI_GLOBAL_LOCK_PSEUDO_PATH	":GLOBAL_LOCK"
> +

That needs to be documented.

>   typedef union {
>   	u32 value;
>   	struct {
> @@ -166,6 +169,14 @@ static const struct ec_sensor_info known_ec_sensors[] = {
>   struct ec_board_info {
>   	const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
>   	unsigned long sensors;
> +	/*
> +	 * Defines which mutex to use for guarding access to the state and the
> +	 * hardware. Can be either a full path to an AML mutex or the
> +	 * pseudo-path ACPI_GLOBAL_LOCK_PSEUDO_PATH to use the global ACPI lock,
> +	 * or left empty to use a regular mutex object, in which case access to
> +	 * the hardware is not guarded.
> +	 */
> +	const char *mutex_path;
>   };
>   
>   static const struct ec_board_info board_info[] __initconst = {
> @@ -173,12 +184,14 @@ static const struct ec_board_info board_info[] __initconst = {
>   		.board_names = {"PRIME X570-PRO"},
>   		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
>   			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"Pro WS X570-ACE"},
>   		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM |
>   			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
>   			SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG CROSSHAIR VIII DARK HERO"},
> @@ -187,6 +200,7 @@ static const struct ec_board_info board_info[] __initconst = {
>   			SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER |
>   			SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW |
>   			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG CROSSHAIR VIII FORMULA"},
> @@ -194,6 +208,7 @@ static const struct ec_board_info board_info[] __initconst = {
>   			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
>   			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
>   			SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {
> @@ -206,6 +221,7 @@ static const struct ec_board_info board_info[] __initconst = {
>   			SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET |
>   			SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU |
>   			SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG CROSSHAIR VIII IMPACT"},
> @@ -213,12 +229,14 @@ static const struct ec_board_info board_info[] __initconst = {
>   			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
>   			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
>   			SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG STRIX B550-E GAMING"},
>   		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
>   			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
>   			SENSOR_FAN_CPU_OPT,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG STRIX B550-I GAMING"},
> @@ -226,6 +244,7 @@ static const struct ec_board_info board_info[] __initconst = {
>   			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
>   			SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU |
>   			SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG STRIX X570-E GAMING"},
> @@ -233,17 +252,20 @@ static const struct ec_board_info board_info[] __initconst = {
>   			SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM |
>   			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
>   			SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG STRIX X570-F GAMING"},
>   		.sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB |
>   			SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{
>   		.board_names = {"ROG STRIX X570-I GAMING"},
>   		.sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS |
>   			SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU |
>   			SENSOR_IN_CPU_CORE,
> +		.mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX,
>   	},
>   	{}
>   };
> @@ -253,6 +275,57 @@ struct ec_sensor {
>   	s32 cached_value;
>   };
>   
> +struct lock_data {
> +	union {
> +		acpi_handle aml;
> +		u32 global_lock_handle;
> +		struct mutex regular;
> +	} mutex;
> +	int (*lock)(struct lock_data *data);
> +	int (*unlock)(struct lock_data *data);
> +};
> +
> +/*
> + * The next function pairs implement options for locking access to the
> + * state and the EC
> + */
> +static int lock_via_acpi_mutex(struct lock_data *data)
> +{
> +	/*
> +	 * ASUS DSDT does not specify that access to the EC has to be guarded,
> +	 * but firmware does access it via ACPI
> +	 */
> +	return acpi_acquire_mutex(data->mutex.aml, NULL,
> +				  ACPI_LOCK_DELAY_MS);
> +}
> +
> +static int unlock_acpi_mutex(struct lock_data *data)
> +{
> +	return acpi_release_mutex(data->mutex.aml, NULL);
> +}
> +
> +static int lock_via_global_acpi_lock(struct lock_data *data)
> +{
> +	return acpi_acquire_global_lock(ACPI_LOCK_DELAY_MS,
> +					&data->mutex.global_lock_handle);
> +}
> +
> +static int unlock_global_acpi_lock(struct lock_data *data)
> +{
> +	return acpi_release_global_lock(data->mutex.global_lock_handle);
> +}
> +
> +static int lock_via_mutex(struct lock_data *data)
> +{
> +	return mutex_trylock(&data->mutex.regular) ? 0 : -EBUSY;
> +}

There is some type confusion in the above lock functions. Some return
ACPI error codes, some return Linux error codes. Please make return
values consistent.

Also, why use mutex_trylock() instead of mutex_lock() ? This is
unusual since it will result in errors if more than one user
tries to access the data (eg multiple processes reading sysfs
attributes at the same time), and thus warrants a detailed
explanation.

> +
> +static int unlock_mutex(struct lock_data *data)
> +{
> +	mutex_unlock(&data->mutex.regular);
> +	return 0;
> +}
> +
>   struct ec_sensors_data {
>   	struct ec_board_info board_info;
>   	struct ec_sensor *sensors;
> @@ -263,7 +336,9 @@ struct ec_sensors_data {
>   	u8 banks[ASUS_EC_MAX_BANK + 1];
>   	/* in jiffies */
>   	unsigned long last_updated;
> -	acpi_handle aml_mutex;
> +	struct lock_data lock_data;
> +	/* number of board EC sensors */
> +	u8 nr_sensors;

Ok, I must admit I am more than a bit lost. In patch 1/4
you removed this variable (and argued that removing it was
for "deduplication"), only to re-introduce it here.
Sorry, I don't follow the logic.

>   	/*
>   	 * number of EC registers to read
>   	 * (sensor might span more than 1 register)
> @@ -370,23 +445,36 @@ static void __init fill_ec_registers(struct ec_sensors_data *ec)
>   	}
>   }
>   
> -static acpi_handle __init asus_hw_access_mutex(struct device *dev)
> +static int __init setup_lock_data(struct device *dev)
>   {
>   	const char *mutex_path;
> -	acpi_handle res;
>   	int status;
> +	struct ec_sensors_data *state = dev_get_drvdata(dev);
>   
>   	mutex_path = mutex_path_override ?
> -		mutex_path_override : ASUS_HW_ACCESS_MUTEX_ASMX;
> -
> -	status = acpi_get_handle(NULL, (acpi_string)mutex_path, &res);
> -	if (ACPI_FAILURE(status)) {
> -		dev_err(dev,
> -			"Could not get hardware access guard mutex '%s': error %d",
> -			mutex_path, status);
> -		return NULL;
> +		mutex_path_override : state->board_info.mutex_path;
> +
> +	if (!mutex_path || !strlen(mutex_path)) {

When would mutex_path be NULL ?

> +		mutex_init(&state->lock_data.mutex.regular);
> +		state->lock_data.lock = lock_via_mutex;
> +		state->lock_data.unlock = unlock_mutex;
> +	} else if (!strcmp(mutex_path, ACPI_GLOBAL_LOCK_PSEUDO_PATH)) {
> +		state->lock_data.lock = lock_via_global_acpi_lock;
> +		state->lock_data.unlock = unlock_global_acpi_lock;
> +	} else {
> +		status = acpi_get_handle(NULL, (acpi_string)mutex_path,
> +					 &state->lock_data.mutex.aml);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(dev,
> +				"Failed to get hardware access guard AML mutex"
> +				"'%s': error %d",

Please no string splits. And the negative impact can be seen here:
No space between "mutex" and "'%s'".

> +				mutex_path, status);
> +			return -ENOENT;
> +		}
> +		state->lock_data.lock = lock_via_acpi_mutex;
> +		state->lock_data.unlock = unlock_acpi_mutex;
>   	}
> -	return res;
> +	return 0;
>   }
>   
>   static int asus_ec_bank_switch(u8 bank, u8 *old)
> @@ -417,7 +505,8 @@ static int asus_ec_block_read(const struct device *dev,
>   	if (prev_bank) {
>   		/* oops... somebody else is working with the EC too */
>   		dev_warn(dev,
> -			"Concurrent access to the ACPI EC detected.\nRace condition possible.");
> +			"Concurrent access to the ACPI EC detected.\n"
> +			"Race condition possible.");

Why this change, and how is it related to this patch ?

>   	}
>   
>   	/* read registers minimizing bank switches. */
> @@ -489,15 +578,9 @@ static int update_ec_sensors(const struct device *dev,
>   {
>   	int status;
>   
> -	/*
> -	 * ASUS DSDT does not specify that access to the EC has to be guarded,
> -	 * but firmware does access it via ACPI
> -	 */
> -	if (ACPI_FAILURE(acpi_acquire_mutex(ec->aml_mutex, NULL,
> -					    ACPI_LOCK_DELAY_MS))) {
> -		dev_err(dev, "Failed to acquire AML mutex");
> -		status = -EBUSY;
> -		goto cleanup;
> +	if (ec->lock_data.lock(&ec->lock_data)) {
> +		dev_warn(dev, "Failed to acquire mutex");
> +		return -EBUSY;
>   	}
>   
>   	status = asus_ec_block_read(dev, ec);
> @@ -505,10 +588,10 @@ static int update_ec_sensors(const struct device *dev,
>   	if (!status) {
>   		update_sensor_values(ec, ec->read_buffer);
>   	}
> -	if (ACPI_FAILURE(acpi_release_mutex(ec->aml_mutex, NULL))) {
> -		dev_err(dev, "Failed to release AML mutex");
> -	}
> -cleanup:
> +
> +	if (ec->lock_data.unlock(&ec->lock_data))
> +		dev_err(dev, "Failed to release mutex");
> +
>   	return status;
>   }
>   
> @@ -648,6 +731,7 @@ static int __init asus_ec_probe(struct platform_device *pdev)
>   	enum hwmon_sensor_types type;
>   	struct device *hwdev;
>   	unsigned int i;
> +	int status;
>   
>   	pboard_info = get_board_info();
>   	if (!pboard_info)
> @@ -663,6 +747,11 @@ static int __init asus_ec_probe(struct platform_device *pdev)
>   	ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info),
>   					sizeof(struct ec_sensor), GFP_KERNEL);
>   
> +	status = setup_lock_data(dev);
> +	if (status) {
> +		dev_err(dev, "Failed to setup state/EC locking: %d", status);
> +		return status;
> +	}
>   	setup_sensor_data(ec_data);
>   	ec_data->registers = devm_kcalloc(dev, ec_data->nr_registers,
>   					  sizeof(u16), GFP_KERNEL);
> @@ -674,8 +763,6 @@ static int __init asus_ec_probe(struct platform_device *pdev)
>   
>   	fill_ec_registers(ec_data);
>   
> -	ec_data->aml_mutex = asus_hw_access_mutex(dev);
> -
>   	for (i = 0; i < sensor_count(&ec_data->board_info); ++i) {
>   		si = get_sensor_info(ec_data, i);
>   		if (!nr_count[si->type])

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

On Tue, 29 Mar 2022 at 23:23, Guenter Roeck <linux@roeck-us.net> wrote:

> > +/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
> > +#define ACPI_GLOBAL_LOCK_PSEUDO_PATH ":GLOBAL_LOCK"
> > +
>
> That needs to be documented.

Do you mean a note in the /Documentation/..../...rst or adding details
here? There is an additional bit of information on this identifier
below, in the ec_board_info struct declaration.

> There is some type confusion in the above lock functions. Some return
> ACPI error codes, some return Linux error codes. Please make return
> values consistent.
>
> Also, why use mutex_trylock() instead of mutex_lock() ? This is
> unusual since it will result in errors if more than one user
> tries to access the data (eg multiple processes reading sysfs
> attributes at the same time), and thus warrants a detailed
> explanation.
OK.

> > +     struct lock_data lock_data;
> > +     /* number of board EC sensors */
> > +     u8 nr_sensors;
>
> Ok, I must admit I am more than a bit lost. In patch 1/4
> you removed this variable (and argued that removing it was
> for "deduplication"), only to re-introduce it here.
> Sorry, I don't follow the logic.

Sorry for that. This is my mistake which I tried to warn you about in
my first reply to the email with this patch.

> > +     if (!mutex_path || !strlen(mutex_path)) {
>
> When would mutex_path be NULL ?
When it is set to NULL in the board definition struct ec_board_info.

> > +             if (ACPI_FAILURE(status)) {
> > +                     dev_err(dev,
> > +                             "Failed to get hardware access guard AML mutex"
> > +                             "'%s': error %d",
>
> Please no string splits. And the negative impact can be seen here:
> No space between "mutex" and "'%s'".

Yes, of course.

> >               dev_warn(dev,
> > -                     "Concurrent access to the ACPI EC detected.\nRace condition possible.");
> > +                     "Concurrent access to the ACPI EC detected.\n"
> > +                     "Race condition possible.");
>
> Why this change, and how is it related to this patch ?
Same as above, will be corrected.

Thank you,
Eugene

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Guenter Roeck]

On 3/29/22 15:11, Eugene Shalygin wrote:
> On Tue, 29 Mar 2022 at 23:23, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> +/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
>>> +#define ACPI_GLOBAL_LOCK_PSEUDO_PATH ":GLOBAL_LOCK"
>>> +
>>
>> That needs to be documented.
> 
> Do you mean a note in the /Documentation/..../...rst or adding details
> here? There is an additional bit of information on this identifier
> below, in the ec_board_info struct declaration.
> 
My understanding was that the user would/could request its use via
the module parameter, so it needs to be documented in the rst file.

>> There is some type confusion in the above lock functions. Some return
>> ACPI error codes, some return Linux error codes. Please make return
>> values consistent.
>>
>> Also, why use mutex_trylock() instead of mutex_lock() ? This is
>> unusual since it will result in errors if more than one user
>> tries to access the data (eg multiple processes reading sysfs
>> attributes at the same time), and thus warrants a detailed
>> explanation.
> OK.
> 
>>> +     struct lock_data lock_data;
>>> +     /* number of board EC sensors */
>>> +     u8 nr_sensors;
>>
>> Ok, I must admit I am more than a bit lost. In patch 1/4
>> you removed this variable (and argued that removing it was
>> for "deduplication"), only to re-introduce it here.
>> Sorry, I don't follow the logic.
> 
> Sorry for that. This is my mistake which I tried to warn you about in
> my first reply to the email with this patch.
> 
>>> +     if (!mutex_path || !strlen(mutex_path)) {
>>
>> When would mutex_path be NULL ?
> When it is set to NULL in the board definition struct ec_board_info.
> 

Are there any such board definitions ? I don't recall seeing any.

Thanks,
Guenter

>>> +             if (ACPI_FAILURE(status)) {
>>> +                     dev_err(dev,
>>> +                             "Failed to get hardware access guard AML mutex"
>>> +                             "'%s': error %d",
>>
>> Please no string splits. And the negative impact can be seen here:
>> No space between "mutex" and "'%s'".
> 
> Yes, of course.
> 
>>>                dev_warn(dev,
>>> -                     "Concurrent access to the ACPI EC detected.\nRace condition possible.");
>>> +                     "Concurrent access to the ACPI EC detected.\n"
>>> +                     "Race condition possible.");
>>
>> Why this change, and how is it related to this patch ?
> Same as above, will be corrected.
> 
> Thank you,
> Eugene

Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Eugene Shalygin]

On Wed, 30 Mar 2022 at 01:37, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/29/22 15:11, Eugene Shalygin wrote:
> > On Tue, 29 Mar 2022 at 23:23, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> >>> +/* Moniker for the ACPI global lock (':' is not allowed in ASL identifiers) */
> >>> +#define ACPI_GLOBAL_LOCK_PSEUDO_PATH ":GLOBAL_LOCK"
> >>> +
> >>
> >> That needs to be documented.
> >
> > Do you mean a note in the /Documentation/..../...rst or adding details
> > here? There is an additional bit of information on this identifier
> > below, in the ec_board_info struct declaration.
> >
> My understanding was that the user would/could request its use via
> the module parameter, so it needs to be documented in the rst file.
I see now, thank you.

> >>> +     if (!mutex_path || !strlen(mutex_path)) {
> >>
> >> When would mutex_path be NULL ?
> > When it is set to NULL in the board definition struct ec_board_info.
> >
>
> Are there any such board definitions ? I don't recall seeing any.

You are right, there are no such definitions neither in this
submission nor before. I'm messaging with a user who showed a hint
that such boards might exist [1]. We have not confirmed it yet, but I
thought this little test is obvious enough to be put in the code
already.

Best regards,
Eugene

[1] https://github.com/zeule/asus-ec-sensors/issues/19

Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Eugene Shalygin]

On Tue, 29 Mar 2022 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/29/22 12:22, Eugene Shalygin wrote:
> > On Tue, 29 Mar 2022 at 15:44, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>
> >>>    struct ec_sensors_data {
> >>> -     unsigned long board_sensors;
> >>> +     struct ec_board_info board_info;
> >>
> >> Please explain why this needs to be the entire structure and not
> >> just a pointer to it.
> >
> > I marked the board_info array as __initconst assuming that this large
> > array will be unloaded from memory after the init phase, while we keep
> > only a single element. Is that assumption incorrect?
> >
>
> What happens if you build the driver into the kernel and then instantiate
> and de-instantiate it multiple times ?

Sorry, I have no idea because I don't know how to load a built-in
driver multiple times. But since this driver is attached to a
motherboard device, which is persistent and always single, do I need
to consider such a scenario?

>
> >>> +static int sensor_count(const struct ec_board_info *board)
> >>> +{
> >>> +     return hweight_long(board->sensors);
> >>> +}
> >>
> >> This function is called several times. Does it really make sense, or is it
> >> necessary, to re-calculate the number of sensors over and over again
> >> instead of keeping it in ec->nr_sensors as before ? What are the benefits ?
> >> Unless there is a good explanation I see that as unrelated and unnecessary
> >> change.
> >
> > This had something to do with data deduplication. However, I need the
> > count value only for looping over the sensor array, thus I can as well
> > add an invalid element to the end of the array. I rushed to submit
> > this driver to replace the wmi one, and it still has an artifact for
> > the WMI code I'd like to get rid of eventually, which is the read
> > buffer and the registers array. This will remove all the nr_ variables
> > and two dynamically allocated arrays. I will understand, of course, if
> > you ask to submit that refactoring separately.
> >
>
> The rule of "one logical change per patch" still applies. If you start
> intermixing parts of future clean-up efforts into current patches, you'll
> see a very unhappy maintainer - especially since this change makes up
> a significant part of this patch, complicates review significantly,
> and makes me wonder if other unrelated changes are included that I don't
> see right now due to all the noise.
>
> Besides, at least in this patch, I don't buy the "deduplication" argument.
> Keeping a single additional variable in a data structure is much simpler
> and straightforward than calling hweight_long() several times. I'd call
> that "complification".

OK, I'll roll it back until I remove the other size variables and arrays.

Best regards,
Eugene

Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info struct for board data

[Guenter Roeck]

On 3/30/22 00:51, Eugene Shalygin wrote:
> On Tue, 29 Mar 2022 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 3/29/22 12:22, Eugene Shalygin wrote:
>>> On Tue, 29 Mar 2022 at 15:44, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>
>>>>>     struct ec_sensors_data {
>>>>> -     unsigned long board_sensors;
>>>>> +     struct ec_board_info board_info;
>>>>
>>>> Please explain why this needs to be the entire structure and not
>>>> just a pointer to it.
>>>
>>> I marked the board_info array as __initconst assuming that this large
>>> array will be unloaded from memory after the init phase, while we keep
>>> only a single element. Is that assumption incorrect?
>>>
>>
>> What happens if you build the driver into the kernel and then instantiate
>> and de-instantiate it multiple times ?
> 
> Sorry, I have no idea because I don't know how to load a built-in
> driver multiple times. But since this driver is attached to a
> motherboard device, which is persistent and always single, do I need
> to consider such a scenario?
> 

Drivers have "unbind" and "bind" attributes, which can be used to unbind/bind
the driver mutliple times. That is quite useful for testing, including for
built-in drivers. As long as the attributes exists, they have to be supported.
This is not about having to consider a scenario, it is about preventing crashes
if existing functionality is used.

Having said that, I notice that the probe function is marked __init. I guess
I didn't pay attention. It may be interesting to build the driver into the
kernel, unbind/bind it, and see what happens.

Guenter