lkml.org 
[lkml]   [2022]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock
From
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

\
 
 \ /
  Last update: 2022-03-30 01:40    [W:0.064 / U:0.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site