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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7683 bytes --]

On Wed, May 27, 2026 at 09:34:27AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
> - [High] Masking out the sign bit of a two's complement value incorrectly reports small negative voltages as massive positive spikes.
> - [High] Hardware initialization occurs after device registration and uses `regmap_write`, creating a race condition and clobbering existing register state.
> - [High] Missing return value checks for `regmap_read` expose uninitialized kernel stack memory to userspace.
> - [High] Blind dereference of `pdev->dev.parent` in the probe function.
> - [Medium] The `update_interval` attribute is exposed and expected in microseconds, violating the hwmon sysfs ABI standard.
> - [Medium] Boolean sysfs enable attributes fail to properly reject negative values.
> - [Medium] The driver uses syscon APIs but its Kconfig entry does not depend on or select `MFD_SYSCON`.
> - [Medium] The driver ignores return values from hardware write operations, masking potential hardware failures.
> - [Low] Using a bitwise right shift (`>> 4`) on potentially negative temperatures relies on implementation-defined behavior and rounds incorrectly.
> - [Low] The comment explaining the absolute zero constant is mathematically incorrect.
> --
> 
> commit 0cc52b7af906d819cbf1265ac544bb7d896a59da
> 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/Kconfig b/drivers/hwmon/Kconfig
> > index 14e4cea48acc4..194c8116fd018 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> 
> [ ... ]
> 
> > @@ -930,6 +930,18 @@ config SENSORS_JC42
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called jc42.
> >  
> > +config SENSORS_POLARFIRE_SOC_TVS
> > +	tristate "PolarFire SoC (MPFS) temperature and voltage sensor"
> > +	depends on POLARFIRE_SOC_MAILBOX
> 
> [Severity: Medium]
> Should this Kconfig entry depend on or select MFD_SYSCON?
> 
> The driver explicitly calls device_node_to_regmap() which is provided by
> the syscon framework. If MFD_SYSCON is not enabled, the fallback stub
> unconditionally returns -ENOTSUPP, which would cause the driver to silently
> fail to probe at runtime without producing any build-time warnings.

I'm not sure why this uses the mailbox as a dep, but it depends on
MFD_SYSCON so that satisfies the condition. The deps here need rework.

> > +/*
> > + * For all of these the value in millivolts is stored in 16 bits, with an upper
> > + * sign bit and a lower 2 bits of decimal. These masks discard the sign bit and
> > + * decimal places, because if Linux is running these voltages cannot be negative
> > + * and so avoid having to convert to two's complement.
> > + */
> > +#define MPFS_OUTPUT0_V1P8_MASK	GENMASK(30, 19)
> > +#define MPFS_OUTPUT0_V1P05_MASK	GENMASK(14, 3)
> > +#define MPFS_OUTPUT1_V2P5_MASK	GENMASK(14, 3)
> 
> [Severity: High]
> Does discarding the sign bit cause issues with slight negative readings?
> 
> If the hardware measures a slightly negative voltage due to sensor noise
> (for example, -1 mV), wouldn't the two's complement representation have the
> integer bits set to all 1s?
> 
> Since the driver discards the sign bit and extracts the remaining bits via
> FIELD_GET, it seems this could result in reporting the maximum positive
> integer (4095 mV) instead of clamping to 0.

The comment covers this.

> > +static int mpfs_tvs_voltage_read(struct mpfs_tvs *data, u32 attr,
> > +				 int channel, long *val)
> > +{
> > +	u32 tmp, control;
> > +
> > +	if (attr != hwmon_in_input && attr != hwmon_in_enable)
> > +		return -EOPNOTSUPP;
> > +
> > +	regmap_read(data->regmap, MPFS_TVS_CTRL, &control);
> 
> [Severity: High]
> Can a failed regmap_read() leave the control variable uninitialized?
> 
> If the underlying bus read fails, the control variable remains uninitialized,
> and this function could extract bits from uninitialized kernel stack memory
> and return it to userspace via sysfs. This also appears to happen with the
> tmp variable in the other read functions below.

It's an MMIO bus, I've never bothered checking the results of
regmap_read() etc cos I can't check the result of readl().

> > +static int mpfs_tvs_temp_read(struct mpfs_tvs *data, u32 attr, long *val)
> > +{
> > +	u32 tmp, control;
> > +
> > +	if (attr != hwmon_temp_input && attr != hwmon_temp_enable)
> > +		return -EOPNOTSUPP;
> > +
> > +	regmap_read(data->regmap, MPFS_TVS_CTRL, &control);
> > +
> > +	if (attr == hwmon_temp_enable) {
> > +		*val = FIELD_GET(MPFS_TVS_CTRL_TEMP_ENABLE, control);
> > +		return 0;
> > +	}
> > +
> > +	if (!(control & MPFS_TVS_CTRL_TEMP_VALID))
> > +		return -EINVAL;
> > +
> > +	regmap_read(data->regmap, MPFS_TVS_OUTPUT1, &tmp);
> > +	*val = FIELD_GET(MPFS_OUTPUT1_TEMP_MASK, tmp);
> > +	*val -= MPFS_TVS_K_TO_C;
> > +	*val = (1000 * *val) >> 4; /* fixed point (11.4) to millidegrees */
> 
> [Severity: Low]
> Does using a bitwise right shift on a potentially negative temperature
> value rely on implementation-defined behavior?
> 
> Subtracting MPFS_TVS_K_TO_C can yield a negative number. An arithmetic
> right shift rounds towards negative infinity rather than towards zero like
> standard division, which might introduce minor inaccuracies for sub-zero
> temperatures.

Does this matter?

> > +static int mpfs_tvs_probe(struct platform_device *pdev)
> > +{
> > +	struct device *hwmon_dev;
> > +	struct mpfs_tvs *data;
> > +
> > +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->regmap = device_node_to_regmap(pdev->dev.parent->of_node);
> 
> [Severity: High]
> Could pdev->dev.parent be NULL if the platform device is instantiated
> manually?
> 
> If a privileged user writes to sysfs to instantiate the device or it is
> bound such that pdev->dev.parent is NULL, would this unconditional
> dereference trigger a NULL pointer dereference?

I don't think I have ever seen code check this.

> > +	if (IS_ERR(data->regmap))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(data->regmap),
> > +				     "Failed to find syscon regmap\n");
> > +
> > +	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");
> > +
> > +	regmap_write(data->regmap, MPFS_TVS_CTRL, MPFS_TVS_CTRL_ENABLE_ALL);
> 
> [Severity: High]
> Does registering the device before enabling the sensors create a race window?
> 
> The thermal core or userspace might attempt to read sensors after
> devm_hwmon_device_register_with_info() completes but before they are
> enabled.

Is this a realistic concern? Moving it is effectively free I think
anyway, not really even worth turning them off in a remove callback.

> Also, does this regmap_write() clobber the update_interval field?
> Overwriting the entire control register with MPFS_TVS_CTRL_ENABLE_ALL
> appears to zero out bits 15:8, destroying any interval settings made during
> the race window or pre-configured by the bootloader.

Intentionally, to get a known-good state.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]