| 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: <744ba422-b683-4b87-abdc-abb5d662eed5@roeck-us.net>
Date: Wed, 3 Jul 2024 13:55:35 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Quentin Schulz <quentin.schulz@...rry.de>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH v2 09/11] hwmon: (amc6821) Convert to use regmap
On 7/3/24 09:19, Quentin Schulz wrote:
> Hi Guenter,
>
> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>> Use regmap for register accesses and for most caching.
>>
>> While at it, use sysfs_emit() instead of sprintf() to write sysfs
>> attribute data, and remove spurious debug messages which would
>> only be seen as result of a bug in the code.
>>
>> No functional change intended.
>>
>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>> ---
>> v2: Drop another spurious debug message in this patch instead of patch 10
>> Add missing "select REGMAP_I2C" to Kconfig
>> Change misleading variable name from 'mask' to 'mode'.
>> Use sysfs_emit instead of sprintf everywhere
>>
>> drivers/hwmon/Kconfig | 1 +
>> drivers/hwmon/amc6821.c | 713 ++++++++++++++++++----------------------
>> 2 files changed, 329 insertions(+), 385 deletions(-)
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index e14ae18a973b..a8fa87a96e8f 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2127,6 +2127,7 @@ config SENSORS_ADS7871
>> config SENSORS_AMC6821
>> tristate "Texas Instruments AMC6821"
>> depends on I2C
>> + select REGMAP_I2C
>> help
>> If you say yes here you get support for the Texas Instruments
>> AMC6821 hardware monitoring chips.
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 028998d3bedf..3fe0bfeac843 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -8,15 +8,16 @@
>> * Copyright (C) 2007 Hans J. Koch <hjk@...sjkoch.de>
>> */
>> +#include <linux/bitops.h>
>> #include <linux/bits.h>
>> #include <linux/err.h>
>> #include <linux/hwmon.h>
>> #include <linux/hwmon-sysfs.h>
>> #include <linux/i2c.h>
>> #include <linux/init.h>
>> -#include <linux/jiffies.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> +#include <linux/regmap.h>
>> #include <linux/slab.h>
>> /*
>> @@ -44,6 +45,7 @@ module_param(init, int, 0444);
>> #define AMC6821_REG_CONF4 0x04
>> #define AMC6821_REG_STAT1 0x02
>> #define AMC6821_REG_STAT2 0x03
>> +#define AMC6821_REG_TEMP_LO 0x06
>> #define AMC6821_REG_TDATA_LOW 0x08
>> #define AMC6821_REG_TDATA_HI 0x09
>> #define AMC6821_REG_LTEMP_HI 0x0A
>> @@ -61,11 +63,8 @@ module_param(init, int, 0444);
>> #define AMC6821_REG_DCY_LOW_TEMP 0x21
>> #define AMC6821_REG_TACH_LLIMITL 0x10
>> -#define AMC6821_REG_TACH_LLIMITH 0x11
>> #define AMC6821_REG_TACH_HLIMITL 0x12
>> -#define AMC6821_REG_TACH_HLIMITH 0x13
>> #define AMC6821_REG_TACH_SETTINGL 0x1e
>> -#define AMC6821_REG_TACH_SETTINGH 0x1f
>> #define AMC6821_CONF1_START BIT(0)
>> #define AMC6821_CONF1_FAN_INT_EN BIT(1)
>> @@ -130,224 +129,169 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
>> AMC6821_REG_TACH_HLIMITL,
>> AMC6821_REG_TACH_SETTINGL, };
>> -static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI,
>> - AMC6821_REG_TACH_LLIMITH,
>> - AMC6821_REG_TACH_HLIMITH,
>> - AMC6821_REG_TACH_SETTINGH, };
>> -
>> /*
>> * Client data (each client gets its own)
>> */
>> struct amc6821_data {
>> - struct i2c_client *client;
>> + struct regmap *regmap;
>> struct mutex update_lock;
>> - bool valid; /* false until following fields are valid */
>> - unsigned long last_updated; /* in jiffies */
>> - /* register values */
>> - int temp[TEMP_IDX_LEN];
>> -
>> - u16 fan[FAN1_IDX_LEN];
>> - u8 fan1_pulses;
>> -
>> - u8 pwm1;
>> u8 temp1_auto_point_temp[3];
>> u8 temp2_auto_point_temp[3];
>> - u8 pwm1_auto_point_pwm[3];
>> - u8 pwm1_enable;
>> - u8 pwm1_auto_channels_temp;
>> -
>> - u8 stat1;
>> - u8 stat2;
>> };
>> -static struct amc6821_data *amc6821_update_device(struct device *dev)
>> +static int amc6821_init_auto_point_data(struct amc6821_data *data)
>> {
>> - struct amc6821_data *data = dev_get_drvdata(dev);
>> - struct i2c_client *client = data->client;
>> - int timeout = HZ;
>> - u8 reg;
>> - int i;
>> + struct regmap *regmap = data->regmap;
>> + u32 pwm, regval;
>> + int err;
>> - mutex_lock(&data->update_lock);
>> + err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
>
> I think this is incorrect logic.
>
> amc6821_init_auto_point_data is only called once in init_client, in probe. While we currently do not write to AMC6821_REG_DCY_LOW_TEMP, we could in the future. But writing to it would desynchronise the auto_point_temp values for the new value of the register.
>
We do write the register, in pwm1_auto_point_pwm_store().
> I suggest we put the logic into a function and return the value for a given temp_auto_point (1/2 currently) so that we are calling this function instead of a member in the struct so that we are never caching it unknowingly (regmap would do it for us in any case). It's a bit odd anyway to have only **those** values be cached in the struct as members and migrating everything else.
Yes, I know, that is a bit odd. Essentially I was too lazy to clean that up.
But, yes, you are correct, that results in the cached values being wrong
after AMC6821_REG_DCY_LOW_TEMP is updated. Back to the drawing board,
and thanks for finding this.
[ ... ]
>> default:
>> - dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
>
> Was this an intended removal? I think we can afford keeping it in?
Yes, it is intentional. It (and the others below) indicate a coding
error, which would have been found during development. While it does make
sense to keep the default: case to make the compiler happy, it doesn't
make sense to keep the pointless message in the code.
>> @@ -561,10 +538,11 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>> const char *buf, size_t count)
>> {
>> struct amc6821_data *data = dev_get_drvdata(dev);
>> - struct i2c_client *client = data->client;
>> - int dpwm;
>> + struct regmap *regmap = data->regmap;
>> u8 val;
>> - int ret = kstrtou8(buf, 10, &val);
>> + int ret;
>> +
>> + ret = kstrtou8(buf, 10, &val);
>
> Not sure this cosmetic change is worth it? Or maybe squash with an earlier commit?
>
That slipped in, I think from the first patch of the series. I'll move it there.
>> if (ret)
>> return ret;
>> @@ -572,27 +550,24 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>> return -EINVAL;
>> mutex_lock(&data->update_lock);
>> - data->pwm1_auto_point_pwm[1] = val;
>> - if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
>> - data->pwm1_auto_point_pwm[1])) {
>> - dev_err(&client->dev, "Register write error, aborting.\n");
>> - count = -EIO;
>> - goto EXIT;
>> + ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
>> + if (ret)
>
> I think we're missing a count = something here?
>
Yes.
>> + goto unlock;
>> +
>> + ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
>> + data->temp1_auto_point_temp);
>> + if (ret) {
>> + count = ret;
>
> In some places, we replace set_slope_register return code with -EIO (like it was) and sometimes we propagate it, like here. Is there some reason for this or can we have some kind of consistency across the code base here?
>
Thanks for noticing. The code should be consistent and always propagate the error code.
I'll fix it.
Thanks,
Guenter
Powered by blists - more mailing lists