[PATCH] hwmon: (spd5118) Select page 0 unconditionally during probe

[PATCH] hwmon: (spd5118) Select page 0 unconditionally during probe

[Armin Wolf]

Some Intel i2c controllers can be configured by the BIOS to reject
writes to the SPD device. This often causes problems when the register
page needs to be changed, usually during resume.

Avoid probing on affected devices by unconditionally selecting page 0
by writing the SPD5118_REG_I2C_LEGACY_MODE register during probe.
This will fail on affected controllers and thus prevent the driver
from probing.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/spd5118.c | 57 ++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 9724cf70b61d..65a0d5852ce8 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -637,45 +637,32 @@ static int spd5118_i2c_init(struct i2c_client *client)
 				     I2C_FUNC_SMBUS_WORD_DATA))
 		return -ENODEV;
 
-	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
-	if (regval < 0 || (regval && regval != 0x5118))
-		return -ENODEV;
-
 	/*
-	 * If the device type registers return 0, it is possible that the chip
-	 * has a non-zero page selected and takes the specification literally,
+	 * We must first select page 0 to ensure that we can reliably read
+	 * the volatile registers on chips that take the specification literally,
 	 * i.e. disables access to volatile registers besides the page register
 	 * if the page is not 0. The Renesas/ITD SPD5118 Hub Controller is known
-	 * to show this behavior. Try to identify such chips.
+	 * to show this behavior.
+	 *
+	 * We must also perform an unconditional register write to detect if
+	 * the i2c controller blocks write accesses to the SPD device. Some Intel
+	 * controllers might be configured by the BIOS to do this.
 	 */
-	if (!regval) {
-		/* Vendor ID registers must also be 0 */
-		regval = i2c_smbus_read_word_data(client, SPD5118_REG_VENDOR);
-		if (regval)
-			return -ENODEV;
-
-		/* The selected page in MR11 must not be 0 */
-		mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
-		if (mode < 0 || (mode & ~SPD5118_LEGACY_MODE_MASK) ||
-		    !(mode & SPD5118_LEGACY_PAGE_MASK))
-			return -ENODEV;
-
-		err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE,
-						mode & SPD5118_LEGACY_MODE_ADDR);
-		if (err)
-			return -ENODEV;
-
-		/*
-		 * If the device type registers are still bad after selecting
-		 * page 0, this is not a SPD5118 device. Restore original
-		 * legacy mode register value and abort.
-		 */
-		regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
-		if (regval != 0x5118) {
-			i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
-			return -ENODEV;
-		}
-	}
+	mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
+	if (mode < 0)
+		return mode;
+
+	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE,
+					mode & SPD5118_LEGACY_MODE_ADDR);
+	if (err < 0)
+		return err;
+
+	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
+	if (regval < 0)
+		return regval;
+
+	if (regval != 0x5118)
+		return -ENODEV;
 
 	/* We are reasonably sure that this is really a SPD5118 hub controller */
 	return 0;
-- 
2.39.5

Re: [PATCH] hwmon: (spd5118) Select page 0 unconditionally during probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unsafe unconditional I2C write during probe before device verification risks permanent data corruption on unrelated devices.
- [Medium] The I2C write to select page 0 unintentionally clears bits 7:4 of the legacy mode register due to an incorrect bitmask.
--

commit 270ea09e9966102e26a3fc6a4bba86ec060d0e37
Author: Armin Wolf <W_Armin@gmx.de>
hwmon: (spd5118) Select page 0 unconditionally during probe

This commit modifies the probe sequence of the spd5118 driver to
unconditionally select page 0 before attempting to read the device type
register. This change is intended to bypass BIOS-level restrictions on I2C
writes to the SPD device and properly handle chips that restrict access to
volatile registers when a non-zero page is selected.

> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 9724cf70b61d4..65a0d5852ce86 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
[ ... ]
> @@ -637,45 +637,32 @@ static int spd5118_i2c_init(struct i2c_client *client)
[ ... ]
> +	mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
> +	if (mode < 0)
> +		return mode;
> +
> +	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE,
> +					mode & SPD5118_LEGACY_MODE_ADDR);

[Severity: High]
Does this unconditional write execute before any device identity checks,
creating a risk of data corruption for incorrectly bound devices?

If this driver binds to an incorrect I2C device, such as a standard DDR4 SPD
EEPROM residing at the same 0x50-0x57 address range, it will blindly overwrite
byte 0x0B of the device before verifying its identity.

[Severity: Medium]
Does this bitwise AND inadvertently zero out bits 7:4 of the legacy mode
register?

The masking operation mode & SPD5118_LEGACY_MODE_ADDR effectively masks out
all bits except bit 3, forcing bits 7:4 (and bits 2:0) to zero. In the
previous implementation, the driver explicitly verified that bits 7:4 were
zero before applying this mask. With that check removed, any device with bits
7:4 set will have them silently wiped out.

To safely clear only the page bits (2:0), should the mask logically be
mode & ~SPD5118_LEGACY_PAGE_MASK?

> +	if (err < 0)
> +		return err;
> +
> +	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
> +	if (regval < 0)
> +		return regval;
> +
> +	if (regval != 0x5118)
> +		return -ENODEV;

[Severity: High]
If the type mismatch occurs here, the probe aborts without rolling back the
write to SPD5118_REG_I2C_LEGACY_MODE. Does this leak the state modification
to misidentified devices?

>  
>  	/* We are reasonably sure that this is really a SPD5118 hub controller */
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830231036.203719-1-W_Armin@gmx.de?part=1