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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] The probe function unconditionally fails for devices without an IRQ, breaking I2C deployments.
- [High] `sysmon_write_event_value` overwrites the entire threshold register, incorrectly clearing upper bits.
- [High] The delayed worker blindly clears all pending hardware interrupts in the ISR register, causing unmasked hardware events to be permanently lost.
- [High] Missing 16-bit mask for negative temperature thresholds writes corrupted data (1s) to upper hardware register bits.
- [Medium] `sysmon_read_event_config` exposes transient internal polling mask state to userspace instead of the administrative state.
- [Medium] Unbounded user input from sysfs causes signed integer overflow in arithmetic conversions for temperature and voltage thresholds.
- [Medium] A race condition between `sysmon_write_event_config` and `sysmon_unmask_worker` can leave a disabled temperature interrupt enabled in hardware.
- [Medium] The driver will cause a 'scheduling while atomic' BUG on the I2C path if an interrupt is erroneously provided in the device tree.
--

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

This patch adds threshold event support for temperature and supply voltage
channels. The driver uses hysteresis mode for temperature and schedules a
delayed worker to poll for condition clear before unmasking.

> The interrupt handler masks active threshold interrupts (which are
> level-sensitive) and schedules a delayed worker to poll for condition
> clear before unmasking. When no hardware IRQ is available, event
> channels are not created and interrupt init is skipped, since the
> I2C regmap backend cannot be called from atomic context.

Does the unconditional check in sysmon_core_probe cause the probe to fail
on I2C devices instead of falling back to polled mode? The code aborts if
fwnode_irq_get returns a negative error code, which contradicts this part
of the commit message.

> diff --git a/drivers/iio/adc/versal-sysmon-core.c b/drivers/iio/adc/versal-sysmon-core.c
> index c7b5eecc93cfb..5fbd509089bbd 100644
> --- a/drivers/iio/adc/versal-sysmon-core.c
> +++ b/drivers/iio/adc/versal-sysmon-core.c
[ ... ]
> +static void sysmon_millicelsius_to_q8p7(u32 *raw_data, int val)
> +{
> +	*raw_data = (val << SYSMON_FRACTIONAL_SHIFT) / (int)MILLI;
> +}

[Severity: Medium]
Could the arithmetic left shift overflow the 32-bit integer for large
user inputs?

[Severity: High]
If the division yields a negative signed integer for sub-zero temperatures,
does this result in an implicitly sign-extended 32-bit value that writes
1s to the upper 16 bits of the hardware register?

[ ... ]
> +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]
Could this multiplication overflow a 32-bit signed integer if userspace
provides a moderately high voltage threshold?

[ ... ]
> +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;
> +
> +	if (chan->type == 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);
> +	}
> +
> +	return !!(imr & alarm_event_mask);
> +}

[Severity: Medium]
Does this expose the transient internal polling mask state from the
hardware instead of the persistent administrative state? If userspace
reads this during the 500ms delayed worker polling window, could it falsely
report that the interrupt is disabled?

[ ... ]
> +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)
> +{
[ ... ]
> +	if (chan->type == 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;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
Is there a race condition here if sysmon_unmask_worker runs exactly
between the regmap_write to SYSMON_IDR and the spinlock acquisition?
Could the worker read the old sysmon->temp_mask and incorrectly re-enable
the interrupt in hardware?

[ ... ]
> +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)
> +{
[ ... ]
> +	if (chan->type == 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);
> +	}
> +
> +	return -EINVAL;
> +}

[Severity: High]
Does this overwrite the entire 32-bit threshold register with a
zero-extended 16-bit value? Since sysmon_supply_processedtoraw casts the
result to a u16 before assigning it to the u32 raw_val pointer, does
writing it back directly clear the upper 16 bits of the hardware register?

[ ... ]
> +static void sysmon_unmask_worker(struct work_struct *work)
> +{
> +	struct sysmon *sysmon =
> +		container_of(work, struct sysmon, sysmon_unmask_work.work);
> +	unsigned int isr;
> +
> +	/*
> +	 * regmap errors are not checked here because the worker and IRQ
> +	 * handler cannot propagate errors. The MMIO regmap uses fast_io
> +	 * with direct readl/writel which cannot fail.
> +	 */
> +	spin_lock_irq(&sysmon->irq_lock);
> +	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);
> +	regmap_write(sysmon->regmap, SYSMON_ISR, isr);
> +	sysmon_unmask_temp(sysmon, isr);
> +	spin_unlock_irq(&sysmon->irq_lock);

[Severity: High]
Does writing the exact read value back to the write-1-to-clear SYSMON_ISR
register permanently clear all pending hardware interrupts? If a supply
voltage alarm triggers just before this runs, could it be silently dropped
since the worker only processes temperature bits?

[ ... ]
> +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);
> +
> +	regmap_read(sysmon->regmap, SYSMON_ISR, &isr);

[Severity: Medium]
Will this cause a scheduling while atomic bug on the I2C path if an
interrupt is erroneously provided in the device tree? Since sysmon_core_probe
unconditionally accepts the IRQ, and this handler uses a spinlock while
performing a regmap_read, could the I2C transfer function attempt to take
a mutex in atomic context?

[ ... ]
> @@ -269,13 +904,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 < 0)
> +		return dev_err_probe(dev, irq, "failed to get IRQ\n");

[Severity: High]
Does this unconditional check cause the probe to fail on I2C devices instead
of falling back to polled mode?

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