| 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: <13b73e86-fed0-4ef0-9815-eda765f2a70c@roeck-us.net>
Date: Mon, 1 Jul 2024 06:47:45 -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 09/10] hwmon: (amc6821) Convert to use regmap
On 7/1/24 06:01, Quentin Schulz wrote:
>> -#define AMC6821_CONF1_FDRC1 BIT(7)
>> +#define AMC6821_CONF1_FDRC1 BIT(6)
>
> This should have been squashed with a previous commit.
>
Yes. I had found the bug but then fixed it in the wrong patch of the series.
...
>> static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
>> char *buf)
>> {
>> - struct amc6821_data *data = amc6821_update_device(dev);
>> + struct amc6821_data *data = dev_get_drvdata(dev);
>> int ix = to_sensor_dev_attr(devattr)->index;
>> + u32 regval;
>
> Why not a u8 directly here? We know single reads are going to return a u8 so no need to store more?
>
The parameter of regmap_read is a pointer to unsigned int.
> I'm not too fluent in type conversion, but maybe even an s8 since this is actually what's stored in the register?
>
>> + int err;
>> - return sprintf(buf, "%d\n", data->temp[ix] * 1000);
>> + err = regmap_read(data->regmap, temp_reg[ix], ®val);
>> + if (err)
>> + return err;
>> +
>> + return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
>
> The type casting choice is odd here. Considering we'll be printing an int and that 1000 cannot be stored in an int8_t, maybe just cast it to an int directly?
>
This is a trick which actually originates from the original code, only
there it is hidden in amc6821_update_device(). This is equivalent to
sign_extend32(regval, 7) which is more expensive, so I decided to stick
with it. On the other side, you are correct, it makes the code more
difficult to understand. I'll change it to use sign_extend32().
>> static ssize_t temp_alarm_show(struct device *dev,
>> struct device_attribute *devattr, char *buf)
>> {
>> - struct amc6821_data *data = amc6821_update_device(dev);
>> + struct amc6821_data *data = dev_get_drvdata(dev);
>> int ix = to_sensor_dev_attr(devattr)->index;
>> - u8 flag;
>> + u32 regval, mask, reg;
>
> Why not u8 for regval?
>
Same as above. regmap() expects a pointer to unsigned int.
>> static ssize_t temp2_fault_show(struct device *dev,
>> struct device_attribute *devattr, char *buf)
>> {
>> - struct amc6821_data *data = amc6821_update_device(dev);
>> - if (data->stat1 & AMC6821_STAT1_RTF)
>> - return sprintf(buf, "1");
>> - else
>> - return sprintf(buf, "0");
>> + struct amc6821_data *data = dev_get_drvdata(dev);
>> + u32 regval;
>
> Ditto.
>
Same here and everywhere else.
>> static ssize_t pwm1_enable_store(struct device *dev,
>> @@ -383,49 +346,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
>> const char *buf, size_t count)
>> {
>> struct amc6821_data *data = dev_get_drvdata(dev);
>> - struct i2c_client *client = data->client;
>> long val;
>> - int config = kstrtol(buf, 10, &val);
>> - if (config)
>> - return config;
>> + u32 mask;
>
> Please rename to something else, e.g. val, as this is NOT used as a mask but rather the value to write in the masked bitfield (which is hardcoded to AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1.
>
I'll rename it to mode.
>> +
>> + err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
>> + AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
>> + mask);
>> + if (err < 0)
>
> Shouldn't we check for err != 0 (so err) instead to be consistent with the rest of the code base in the driver?
>
Ok.
>> @@ -564,38 +544,37 @@ 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;
>> - unsigned long val;
>> - int ret = kstrtoul(buf, 10, &val);
>> + struct regmap *regmap = data->regmap;
>> + long val;
>> + int ret;
>> +
>> + ret = kstrtoul(buf, 10, &val);
>> if (ret)
>> return ret;
>> - if (val > 255)
>> + if (val > 254)
>
> I think this should have been squashed with an earlier commit?
>
Yes, another fix applied to the wrong patch.
>> @@ -603,58 +582,72 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>> static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
>> char *buf)
>> {
>> - struct amc6821_data *data = amc6821_update_device(dev);
>> + struct amc6821_data *data = dev_get_drvdata(dev);
>> int ix = to_sensor_dev_attr(devattr)->index;
>> - if (0 == data->fan[ix])
>> - return sprintf(buf, "6000000");
>> - return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
>> + u32 regval;
>> + u8 regs[2];
>
> Can't this be a u16 directly.......
>
>> + int err;
>> +
>> + err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
>> + if (err)
>> + return err;
>> + regval = (regs[1] << 8) | regs[0];
>> +
>
>
> ..... to avoid doing this here?
>
Then it would need an endianness conversion instead. That might save a couple
of cycles, but I think it would make the code more difficult to read.
>> + return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
>> }
>> static ssize_t fan1_fault_show(struct device *dev,
>> struct device_attribute *devattr, char *buf)
>> {
>> - struct amc6821_data *data = amc6821_update_device(dev);
>> - if (data->stat1 & AMC6821_STAT1_FANS)
>> - return sprintf(buf, "1");
>> - else
>> - return sprintf(buf, "0");
>> + struct amc6821_data *data = dev_get_drvdata(dev);
>> + u32 regval;
>> + int err;
>> +
>> + err = regmap_read(data->regmap, AMC6821_REG_STAT1, ®val);
>> + if (err)
>> + return err;
>> +
>> + return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
>> }
>> static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> struct amc6821_data *data = dev_get_drvdata(dev);
>> - struct i2c_client *client = data->client;
>> - long val;
>> int ix = to_sensor_dev_attr(attr)->index;
>> - int ret = kstrtol(buf, 10, &val);
>> - if (ret)
>> - return ret;
>> - val = val < 1 ? 0xFFFF : 6000000 / val;
>> + u8 regs[2];
>
> Ditto.
>
>> + long val;
>> + int err;
>> +
>> + err = kstrtol(buf, 10, &val);
>> + if (err)
>> + return err;
>> +
>> + val = val < 1 ? 0xFFFF : 6000000 / val;
>> + val = clamp_val(val, 0, 0xFFFF);
>> +
>> + regs[0] = val & 0xff;
>> + regs[1] = val >> 8;
>> +
>
> to avoid this here.
> Again, this would require an endianness conversion. Sure, that would save a couple
of cycles, but it would make the code more difficult to read.
>> static int amc6821_probe(struct i2c_client *client)
>> {
>> struct device *dev = &client->dev;
>> struct amc6821_data *data;
>> struct device *hwmon_dev;
>> + struct regmap *regmap;
>
> Save this variable by using data->client directly?
>
Then I'd have to dereference it twice to check for error.
In such situations I prefer to use a separate variable.
Thanks,
Guenter
Powered by blists - more mailing lists