linux-kernel - Re: [PATCH 2/2] i2c: i801: don't instantiate spd5118 if SPD Write Disable is set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f6ee05c8-e762-447a-8909-58f629541b06@roeck-us.net>
Date: Thu, 24 Apr 2025 05:21:40 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Yo-Jung (Leo) Lin" <leo.lin@...onical.com>,
 Jean Delvare <jdelvare@...e.com>, Andi Shyti <andi.shyti@...nel.org>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: "Chia-Lin Kao (AceLan)" <acelan.kao@...onical.com>,
 linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] i2c: i801: don't instantiate spd5118 if SPD Write
 Disable is set

On 4/23/25 20:35, Yo-Jung (Leo) Lin wrote:
> If SPD Write Disable bit in the SMBus is enabled, writing data to
> addresses from 0x50 to 0x57 is forbidden. This may lead to the
> following issues for spd5118 device:
> 
>    1) Writes to the sensor hwmon sysfs attributes will always result
>       in ENXIO.
> 
>    2) System-wide resume, errors may occur during regcache sync,
>       resulting in the following error messages:
> 
>       kernel: spd5118 1-0050: Failed to write b = 0: -6
>       kernel: spd5118 1-0050: PM: dpm_run_callback(): spd5118_resume [spd5118] returns -6
>       kernel: spd5118 1-0050: PM: failed to resume async: error -6
> 
>    3) nvmem won't be usable, because writing to the page selector becomes
>       impossible.
> 
> Also, spd5118 from some manufacturers may set the page to a value != 0

It is the BIOS vendor, not the spd5118 chip vendor.

> after a sensor reset. This will make the sensor not functional unless

board reset

> its MR11 register can be changed, which is impossible due to writes being
> disabled.
> 
> To address these issues, don't instantiate it at all if SPD Write Disable
> bit is set.
> 
> Signed-off-by: Yo-Jung (Leo) Lin <leo.lin@...onical.com>
> ---
>   drivers/i2c/i2c-smbus.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 97e833895dd7..cb7ef6a7a174 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -378,6 +378,7 @@ void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
>   	u16 handle;
>   	u8 common_mem_type = 0x0, mem_type;
>   	u64 mem_size;
> +	bool scan_needed = true;

I'd suggest to name the variable "instantiate", but that is really a nitpick.

Reviewed-by: Guenter Roeck <linux@...ck-us.net>

>   	const char *name;
>   
>   	while ((handle = dmi_memdev_handle(slot_count)) != 0xffff) {
> @@ -438,6 +439,7 @@ void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
>   	case 0x22:	/* DDR5 */
>   	case 0x23:	/* LPDDR5 */
>   		name = "spd5118";
> +		scan_needed = !write_disabled;
>   		break;
>   	default:
>   		dev_info(&adap->dev,
> @@ -461,6 +463,9 @@ void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
>   		addr_list[0] = 0x50 + n;
>   		addr_list[1] = I2C_CLIENT_END;
>   
> +		if (!scan_needed)
> +			continue;
> +
>   		if (!IS_ERR(i2c_new_scanned_device(adap, &info, addr_list, NULL))) {
>   			dev_info(&adap->dev,
>   				 "Successfully instantiated SPD at 0x%hx\n",
>