| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2046d2c3-bbf6-4842-bc51-b2f567f33c0a@gmx.de>
Date: Sun, 16 Jun 2024 20:09:54 +0200
From: Armin Wolf <W_Armin@....de>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Wolfram Sang <wsa+renesas@...g-engineering.com>,
René Rebe <rene@...ctcode.de>,
Thomas Weißschuh <linux@...ssschuh.net>,
Stephen Horvath <s.horvath@...look.com.au>,
Paul Menzel <pmenzel@...gen.mpg.de>, Sasha Kozachuk <skozachuk@...gle.com>,
John Hamrick <johnham@...gle.com>
Subject: Re: [RFT PATCH] hwmon: (spd5118) Add support for Renesas/ITD SPD5118
hub controllers
Am 14.06.24 um 20:59 schrieb Guenter Roeck:
> The SPD5118 specification says, in its documentation of the page bits
> in the MR11 register:
>
> "
> This register only applies to non-volatile memory (1024) Bytes) access of
> SPD5 Hub device.
> For volatile memory access, this register must be programmed to '000'.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> "
>
> Renesas/ITD SPD5118 hub controllers take this literally and disable access
> to volatile memory if the page selected in MR11 is != 0. Since the BIOS or
> ROMMON will access the non-volatile memory and likely select a page != 0,
> this means that the driver will not instantiate since it can not identify
> the chip. Even if the driver instantiates, access to volatile registers
> is blocked after a nvram read operation which selects a page other than 0.
>
> To solve the problem, add initialization code to select page 0 during
> probe. Before doing that, use basic validation to ensure that this is
> really a SPD5118 device and not some random EEPROM. Explicitly select
> page 0 when accessing the volatile register space, and protect volatile
> register access against nvmem access using the device mutex.
Hi,
maybe we can use struct regmap_range_cfg so the paged register accesses are being
done by the regmap code itself?
Thanks,
Armin Wolf
>
> Cc: Sasha Kozachuk <skozachuk@...gle.com>
> Cc: John Hamrick <johnham@...gle.com>
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
> This patch depends on the spd5118 patch series submitted earlier.
>
> RFT: I was only able to test this patch with DDR5 using the Montage
> Technology SPD5118 hub controller. It needs testing with the Renesas
> hub controller, and could use some additional testing with other DIMMs.
>
> drivers/hwmon/spd5118.c | 164 +++++++++++++++++++++++++++++-----------
> 1 file changed, 119 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index ac94a6779360..96052ef4256b 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
> @@ -74,7 +74,7 @@ static const unsigned short normal_i2c[] = {
>
> struct spd5118_data {
> struct regmap *regmap;
> - struct mutex nvmem_lock;
> + struct mutex access_lock;
> };
>
> /* hwmon */
> @@ -92,6 +92,29 @@ static u16 spd5118_temp_to_reg(long temp)
> return (DIV_ROUND_CLOSEST(temp, SPD5118_TEMP_UNIT) & 0x7ff) << 2;
> }
>
> +static int spd5118_set_page(struct regmap *regmap, int page)
> +{
> + unsigned int old_page;
> + int err;
> +
> + err = regmap_read(regmap, SPD5118_REG_I2C_LEGACY_MODE, &old_page);
> + if (err)
> + return err;
> +
> + if (page != (old_page & SPD5118_LEGACY_MODE_MASK)) {
> + /* Update page and explicitly select 1-byte addressing */
> + err = regmap_update_bits(regmap, SPD5118_REG_I2C_LEGACY_MODE,
> + SPD5118_LEGACY_MODE_MASK, page);
> + if (err)
> + return err;
> +
> + /* Selected new NVMEM page, drop cached data */
> + regcache_drop_region(regmap, SPD5118_EEPROM_BASE, 0xff);
> + }
> +
> + return 0;
> +}
> +
> static int spd5118_read_temp(struct regmap *regmap, u32 attr, long *val)
> {
> int reg, err;
> @@ -174,28 +197,44 @@ 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;
> + int err;
>
> if (type != hwmon_temp)
> return -EOPNOTSUPP;
>
> + mutex_lock(&data->access_lock);
> +
> + err = spd5118_set_page(regmap, 0);
> + if (err)
> + goto unlock;
> +
> switch (attr) {
> case hwmon_temp_input:
> case hwmon_temp_max:
> case hwmon_temp_min:
> case hwmon_temp_crit:
> case hwmon_temp_lcrit:
> - return spd5118_read_temp(regmap, attr, val);
> + err = spd5118_read_temp(regmap, attr, val);
> + break;
> case hwmon_temp_max_alarm:
> case hwmon_temp_min_alarm:
> case hwmon_temp_crit_alarm:
> case hwmon_temp_lcrit_alarm:
> - return spd5118_read_alarm(regmap, attr, val);
> + err = spd5118_read_alarm(regmap, attr, val);
> + break;
> case hwmon_temp_enable:
> - return spd5118_read_enable(regmap, val);
> + err = spd5118_read_enable(regmap, val);
> + break;
> default:
> - return -EOPNOTSUPP;
> + err = -EOPNOTSUPP;
> + break;
> }
> +
> +unlock:
> + mutex_unlock(&data->access_lock);
> + return err;
> }
>
> static int spd5118_write_temp(struct regmap *regmap, u32 attr, long val)
> @@ -256,14 +295,28 @@ 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;
> + int err;
> +
> + mutex_lock(&data->access_lock);
> +
> + err = spd5118_set_page(regmap, 0);
> + if (err)
> + goto unlock;
>
> switch (type) {
> case hwmon_temp:
> - return spd5118_temp_write(regmap, attr, val);
> + err = spd5118_temp_write(regmap, attr, val);
> + break;
> default:
> - return -EOPNOTSUPP;
> + err = -EOPNOTSUPP;
> + break;
> }
> +
> +unlock:
> + mutex_unlock(&data->access_lock);
> + return err;
> }
>
> static umode_t spd5118_is_visible(const void *_data, enum hwmon_sensor_types type,
> @@ -382,35 +435,12 @@ static const struct hwmon_chip_info spd5118_chip_info = {
>
> /* nvmem */
>
> -static int spd5118_nvmem_set_page(struct regmap *regmap, int page)
> -{
> - unsigned int old_page;
> - int err;
> -
> - err = regmap_read(regmap, SPD5118_REG_I2C_LEGACY_MODE, &old_page);
> - if (err)
> - return err;
> -
> - if (page != (old_page & SPD5118_LEGACY_MODE_MASK)) {
> - /* Update page and explicitly select 1-byte addressing */
> - err = regmap_update_bits(regmap, SPD5118_REG_I2C_LEGACY_MODE,
> - SPD5118_LEGACY_MODE_MASK, page);
> - if (err)
> - return err;
> -
> - /* Selected new NVMEM page, drop cached data */
> - regcache_drop_region(regmap, SPD5118_EEPROM_BASE, 0xff);
> - }
> -
> - return 0;
> -}
> -
> static ssize_t spd5118_nvmem_read_page(struct regmap *regmap, char *buf,
> unsigned int offset, size_t count)
> {
> int err;
>
> - err = spd5118_nvmem_set_page(regmap, offset >> SPD5118_PAGE_SHIFT);
> + err = spd5118_set_page(regmap, offset >> SPD5118_PAGE_SHIFT);
> if (err)
> return err;
>
> @@ -439,19 +469,19 @@ static int spd5118_nvmem_read(void *priv, unsigned int off, void *val, size_t co
> if (off + count > SPD5118_EEPROM_SIZE)
> return -EINVAL;
>
> - mutex_lock(&data->nvmem_lock);
> + mutex_lock(&data->access_lock);
>
> while (count) {
> ret = spd5118_nvmem_read_page(data->regmap, buf, off, count);
> if (ret < 0) {
> - mutex_unlock(&data->nvmem_lock);
> + mutex_unlock(&data->access_lock);
> return ret;
> }
> buf += ret;
> off += ret;
> count -= ret;
> }
> - mutex_unlock(&data->nvmem_lock);
> + mutex_unlock(&data->access_lock);
> return 0;
> }
>
> @@ -524,15 +554,65 @@ static const struct regmap_config spd5118_regmap_config = {
> .cache_type = REGCACHE_MAPLE,
> };
>
> +static int spd5118_init(struct i2c_client *client)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + int err, regval, mode;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA))
> + return -ENODEV;
> +
> + regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
> + if (regval < 0 || (regval && regval != 0x5118))
> + return -ENODEV;
> +
> + /*
> + * If the type register returns 0, it is possible that the chip has a
> + * non-zero page selected and takes the specification literally, i.e.
> + * disables access to volatile registers besides the page register if
> + * the page is not 0. Try to identify such chips.
> + */
> + if (!regval) {
> + mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
> + if (mode < 0 || (mode & 0xf0) || !(mode & 0x07))
> + return -ENODEV;
> +
> + err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, 0);
> + if (err)
> + return -ENODEV;
> +
> + regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
> + if (regval != 0x5118) {
> + i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE, mode);
> + return -ENODEV;
> + }
> + }
> +
> + regval = i2c_smbus_read_byte_data(client, SPD5118_REG_CAPABILITY);
> + if (regval < 0)
> + return -ENODEV;
> +
> + if (!(regval & SPD5118_CAP_TS_SUPPORT))
> + return -ENODEV;
> +
> + /* We are reasonably sure that this is really a SPD5118 hub controller */
> + return 0;
> +}
> +
> static int spd5118_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> - unsigned int regval, revision, vendor, bank;
> + unsigned int revision, vendor, bank;
> struct spd5118_data *data;
> struct device *hwmon_dev;
> struct regmap *regmap;
> int err;
>
> + err = spd5118_init(client);
> + if (err)
> + return err;
> +
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
> @@ -541,12 +621,6 @@ static int spd5118_probe(struct i2c_client *client)
> if (IS_ERR(regmap))
> return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed\n");
>
> - err = regmap_read(regmap, SPD5118_REG_CAPABILITY, ®val);
> - if (err)
> - return err;
> - if (!(regval & SPD5118_CAP_TS_SUPPORT))
> - return -ENODEV;
> -
> err = regmap_read(regmap, SPD5118_REG_REVISION, &revision);
> if (err)
> return err;
> @@ -561,7 +635,7 @@ static int spd5118_probe(struct i2c_client *client)
> return -ENODEV;
>
> data->regmap = regmap;
> - mutex_init(&data->nvmem_lock);
> + mutex_init(&data->access_lock);
> dev_set_drvdata(dev, data);
>
> err = spd5118_nvmem_init(dev, data);
> @@ -572,7 +646,7 @@ static int spd5118_probe(struct i2c_client *client)
> }
>
> 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);
Powered by blists - more mailing lists