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: 
 <CAGwozwHrh2ODuUPAHxmkoATJ82Fguqgyb8H1z+er-AnVvBXGZQ@mail.gmail.com>
Date: Mon, 12 May 2025 23:51:14 +0200
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Kurt Borja <kuurtb@...il.com>
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, 12 May 2025 at 23:24, Kurt Borja <kuurtb@...il.com> wrote:
>
> 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.

The _unlocked variant was meant to be used for operations that read,
mutate, then write the same value. Therefore, we should ensure that it
is not possible to call WMI functions in-between that could
potentially cause that value to be overwritten.

Theoretically, at least. Although after I simplified shift mode and
the battery threshold, I ended up doing only writes for them.

Off the top of my head I think that both shift mode and Set_AP (used
for enabling the custom fan curve) touch the same register (Set_AP
touches 3 registers and one of them is shift mode[1]). So it is safer
to use a single lock and avoid edge cases like this one.

> 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.

I would say marginally, sure, although the operations we extend the
lock outside of are trivial. A case could be made that locking
multiple times hurts performance more, as if we introduced a fan curve
lock, each fan operation would have to lock at least three times
instead of one (e.g., disabling a custom fan curve would take 6
locks).

Antheas

[1] https://github.com/hhd-dev/hwinfo/blob/master/devices/msi_claw8/acpi/decoded/dsdt.dsl

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