From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) (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 E5B964963B8 for ; Tue, 4 Aug 2026 17:53:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.67.36.65 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785866012; cv=none; b=G9fpatUsN5OUwC3IGN0shippnE8taKt9gdLowQmZJg/TYZA3icqazyGClKhnQRSEQZodj/lBWmV3lfefrnzD7cy9VXSzlFFvJZw7Xsy5uI1mriWIyafv3yh4qmCYj/PhQX2tEoRYUX7Z49ZWy0UE/rjFUzo1YwR6xwan26UawjI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785866012; c=relaxed/simple; bh=z6CLaZXLx+vz3IwlbX7K/sKQRR6KxuugFRcv84Yn2H8=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=X8UjZUIlocsb4XgZX1lb2VVEqNPWOhfnzySW7Go4ry7hHMJZ/sPdGvBDYA/s2wIXO7VDb4EXJB1xFVjdLuEB8+cF0mxsv8yU8rgWamfuxhQAIrTXr0i67rEPXD5OnksBIr1VfAc3XGVwbFCdZOZkkk5DrR5DHXRV+mc9WAcO2wI= 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=e8PKFmvk; arc=none smtp.client-ip=185.67.36.65 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="e8PKFmvk" Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id BA3B7240027 for ; Tue, 4 Aug 2026 19:53:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=posteo.net; s=1984.8680eb; t=1785865998; bh=k0mKZrn0AjnW4t1hfujLcLrJXrt+dgbSUNLh9kviDm0=; h=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type: Content-Transfer-Encoding:From; b=e8PKFmvkjk1hOSkfHPD4eOvZq5nCSEw4Ynx4iNqima3fFrmjtrNRNXIiJWOr47eS3 J+hg4FEZNq9AvYqtf038ZpVvkWZJ5x9bwlSHyG0Dcng+hmEN4/foPlErRsgZ+vdRhj wLkFMURF9iXgzKUqQ2T7HJbmX9r+eH2D3kl5oCLbYPgVUCylH1wx5injC5dkkjBr6x 116uNe3hHdv+2If8+KvSCeQ1wXKkGf7CpTXZvrQczy+2wVw9E4uJHgrGtQQA0u6swr jD2gBp4VuAU2ngIfrNa9BNmzYGy4nNuXpRJ4uX55p8b9WCjMlqL2I8pa5SKDIvmbbG Uv8uu+Ic4fhOA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4hF1Nt0S5Rz6tvx; Tue, 4 Aug 2026 19:53:17 +0200 (CEST) Date: Tue, 04 Aug 2026 17:53:18 +0000 From: Wilken Gottwalt To: Guenter Roeck Cc: Ali Ahmet Memis , linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] hwmon: (corsair-psu) serialize debugfs access against hwmon Message-ID: <20260804195317.7940b34c@posteo.net> In-Reply-To: <0a50dbe2-4df8-4952-8090-cd97ece2a6c9@roeck-us.net> References: <20260802123653.19532-1-ali@iusegentoo.com> <20260802145745.6f444fc2@posteo.net> <85f8ccd7-0d2a-43db-8690-cf220bd76e32@roeck-us.net> <20260804001347.164873-1-ali@iusegentoo.com> <20260804061110.575adfc2@posteo.net> <0a50dbe2-4df8-4952-8090-cd97ece2a6c9@roeck-us.net> 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=US-ASCII Content-Transfer-Encoding: 7bit On Tue, 4 Aug 2026 09:34:38 -0700 Guenter Roeck wrote: > On Tue, Aug 04, 2026 at 04:11:11AM +0000, Wilken Gottwalt wrote: > > > > > > Gemini tells me that fixing the raw event problem will require a spinlock to > > > protect the completion and a separate receive buffer. No idea if it is correct, > > > but other drivers do the same, so it may have a point. Either case, this is a > > > bit too much to do without hardware to test, and I'd rather prefer to leave this > > > up to Wilken. > > > > I was working at that one, too, because I saw Claude Opus hinting on that one. > > But it drove me crazy, because every AI is saying something slighty different. I > > can not really pin down which one is actually the real solution. I tried to read > > through the subsystems code and other drivers, but, argh, I don't know. I was > > playing with the idea to (1) remove the raw HID mode completely or (2) make the > > driver switchable, raw HID or normal HID, but not both at the same time. On the > > other hand, in my Github repo where I develop the driver, I also have a tool > > which demonstrates how to access the PSU completely in userspace via libhidpi. > > There is actually no need to provide the raw HID access. > > > > Have a look at the patch below. It is part AI (Gemini) generated and part me. > Sashiko is happy with it, but of course that doesn't mean it is perfect or > even correct. It does look good to me, though. Yeah, that is the standard proposal basically all decent LLMs produce. Deepseek V4 calls it rx_buffer and tx_buffer and talks about preventing race conditions by adding a spinlock and quote "don't use a mutex". Hmm, I put it on my "check it in the next days" list. greetings Wilken > Making Sashiko happy required all core elements of the patch: > - the spinlock > - the separate receive buffer > - the rcv_pending boolean > - the size check in corsairpsu_raw_event() > > Sashiko reports race conditions if I drop just one of those elements. > > Guenter > > --- > From 50fae138603a9a6b1929cbe9a644310495688eea Mon Sep 17 00:00:00 2001 > From: Guenter Roeck > Date: Mon, 3 Aug 2026 17:39:21 -0700 > Subject: [PATCH] hwmon: (corsair-psu) Separate request/response buffers and > validate reply echo > > In corsairpsu_usb_cmd(), a single shared buffer (priv->cmd_buffer) is used > both for transmitting command reports via hid_hw_output_report() and for > receiving device responses in corsairpsu_raw_event(). > > If a command sent via corsairpsu_usb_cmd() times out, the caller stops > waiting, but the hardware may still process the command and send a delayed > response later. If a subsequent command is being prepared or transmitted > when this delayed response arrives, corsairpsu_raw_event() blindly copies > the incoming report into priv->cmd_buffer and completes wait_completion: > > corsairpsu_raw_event() > if (completion_done(&priv->wait_completion)) > return 0; > > memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size)); > complete(&priv->wait_completion); > > This causes a data race where priv->cmd_buffer can be overwritten with > the old delayed response while hid_hw_output_report() is transmitting the > new command, potentially causing the PSU to receive invalid parameters or > shut down. In addition, the caller of the subsequent command will wake up > early and either consume stale data or fail unexpectedly. > > Fix the problem by: > - Allocating a separate response buffer (priv->res_buffer) so that > corsairpsu_raw_event() never modifies priv->cmd_buffer during outgoing > transfers. > - Validating incoming reports in corsairpsu_raw_event() to ensure that the > echoed length and command opcode match the pending command in > priv->cmd_buffer (or indicate an unsupported command opcode with 0). > - Protecting buffer initialization, reinit_completion(), and report > validation/completion with a spinlock (wait_completion_lock). > - Adding a boolean flag indicating that the code is waiting for a response, > and only copying the reply into the receive buffer if that is the case. > > Reported-by: Sashiko > Signed-off-by: Guenter Roeck > --- > drivers/hwmon/corsair-psu.c | 33 +++++++++++++++++++++++++++------ > 1 file changed, 27 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c > index 5592b927e9d4..3eebf8494ed8 100644 > --- a/drivers/hwmon/corsair-psu.c > +++ b/drivers/hwmon/corsair-psu.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > /* > @@ -122,7 +123,10 @@ struct corsairpsu_data { > struct device *hwmon_dev; > struct dentry *debugfs; > struct completion wait_completion; > + spinlock_t completion_lock; /* locks wait_completion, cmd_buffer, and res_buffer > */ u8 *cmd_buffer; > + u8 *res_buffer; > + bool rcv_pending; > char vendor[REPLY_SIZE]; > char product[REPLY_SIZE]; > long temp_crit[TEMP_COUNT]; > @@ -158,15 +162,19 @@ static int corsairpsu_dutycycle_to_pwm(const long dutycycle) > > static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2, void *data) > { > + unsigned long flags; > unsigned long time; > int ret; > > + spin_lock_irqsave(&priv->completion_lock, flags); > memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE); > + memset(priv->res_buffer, 0, CMD_BUFFER_SIZE); > priv->cmd_buffer[0] = p0; > priv->cmd_buffer[1] = p1; > priv->cmd_buffer[2] = p2; > - > reinit_completion(&priv->wait_completion); > + priv->rcv_pending = true; > + spin_unlock_irqrestore(&priv->completion_lock, flags); > > ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE); > if (ret < 0) > @@ -182,11 +190,11 @@ static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, > u8 p2, > * was send, not every command is supported on every device class, if a command is not > * supported, the length value in the reply is okay, but the command value is set to 0 > */ > - if (p0 != priv->cmd_buffer[0] || p1 != priv->cmd_buffer[1]) > + if (p0 != priv->res_buffer[0] || p1 != priv->res_buffer[1]) > return -EOPNOTSUPP; > > if (data) > - memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE); > + memcpy(data, priv->res_buffer + 2, REPLY_SIZE); > > return 0; > } > @@ -781,6 +789,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct > hid_device_id if (!priv->cmd_buffer) > return -ENOMEM; > > + priv->res_buffer = devm_kmalloc(&hdev->dev, CMD_BUFFER_SIZE, GFP_KERNEL); > + if (!priv->res_buffer) > + return -ENOMEM; > + > ret = hid_parse(hdev); > if (ret) > return ret; > @@ -795,6 +807,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct > hid_device_id > priv->hdev = hdev; > hid_set_drvdata(hdev, priv); > + spin_lock_init(&priv->completion_lock); > init_completion(&priv->wait_completion); > > hid_device_io_start(hdev); > @@ -848,12 +861,20 @@ static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report > *repo int size) > { > struct corsairpsu_data *priv = hid_get_drvdata(hdev); > + unsigned long flags; > > - if (completion_done(&priv->wait_completion)) > + if (size < 2) > return 0; > > - memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size)); > - complete(&priv->wait_completion); > + spin_lock_irqsave(&priv->completion_lock, flags); > + if (priv->rcv_pending && !completion_done(&priv->wait_completion) && > + data[0] == priv->cmd_buffer[0] && > + (data[1] == priv->cmd_buffer[1] || data[1] == 0)) { > + memcpy(priv->res_buffer, data, min(CMD_BUFFER_SIZE, size)); > + complete(&priv->wait_completion); > + priv->rcv_pending = false; > + } > + spin_unlock_irqrestore(&priv->completion_lock, flags); > > return 0; > }