linux-kernel - Re: [PATCH 10/10] hwmon: (amc6821) Convert to with

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ac6b3f9-7e08-470f-ad46-982a88623ba3@roeck-us.net>
Date: Mon, 1 Jul 2024 11:24:48 -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 10/10] hwmon: (amc6821) Convert to with_info API

Hi uentin,

On 7/1/24 10:46, Quentin Schulz wrote:

>>           return err;
>> -    return sysfs_emit(buf, "%d\n", !!(regval & mask));
>> +    *val = (int8_t)regval * 1000;
> 
> The discussed signed_extended32() in another patch would need to make it here too.
> 
Yes. Thanks for the reminder, I had missed that one.

>> -    return sprintf(buf, "%d\n", val);
>> +    err = regmap_bulk_read(regmap, reg, regs, 2);
>> +    if (err)
>> +        return err;
>> +
>> +    regval = (regs[1] << 8) | regs[0];
>> +    *val = 6000000 / (regval ? : 1);
>> +
> 
> Just putting a "bookmark" here since we're having a discussion about this logic in another thread for another patch.
> 
> Also... missing 0 here between the question mark and the colon?
> 
It translates to
	 *val = 6000000 / (regval ? regval : 1);
which is what I had wanted (divide by regval if regval != 0,
otherwise divide by 1 [i.e., return 6000000]).

I have it now as
	*val = regval ? 6000000 / regval : 0;

>> +static int amc6821_fan_write(struct device *dev, u32 attr, long val)
>> +{
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>> +    struct regmap *regmap = data->regmap;
>> +    u8 regs[2];
>> +    int reg;
>> +
>> +    if (attr == hwmon_fan_pulses) {
>> +        if (val != 2 && val != 4)
>> +            return -EINVAL;
>> +        return regmap_update_bits(regmap, AMC6821_REG_CONF4,
>> +                     AMC6821_CONF4_PSPR,
>> +                     val == 4 ? AMC6821_CONF4_PSPR : 0);
>> +    }
>> +
>> +    if (val < 0)
>> +        return -EINVAL;
>> +
>> +    val = clamp_val(6000000 / (val ? : 1), 0, 0xffff);
>> +
> 
> And another "bookmark" here.
> 

That is now
	val = val ? 6000000 / clamp_val(val, 1, 6000000) : 0;
         val = clamp_val(val, 0, 0xffff);

'val' is checked earlier and must be > 0 for the minimum
and target fan speed.
	
>> +    switch (attr) {
>> +    case hwmon_fan_min:
>> +        reg = AMC6821_REG_TACH_LLIMITL;
>> +        break;
>> +    case hwmon_fan_max:
>> +        reg = AMC6821_REG_TACH_HLIMITL;
>> +        break;
>> +    case hwmon_fan_target:
>> +        reg = AMC6821_REG_TACH_SETTINGL;
>> +        break;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    regs[0] = val & 0xff;
>> +    regs[1] = val >> 8;
>> +
>> +    return regmap_bulk_write(data->regmap, reg, regs, 2);
>>   }
>>   static ssize_t temp_auto_point_temp_show(struct device *dev,
>>                        struct device_attribute *devattr,
>>                        char *buf)
>>   {
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr_2(devattr)->index;
>>       int nr = to_sensor_dev_attr_2(devattr)->nr;
>> -    struct amc6821_data *data = dev_get_drvdata(dev);
> 
> Should be squashed with the appropriate commit?
> 

Yes, the previous one.

>>       switch (nr) {
>>       case 1:
>> @@ -423,7 +495,6 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
>>           return sprintf(buf, "%d\n",
>>               data->temp2_auto_point_temp[ix] * 1000);
>>       default:
>> -        dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
> 
> Not sure this is related? Maybe a separate commit?
> 

It should have been part of the previous commit, where it is explained ("remove
spurious debug messages which would only be seen as result of a bug in the code")

>> @@ -872,7 +902,6 @@ static int amc6821_probe(struct i2c_client *client)
>>       if (!data)
>>           return -ENOMEM;
>> -
> 
> Should be squashed with the previous commit.
> 
Ok.

> Nice clean-up albeit very difficult to review in patch form, not sure there's anything we can do about it though. Maybe migrating one attribute at a time, but I would myself not be very happy if I was asked to do it :)
> 

Yes, that is always the problem with the conversion to the with_info API.
I don't think it would make much sense to try to split it further.

Thanks a lot for the detailed review!

Guenter