Re: [PATCH v7 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The IRQ is requested before the PM runtime cleanup action is registered, leading to a possible use-after-disable of the hardware clock in the ISR on driver removal.
- [Medium] The interrupt handler `eic7700_pvt_hard_isr` does not check the hardware interrupt status bit to verify if the device actually generated the interrupt.
--

commit 0f4c8aefb83780ee09652639eb2843f01c2baf38
Author: Huan He <hehuan1@eswincomputing.com>

hwmon: Add Eswin EIC7700 PVT sensor driver

Add support for ESWIN EIC7700 Voltage and Temperature sensor. The driver
supports temperature and voltage monitoring with polynomial conversion,
and provides sysfs interface for sensor data access.

> diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
> new file mode 100644
> index 0000000000000..3b3aed74c3941
> --- /dev/null
> +++ b/drivers/hwmon/eic7700-pvt.c

[ ... ]

> +static irqreturn_t eic7700_pvt_hard_isr(int irq, void *data)
> +{
> +	struct pvt_hwmon *pvt = data;
> +	int active;
> +	u32 val;
> +
> +	if (IS_ENABLED(CONFIG_PM)) {
> +		active = pm_runtime_get_if_active(pvt->dev);
> +		if (active <= 0)
> +			return IRQ_NONE;
> +	}
> +
> +	eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
> +	/*
> +	 * Read the data, update the cache and notify a waiter of this event.
> +	 */
> +	val = readl(pvt->regs + PVT_DATA);
> +	WRITE_ONCE(pvt->data_cache, FIELD_GET(PVT_DATA_OUT, val));
> +	complete(&pvt->conversion);

[Severity: Medium]
Does this unconditionally process the interrupt without checking if the device
actually generated it?

If a spurious interrupt occurs, the ISR will prematurely clear the interrupt,
read whatever stale value is in PVT_DATA, and call complete(&pvt->conversion).
This will cause the waiting thread in eic7700_pvt_read_data() to wake up early
and return stale or garbage data to userspace.

Should the ISR read the status register and return IRQ_NONE if its device did
not assert the interrupt?

[ ... ]

> +	ret = eic7700_pvt_request_irq(pvt);
> +	if (ret) {
> +		if (!IS_ENABLED(CONFIG_PM))
> +			clk_disable_unprepare(pvt->clk);
> +		return ret;
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 3000);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_get_noresume(&pdev->dev);
> +
> +	ret = devm_add_action_or_reset(pvt->dev, eic7700_pvt_disable_pm_runtime,
> +				       pvt);

[Severity: High]
Can this devres setup lead to accessing unclocked registers during device
removal?

Because devres executes teardown callbacks in reverse order of their
registration, eic7700_pvt_disable_pm_runtime() will run before the IRQ
is freed by devm_request_threaded_irq()'s teardown.

During removal, eic7700_pvt_disable_pm_runtime() disables the clock:

eic7700_pvt_disable_pm_runtime() {
    ...
    if (!pm_runtime_status_suspended(pvt->dev)) {
        clk_disable_unprepare(pvt->clk);
        pm_runtime_set_suspended(pvt->dev);
    }
}

If an interrupt fires concurrently and the ISR executes past the
pm_runtime_get_if_active() check before the clock is disabled, the ISR
will attempt to access hardware registers while unclocked, which can lead
to an external abort or kernel panic:

eic7700_pvt_hard_isr() {
    ...
    eic7700_pvt_update(pvt->regs + PVT_INT, ...);
}

Should the PM runtime cleanup action be registered before the IRQ is requested?

> +	if (ret) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Can't register PM cleanup\n");
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611090505.734-1-hehuan1@eswincomputing.com?part=2