Re: [PATCH v3] hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Negative values written to `update_interval` are incorrectly clamped to the maximum interval instead of returning an error or clamping to 0.
- [Medium] Reading `update_interval` inconsistently under-reports the configured value due to integer division truncation.
- [Medium] Hardware sensors are enabled in `probe` but are not disabled on probe failure or driver unbind.
--

commit 91a403f5c5eef9fbcfcab3857179dc2f03ad3c89
Author: Lars Randers <lranders@mail.dk>

hwmon: add a driver for the temp/voltage sensor on PolarFire SoC

Add a driver for the temperature and voltage sensors on PolarFire SoC.
The temperature reports how hot the die is, and the voltages are the
SoC's 1.05, 1.8 and 2.5 volt rails respectively.

> diff --git a/drivers/hwmon/tvs-mpfs.c b/drivers/hwmon/tvs-mpfs.c
> new file mode 100644
> index 0000000000000..48de8d98b3444
> --- /dev/null
> +++ b/drivers/hwmon/tvs-mpfs.c

[ ... ]

> +static int mpfs_tvs_interval_read(struct mpfs_tvs *data, u32 attr, long *val)
> +{
> +	u32 tmp;
> +
> +	if (attr != hwmon_chip_update_interval)
> +		return -EOPNOTSUPP;
> +
> +	regmap_read(data->regmap, MPFS_TVS_CTRL, &tmp);
> +	*val = FIELD_GET(MPFS_TVS_INTERVAL_MASK, tmp);
> +	*val *= MPFS_TVS_INTERVAL_SCALE;
> +	*val /= 1000;

[Severity: Medium]
Will this return an incorrectly truncated value for certain intervals?

Since integer division is used both here and in mpfs_tvs_interval_write(), 
the truncation can cause the readback value to be lower than what was written.
For example, writing 7 ms calculates `(7 * 1000) / 32 = 218`. Reading it back
computes `(218 * 32) / 1000 = 6` ms. 

Should DIV_ROUND_CLOSEST be used for these conversions to ensure the value
survives the round trip?

> +
> +	return 0;
> +}
> +
> +static int mpfs_tvs_interval_write(struct mpfs_tvs *data, u32 attr, long val)
> +{
> +	unsigned long temp = val;
> +
> +	if (attr != hwmon_chip_update_interval)
> +		return -EOPNOTSUPP;
> +
> +	temp = clamp(temp, 0U, MPFS_TVS_INTERVAL_MAX_MS);

[Severity: Medium]
Does this correctly handle negative input values?

The parameter `val` is signed, but it is assigned to `unsigned long temp`. If
a negative value is passed (e.g., -1), it undergoes an unsigned conversion and
becomes a large positive number like ULONG_MAX. 

When clamp() evaluates this against 8 ms, it incorrectly clamps to 8 instead of
returning -EINVAL or properly clamping to 0.

> +
> +	temp *= 1000;
> +	temp /= MPFS_TVS_INTERVAL_SCALE;

[Severity: Medium]
Similar to mpfs_tvs_interval_read(), does this integer truncation prevent
accurately restoring the configured value? Should DIV_ROUND_CLOSEST be
used here as well?

> +
> +	temp <<= MPFS_TVS_INTERVAL_OFFSET;
> +	regmap_update_bits(data->regmap, MPFS_TVS_CTRL,
> +			   MPFS_TVS_INTERVAL_MASK, temp);
> +
> +	return 0;
> +}

[ ... ]

> +static int mpfs_tvs_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	/*
> +	 * It's an MMIO regmap with no resources, there's nothing that can fail
> +	 * and return an error
> +	 */
> +	regmap_write(data->regmap, MPFS_TVS_CTRL, MPFS_TVS_CTRL_ENABLE_ALL);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "mpfs_tvs",
> +							 data,
> +							 &mpfs_tvs_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(hwmon_dev),
> +				     "hwmon device registration failed.\n");

[Severity: Medium]
Does the hardware remain enabled if hwmon device registration fails, or if the
driver is unbound?

The hardware sensors are enabled here by writing MPFS_TVS_CTRL_ENABLE_ALL, but
there doesn't seem to be an error path or devm action to disable them if
devm_hwmon_device_register_with_info() fails. 

Could this lead to unnecessary power consumption when the driver fails to probe
or is not actively loaded?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-blank-footprint-5504b819baec@spud?part=1