linux-kernel - Re: [PATCH v5 2/2] hwmon:(ina238)Add support for SQ52206

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: <20250324123852.4120-1-wenliang202407@163.com>
Date: Mon, 24 Mar 2025 08:38:52 -0400
From: Wenliang Yan <wenliang202407@....com>
To: linux@...ck-us.net
Cc: christophe.jaillet@...adoo.fr,
	conor+dt@...nel.org,
	corbet@....net,
	jdelvare@...e.com,
	krzk+dt@...nel.org,
	linux-hwmon@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	robh@...nel.org,
	wenliang202407@....com
Subject: Re: [PATCH v5 2/2] hwmon:(ina238)Add support for SQ52206

At 2025-03-23 01:25:11, "Guenter Roeck" <linux@...ck-us.net> wrote:
>On 3/13/25 01:02, Wenliang Yan wrote:
>> Add support for SQ52206 to the Ina238 driver. Add registers,
>> add calculation formulas, increase compatibility, add
>> compatibility programs for multiple chips.
>> 
>> Signed-off-by: Wenliang Yan <wenliang202407@....com>
>
>The patch unfortunately combines adding support for a new chip
>with adding the necessary infrastructure. I finally found the time
>to look into this further and split the changes, trying to find out
>what actually changed. Unfortunately there are some problems.
>Some of them are listed below.
>
>This is not a complete review. Also, I'll have to write module tests
>to ensure that support for existing chips is not broken.
>
>>   #define INA238_SHUNT_VOLTAGE_LSB	5 /* 5 uV/lsb */
>>   #define INA238_BUS_VOLTAGE_LSB		3125 /* 3.125 mV/lsb */
>> -#define INA238_DIE_TEMP_LSB		125 /* 125 mC/lsb */
>> +#define INA238_DIE_TEMP_LSB			1250000 /* 125 mC/lsb */
>
>This is not correct. The unit is 10ths of uC.
>

Since the temp_lsb of sq52206 is 7.8125 mC/LSB, in order to express it as an interger, 
my original intention was to use 7.8125*10000 to represent 7.8125 mC/lsb. 
At the same time, for the consistency of different devides, using 125 * 10000 to represent 125mC/lsb.

>> +#define SQ52206_BUS_VOLTAGE_LSB		3750 /* 3.75 mV/lsb */
>> +#define SQ52206_DIE_TEMP_LSB		78125 /* 7.8125 mC/lsb */
>>   
>... expressed in 10ths of uC.
>
>>   static const struct regmap_config ina238_regmap_config = {
>>   	.max_register = INA238_REGISTERS,
>> @@ -102,7 +114,20 @@ static const struct regmap_config ina238_regmap_config = {
>>   	.val_bits = 16,
>>   };
>>   
>> +enum ina238_ids { ina238, ina237, sq52206 };
>> +
>> +struct ina238_config {
>> +	bool has_power_highest;		/* chip detection power peak */
>> +	bool has_energy;		/* chip detection energy */
>> +	u8 temp_shift;
>> +	u32 power_calculate_factor;		/*fixed parameters for power calculate*/
>> +	u16 config_default;
>> +	int bus_voltage_lsb;    /* uV */
>> +	int temp_lsb;   /* mC */
>
>No, this is not the temperature in mC. It is the temperature in 10th of uC.
>

This is indeed a problem, I need to handle temp_lsb appropriately.

>>   			/* gain of 1 -> LSB / 4 */
>> -			*val = (regval * INA238_SHUNT_VOLTAGE_LSB) /
>> -			       (1000 * (4 - data->gain + 1));
>> +			*val = (regval * INA238_SHUNT_VOLTAGE_LSB) *
>> +					data->gain / (1000 * 4);
>
>The slight benefit of this change is that the divisor is now a constant,
>which may enable some compiler optimization. Still, it is not a necessary
>change and just makes review more difficult.
>

The original formula takes effect only when gain=1 or 4, but sq52206 has a gain=2.
The principle of this formula is the same as before.

>>   		else
>> -			*val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000;
>> +			*val = (regval * data->config->bus_voltage_lsb) / 1000;
>>   		break;
>>   	case hwmon_in_max_alarm:
>>   	case hwmon_in_min_alarm:
>> @@ -225,8 +298,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
>>   	case 0:
>>   		/* signed value, clamp to max range +/-163 mV */
>>   		regval = clamp_val(val, -163, 163);
>> -		regval = (regval * 1000 * (4 - data->gain + 1)) /
>> -			 INA238_SHUNT_VOLTAGE_LSB;
>> +		regval = (regval * 1000 * 4) /
>> +			 INA238_SHUNT_VOLTAGE_LSB * data->gain;
>
>This change is both unnecessary and wrong: The result is multiplied by data->gain,
>not divided by it. "INA238_SHUNT_VOLTAGE_LSB * data->gain" would have to be in ()
>to yield the same result as the original code. Also, unlike the change above,
>it makes the divisor non-constant. I don't see why this would be beneficial.
>

The original formula takes effect only when gain=1 or 4, but sq52206 has a gain=2.
"INA238_SHUNT_VOLTAGE_LSB * data->gain" should to be in (), I will change.

> > -
> > -               /* Signed, bits 15-4 of register, result in mC */
> > -               *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
> > +               /* Signed, result in mC */
> > +               *val = div_s64(((s16)regval >> data->config->temp_shift) *
> > +                                               data->config->temp_lsb, 10000);
>
>This will overflow since there is no type cast to s64.
>(32767 >> 4) * 1250000 == 0x98836D30. That means that large positive temperatures
>will be reported as negative temperatures.
>
>I did not have time to validate the rest of the calculation changes,
>but each of them will require close scrutiny. I suspect that most if not all
>of the 64-bit operations can and will overflow because the first parameter is
>not type cast to s64/u64.
>
>Also, the patch should really be
>split into multiple patches:
>- Introduce ina238_config
>- Calculation optimizations such as the ones above, with rationale
>- Introduce support for SQ52206
>to simplify (and even enable) review.
>
>Thanks,
>Guenter

I will add a theoretical analysis to the calculation of each change as soon as possible
to prevent errors and reduce your review difficulty.

I will split this patch to show the changes more clearly in v6.


Thanks,
Wenliang Yan