Message-ID: <CABscksOHUdS2jJ2PZQXymheLuX25spxYVGX5Bu0YJxEybvYw+Q@mail.gmail.com>
Date: Wed, 23 Apr 2025 10:25:20 +0800
From: "Yo-Jung (Leo) Lin" <leo.lin@...onical.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] hwmon: (spd5118) restrict writes under SPD write protection

On Thu, Apr 17, 2025 at 6:39 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> 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.

For now I'll probably approach this from the i801 side (as later part
of your comments suggest), and skip the device instantiation if write
protection is enabled.

>
> 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.

>From ee1004_probe_temp_sensor() I think that the temperature sensor on
ee1004 (jc42) uses address 0x18, and the ee1004 itself at 0x5*
addresses seems to be just an read-only eeprom and has no pm
operations. That's probably why it evades the issue of write
disabling.

>
> > +}
> > +
> >   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.

Although I'll try not instantiate the device at all for now, in case
that there are some users that still think reading DRAM temperature is
helpful, if I were to add a device property here at runtime (e.g. in
i2c_register_spd), should I also update its devicetree binding?

>
> >       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.
Wouldn't this already be catched spd5118_init() function, where the
driver would attempt to overwrite the MR11, and the device won't even
probe successfully if that fails?


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

Powered by blists - more mailing lists