Re: [PATCH 1/3] hwmon: spd5118: Do not fail resume on temporary I2C errors

[TINSAE TADESSE]

On Sun, Feb 1, 2026 at 6:21 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 1/31/26 23:30, TINSAE TADESSE wrote:
> > On Sun, Feb 1, 2026 at 4:25 AM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >>
> >> On 1/31/26 11:50, TINSAE TADESSE wrote:
> >> ...
> >>>
> >>> Hi Guenter,
> >>>
> >>> While investigating this issue, I previously mentioned
> >>> about a flow where SPD write disabled state can be
> >>> exported as a capability from the i801 controller, so
> >>> that the SPD5118 hwmon driver consumes it.
> >>>
> >>> The SPD write disabled state is known to the controller
> >>> driver (i2c-i801), but this information is not
> >>> propagated to client drivers. As a result, auto-detected
> >>> devices may be instantiated and probed even though the
> >>> controller cannot support the required access model.
> >>>
> >>> This raises a major architectural question:
> >>>
> >>> Should SMBus / I2C controller drivers be able to
> >>> advertise bus-level capability constraints (such as SPD
> >>> write disabled state) to client drivers, so that clients can
> >>> make an informed decision during probe?
> >>>
> >>> A capability-based approach would allow:
> >>> * controller drivers to expose what is possible on a given bus
> >>> * client drivers to decide whether they can operate correctly
> >>> * avoidance of device-specific policy in controller drivers
> >>> * consistent handling across different SPD-capable devices
> >>>
> >>> I actually tested the possibility of detecting, propagating,
> >>> and consuming the SPD write disabled state using an I2C
> >>> adapter capability flag. Using this approach, I was able to
> >>> fix the issue even with the CONFIG_SENSORS_SPD5118_DETECT
> >>> kernel configuration enabled.
> >>>
> >>> At this stage, I am not proposing a specific implementation.
> >>> The goal of this RFC is to get agreement on whether this type
> >>> of problem should be solved through capability propagation,
> >>> and if so, what mechanism would be preferred.
> >>>
> >>> Any feedback on design direction, or existing infrastructure
> >>> that could be reused would be very welcome.
> >>>
> >>
> >> I think it is a good idea, but how would the flag look like ?
> >> The i801 controller only write protects a range of addresses;
> >> I think it is 0x50..0x57. So any I2C_FUNC flag would presumably
> >> have to be address range specific. You could try adding
> >> something like I2C_FUNC_SPD_WRITE_PROTECTED. Either case,
> >> you'll have to ask the I2C subsystem maintainers for advice.
> >> I would suggest to give it a try.
> >>
> >> Thanks,
> >> Guenter
> >>
> >
> > Hello Guenter,
> >
> > Thanks for the feedback.
> > I have attached one implementation that I tested and confirmed to
> > resolve the issue.
> > I will forward an RFC to the i2c maintainers, and if you don't mind,
> > I will also add you to the email list.
> >
> > Thanks again for the guidance.
>
> Looks good. I didn't know about the quirks, and that drivers are allowed to use them.
> We live and learn. Go ahead, and please copy the hardware monitoring mailing list.
> I'll hold off sending my patch upstream until we get a response; your solution is
> much cleaner.
>
> Thanks,
> Guenter
>


Hi I2C maintainers,

I am working on a platform where the Intel i801 SMBus controller
exposes a hardware feature called "SPD Write Disable". When
enabled, the controller blocks all write transactions to the
SPD EEPROM address range (typically 0x50–0x57), while still
allowing reads.

This creates a problem for the spd5118 hwmon driver, as SPD5118
requires write access for correct operation (page selection,
configuration, suspend/resume cache synchronization). If the controller
silently rejects writes, the driver can misbehave and generate
repeated SMBus DEV_ERR storms.

Currently, the I2C core does not provide a mechanism to expose
write restrictions. Rather than embedding SPD5118-specific policy
in the i801 driver, we are exploring a __bus-level capability/quirk__
that allows consumer drivers to decide whether they can operate safely.

Conceptually:
1. The SMBus controller (i801) detects SPD-WD in hardware
2. The controller exposes this as an adapter-level quirk or capability
3. The I2C core propagates this to I2C clients (already handled by i2c-core)
4. Drivers such as `spd5118` can refuse probe if write access is required

We have implemented and tested this approach (open to guidance) using
a newly defined 'I2C_AQ_SPD_WRITE_DISABLED' quirk flag. The goal is
to keep controller drivers free of device-specific policy, while giving client
drivers enough information to fail probe cleanly instead of
misbehaving at runtime.

If this approach sounds reasonable, please review the attached patch series.

Thanks for your time and guidance.
From a7a6f6e725b02c2bf99db3c65abb22d390146723 Mon Sep 17 00:00:00 2001
From: Tinsae Tadesse <tinsaetadesse2015@xxxxxxxxx>
Date: Mon, 2 Feb 2026 09:41:37 +0300
Subject: [PATCH 2/2] hwmon: spd5118: Fail probe if SPD writes are disabled

SPD5118 requires write access for page selection, configuration,
and cache synchronization during suspend/resume. If the host
controller does not allow SPD writes, the driver cannot function
properly.

Detect this state using adapter quirks and determine whether to
stop the probe.

Signed-off-by: Tinsae Tadesse <tinsaetadesse2015@xxxxxxxxx>
---
 drivers/hwmon/spd5118.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
index 5da44571b6a0..094d05472562 100644
--- a/drivers/hwmon/spd5118.c
+++ b/drivers/hwmon/spd5118.c
@@ -525,6 +525,8 @@ static int spd5118_common_probe(struct device *dev, struct regmap *regmap,
 	unsigned int capability, revision, vendor, bank;
 	struct spd5118_data *data;
 	struct device *hwmon_dev;
+	struct i2c_client *client;
+	const struct i2c_adapter_quirks *quirks;
 	int err;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
@@ -552,6 +554,19 @@ static int spd5118_common_probe(struct device *dev, struct regmap *regmap,
 	if (!spd5118_vendor_valid(bank, vendor))
 		return -ENODEV;
 
+	/*
+	 * SPD5118 requires write access for correct operation
+	 * (page selection, configuration, and suspend/resume cache sync).
+	 * If the SPD writes are blocked by the SMBus controller, the
+	 * probe fails.
+	 */
+	client = to_i2c_client(dev);
+	quirks = client->adapter->quirks;
+	if (quirks && (quirks->flags & I2C_AQ_SPD_WRITE_DISABLED)) {
+		dev_err_probe(dev, -ENODEV, "SPD Write Disable is set on adapter; refusing probe\n");
+		return -ENODEV;
+	}
+
 	data->regmap = regmap;
 	mutex_init(&data->nvmem_lock);
 	dev_set_drvdata(dev, data);
-- 
2.47.3

From 6d93cc581becf9b0c6ac8c1bd35ad2cd993a82d7 Mon Sep 17 00:00:00 2001
From: Tinsae Tadesse <tinsaetadesse2015@xxxxxxxxx>
Date: Mon, 2 Feb 2026 09:40:32 +0300
Subject: [PATCH 1/2] i2c: i801: Detect SPD Write Disable and expose as adapter
 quirk

Detect SPD Write Disable in SMBHSTCFG and expose it through
I2C adapter quirk. The I2C client driver may decide whether
SPD write operations are supported without implementing
device-specific policies in the SMBus controller driver.

Signed-off-by: Tinsae Tadesse <tinsaetadesse2015@xxxxxxxxx>
---
 drivers/i2c/busses/i2c-i801.c | 16 +++++++++++++++-
 include/linux/i2c.h           |  3 +++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 9e1789725edf..d771e9f5f82f 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1533,6 +1533,11 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int err, i, bar = SMBBAR;
 	struct i801_priv *priv;
+	struct i2c_adapter_quirks *quirks;
+
+	quirks = devm_kzalloc(&dev->dev, sizeof(*quirks), GFP_KERNEL);
+	if (!quirks)
+		return -ENOMEM;
 
 	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -1600,8 +1605,17 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		/* Disable SMBus interrupt feature if SMBus using SMI# */
 		priv->features &= ~FEATURE_IRQ;
 	}
-	if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
+
+	/*
+	 * Detect the SPD Write Disabled status. Mark the adapter
+	 * as unable to perform SPD writes, which allows consuming
+	 * drivers to decide on safe operation.
+	 */
+	if (priv->original_hstcfg & SMBHSTCFG_SPD_WD) {
 		pci_info(dev, "SPD Write Disable is set\n");
+		quirks->flags |= I2C_AQ_SPD_WRITE_DISABLED;
+	}
+	priv->adapter.quirks = quirks;
 
 	/* Clear special mode bits */
 	if (priv->features & (FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER))
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 20fd41b51d5c..4b89f0bf62a1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -726,6 +726,9 @@ struct i2c_adapter_quirks {
 /* adapter cannot do repeated START */
 #define I2C_AQ_NO_REP_START		BIT(7)
 
+/* SPD writes are blocked by host controller */
+#define I2C_AQ_SPD_WRITE_DISABLED	BIT(8)
+
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
  * with the access algorithms necessary to access it.
-- 
2.47.3