On 3/29/22 15:11, Eugene Shalygin wrote:
On Tue, 29 Mar 2022 at 23:23, Guenter Roeck <linux@xxxxxxxxxxxx> 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
