Add driver support for ESWIN EIC7700 PVT controller

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 <[email protected]>

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.

Quoted 5 lines

drivers/hwmon/eic7700-pvt.c new file +0 -0

new file mode 100644
index 0000000000000..3b3aed74c3941

[ ... ]

Quoted 9 lines

drivers/hwmon/eic7700-pvt.c new file +9 -0

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

drivers/hwmon/eic7700-pvt.c line 129

Quoted 10 lines

drivers/hwmon/eic7700-pvt.c new file +10 -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?

[ ... ]

Quoted 4 lines

drivers/hwmon/eic7700-pvt.c new file +4 -0

+	ret = eic7700_pvt_request_irq(pvt);
+	if (ret) {
+		if (!IS_ENABLED(CONFIG_PM))
+			clk_disable_unprepare(pvt->clk);

drivers/hwmon/eic7700-pvt.c line 429

Quoted 10 lines

drivers/hwmon/eic7700-pvt.c new file +10 -0

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

Quoted 5 lines

drivers/hwmon/eic7700-pvt.c new file +5 -0

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

Quoted 46 lines

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 <[email protected]>

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.

Quoted 5 lines

drivers/hwmon/eic7700-pvt.c new file +0 -0

new file mode 100644
index 0000000000000..3b3aed74c3941

[ ... ]

Quoted 9 lines

drivers/hwmon/eic7700-pvt.c new file +9 -0

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

drivers/hwmon/eic7700-pvt.c line 129

Quoted 10 lines

drivers/hwmon/eic7700-pvt.c new file +10 -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?

Quoted 7 lines

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?

Quoted 41 lines

[ ... ]

Quoted 4 lines

drivers/hwmon/eic7700-pvt.c new file +4 -0

+	ret = eic7700_pvt_request_irq(pvt);
+	if (ret) {
+		if (!IS_ENABLED(CONFIG_PM))
+			clk_disable_unprepare(pvt->clk);

drivers/hwmon/eic7700-pvt.c line 429

Quoted 10 lines

drivers/hwmon/eic7700-pvt.c new file +10 -0

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

Quoted 7 lines

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 not solved, these questions will at least have to be answered.

Thanks,
Guenter

Quoted 6 lines

Quoted 5 lines

drivers/hwmon/eic7700-pvt.c new file +5 -0

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