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

[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.

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