Re: [PATCH v6 4/5] iio: adc: versal-sysmon: add threshold event support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Integer overflow and lack of bounds checking on user-supplied sysfs threshold values
- [High] Kernel panic due to sleeping in atomic context when I2C regmap backend is paired with an interrupt
- [High] Resource leak: Hardware interrupts are not quiesced on driver unbind, causing unhandled interrupt storms
- [Medium] Race condition between interrupt masking state updates and the delayed worker
- [Medium] ABI Violation: `sysmon_read_event_config` exposes transient hardware masking state instead of persistent software configuration
--

commit 4bd81e58f57baddd1a0af6e02ac6cb6917070ee7
Author: Salih Erim <salih.erim@amd.com>

iio: adc: versal-sysmon: add threshold event support

This commit adds threshold event support for temperature and supply voltage
channels in the Versal Sysmon driver. It introduces hysteresis alarm mode
for temperature and configurable threshold limits.

diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
--- a/drivers/iio/adc/versal-sysmon-core.c
+++ b/drivers/iio/adc/versal-sysmon-core.c
@@ -105,6 +232,289 @@ static int sysmon_read_raw(struct iio_dev *indio_dev,
[ ... ]
> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> +	*raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;
> +}

[Severity: High]
Does this bitwise shift overflow for large values of val?

If a user writes a very large threshold, val << SYSMON_FRACTIONAL_SHIFT
could overflow. Because there is no bounds checking or 16-bit masking, the
negative value might be sign-extended and write a negative number to a 32-bit
hardware register, potentially corrupting adjacent reserved fields.

[ ... ]

> +static void sysmon_supply_processedtoraw(int val, u32 reg_val, u32 *raw_data)
> +{
> +	int exponent = FIELD_GET(SYSMON_MODE_MASK, reg_val);
> +	int format = FIELD_GET(SYSMON_FMT_MASK, reg_val);
> +	int scale, tmp;
> +
> +	scale = BIT(SYSMON_SUPPLY_MANTISSA_BITS - exponent);
> +	tmp = (val * scale) / (int)MILLI;

[Severity: High]
Can val * scale result in an arithmetic overflow here?

If the user supplies a large valid input (e.g. 100,000), multiplying by scale
(which can be up to 65,536) could overflow a 32-bit signed integer. This
would yield a negative result, which is then clamped to 0 below. This would
accidentally set the hardware threshold to 0 and trigger an immediate false
alarm.

[ ... ]

> +static int sysmon_read_event_config(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir)
> +{
> +	u32 alarm_event_mask = sysmon_get_event_mask(chan->address);
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int imr;
> +	int config_value;
> +	int ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +	if (ret)
> +		return ret;
> +
> +	/* IMR bits are 1=masked, invert to get 1=enabled */
> +	imr = ~imr;
> +
> +	switch (chan->type) {
> +	case IIO_VOLTAGE:
> +		config_value = sysmon_read_alarm_config(sysmon, chan->address);
> +		if (config_value < 0)
> +			return config_value;
> +		return config_value && (imr & alarm_event_mask);
> +
> +	case IIO_TEMP:
> +		return !!(imr & alarm_event_mask);

[Severity: Medium]
Does returning the raw hardware IMR state violate the IIO ABI?

During a temperature alarm, the interrupt handler masks the event in hardware,
and a delayed worker polls before unmasking it. If userspace reads the enable
attribute during this polling window, this function returns 0 (disabled)
because the hardware IMR bit is set, even if the event is administratively
configured as enabled.

Should this return the persistent logical state stored in sysmon->temp_mask
instead?

[ ... ]

> +static int sysmon_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     bool state)
> +{
> +	u32 offset = SYSMON_ALARM_OFFSET(chan->address);
> +	u32 ier = sysmon_get_event_mask(chan->address);
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int alarm_config;
> +	int ret;
> +
> +	guard(mutex)(&sysmon->lock);
> +
> +	switch (chan->type) {
[ ... ]
> +	case IIO_TEMP:
> +		if (state) {
> +			ret = regmap_write(sysmon->regmap, SYSMON_IER, ier);
> +			if (ret)
> +				return ret;
> +
> +			scoped_guard(spinlock_irq, &sysmon->irq_lock)
> +				sysmon->temp_mask &= ~ier;
> +		} else {
> +			ret = regmap_write(sysmon->regmap, SYSMON_IDR, ier);
> +			if (ret)
> +				return ret;
> +
> +			scoped_guard(spinlock_irq, &sysmon->irq_lock)
> +				sysmon->temp_mask |= ier;
> +		}

[Severity: Medium]
Is there a race condition between writing the hardware register and updating
the software tracking mask?

Because the hardware register (SYSMON_IDR) is written outside of the
sysmon->irq_lock, a window exists where the hardware is disabled but
sysmon->temp_mask has not yet been updated.

If the sysmon_unmask_worker executes during this window, it will read the
stale sysmon->temp_mask (which still indicates the event is enabled):

drivers/iio/adc/versal-sysmon-core.c:sysmon_unmask_temp() {
    ...
    unmask &= ~sysmon->temp_mask;
    regmap_write(sysmon->regmap, SYSMON_IER, unmask);
}

This would cause the worker to mistakenly unmask the interrupt in hardware,
while the software subsequently considers it disabled.

[ ... ]

> +static irqreturn_t sysmon_iio_irq(int irq, void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned int isr, imr;
> +
> +	guard(spinlock)(&sysmon->irq_lock);
> +
> +	if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr) ||
> +	    regmap_read(sysmon->regmap, SYSMON_IMR, &imr))
> +		return IRQ_NONE;

[ ... ]

> +static int sysmon_init_interrupt(struct sysmon *sysmon,
> +				 struct device *dev,
> +				 struct iio_dev *indio_dev,
> +				 int irq)
> +{
> +	unsigned int imr;
> +	int ret;
> +
> +	/* Events not supported without IRQ (e.g. I2C path) */
> +	if (!irq)
> +		return 0;
> +
> +	ret = devm_delayed_work_autocancel(dev, &sysmon->sysmon_unmask_work,
> +					   sysmon_unmask_worker);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(sysmon->regmap, SYSMON_IMR, &imr);
> +	if (ret)
> +		return ret;
> +	sysmon->temp_mask = imr & SYSMON_TEMP_INTR_MASK;
> +
> +	return devm_request_irq(dev, irq, sysmon_iio_irq, 0, "sysmon-irq", indio_dev);
> +}

[Severity: High]
Could this cause a scheduling while atomic panic if an I2C device node
specifies an interrupt?

The hardirq handler sysmon_iio_irq() acquires a spinlock and then performs a
regmap_read(). If the DT provides an interrupts property for the I2C backend,
the driver registers the hardirq handler because of the check for irq > 0.

Since I2C regmap transactions can sleep, calling regmap_read() inside the
spinlock-protected hardirq handler will crash the system.

Should there be a check for !regmap_might_sleep(sysmon->regmap) before
registering the interrupt handler?

[ ... ]

> @@ -256,6 +908,7 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
>  	ret = devm_mutex_init(dev, &sysmon->lock);
>  	if (ret)
>  		return ret;
> +	spin_lock_init(&sysmon->irq_lock);
>  
>  	/* Disable all interrupts and clear pending status */
>  	ret = regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
> @@ -265,13 +918,40 @@ int sysmon_core_probe(struct device *dev, struct regmap *regmap)
>  	if (ret)
>  		return ret;
>  
> +	irq = fwnode_irq_get(dev_fwnode(dev), 0);
> +	if (irq == -EPROBE_DEFER)
> +		return dev_err_probe(dev, irq, "failed to get IRQ\n");
> +
>  	indio_dev->name = "versal-sysmon";
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = sysmon_parse_fw(indio_dev, dev);
> +	ret = sysmon_parse_fw(indio_dev, dev, irq);
>  	if (ret)
>  		return ret;
>  
> +	if (irq > 0) {
> +		/* Set hysteresis mode for both temperature channels */
> +		ret = regmap_set_bits(sysmon->regmap, SYSMON_TEMP_EV_CFG,
> +				      SYSMON_OT_HYST_MASK |
> +				      SYSMON_TEMP_HYST_MASK);
> +		if (ret)
> +			return ret;
> +
> +		/* Initialize cached hysteresis from hardware registers */
> +		ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_TEMP_EVENT,
> +					     &sysmon->temp_hysteresis);
> +		if (ret)
> +			return ret;
> +		ret = sysmon_init_hysteresis(sysmon, SYSMON_ADDR_OT_EVENT,
> +					     &sysmon->ot_hysteresis);
> +		if (ret)
> +			return ret;
> +
> +		ret = sysmon_init_interrupt(sysmon, dev, indio_dev, irq);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }

[Severity: High]
Are hardware interrupts left enabled if the driver is unbound?

While interrupts are quiesced during sysmon_core_probe(), there appears to
be no cleanup callback registered (like a devm_add_action_or_reset) to mask
the interrupts on driver removal.

If userspace enables threshold alarms and the driver is then unbound, the
devm infrastructure will free the IRQ handler, but the device's hardware
interrupts will remain active. This could result in an unhandled interrupt
storm that disables the shared IRQ line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611222738.2035062-1-salih.erim@amd.com?part=4