[PATCH 0/2] i2c: i801: Don't instantiate spd5118 under SPD Write Disable

[PATCH 0/2] i2c: i801: Don't instantiate spd5118 under SPD Write Disable

[Yo-Jung (Leo) Lin]

On some PC platforms, the BIOS may enable i801's SPD Write Disable bit and
forbid writes to certain SMBus addresses. For the i801 family those addresses
are 0x50 to 0x57[1], where the spd5118 sensors are usually probed.

The write-disabling bit will make sensor functions related to writes and
nvmem access unusable.

Also, the driver is unable to sync back values from regcache to the sensors
during resume, leading to resume failure. This has been observed on multiple
existing PC platforms from Dell and HP.

Furthermore, for the sensors from certain manufacturers, after a reset they
may need to select page value to 0 before sensor values can be read. Not
being able to write to the registers renders the temperature reading
unusable. See discussion in [2].

To address these issues, do not instantiate spd5118 if SPD Write Disable
bit is set.

[1] 18.1.16 HOSTC—Host Configuration Register (SMBus—D31:F3),
    Intel 8 Series/C220 Series Chipset Family Platform Controller Hub(PCH)

[2] https://lore.kernel.org/all/acf31929-5d13-4fc5-b370-ab7fc5062455@roeck-us.net/

Signed-off-by: Yo-Jung Lin (Leo) <leo.lin@canonical.com>
---
Yo-Jung (Leo) Lin (2):
      i2c: smbus: pass write disabling bit to spd instantiating function
      i2c: i801: don't instantiate spd5118 if SPD Write Disable is set

 drivers/i2c/busses/i2c-i801.c  | 6 ++++--
 drivers/i2c/busses/i2c-piix4.c | 2 +-
 drivers/i2c/i2c-smbus.c        | 7 ++++++-
 include/linux/i2c-smbus.h      | 4 ++--
 4 files changed, 13 insertions(+), 6 deletions(-)
---
base-commit: b425262c07a6a643ebeed91046e161e20b944164
change-id: 20250423-for-upstream-i801-spd5118-no-instantiate-3b52d489e7a6

Best regards,
-- 
Yo-Jung (Leo) Lin <leo.lin@canonical.com>

[PATCH 1/2] i2c: smbus: pass write disabling bit to spd instantiating function

[Yo-Jung (Leo) Lin]

Some SMBus controllers may restrict writes to addresses where SPD
sensors may reside. This may lead to some SPD sensors not functioning
correctly, and might need extra handling. Pass this extra context to
i2c_register_spd() so that it could be handled accordingly.

Signed-off-by: Yo-Jung (Leo) Lin <leo.lin@canonical.com>
---
 drivers/i2c/busses/i2c-i801.c  | 6 ++++--
 drivers/i2c/busses/i2c-piix4.c | 2 +-
 drivers/i2c/i2c-smbus.c        | 2 +-
 include/linux/i2c-smbus.h      | 4 ++--
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 48e1af544b75..95619eb5e798 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1180,7 +1180,8 @@ static void i801_probe_optional_targets(struct i801_priv *priv)
 #ifdef CONFIG_I2C_I801_MUX
 	if (!priv->mux_pdev)
 #endif
-		i2c_register_spd(&priv->adapter);
+		i2c_register_spd(&priv->adapter,
+				 !!(priv->original_hstcfg & SMBHSTCFG_SPD_WD));
 }
 #else
 static void __init input_apanel_init(void) {}
@@ -1283,7 +1284,8 @@ static int i801_notifier_call(struct notifier_block *nb, unsigned long action,
 		return NOTIFY_DONE;
 
 	/* Call i2c_register_spd for muxed child segments */
-	i2c_register_spd(to_i2c_adapter(dev));
+	i2c_register_spd(to_i2c_adapter(dev),
+			 !!(priv->original_hstcfg & SMBHSTCFG_SPD_WD));
 
 	return NOTIFY_OK;
 }
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index dd75916157f0..085d121a88f6 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -971,7 +971,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	 * This would allow the ee1004 to be probed incorrectly.
 	 */
 	if (port == 0)
-		i2c_register_spd(adap);
+		i2c_register_spd(adap, false);
 
 	*padap = adap;
 	return 0;
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 7d40e7aa3799..97e833895dd7 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -372,7 +372,7 @@ EXPORT_SYMBOL_GPL(i2c_free_slave_host_notify_device);
  *  - Only works on systems with 1 to 8 memory slots
  */
 #if IS_ENABLED(CONFIG_DMI)
-void i2c_register_spd(struct i2c_adapter *adap)
+void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
 {
 	int n, slot_count = 0, dimm_count = 0;
 	u16 handle;
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index ced1c6ead52a..57bb3154eb47 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -44,9 +44,9 @@ static inline void i2c_free_slave_host_notify_device(struct i2c_client *client)
 #endif
 
 #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
-void i2c_register_spd(struct i2c_adapter *adap);
+void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled);
 #else
-static inline void i2c_register_spd(struct i2c_adapter *adap) { }
+static inline void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled) { }
 #endif
 
 #endif /* _LINUX_I2C_SMBUS_H */

-- 
2.43.0

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

[Yo-Jung (Leo) Lin]

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
after a sensor reset. This will make the sensor not functional unless
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@canonical.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;
 	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",

-- 
2.43.0

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

[Guenter Roeck]

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@canonical.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@roeck-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",
> 

Re: [PATCH 1/2] i2c: smbus: pass write disabling bit to spd instantiating function

[Guenter Roeck]

On 4/23/25 20:35, Yo-Jung (Leo) Lin wrote:
> Some SMBus controllers may restrict writes to addresses where SPD
> sensors may reside. This may lead to some SPD sensors not functioning
> correctly, and might need extra handling. Pass this extra context to
> i2c_register_spd() so that it could be handled accordingly.
> 
> Signed-off-by: Yo-Jung (Leo) Lin <leo.lin@canonical.com>

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

> ---
>   drivers/i2c/busses/i2c-i801.c  | 6 ++++--
>   drivers/i2c/busses/i2c-piix4.c | 2 +-
>   drivers/i2c/i2c-smbus.c        | 2 +-
>   include/linux/i2c-smbus.h      | 4 ++--
>   4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 48e1af544b75..95619eb5e798 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1180,7 +1180,8 @@ static void i801_probe_optional_targets(struct i801_priv *priv)
>   #ifdef CONFIG_I2C_I801_MUX
>   	if (!priv->mux_pdev)
>   #endif
> -		i2c_register_spd(&priv->adapter);
> +		i2c_register_spd(&priv->adapter,
> +				 !!(priv->original_hstcfg & SMBHSTCFG_SPD_WD));
>   }
>   #else
>   static void __init input_apanel_init(void) {}
> @@ -1283,7 +1284,8 @@ static int i801_notifier_call(struct notifier_block *nb, unsigned long action,
>   		return NOTIFY_DONE;
>   
>   	/* Call i2c_register_spd for muxed child segments */
> -	i2c_register_spd(to_i2c_adapter(dev));
> +	i2c_register_spd(to_i2c_adapter(dev),
> +			 !!(priv->original_hstcfg & SMBHSTCFG_SPD_WD));
>   
>   	return NOTIFY_OK;
>   }
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index dd75916157f0..085d121a88f6 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -971,7 +971,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>   	 * This would allow the ee1004 to be probed incorrectly.
>   	 */
>   	if (port == 0)
> -		i2c_register_spd(adap);
> +		i2c_register_spd(adap, false);
>   
>   	*padap = adap;
>   	return 0;
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 7d40e7aa3799..97e833895dd7 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -372,7 +372,7 @@ EXPORT_SYMBOL_GPL(i2c_free_slave_host_notify_device);
>    *  - Only works on systems with 1 to 8 memory slots
>    */
>   #if IS_ENABLED(CONFIG_DMI)
> -void i2c_register_spd(struct i2c_adapter *adap)
> +void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
>   {
>   	int n, slot_count = 0, dimm_count = 0;
>   	u16 handle;
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index ced1c6ead52a..57bb3154eb47 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -44,9 +44,9 @@ static inline void i2c_free_slave_host_notify_device(struct i2c_client *client)
>   #endif
>   
>   #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
> -void i2c_register_spd(struct i2c_adapter *adap);
> +void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled);
>   #else
> -static inline void i2c_register_spd(struct i2c_adapter *adap) { }
> +static inline void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled) { }
>   #endif
>   
>   #endif /* _LINUX_I2C_SMBUS_H */
> 

Re: [PATCH 1/2] i2c: smbus: pass write disabling bit to spd instantiating function

[Andi Shyti]

Hi,

> @@ -44,9 +44,9 @@ static inline void i2c_free_slave_host_notify_device(struct i2c_client *client)
>  #endif
>  
>  #if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_DMI)
> -void i2c_register_spd(struct i2c_adapter *adap);
> +void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled);
>  #else
> -static inline void i2c_register_spd(struct i2c_adapter *adap) { }
> +static inline void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled) { }
>  #endif

Please don't add random bool flags to function parameters,
especially in library functions. They add confusion and make the
code harder to understand. I even dislike them when they're used
inside drivers.

I'd much prefer something like this:

   static void i2c_register_spd(struct i2c_adapter *adap, bool write_disabled)
   {
   	...
   }
   
   void i2c_register_spd_write_disable(struct i2c_adapter *adap)
   {
   	i2c_register_spd(adap, true);
   }
   
   void i2c_register_spd_write_enable(struct i2c_adapter *adap)
   {
   	i2c_register_spd(adap, false);
   }

This way, we gain readability at the cost of a bit of redundancy,
a trade-off I'm more than happy with.

Of course, this is just my preference (even if I'm pretty strong
about it, especially when it comes to libraries). Wolfram is free
to override me on this.

Andi