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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The delayed worker `sysmon_unmask_worker` silently clears all pending hardware interrupts, causing concurrent voltage alarm events to be permanently dropped.
- [High] Missing bounds checking in threshold value conversion leads to integer overflow and hardware register corruption.
- [High] Race condition between interrupt configuration/teardown and the unmask worker leads to unhandled interrupt storms.
- [High] Voltage alarm handler incorrectly disables the alarm configuration and early returns leak ISR status, causing interrupt storms.
- [High] The hardirq handler `sysmon_iio_irq` takes a spinlock and sleeps when using an I2C regmap.
- [High] Writing a supply voltage threshold silently clears the format (FMT) and mode (MODE) bits in the threshold register.
- [Medium] Unconditional execution of the delayed worker resets hardware tracking status on voltage alarms.
--

commit 4ebb615e40cc90088cf5fe0430223005d0197c79
Author: Salih Erim <salih.erim@amd.com>
Subject: iio: adc: versal-sysmon: add threshold event support

This commit adds threshold event support for temperature and supply voltage
channels on the Versal Sysmon. It configures hysteresis mode for temperature
alarms and window mode for supply voltage channels, along with an interrupt
handler and delayed worker to handle the level-sensitive hardware interrupts.

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index 03a745d3fb4f3..50b5228aa222f 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> @@ -52,6 +102,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 * MILLIDEGREE_PER_DEGREE) >> SYSMON_FRACTIONAL_SHIFT;
> +}
> +
> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> +	*raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / MILLIDEGREE_PER_DEGREE;
> +}

[Severity: High]
Can this overflow if a large or negative value is passed from userspace?

If val << SYSMON_FRACTIONAL_SHIFT overflows, or if val is negative, the
resulting negative integer gets implicitly cast to a u32 without masking. This
sign-extends the value, which then gets written directly to the
SYSMON_TEMP_TH_UP register, potentially corrupting reserved upper bits.

> +
>  static void sysmon_supply_rawtoprocessed(int raw_data, int *val)
>  {
>  	int mantissa, format, exponent;
> @@ -69,6 +129,33 @@ 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: High]
Does val * scale overflow the 32-bit signed integer before it reaches the
clamp() below?

A large positive voltage could map to a negative integer, clamping to 0 and
inadvertently programming a 0V threshold.

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

[ ... ]
> +static int sysmon_write_event_value(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info,
> +				    int val, int val2)
> +{
[ ... ]
> +	case IIO_VOLTAGE:
> +		offset = sysmon_supply_thresh_offset(chan->address, dir);
> +		if (offset < 0)
> +			return offset;
> +
> +		ret = regmap_read(sysmon->regmap, offset, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		sysmon_supply_processedtoraw(val, reg_val, &raw_val);
> +
> +		return regmap_write(sysmon->regmap, offset, raw_val);

[Severity: High]
Does casting tmp to u16 in sysmon_supply_processedtoraw() discard the extracted
exponent and format bits?

When raw_val is later written via regmap_write() here, won't this zero out the
SYSMON_FMT_MASK and SYSMON_MODE_MASK fields in the threshold register,
corrupting the hardware configuration?  Should this use regmap_update_bits()
instead?

[ ... ]
> +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_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));
> +			if (ret)
> +				return ret;
> +		}

[Severity: High]
Does this clear the hardware alarm configuration rather than just masking the
interrupt?

alarm_reg_offset resolves to SYSMON_ALARM_REG, which completely disables the
alarm and forces the sysfs state out of sync.

Also, if regmap_clear_bits() fails and returns early, doesn't this skip writing
to alarm_flag_offset to clear the level-sensitive hardware flag? This would
leave the interrupt unmasked and unhandled, resulting in an immediate interrupt
storm.

[ ... ]
> +static void sysmon_unmask_worker(struct work_struct *work)
> +{
> +	struct sysmon *sysmon =
> +		container_of(work, struct sysmon, sysmon_unmask_work.work);
> +	unsigned int isr;
> +
> +	/*
> +	 * If the ISR read fails, skip processing to avoid acting
> +	 * on undefined data.
> +	 */
> +	scoped_guard(spinlock_irq, &sysmon->irq_lock) {
> +		if (regmap_read(sysmon->regmap, SYSMON_ISR, &isr))
> +			break;
> +		regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> +		sysmon_unmask_temp(sysmon, isr);
> +	}

[Severity: High]
Does this silently drop concurrent voltage alarm events?

SYSMON_ISR is a Write-1-to-Clear register. The code reads the entire register
and writes it back verbatim, clearing all pending interrupts. However,
sysmon_unmask_temp() only processes temperature interrupts. 

If a supply voltage alarm fires just before this read, it is cleared from the
hardware but never dispatched since the worker doesn't call
sysmon_handle_events().

> +
> +	if (sysmon->masked_temp)
> +		schedule_delayed_work(&sysmon->sysmon_unmask_work,
> +				      msecs_to_jiffies(SYSMON_UNMASK_WORK_DELAY_MS));
> +	else
> +		regmap_write(sysmon->regmap, SYSMON_STATUS_RESET, 1);
> +}

[Severity: Medium]
Does this unintentionally flush the hardware's internal min/max tracking
registers for unrelated events?

In sysmon_iio_irq(), schedule_delayed_work() is called unconditionally. If a
voltage alarm triggers the interrupt, sysmon->masked_temp remains 0. When this
worker runs 500ms later, it sees masked_temp is 0 and executes the else branch,
resetting the hardware status on every voltage threshold crossing.

> +
> +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;

[Severity: High]
Can this sleep in a hardirq context if the device is attached via I2C?

The driver requests sysmon_iio_irq as a primary hardirq handler via
devm_request_irq(). Although the commit message mentions I2C won't have an
interrupt, if an I2C device node provides an "interrupts" property, it will
be requested. 

When the IRQ fires, the I2C regmap will attempt to acquire a mutex while
holding a spinlock in hardirq context, leading to a panic. Should this use
a threaded IRQ?

[ ... ]
> +static void sysmon_disable_interrupts(void *data)
> +{
> +	struct sysmon *sysmon = data;
> +
> +	regmap_write(sysmon->regmap, SYSMON_IDR, SYSMON_INTR_ALL_MASK);
> +
> +	scoped_guard(spinlock_irq, &sysmon->irq_lock)
> +		sysmon->masked_temp = 0;
> +
> +	cancel_delayed_work_sync(&sysmon->sysmon_unmask_work);
> +}

[Severity: High]
Is there a race condition here with sysmon_unmask_worker()?

The driver writes to SYSMON_IDR outside of sysmon->irq_lock. Concurrently,
sysmon_unmask_worker() calculates the unmask bit under the lock based on the
software state.

If the worker calculates the unmask state before this teardown path acquires
the lock to clear sysmon->masked_temp, the worker will write to SYSMON_IER,
overriding the teardown path's IDR write. This leaves the hardware interrupt
enabled after driver unbind and IRQ handler removal, resulting in an interrupt
storm.

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