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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D9UHYC9360RO.8BN28N2MJ2G8@gmail.com>
Date: Mon, 12 May 2025 18:23:44 -0300
From: "Kurt Borja" <kuurtb@...il.com>
To: "Antheas Kapenekakis" <lkml@...heas.dev>
Cc: <platform-driver-x86@...r.kernel.org>, "Armin Wolf" <W_Armin@....de>,
 "Jonathan Corbet" <corbet@....net>, "Hans de Goede" <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, "Jean
 Delvare" <jdelvare@...e.com>, "Guenter Roeck" <linux@...ck-us.net>,
 <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH v1 02/10] platform/x86: msi-wmi-platform: Add unlocked
 msi_wmi_platform_query

On Mon May 12, 2025 at 5:51 PM -03, Antheas Kapenekakis wrote:
> On Mon, 12 May 2025 at 21:21, Kurt Borja <kuurtb@...il.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@...heas.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


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)