On Mon, 12 May 2025 at 23:24, Kurt Borja <kuurtb@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxx> > >> > >> 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 >
