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

["Huan He" <hehuan1@eswincomputing.com>]

Hi Sashiko, Guenter,

Thank you very much for your detailed review. We appreciate the feedback.

> 
> On 6/11/26 02:17, sashiko-bot@kernel.org wrote:
> > 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?

I will modify the ISR to read PVT_INT first and return IRQ_NONE when
PVT_INT_STAT is not set.

> > 
> > [ ... ]
> > 
> >> +	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?
> > 

I will modify the probe sequence to register the PM runtime cleanup action
before requesting the IRQ.

Best regards,
Huan He

> 
> If not solved, these questions will at least have to be answered.
> 
> Thanks,
> Guenter
> 
> >> +	if (ret) {
> >> +		pm_runtime_put_noidle(&pdev->dev);
> >> +		return dev_err_probe(&pdev->dev, ret,
> >> +				     "Can't register PM cleanup\n");
> >> +	}
> >