Message-ID: <db48e680-2386-449f-8882-387e8069c541@roeck-us.net>
Date: Sat, 17 Jan 2026 16:43:06 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Wenliang Yan <wenliang202407@....com>, Jean Delvare <jdelvare@...e.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 RESEND 0/8] hwmon: (ina3221) Various improvement and
 add support for SQ52210

On 1/14/26 00:17, Wenliang Yan wrote:
> Resending this series.
> 
> 1.Add review information for the binding(PATCH 1).
> 
> 2.Modify the description of the patch(PATCH 2).
> 
> 3.Add initialization for 'alert_type_select'(PATCH 4).
> 
> 4.Add out-of-bounds checks for the array(PATCH 8).
> 
> ---
> v3: https://lore.kernel.org/linux-hwmon/20251120081921.39412-1-wenliang202407@163.com/
> v2: https://lore.kernel.org/linux-hwmon/20251118125148.95603-1-wenliang202407@163.com/
> v1: https://lore.kernel.org/linux-hwmon/20251111080546.32421-1-wenliang202407@163.com/
> 
> Wenliang Yan (8):
>    dt-bindings: hwmon: ti,ina3221: Add SQ52210
>    hwmon: (ina3221) Add support for SQ52210
>    hwmon: (ina3221) Pre-calculate current and power LSB
>    hwmon: (ina3221) Support alert configuration
>    hwmon: (ina3221) Introduce power attribute and alert characteristics
>    hwmon: (ina3221) Support for writing alert limit values and modify the
>      'ina3221_read_value' function
>    hwmon: (ina3221) Support write/read functions for 'power' attribute
>    hwmon: (ina3221) Modify write/read functions for 'in' and 'curr'
>      attribute
> 

I applied the series to the branch and asked to an AI code review.
The applied patches are:

2befc15cc315 (HEAD -> ina3221) hwmon: (ina3221) Modify write/read functions for 'in' and 'curr' attribute
fe1c157ccfb6 hwmon: (ina3221) Support write/read functions for 'power' attribute
12ac733cc62c hwmon: (ina3221) Support for writing alert limit values and modify the 'ina3221_read_value' function
306f47c14733 hwmon: (ina3221) Introduce power attribute and alert characteristics
bd15aff74e6b hwmon: (ina3221) Support alert configuration
5780752da297 hwmon: (ina3221) Pre-calculate current and power LSB
dabf3c279a1f hwmon: (ina3221) Add support for SQ52210

This is the AI feedback:

Review Summary

   The top 7 patches (commits dabf3c279a1f to 2befc15cc315) introduce support for the Silergy SQ52210 chip. While the overall structure follows the project conventions, there are several critical issues that need to be addressed.

   1. Arithmetic Overflows on 32-bit Systems
   Several calculations can overflow a 32-bit long, which is common on many embedded architectures supported by the Linux kernel.

    * `ina3221_read_power` (Patch 2: `fe1c157ccfb6`):

    1     *val = DIV_ROUND_CLOSEST(regval * ina->power_lsb_uW, 1000);
       With a 16-bit regval (up to 65535) and a possible power_lsb_uW of up to 800,000,000 (for a 1µΩ shunt), the product can reach ~52.4e12, significantly exceeding the ~2.1e9 limit of a 32-bit signed long. Even with a standard 10mΩ shunt,
   the product is ~5.2e9, which still overflows.

    * `ina3221_read_curr` (Patch 1: `2befc15cc315`):

    1     *val = DIV_ROUND_CLOSEST(regval * ina->current_lsb_uA, 1000);
       Similar to power, regval * current_lsb_uA can exceed 2^31 if the shunt resistor is small (e.g., < 1mΩ).

    * `sq52210_alert_limit_write` (Patch 3: `12ac733cc62c`):

    1     regval = DIV_ROUND_CLOSEST(val * 1000, ina->current_lsb_uA) & 0xfff8;
    2     ...
    3     regval = DIV_ROUND_CLOSEST(val * 1000, ina->power_lsb_uW) & 0xfff8;
       The val * 1000 operation can overflow if val (in mA or mW) is large. For power, a value above 2.1kW will overflow.

   Recommendation: Use u64 for intermediate products and div_u64 or DIV_S64_ROUND_CLOSEST to ensure 64-bit arithmetic is used where necessary.

   2. Documentation Typo (Patch 4: 306f47c14733)
   In Documentation/hwmon/ina3221.rst:

    1 power[123]_input        Current for channels 1, 2, and 3 respectively
   The description for power[123]_input should be "Power for channels..." instead of "Current".

   3. Missing Channel Validation (Patch 1: 2befc15cc315)
   In ina3221_write_in, there is no check to ensure the channel index is within the valid range for alerts (0-2) before calling sq52210_alert_limit_write.

    1 static int ina3221_write_in(struct device *dev, u32 attr, int channel, long val)
    2 {
    3     ...
    4     case hwmon_in_lcrit:
    5         return sq52210_alert_limit_write(ina, SQ52210_ALERT_BUL, channel, val);
   Since in4_lcrit through in7_lcrit might be visible (due to ina3221_is_visible logic), a user could write to these attributes. sq52210_alert_limit_write then performs channel % INA3221_NUM_CHANNELS, which would cause in4_lcrit (shunt
   voltage) to overwrite the bus voltage alert for in1.

   Recommendation: Add if (channel >= INA3221_NUM_CHANNELS) return -EOPNOTSUPP; to ina3221_write_in for the lcrit and crit attributes.

   4. Incorrect Current Alert Validation (Patch 3: 12ac733cc62c)
   In sq52210_alert_limit_write:

    1 if (item >= ARRAY_SIZE(alert_groups) || val < 0)
    2     return -EINVAL;
   This rejects negative values for all alert types. However, the SQ52210 (and INA3221) supports bi-directional current measurement. A "Shunt Under Limit" (SUL) alert should logically be able to trigger on negative current values if the
   hardware supports it. The subsequent code in the same function even attempts to handle negative values with clamp_val(regval, -32760, 32760).

   Recommendation: Allow negative values for SQ52210_ALERT_SUL.

   5. Inconsistent Register Masking (Patch 3: 12ac733cc62c)
   In sq52210_alert_limit_write, the SUL and POL types apply & 0xfff8 to the register value, but BOL and BUL (Bus Over/Under Limit) do not. If the hardware ignores the lower 3 bits for all limit types on these registers (as the comments
   suggest), this masking should be applied consistently.

I did not verify if the entire feedback is correct, but it does look reasonable.
Please fix or explain why the feedback does not apply.

Thanks,
Guenter

Powered by blists - more mailing lists