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

[PATCH v2] 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>
---
Changes since v1:
- Avoid zeroing reserved bits inside SPD5118_REG_I2C_LEGACY_MODE
---
 drivers/hwmon/spd5118.c | 58 ++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 9724cf70b61d..95b7cbd2f111 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -14,6 +14,7 @@
  * memory modules.
  */
 
+#include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/bits.h>
 #include <linux/err.h>
@@ -637,45 +638,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;
+
+	FIELD_MODIFY(SPD5118_LEGACY_PAGE_MASK, &mode, 0);
+	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
+	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 v2] 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] Unconditional I2C write in `probe()` before verifying device identity can cause permanent data corruption on mismatched devices.
- [Medium] Failing probe on read-only buses entirely drops temperature monitoring capabilities.
--

commit 83cea797ff679bd0608e7bc406325588975c70cb
Author: Armin Wolf <W_Armin@gmx.de>

hwmon: (spd5118) Select page 0 unconditionally during probe

This commit makes the driver write to the legacy mode register
unconditionally during probe to select page 0. This intentional write
fails on controllers where the BIOS blocks writes to the SPD device,
thereby avoiding issues on such controllers by refusing to probe.

> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 9724cf70b61d4..95b7cbd2f111c 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
[ ... ]
> @@ -637,45 +638,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;
> -
[ ... ]
> +	mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
> +	if (mode < 0)
> +		return mode;
> +
> +	FIELD_MODIFY(SPD5118_LEGACY_PAGE_MASK, &mode, 0);
> +	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);

[Severity: High]
Does this unconditional I2C write in spd5118_i2c_init() risk permanent data
corruption if the driver is bound to a mismatched device?

Because the device identity check reading SPD5118_REG_TYPE has been moved
until after this write, any device mistakenly probed (via sysfs or incorrect
firmware tables) in the 0x50-0x57 address range will have its byte at offset
0x0B overwritten.

For an ordinary EEPROM, this clears bits 0-2 permanently before the
subsequent type check fails and aborts the probe.

> +	if (err < 0)
> +		return err;

[Severity: Medium]
Will failing the probe completely on read-only buses deprive users of all
temperature monitoring capabilities?

If spd5118_i2c_init() returns an error here when probed on an Intel
controller that rejects writes, the driver completely drops support for that
device.

Previously, the probe would succeed and at least allow reading the memory
temperature. Could this degrade gracefully instead, perhaps by marking the
regmap as read-only and disabling suspend/resume writes, rather than totally
preventing hardware monitoring?

> +
> +	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;

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

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

[Armin Wolf]

Am 31.08.26 um 01:41 schrieb sashiko-bot@kernel.org:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Unconditional I2C write in `probe()` before verifying device identity can cause permanent data corruption on mismatched devices.
> - [Medium] Failing probe on read-only buses entirely drops temperature monitoring capabilities.
> --
>
> commit 83cea797ff679bd0608e7bc406325588975c70cb
> Author: Armin Wolf <W_Armin@gmx.de>
>
> hwmon: (spd5118) Select page 0 unconditionally during probe
>
> This commit makes the driver write to the legacy mode register
> unconditionally during probe to select page 0. This intentional write
> fails on controllers where the BIOS blocks writes to the SPD device,
> thereby avoiding issues on such controllers by refusing to probe.
>
>> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
>> index 9724cf70b61d4..95b7cbd2f111c 100644
>> --- a/drivers/hwmon/spd5118.c
>> +++ b/drivers/hwmon/spd5118.c
> [ ... ]
>> @@ -637,45 +638,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;
>> -
> [ ... ]
>> +	mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
>> +	if (mode < 0)
>> +		return mode;
>> +
>> +	FIELD_MODIFY(SPD5118_LEGACY_PAGE_MASK, &mode, 0);
>> +	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
> [Severity: High]
> Does this unconditional I2C write in spd5118_i2c_init() risk permanent data
> corruption if the driver is bound to a mismatched device?
>
> Because the device identity check reading SPD5118_REG_TYPE has been moved
> until after this write, any device mistakenly probed (via sysfs or incorrect
> firmware tables) in the 0x50-0x57 address range will have its byte at offset
> 0x0B overwritten.
>
> For an ordinary EEPROM, this clears bits 0-2 permanently before the
> subsequent type check fails and aborts the probe.

The detection code is unchanged, and we need that write in order to read registers.

>> +	if (err < 0)
>> +		return err;
> [Severity: Medium]
> Will failing the probe completely on read-only buses deprive users of all
> temperature monitoring capabilities?
>
> If spd5118_i2c_init() returns an error here when probed on an Intel
> controller that rejects writes, the driver completely drops support for that
> device.
>
> Previously, the probe would succeed and at least allow reading the memory
> temperature. Could this degrade gracefully instead, perhaps by marking the
> regmap as read-only and disabling suspend/resume writes, rather than totally
> preventing hardware monitoring?

No, because the driver will then block suspend/resume on those devices.

Thanks,
Armin Wolf

>> +
>> +	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;

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

[Guenter Roeck]

Hi Armin,

On 8/30/26 16:48, Armin Wolf wrote:
> Am 31.08.26 um 01:41 schrieb sashiko-bot@kernel.org:
> 
>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>> - [High] Unconditional I2C write in `probe()` before verifying device identity can cause permanent data corruption on mismatched devices.
>> - [Medium] Failing probe on read-only buses entirely drops temperature monitoring capabilities.
>> -- 
>>
>> commit 83cea797ff679bd0608e7bc406325588975c70cb
>> Author: Armin Wolf <W_Armin@gmx.de>
>>
>> hwmon: (spd5118) Select page 0 unconditionally during probe
>>
>> This commit makes the driver write to the legacy mode register
>> unconditionally during probe to select page 0. This intentional write
>> fails on controllers where the BIOS blocks writes to the SPD device,
>> thereby avoiding issues on such controllers by refusing to probe.
>>
>>> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
>>> index 9724cf70b61d4..95b7cbd2f111c 100644
>>> --- a/drivers/hwmon/spd5118.c
>>> +++ b/drivers/hwmon/spd5118.c
>> [ ... ]
>>> @@ -637,45 +638,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;
>>> -
>> [ ... ]
>>> +    mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
>>> +    if (mode < 0)
>>> +        return mode;
>>> +
>>> +    FIELD_MODIFY(SPD5118_LEGACY_PAGE_MASK, &mode, 0);
>>> +    err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
>> [Severity: High]
>> Does this unconditional I2C write in spd5118_i2c_init() risk permanent data
>> corruption if the driver is bound to a mismatched device?
>>
>> Because the device identity check reading SPD5118_REG_TYPE has been moved
>> until after this write, any device mistakenly probed (via sysfs or incorrect
>> firmware tables) in the 0x50-0x57 address range will have its byte at offset
>> 0x0B overwritten.
>>
>> For an ordinary EEPROM, this clears bits 0-2 permanently before the
>> subsequent type check fails and aborts the probe.
> 
> The detection code is unchanged, and we need that write in order to read registers.
> 

The device could have been instantiated manually with a write to new_device,
which would bypass detection. It doesn't hurt to write the old mode back if
the write succeeded and this turns out to not be a spd5118 device, so
let's do that if the code below bails out.

Thanks,
Guenter

>>> +    if (err < 0)
>>> +        return err;
>> [Severity: Medium]
>> Will failing the probe completely on read-only buses deprive users of all
>> temperature monitoring capabilities?
>>
>> If spd5118_i2c_init() returns an error here when probed on an Intel
>> controller that rejects writes, the driver completely drops support for that
>> device.
>>
>> Previously, the probe would succeed and at least allow reading the memory
>> temperature. Could this degrade gracefully instead, perhaps by marking the
>> regmap as read-only and disabling suspend/resume writes, rather than totally
>> preventing hardware monitoring?
> 
> No, because the driver will then block suspend/resume on those devices.
> 
> Thanks,
> Armin Wolf
> 
>>> +
>>> +    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;
>