All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sasha Levin <sashal@kernel.org>
To: patches@lists.linux.dev, stable@vger.kernel.org
Cc: Yikai Tsai <yikai.tsai.wiwynn@gmail.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Sasha Levin <sashal@kernel.org>,
	mail@carsten-spiess.de, jdelvare@suse.com,
	linux-hwmon@vger.kernel.org
Subject: [PATCH AUTOSEL 6.15 14/21] hwmon: (isl28022) Fix current reading calculation
Date: Fri,  6 Jun 2025 11:41:39 -0400	[thread overview]
Message-ID: <20250606154147.546388-14-sashal@kernel.org> (raw)
In-Reply-To: <20250606154147.546388-1-sashal@kernel.org>

From: Yikai Tsai <yikai.tsai.wiwynn@gmail.com>

[ Upstream commit b2446a16dbf2347a07af0cf994ca36576d94df77 ]

According to the ISL28022 datasheet, bit15 of the current register is
representing -32768. Fix the calculation to properly handle this bit,
ensuring correct measurements for negative values.

Signed-off-by: Yikai Tsai <yikai.tsai.wiwynn@gmail.com>
Link: https://lore.kernel.org/r/20250519084055.3787-2-yikai.tsai.wiwynn@gmail.com
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

Now let me check for similar current register handling issues in other
hwmon drivers. Let me look at the commit to understand the exact issue:

Looking at the commit changes, I can see that:

**Before (broken):**
```c
*val = ((long)regval * 1250L * (long)data->gain) / (long)data->shunt;
```

**After (fixed):**
```c
u16 sign_bit;
sign_bit = (regval >> 15) & 0x01;
*val = (((long)(((u16)regval) & 0x7FFF) - (sign_bit * 32768)) * 1250L *
(long)data->gain) / (long)data->shunt;
```

The issue is that the ISL28022 current register is a 16-bit two's
complement signed integer where bit 15 represents the sign bit
(equivalent to -32768), but the original code was treating it as an
unsigned value.

Let me analyze based on the established criteria:

**Analysis:**

**YES - This commit should be backported to stable kernel trees.**

**Detailed Analysis:**

1. **Bug Fix Nature**: This is a clear bug fix that addresses incorrect
   current readings for negative values. According to the ISL28022
   datasheet mentioned in the commit, the current register (04h) is a
   16-bit two's complement value where bit 15 represents -32768, but the
   driver was treating it as unsigned.

2. **User Impact**: This bug significantly affects users who rely on
   accurate current measurements, especially when current flows in the
   negative direction. The bug would cause:
   - Incorrect reporting of negative currents as large positive values
   - Complete failure to detect reverse current flow
   - Invalid power calculations that depend on current readings

3. **Code Analysis**:
   - **Problem**: Lines 164-165 in
     `/home/sasha/linux/drivers/hwmon/isl28022.c:164-165` treat `regval`
     as unsigned, causing negative currents to be interpreted as large
     positive values
   - **Solution**: The fix properly implements sign extension by
     extracting bit 15 as the sign bit and manually computing the two's
     complement value: `(value & 0x7FFF) - (sign_bit * 32768)`
   - **Consistency**: The fix follows the exact same pattern already
     used in the shunt voltage reading function (`isl28022_read_in`) at
     lines 120-122, showing this is the established correct approach in
     this driver

4. **Risk Assessment**:
   - **Minimal Risk**: The change is very small and localized to the
     current reading calculation
   - **Well-Tested Pattern**: Uses the identical sign handling logic
     already proven correct in the shunt voltage function
   - **No Side Effects**: Only affects current register interpretation,
     doesn't change any other functionality

5. **Driver Context**:
   - The ISL28022 driver was introduced in October 2024 (commit
     39671a14df4f), making it relatively new
   - The driver is hardware-specific and contained within the hwmon
     subsystem
   - This follows the same pattern as other successful backports shown
     in the examples

6. **Comparison to Similar Commits**: This commit is nearly identical to
   the reference examples that were marked for backporting:
   - **Similar Commit #1** (ina2xx): Fixed sign extension for current
     register by adding `(s16)` cast
   - **Similar Commit #3** (tmp513): Fixed sign extension with
     `sign_extend32(regval, 15)` and type casting
   - **Similar Commit #4** (nct7904): Fixed sign extension by converting
     to `s8`
   - **Similar Commit #5** (ina3221): Fixed shunt sum register bit
     shifting

7. **Critical Subsystem**: Hardware monitoring is critical for system
   health monitoring, thermal management, and power management.
   Incorrect current readings could lead to improper system behavior or
   failure to detect dangerous conditions.

**Conclusion**: This is a textbook stable tree candidate - it fixes a
clear functional bug with minimal risk, follows established patterns,
and addresses a real user-impacting issue in hardware monitoring
functionality.

 drivers/hwmon/isl28022.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
index 1fb9864635db9..1b4fb0824d6c0 100644
--- a/drivers/hwmon/isl28022.c
+++ b/drivers/hwmon/isl28022.c
@@ -154,6 +154,7 @@ static int isl28022_read_current(struct device *dev, u32 attr, long *val)
 	struct isl28022_data *data = dev_get_drvdata(dev);
 	unsigned int regval;
 	int err;
+	u16 sign_bit;
 
 	switch (attr) {
 	case hwmon_curr_input:
@@ -161,8 +162,9 @@ static int isl28022_read_current(struct device *dev, u32 attr, long *val)
 				  ISL28022_REG_CURRENT, &regval);
 		if (err < 0)
 			return err;
-		*val = ((long)regval * 1250L * (long)data->gain) /
-			(long)data->shunt;
+		sign_bit = (regval >> 15) & 0x01;
+		*val = (((long)(((u16)regval) & 0x7FFF) - (sign_bit * 32768)) *
+			1250L * (long)data->gain) / (long)data->shunt;
 		break;
 	default:
 		return -EOPNOTSUPP;
-- 
2.39.5


  parent reply	other threads:[~2025-06-06 15:42 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-06 15:41 [PATCH AUTOSEL 6.15 01/21] cifs: Correctly set SMB1 SessionKey field in Session Setup Request Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 02/21] cifs: Fix cifs_query_path_info() for Windows NT servers Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 03/21] cifs: Fix encoding of SMB1 Session Setup NTLMSSP Request in non-UNICODE mode Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 04/21] NFSv4: Always set NLINK even if the server doesn't support it Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 05/21] NFSv4.2: fix listxattr to return selinux security label Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 06/21] NFSv4.2: fix setattr caching of TIME_[MODIFY|ACCESS]_SET when timestamps are delegated Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 07/21] mailbox: Not protect module_put with spin_lock_irqsave Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 08/21] mfd: max77541: Fix wakeup source leaks on device unbind Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 09/21] mfd: max14577: " Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 10/21] mfd: max77705: " Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 11/21] mfd: 88pm886: " Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 12/21] mfd: sprd-sc27xx: " Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 13/21] sunrpc: don't immediately retransmit on seqno miss Sasha Levin
2025-06-06 15:41 ` Sasha Levin [this message]
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 15/21] dm vdo indexer: don't read request structure after enqueuing Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 16/21] leds: multicolor: Fix intensity setting while SW blinking Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 17/21] fuse: fix race between concurrent setattrs from multiple nodes Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 18/21] cxl/region: Add a dev_err() on missing target list entries Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 19/21] cxl: core/region - ignore interleave granularity when ways=1 Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 20/21] NFSv4: xattr handlers should check for absent nfs filehandles Sasha Levin
2025-06-06 15:41 ` [PATCH AUTOSEL 6.15 21/21] hwmon: (pmbus/max34440) Fix support for max34451 Sasha Levin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250606154147.546388-14-sashal@kernel.org \
    --to=sashal@kernel.org \
    --cc=jdelvare@suse.com \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=mail@carsten-spiess.de \
    --cc=patches@lists.linux.dev \
    --cc=stable@vger.kernel.org \
    --cc=yikai.tsai.wiwynn@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.