Re: [PATCH v7 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] Incorrect devm teardown sequence allows the delayed worker to re-enable hardware interrupts after the IRQ handler is freed, causing an unhandled interrupt storm.
- [High] Writing a supply voltage threshold replaces the entire hardware register with the 16-bit mantissa, silently clearing the channel's format and mode bits.
- [High] Voltage threshold events are silently disabled in hardware after they fire, making them unexpectedly one-shot.
- [Medium] Unbounded sysfs threshold values cause signed integer overflows and undefined behavior during conversion.
- [Medium] Converting negative temperature thresholds writes 1s to the reserved upper 16 bits of the hardware threshold registers.
--

commit 4db3b58bd3561251c48f5d1c527c846b332e37fd
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 enables rising thresholds and
hysteresis for temperature channels, and rising/falling thresholds for supply
voltage channels. It also sets up interrupt handling, including a delayed
worker for polling and re-arming temperature events.

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 5b7439e443fca..16feb373bc704 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> @@ -51,6 +100,16 @@ static const struct iio_chan_spec temp_channels[] = {
>  	SYSMON_CHAN_TEMP(3, SYSMON_TEMP_MIN_MIN, "min_min"),
>  };
>  
> +static void sysmon_q8p7_to_millicelsius(s16 raw_data, int *val)
> +{
> +	*val = (raw_data * (int)MILLI) >> SYSMON_FRACTIONAL_SHIFT;
> +}
> +
> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> +	*raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;

[Severity: Medium]
Will this 32-bit left shift overflow for large sysfs input values?

Since val is a 32-bit signed integer, an overflow could lead to undefined
behavior during the conversion. Is a cast to a 64-bit integer needed before
the arithmetic operation?

[Severity: Medium]
If a negative temperature threshold is provided, does this calculation
sign-extend the negative integer and write 1s to the upper 16 bits?

The negative 32-bit integer is stored in the u32 pointer. Later in
sysmon_write_event_value(), this unmasked value is written directly to the
threshold register:

sysmon_write_event_value() {
    ...
    sysmon_millicelsius_to_q8p7(&raw_val, val);
    ret = regmap_write(sysmon->regmap, SYSMON_TEMP_TH_UP, raw_val);
    ...
}

Could this overwrite reserved hardware fields or adjacent configuration bits
in the upper 16 bits of the register?

> +}
> +
> +static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
> +{
[ ... ]
> @@ -68,6 +127,34 @@ static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
>  	*val = (mantissa * (int)MILLI) >> exponent;
>  }
>  
> +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: Medium]
Can this 32-bit multiplication overflow when val is a large sysfs input?

Since scale can be up to 65536, multiplying it by the 32-bit signed integer
val might overflow. Should this use a 64-bit cast before the multiplication
to prevent undefined behavior?

> +
> +	if (format)
> +		tmp = clamp(tmp, S16_MIN, S16_MAX);
> +	else
> +		tmp = clamp(tmp, 0, U16_MAX);
> +
> +	*raw_data = (u16)tmp;

[Severity: High]
Does this discard the extracted mode and format bits by returning only the
16-bit mantissa?

When sysmon_write_event_value() writes this value back to the hardware, it uses
regmap_write() which overwrites the entire register:

sysmon_write_event_value() {
    ...
    sysmon_supply_processedtoraw(val, reg_val, &raw_val);
    return regmap_write(sysmon->regmap, offset, raw_val);
}

Will this overwrite the entire 32-bit threshold register and silently clear
the channel's format and mode bits?

> +}
> +
> +static int sysmon_supply_thresh_offset(int address,
[ ... ]
> @@ -127,24 +473,244 @@ static int sysmon_read_label(struct iio_dev *indio_dev,
[ ... ]
> +static int sysmon_handle_event(struct iio_dev *indio_dev, u32 event)
> +{
> +	u32 alarm_flag_offset = SYSMON_ALARM_FLAG + event * SYSMON_REG_STRIDE;
> +	u32 alarm_reg_offset = SYSMON_ALARM_REG + event * SYSMON_REG_STRIDE;
> +	struct sysmon *sysmon = iio_priv(indio_dev);
> +	unsigned long alarm_flag_reg;
> +	unsigned int reg_val;
> +	u32 address, bit;
> +	int ret;
> +
> +	switch (event) {
> +	case SYSMON_BIT_TEMP:
> +		sysmon_push_event(indio_dev, SYSMON_TEMP_MAX);
> +
> +		ret = regmap_write(sysmon->regmap, SYSMON_IDR, BIT(SYSMON_BIT_TEMP));
> +		if (ret)
> +			return ret;
> +
> +		sysmon->masked_temp |= BIT(SYSMON_BIT_TEMP);
> +		return 0;
> +
> +	case SYSMON_BIT_ALARM0:
> +	case SYSMON_BIT_ALARM1:
> +	case SYSMON_BIT_ALARM2:
> +	case SYSMON_BIT_ALARM3:
> +	case SYSMON_BIT_ALARM4:
> +		ret = regmap_read(sysmon->regmap, alarm_flag_offset, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		alarm_flag_reg = reg_val;
> +
> +		for_each_set_bit(bit, &alarm_flag_reg, SYSMON_ALARM_BITS_PER_REG) {
> +			address = bit + SYSMON_ALARM_BITS_PER_REG * event;
> +			sysmon_push_event(indio_dev, address);
> +			ret = regmap_clear_bits(sysmon->regmap, alarm_reg_offset, BIT(bit));

[Severity: High]
Does clearing the alarm configuration bit here permanently disable the voltage
event after it fires?

Unlike the temperature alarms which use a delayed worker to poll and unmask,
there doesn't appear to be a mechanism to re-arm voltage alarms once the
condition clears. Does this make the voltage threshold events unexpectedly
one-shot, rather than remaining enabled until explicitly disabled by the user?

> +			if (ret)
> +				return ret;
> +		}
> +
> +		return regmap_write(sysmon->regmap, alarm_flag_offset, alarm_flag_reg);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
[ ... ]
> +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;
> +
> +	ret = devm_request_irq(dev, irq, sysmon_iio_irq, 0, "sysmon-irq", indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, sysmon_disable_interrupts, sysmon);

[Severity: High]
Does this devm teardown sequence allow the delayed worker to execute after the
IRQ handler has been freed?

Since devm actions execute in reverse order, the IRQ is freed before the
delayed worker is canceled. If sysmon_unmask_worker() runs during this
window, it can unmask the hardware interrupt by calling sysmon_unmask_temp():

sysmon_unmask_worker() -> sysmon_unmask_temp():
    ...
    regmap_write(sysmon->regmap, SYSMON_IER, unmask);

Could this trigger an unhandled interrupt storm since the IRQ handler is no
longer registered?

> +}

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