Message-ID: <e05a1228-8b6b-491e-8b2a-925319cb50a2@roeck-us.net>
Date: Tue, 22 Apr 2025 20:33:10 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Yo-Jung (Leo) Lin" <leo.lin@...onical.com>
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 4/22/25 19:25, Yo-Jung (Leo) Lin wrote:
> 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.
> 

They also have multiple pages, but it looks like they use I2C addresses outside
the 50..57 rage to select the current page. So, yes, they should be fine.

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

I don't think so - the device property would not be visible in devicetree
and only be used internally.

>>
>>>        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?
> 
Yes, you are correct, but it is still unpredictable: Depending on what the BIOS
is doing, the page may be set to different values (for example, it might depend
on the type of reboot - soft, hard, or powercycle).

Thanks,
Guenter

Powered by blists - more mailing lists