From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A201A387375 for ; Thu, 6 Aug 2026 05:23:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.67.36.66 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785993824; cv=none; b=RTSmDe1N+wFX5oq4SfvD5LetOVZ5iov1jUy4A8OVhZ7KsU+yKXLc3ifmhqFeEhvqkoDJmJ21qHD7j/2uzL+3FLJsPip1uoawfB8l16HnTg80kvoNBgtKBIYxRXSTJuSd+KkuaEuva8P8LK6a09AqJtqc8X8fgKXMUAQspR8O2SI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785993824; c=relaxed/simple; bh=XbqsjtGj67DMWugwFQZMgJ8NQ1v8abHFUfJsNZ9iMxw=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=pfw46r+w4RE/icsufvbYt3CNY/+MVVcGKB8qTefO5T4g+/2LaKZEqCdg+BP11scrmaBcrfBgjDrZ2vyBRJAd83TAZeOSHoj8cQ8tpunbeMw3f8EOuvE6nbl8JXtmlCnz8aNhEpzlEtxeTOSlDj0bpZPWjvRTEdnX6Bz3RzQ5I0k= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=posteo.net; spf=pass smtp.mailfrom=posteo.net; dkim=pass (2048-bit key) header.d=posteo.net header.i=@posteo.net header.b=djr/GwtW; arc=none smtp.client-ip=185.67.36.66 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=posteo.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=posteo.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=posteo.net header.i=@posteo.net header.b="djr/GwtW" Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id E2DF3240101 for ; Thu, 6 Aug 2026 07:23:38 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=posteo.net; s=1984.8680eb; t=1785993818; bh=rypBXhRtpQmTVGev87waIzq+w2KhT5u06v7zI6vDSAQ=; h=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type: Content-Transfer-Encoding:From; b=djr/GwtWFy3T4hOugS5dZ9cfGgCXd+vYwz9kxrJHqpvowoynDNjdVluyx0OIMc7g7 use0TBU9qV2Aa0xW71lenxv0eZw9v6eLQonqPJN8cDzIYfEQGTgOtAmdym7xkdMchH G51KwRs1vHYE6EMzbWeNZAUE/cKJulV3go0ZdBgsBhPhMYdwCstrgiCp/DKsAbJlB/ MrXMAp8Q90EgrbL+kliCugHI5laRJDDNTYvJKisVWyrIorli4ZsVxlGbf4N+VwaC1c 51ZcxTZ5YxS9jfP0HLYdEDE9jL4+qE0976j6V0OyazYXDY0cJqRUgkvzrydpUjyy2I C4DHkogzwrAFw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4hFwfy1CSHz6twG; Thu, 6 Aug 2026 07:23:38 +0200 (CEST) Date: Thu, 06 Aug 2026 05:23:38 +0000 From: Wilken Gottwalt To: Ali Ahmet Memis Cc: Guenter Roeck , linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] hwmon: (corsair-psu) serialize debugfs access against hwmon Message-ID: <20260806072337.15496c74@posteo.net> In-Reply-To: <20260802123653.19532-1-ali@iusegentoo.com> References: <20260802123653.19532-1-ali@iusegentoo.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Sun, 2 Aug 2026 12:36:53 +0000 Ali Ahmet Memis wrote: > corsairpsu_request() sends a rail select command and then the actual > read as two separate transfers, both going through the single shared > cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core > serializes its own callers, but the debugfs files call > corsairpsu_get_value() directly and never take that lock, so a debugfs > read can land between another reader's rail select and its value read. >=20 > The result is a value from the wrong rail reported as the right one, > because corsairpsu_usb_cmd() only checks the command echo and both > transfers echo the command it expects. It can also make a caller consume > the reply meant for the other one, since raw_event() writes into the > shared buffer and completes whoever happens to be waiting. >=20 > Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely > on subsystem locking") on the grounds that the subsystem serializes for > us, which holds for sysfs but not for these files. Take > the same lock in the debugfs paths that issue commands, using the guard > added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard > for subsystem locks"), as suggested in [1]. >=20 > The lock cannot go into corsairpsu_request() itself: the hwmon core > already holds it across ->read, so every sysfs read would deadlock. > vendor_show() and product_show() only print strings cached during probe > and issue no command, and corsairpsu_get_criticals() and > corsairpsu_check_cmd_support() run before either interface is > registered, so none of them need it. >=20 > [1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roec= k-us.net/ >=20 > Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking") > Signed-off-by: Ali Ahmet Memis > --- > This is the fix Guenter asked for in the May thread, written the way he > suggested there. Wilken's patch used a driver private mutex around > corsairpsu_request(); that thread stalled and the race is still present. >=20 > Wilken, does this cover the chained command case you were worried about? > As far as I can tell it does: the whole select-rail plus read sequence > now runs under the same lock the hwmon core takes around ->read, so a > debugfs reader cannot land in the middle of one. If you had a case in > mind that this misses, I would rather hear it than guess. >=20 > I have no Corsair PSU, so this is reasoned from the code rather than > measured on hardware. What I did check: >=20 > - hwmon_lock() takes hwdev->lock, and the hwmon core takes the same > mutex around ->read and ->write, so this really does serialize the > two entry points > - the lock therefore cannot go into corsairpsu_request(), the sysfs > path would deadlock on itself > - probe registers the hwmon device before creating the debugfs files > and remove tears them down in the opposite order, so priv->hwmon_dev > is always valid inside a debugfs read >=20 > Prior discussion: > https://lore.kernel.org/all/agR9YW7hGTJ_l7ms@monster.localdomain/ >=20 > drivers/hwmon/corsair-psu.c | 4 ++++ > 1 file changed, 4 insertions(+) >=20 > diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c > index ce958cdaef58..24100519cd83 100644 > --- a/drivers/hwmon/corsair-psu.c > +++ b/drivers/hwmon/corsair-psu.c > @@ -664,6 +664,8 @@ static void print_uptime(struct seq_file *seqf, u8 cm= d) > long val; > int ret; > =20 > + guard(hwmon_lock)(priv->hwmon_dev); > + > ret =3D corsairpsu_get_value(priv, cmd, 0, &val); > if (ret < 0) { > seq_puts(seqf, "N/A\n"); > @@ -730,6 +732,8 @@ static int ocpmode_show(struct seq_file *seqf, void *= unused) > * getting of the value itself can also fail during this. Because of th= is every other > value > * than OCP_MULTI_RAIL can be considered as "single rail". > */ > + guard(hwmon_lock)(priv->hwmon_dev); > + > ret =3D corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val); > if (ret < 0) > seq_puts(seqf, "N/A\n"); >=20 > base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952 That does not even compile on a current 7.1.5/7.1.6 kernel. Though, not sure yet, what that is. But I can not risk running a trunk kernel on my workstat= ion. /usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:302:9: error: = unknown type name =E2=80=98class_hwmon_lock_t=E2=80=99; did you mean =E2=80= =98class_task_lock_t=E2=80=99? 302 | class_##_name##_t var __cleanup(class_##_name##_destructor)= =3D \ | ^~~~~~ /usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:422:9: note: i= n expansion of macro =E2=80=98CLASS=E2=80=99 422 | CLASS(_name, __UNIQUE_ID(guard)) | ^~~~~ corsair-psu.c:667:9: note: in expansion of macro =E2=80=98guard=E2=80=99 667 | guard(hwmon_lock)(priv->hwmon_dev); | ^~~~~ corsair-psu.c:667:9: error: cleanup argument not a function 667 | guard(hwmon_lock)(priv->hwmon_dev); | ^~~~~ greetings, Wilken