Message-ID: <acf31929-5d13-4fc5-b370-ab7fc5062455@roeck-us.net>
Date: Wed, 16 Apr 2025 15:39:17 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Yo-Jung (Leo) Lin" <leo.lin@...onical.com>,
 Jean Delvare <jdelvare@...e.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] hwmon: (spd5118) restrict writes under SPD write
 protection

On 4/15/25 23:46, Yo-Jung (Leo) Lin wrote:
> On some platforms, SPD Write Protection for the SMBus controller may be
> enabled. For the i801 family, this will forbid writing data to devices
> residing on addresses from 0x50 to 0x57. This may lead to the following
> issues:
> 
>    1) Writes to the sensor hwmon sysfs attributes will always result in
>       ENXIO.
> 
>    2) System-wide resume will encounter errors during regcache sync back,
>       resulting in the following messages during resume:
> 
>       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
> 

Missing:

3) NVMEM access will fail

> To address this, check if the sensor can be written to at probe, and bypass
> write-related functions if writing to the sensor is not possible.
> 
> Signed-off-by: Yo-Jung (Leo) Lin <leo.lin@...onical.com>
> ---
>   drivers/hwmon/spd5118.c | 31 +++++++++++++++++++++++++++++--
>   1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 3cb2eb2e0227..9dd5342c31dd 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -75,6 +75,7 @@ static const unsigned short normal_i2c[] = {
>   struct spd5118_data {
>   	struct regmap *regmap;
>   	struct mutex nvmem_lock;
> +	bool write_protected;
>   };
>   
>   /* hwmon */
> @@ -284,7 +285,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:
> @@ -499,7 +500,7 @@ static const struct regmap_range_cfg spd5118_regmap_range_cfg[] = {
>   	},
>   };
>   
> -static const struct regmap_config spd5118_regmap_config = {
> +static struct regmap_config spd5118_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
>   	.max_register = 0x7ff,
> @@ -563,6 +564,21 @@ static int spd5118_init(struct i2c_client *client)
>   	return 0;
>   }
>   
> +static bool spd5118_write_protected(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	int mode = 0;
> +	int err = 0;

Both initializations are unnecessary.

> +
> +	mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
> +	if (mode < 0)
> +		dev_warn(dev, "Failed to read MR11: %d", mode);
> +
> +	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
> +

That would try to write the error back if MR11 can not be read, which would be
a bad idea. If the register is not even readable, being able to write it is of
secondary concern.

> +	return (err < 0);

I think this requires a better means to determine if the address range is write
protected. The above is just a wild guess, after all.

Isn't this already handled somehow for DDR4 nvmem (ee1004) ? That has ultimately
the same problem because ee1004 devices can not be accessed if the i2c address
range is write protected.

> +}
> +
>   static int spd5118_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
> @@ -580,6 +596,11 @@ static int spd5118_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	if (spd5118_write_protected(client)) {
> +		data->write_protected = true;
> +		spd5118_regmap_config.cache_type = REGCACHE_NONE;
> +	}
> +

This is insufficient, and overwriting spd5118_regmap_config is not a good idea.
It should be a completely separate configuration which does not list any writeable
registers. I also don't see the value in dropping register caching entirely.

However, even that is insufficient: The driver relies on the register range
being writeable. It is not immediately visible, but the regmap code writes
MR11 to select the nvmem page. If this fails, the entire driver is unusable.
At the very least nvmem access would have to be disabled. However, if the ROM
monitor left the chip in read-only state and had set the page to a value != 0
(I have seen that with some motherboards), temperature monitoring would not work
either at least for memory with spd chips from manufacturers who took the
specification literally (such as Renesas).

That does not apply if the chip is programmed in 16-bit mode (which is not
currently supported), making handling the situation even more complicated.

I think that drivers/i2c/busses/i2c-i801.c should detect if the address space
is write protected, and the driver should not even try to instantiate if that
is the case.

_If_ the driver is to be instantiated, the write protect flag should be passed
to the driver from the instantiating code, for example with a device property.

>   	regmap = devm_regmap_init_i2c(client, &spd5118_regmap_config);
>   	if (IS_ERR(regmap))
>   		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
> @@ -638,6 +659,9 @@ static int spd5118_suspend(struct device *dev)
>   	u32 regval;
>   	int err;
>   
> +	if (data->write_protected)
> +		return 0;
> +
>   	/*
>   	 * Make sure the configuration register in the regmap cache is current
>   	 * before bypassing it.
> @@ -662,6 +686,9 @@ static int spd5118_resume(struct device *dev)
>   	struct spd5118_data *data = dev_get_drvdata(dev);
>   	struct regmap *regmap = data->regmap;
>   
> +	if (data->write_protected)
> +		return 0;
> +

suspend/resume support should be disabled completely in this situation
since it is very much pointless.

Worse, if the BIOS for some reason decides to select a different (non-zero)
page on resume, the driver would be completely inoperable after resume.
That is another argument for not instantiating it in the first place
if this is the case. The impact is just completely unpredictable.

Guenter

>   	regcache_cache_only(regmap, false);
>   	return regcache_sync(regmap);
>   }
> 

Powered by blists - more mailing lists