| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <16a36e23-bab2-4314-54ea-ff6b1011e6d7@roeck-us.net>
Date: Tue, 29 Mar 2022 06:44:04 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Eugene Shalygin <eugene.shalygin@...il.com>
Cc: darcagn@...tonmail.com, Jean Delvare <jdelvare@...e.com>,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info
struct for board data
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@...il.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,
Powered by blists - more mailing lists