[PATCH 1/1] hwmon: (spd5118) support write protected devices

[PATCH 1/1] hwmon: (spd5118) support write protected devices

[Kean Ren]

Some systems do not allow writes to SPD devices. On the i801 SMBus
controller family this happens when the BIOS sets SPD Write Disable, in
which case the controller refuses writes to addresses 0x50-0x57; other
platforms may enforce the same in firmware. There is no way for the driver
to query this.

spd5118 assumes that it can write to the device, which does not hold on
such systems:

  - writes to the hwmon attributes always fail with -ENXIO
  - the temperature sensor disable in spd5118_suspend() fails silently
  - spd5118_resume() fails in regcache_sync(), and the system reports

  spd5118 0-0050: Failed to write b = 0: -6
  spd5118 0-0050: PM: failed to resume async: error -6

Register 0xb in that message is the page selector register, which regmap
writes back from its cache at the end of regcache_sync(). The write of the
saved configuration register before that fails as well, but the cache code
does not report that error.

Detect write protection once at probe time by writing the page selector
register back with the value just read from it. This is a no-op on a device
which accepts writes, and it fails if writes are blocked.

If writes are not possible, run the device read-only: drop the register
cache so that regcache_sync() cannot attempt any write, skip the writes in
the suspend and resume callbacks, and make the writable attributes read
only. Alarm attributes are read only already; stop clearing a latched
status from them, since that requires a write, and report the condition as
the device presents it.

Reading the SPD NVRAM beyond the first page requires switching pages, which
is itself a write. Restrict the 'eeprom' attribute to the page which can be
read in that case, instead of returning errors for the accessible part.

The temperature sensor remains fully usable; only its configuration
becomes read-only. Document that, and leave the shared regmap
configuration alone by selecting a separate one without a register
cache.

Signed-off-by: Kean Ren <rh_king@163.com>
---
 Documentation/hwmon/spd5118.rst |  10 ++++
 drivers/hwmon/spd5118.c         | 110 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 108 insertions(+), 12 deletions(-)

diff --git a/Documentation/hwmon/spd5118.rst b/Documentation/hwmon/spd5118.rst
index ef7338f46575..dc5a464372d7 100644
--- a/Documentation/hwmon/spd5118.rst
+++ b/Documentation/hwmon/spd5118.rst
@@ -54,6 +54,12 @@ temp1_crit_alarm	Temperature critical alarm
 Alarm attributes are sticky until read and will be cleared afterwards
 unless the alarm condition still applies.
 
+If the chip cannot be written to, for example because the SMBus controller has
+SPD Write Disable set or because the firmware protects the SPD, the driver runs
+the chip read-only. The writable attributes listed above are read-only then, and
+latched alarm conditions are reported as presented by the chip because clearing
+them requires a write.
+
 
 SPD (Serial Presence Detect) support
 ------------------------------------
@@ -61,3 +67,7 @@ SPD (Serial Presence Detect) support
 The driver also supports reading the SPD NVRAM on SPD5118 compatible chips.
 SPD data is available from the 'eeprom' binary attribute file attached to the
 chip's I2C device.
+
+On write protected chips only the first SPD page can be read, because selecting
+another page requires a write. The 'eeprom' attribute is limited to that page in
+this case.
diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 9724cf70b61d..0f70f28b92f7 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -76,6 +76,7 @@ static const unsigned short normal_i2c[] = {
 struct spd5118_data {
 	struct regmap *regmap;
 	struct mutex nvmem_lock;
+	bool write_protected;
 };
 
 /* hwmon */
@@ -129,8 +130,9 @@ static int spd5118_read_temp(struct regmap *regmap, u32 attr, long *val)
 	return 0;
 }
 
-static int spd5118_read_alarm(struct regmap *regmap, u32 attr, long *val)
+static int spd5118_read_alarm(struct spd5118_data *data, u32 attr, long *val)
 {
+	struct regmap *regmap = data->regmap;
 	unsigned int mask, regval;
 	int err;
 
@@ -155,7 +157,12 @@ static int spd5118_read_alarm(struct regmap *regmap, u32 attr, long *val)
 	if (err < 0)
 		return err;
 	*val = !!(regval & mask);
-	if (*val)
+	/*
+	 * Clearing a latched alarm condition requires a register write. If the
+	 * device is write protected, leave the status bit alone and report the
+	 * condition as the device presents it.
+	 */
+	if (*val && !data->write_protected)
 		return regmap_write(regmap, SPD5118_REG_TEMP_CLR, mask);
 	return 0;
 }
@@ -175,7 +182,8 @@ static int spd5118_read_enable(struct regmap *regmap, long *val)
 static int spd5118_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct spd5118_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
 
 	if (type != hwmon_temp)
 		return -EOPNOTSUPP;
@@ -191,7 +199,7 @@ static int spd5118_read(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_temp_min_alarm:
 	case hwmon_temp_crit_alarm:
 	case hwmon_temp_lcrit_alarm:
-		return spd5118_read_alarm(regmap, attr, val);
+		return spd5118_read_alarm(data, attr, val);
 	case hwmon_temp_enable:
 		return spd5118_read_enable(regmap, val);
 	default:
@@ -257,7 +265,8 @@ static int spd5118_temp_write(struct regmap *regmap, u32 attr, long val)
 static int spd5118_write(struct device *dev, enum hwmon_sensor_types type,
 			 u32 attr, int channel, long val)
 {
-	struct regmap *regmap = dev_get_drvdata(dev);
+	struct spd5118_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
 
 	switch (type) {
 	case hwmon_temp:
@@ -270,6 +279,8 @@ static int spd5118_write(struct device *dev, enum hwmon_sensor_types type,
 static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types type,
 				  u32 attr, int channel)
 {
+	const struct spd5118_data *data = _data;
+
 	if (type != hwmon_temp)
 		return 0;
 
@@ -281,7 +292,7 @@ static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types typ
 	case hwmon_temp_lcrit:
 	case hwmon_temp_crit:
 	case hwmon_temp_enable:
-		return 0644;
+		return data->write_protected ? 0444 : 0644;
 	case hwmon_temp_min_alarm:
 	case hwmon_temp_max_alarm:
 	case hwmon_temp_crit_alarm:
@@ -401,6 +412,14 @@ static int spd5118_nvmem_init(struct device *dev, struct spd5118_data *data)
 	};
 	struct nvmem_device *nvmem;
 
+	/*
+	 * Selecting another SPD page writes the page selector register, which
+	 * fails if the device is write protected, so only the currently
+	 * selected page is readable then. Do not advertise the others.
+	 */
+	if (data->write_protected)
+		nvmem_config.size = SPD5118_PAGE_SIZE;
+
 	nvmem = devm_nvmem_register(dev, &nvmem_config);
 	return PTR_ERR_OR_ZERO(nvmem);
 }
@@ -465,6 +484,24 @@ static const struct regmap_config spd5118_regmap8_config = {
 	.num_ranges = ARRAY_SIZE(spd5118_i2c_regmap_range_cfg),
 };
 
+/*
+ * Variant for write protected devices. Since no write can succeed, there is
+ * nothing to cache and nothing to restore on resume; dropping the cache also
+ * keeps regcache_sync() from attempting writes, which would otherwise make the
+ * system fail to resume.
+ */
+static const struct regmap_config spd5118_regmap8_ro_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x7ff,
+	.writeable_reg = spd5118_writeable_reg,
+	.volatile_reg = spd5118_volatile_reg,
+	.cache_type = REGCACHE_NONE,
+
+	.ranges = spd5118_i2c_regmap_range_cfg,
+	.num_ranges = ARRAY_SIZE(spd5118_i2c_regmap_range_cfg),
+};
+
 /*
  * SPD5118 2-byte register address format (JESD300-5, Tables 7 & 20):
  *   Byte 1 (on wire first):  MemReg | BlkAddr[0] | Address[5:0]
@@ -493,6 +530,15 @@ static int spd5118_suspend(struct device *dev)
 	u32 regval;
 	int err;
 
+	/*
+	 * Disabling the temperature sensor means writing to the device, and
+	 * syncing back the register cache on resume means writing as well.
+	 * Neither is possible if the device is write protected, so leave the
+	 * device alone in that case.
+	 */
+	if (data->write_protected)
+		return 0;
+
 	/*
 	 * Make sure the configuration register in the regmap cache is current
 	 * before bypassing it.
@@ -517,13 +563,18 @@ static int spd5118_resume(struct device *dev)
 	struct spd5118_data *data = dev_get_drvdata(dev);
 	struct regmap *regmap = data->regmap;
 
+	/* Nothing was changed on suspend, and writes are not possible */
+	if (data->write_protected)
+		return 0;
+
 	regcache_cache_only(regmap, false);
 	return regcache_sync(regmap);
 }
 
 static DEFINE_SIMPLE_DEV_PM_OPS(spd5118_pm_ops, spd5118_suspend, spd5118_resume);
 
-static int spd5118_common_probe(struct device *dev, struct regmap *regmap)
+static int spd5118_common_probe(struct device *dev, struct regmap *regmap,
+				bool write_protected)
 {
 	unsigned int capability, revision, vendor, bank;
 	struct spd5118_data *data;
@@ -554,6 +605,7 @@ static int spd5118_common_probe(struct device *dev, struct regmap *regmap)
 		return -ENODEV;
 
 	data->regmap = regmap;
+	data->write_protected = write_protected;
 	mutex_init(&data->nvmem_lock);
 	dev_set_drvdata(dev, data);
 
@@ -565,7 +617,7 @@ static int spd5118_common_probe(struct device *dev, struct regmap *regmap)
 	}
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, "spd5118",
-							 regmap, &spd5118_chip_info,
+							 data, &spd5118_chip_info,
 							 NULL);
 	if (IS_ERR(hwmon_dev))
 		return PTR_ERR(hwmon_dev);
@@ -681,21 +733,51 @@ static int spd5118_i2c_init(struct i2c_client *client)
 	return 0;
 }
 
+/*
+ * Writes to SPD devices can be blocked, either by the SMBus controller (SPD
+ * Write Disable, as set by the BIOS on many systems) or by firmware. There is
+ * no generic way to query this, so probe for it by writing the page selector
+ * register back with the value just read from it. That is a no-op on a device
+ * which supports writes.
+ */
+static bool spd5118_write_protected(struct i2c_client *client, int mode)
+{
+	int err;
+
+	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
+	if (err >= 0)
+		return false;
+
+	dev_info(&client->dev,
+		 "SPD writes are blocked (%d), sensor will be read-only\n", err);
+	return true;
+}
+
 static int spd5118_i2c_probe(struct i2c_client *client)
 {
+	const struct regmap_config *config = &spd5118_regmap8_config;
 	struct device *dev = &client->dev;
 	struct regmap *regmap;
-	int err;
+	int err, mode;
+	bool write_protected;
 
 	err = spd5118_i2c_init(client);
 	if (err)
 		return err;
 
-	regmap = devm_regmap_init_i2c(client, &spd5118_regmap8_config);
+	mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
+	if (mode < 0)
+		return mode;
+
+	write_protected = spd5118_write_protected(client, mode);
+	if (write_protected)
+		config = &spd5118_regmap8_ro_config;
+
+	regmap = devm_regmap_init_i2c(client, config);
 	if (IS_ERR(regmap))
 		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
 
-	return spd5118_common_probe(dev, regmap);
+	return spd5118_common_probe(dev, regmap, write_protected);
 }
 
 static const struct i2c_device_id spd5118_i2c_id[] = {
@@ -743,7 +825,11 @@ static int spd5118_i3c_probe(struct i3c_device *i3cdev)
 	if (regval[0] != 0x51 || regval[1] != 0x18)
 		return -ENODEV;
 
-	return spd5118_common_probe(dev, regmap);
+	/*
+	 * SPD Write Disable is an SMBus controller feature and does not apply
+	 * to I3C transfers.
+	 */
+	return spd5118_common_probe(dev, regmap, false);
 }
 
 static struct i3c_driver spd5118_i3c_driver = {
-- 
2.53.0

[PATCH 0/1] hwmon: (spd5118) support write protected devices

[Kean Ren]

This patch makes the spd5118 driver usable on systems that do not allow
writes to SPD devices, while keeping the temperature sensor available.

Problem
=======
On many systems the SMBus controller is configured to block writes to SPD
addresses 0x50-0x57. On the i801 family this is SPD Write Disable
(SMBHSTCFG bit 4), set by the BIOS; reads are unaffected. There is no way
for a driver to query this.

The driver assumes that it can write to the device, which does not hold on
such systems:

 - writes to the hwmon attributes always fail with -ENXIO
 - the temperature sensor disable in spd5118_suspend() fails silently
 - spd5118_resume() fails in regcache_sync(), and the kernel reports a
   device resume failure:

     spd5118 0-0050: Failed to write b = 0: -6
     spd5118 0-0050: PM: failed to resume async: error -6

Register 0xb in that message is the page selector register, which regmap
writes back from its cache at the end of regcache_sync(). The write of the
saved configuration register before that fails as well, but the cache code
does not report that error.

What the patch does
===================
Detect write protection once at probe time by writing the page selector
register back with the value just read from it. This is a no-op on a device
which accepts writes, and it fails if writes are blocked. If writes are not
possible, run the device read-only:

 - drop the register cache (REGCACHE_NONE), so that regcache_sync() cannot
   attempt any write; this is what makes resume work
 - skip the writes in the suspend and resume callbacks
 - expose the writable attributes as 0444
 - do not clear a latched alarm status from the alarm read path, as that
   requires a write
 - limit the nvmem 'eeprom' attribute to the currently selected page,
   instead of returning errors for pages that cannot be reached

What is kept
============
The temperature sensor remains usable. For a chip which is already on page
0 - the normal case - reading the temperature and the temperature limits
does not need any write: regmap only writes the page selector when the
selected page actually changes.

What is not available on such systems
=====================================
 - the temperature limits cannot be configured
 - the sensor cannot be disabled during suspend, so it keeps running
 - SPD EEPROM pages 1..7 are not readable, because selecting a page is
   itself a write (the attribute is limited accordingly)
 - a chip which reports a non-zero page and needs the page reset in
   spd5118_i2c_init() is still rejected at probe, since that reset is also
   a write

Relation to earlier proposals
=============================
Two other approaches were posted for the same problem:

1) Make the driver read-only as well, and additionally avoid instantiating
   it on an i801 adapter when SPD writes are disabled:

     hwmon: (spd5118) restrict writes under SPD write protection
     https://lore.kernel.org/all/20250416-for-upstream-spd5118-spd-write-prot-detect-v1-2-8b3bcafe9dad@canonical.com/

     i2c: i801: don't instantiate spd5118 under SPD Write Disable
     https://lore.kernel.org/all/20250430-for-upstream-i801-spd5118-no-instantiate-v2-2f54d91ae2c7@canonical.com/

2) Expose the restriction as an adapter quirk and have spd5118 fail probe,
   on the grounds that write access is mandatory (including a follow-up
   suggestion to skip SPD instantiation on such adapters):

     i2c: i801: Detect SPD Write Disable and expose as adapter quirk
     https://lore.kernel.org/all/20260205102942.28745-1-tinsaetadesse2015@gmail.com/

This patch is deliberately limited to the driver side. It is complementary
to gating instantiation on the adapter side rather than competing with it:
not instantiating avoids probing a driver which cannot work on a given bus,
while tolerating a write protected device keeps the driver correct when it
is instantiated anyway - from devicetree, from userspace via new_device, or
on a controller which blocks SPD writes without the adapter driver knowing
about it.

The trade-off this patch makes is that DDR5 memory temperature monitoring
keeps working on affected systems, instead of not being available at all.

Testing
=======
Tested on a Lenovo ThinkPad P1 Gen 9 (DDR5), with SPD Write Disable set by
the BIOS:

 - before: suspend-to-idle woke immediately; suspend_stats.failed_resume
   incremented on every cycle (14 times) and last_failed_dev was "0-0050"
 - after: 10 suspend/resume cycles with failed_resume and failed_suspend
   remaining 0
 - the chip is still detected and reports its temperature; the writable
   attributes are 0444

Kean Ren (1):
  hwmon: (spd5118) support write protected devices

 Documentation/hwmon/spd5118.rst |  10 ++++
 drivers/hwmon/spd5118.c         | 110 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 108 insertions(+), 12 deletions(-)

-- 
2.53.0