Re: [PATCH 2/4] hwmon: (asus-ec-sensors) implement locking via the ACPI global lock

[Guenter Roeck <linux@roeck-us.net>]

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