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: <801599a6-81d9-4e06-8fc6-e959132eef24@roeck-us.net>
Date: Sat, 22 Mar 2025 10:25:11 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Wenliang Yan <wenliang202407@....com>,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 Jean Delvare <jdelvare@...e.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] hwmon:(ina238)Add support for SQ52206

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.

> +#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.

>   			/* 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.

>   		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.

 > -
 > -               /* 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