Re: [PATCH v1 02/10] platform/x86: msi-wmi-platform: Add unlocked msi_wmi_platform_query

["Kurt Borja" <kuurtb@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

On Mon May 12, 2025 at 5:51 PM -03, Antheas Kapenekakis wrote:
> On Mon, 12 May 2025 at 21:21, Kurt Borja <kuurtb@gmail.com> wrote:
>>
>> On Sun May 11, 2025 at 5:44 PM -03, Antheas Kapenekakis wrote:
>> > This driver requires to be able to handle transactions that perform
>> > multiple WMI actions at a time. Therefore, it needs to be able to
>> > lock the wmi_lock mutex for multiple operations.
>> >
>> > Add msi_wmi_platform_query_unlocked() to allow the caller to
>> > perform the WMI query without locking the wmi_lock mutex, by
>> > renaming the existing function and adding a new one that only
>> > locks the mutex.
>> >
>> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
>>
>> You only use msi_wmi_platform_query_unlocked() to protect the
>> fan_curve/AP state right?
>>
>> If that's the case I think we don't need it. AFAIK sysfs reads/writes
>> are already synchronized/locked, and as I mentioned in Patch 10, I don't
>> think you need this variant in probe/remove either.
>>
>> I'd like to hear more opinions on this though.
>
> Are sysfs reads/writes between different files of the same driver
> synced? If not, it is better to lock.

You are right, you definitely need locking there.

However, what do you think about introducing a new lock specifically for
this state?

IMO locks should never be multi-function and I don't see why all WMI
calls have to contest the same lock that we use for fan stuff. This
would eliminate the need for this extra function.

Also keep in mind that by introducing this patch you are also extending
the time the lock is held per WMI call, which is also unnecessary.

-- 
 ~ Kurt

>
> I want a second opinion here too.
>
> You are correct on probe/remove.
>
> Antheas
>
>> --
>>  ~ Kurt


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]