Messages in this thread | | | Date | Tue, 29 Mar 2022 16:37:02 -0700 | Subject | Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock | From | 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
| |