| Message ID | 20220327121404.1702631-3-eugene.shalygin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | asus-ec-sensors: add support for board families | expand |
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
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
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
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
> 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
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
Thank you, Nathan!
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])
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
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
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
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])
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(-)