Re: Re: [PATCH v5 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 5/15/26 03:03, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> > - [Critical] The timeout calculation for sensor conversion evaluates to 0 jiffies, causing all reads to instantly fail.
> > - [High] Missing pm_runtime_disable() in the devres cleanup path leads to an unbalanced disable_depth.
> > - [High] Clock leakage on the IRQ request error path when CONFIG_PM is disabled.
> > - [High] Race condition between ISR and main thread leaks PM runtime active state.
> > - [High] Incorrect initialization order causes an IRQ storm during probe if an interrupt fires early.
> > - [High] Lack of internal locking in eic7700_pvt_hwmon_read allows concurrent hardware state corruption.
> > - [Medium] Useless and incorrectly ordered devres action for unblocking sysfs readers.
> > --
> > 
> > commit fe78922d5101b3a07ca5ea0fbd0fd5e9cba0dcc9
> > 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
> >> index 0000000000000..cdcfa031cdbdb
> >> --- /dev/null
> >> +++ b/drivers/hwmon/eic7700-pvt.c
> > [ ... ]
> >> +static irqreturn_t eic7700_pvt_hard_isr(int irq, void *data)
> >> +{
> > [ ... ]
> >> +	val = readl(pvt->regs + PVT_DATA);
> >> +	WRITE_ONCE(pvt->data_cache, FIELD_GET(PVT_DATA_OUT, val));
> >> +	complete(&pvt->conversion);
> >> +
> >> +	if (IS_ENABLED(CONFIG_PM))
> >> +		pm_runtime_put_noidle(pvt->dev);
> >> +
> >> +	return IRQ_HANDLED;
> >> +}
> > 
> > If the main thread in eic7700_pvt_hwmon_read() drops the usage count from 2
> > to 1 before the ISR drops it from 1 to 0, will both threads fail to trigger
> > the idle callback?
> > 
> > This could leave the device indefinitely in the active state. Should the ISR
> > use pm_runtime_put_autosuspend() or pm_runtime_put() instead of
> > pm_runtime_put_noidle()?

I will fix this by replacing pm_runtime_put_noidle() in the ISR with
pm_runtime_put(), so the final reference drop can still trigger runtime PM
idle handling.

The ISR will be changed along these lines:

    complete(&pvt->conversion);

    if (IS_ENABLED(CONFIG_PM))
-       pm_runtime_put_noidle(pvt->dev);
+       pm_runtime_put(pvt->dev);

> > 
> > [ ... ]
> >> +static int eic7700_pvt_read_data(struct pvt_hwmon *pvt,
> >> +				 enum pvt_sensor_type type, long *val)
> >> +{
> >> +	unsigned long timeout;
> > [ ... ]
> >> +	timeout = 2 * usecs_to_jiffies(ktime_to_us(pvt->timeout));
> >> +	ret = wait_for_completion_timeout(&pvt->conversion, timeout);
> > 
> > Because pvt->timeout is 333 microseconds, will usecs_to_jiffies(333) evaluate
> > to 0 on standard kernel configurations like HZ=1000?
> > 
> > If so, this acts as a non-blocking check and wait_for_completion_timeout()
> > could instantly return 0, causing all reads to permanently fail with
> > -ETIMEDOUT.

This looks like a false positive.

usecs_to_jiffies() eventually goes through _usecs_to_jiffies(), which
rounds up instead of truncating to zero:

    static inline unsigned long _usecs_to_jiffies(const unsigned int u)
    {
        return (u + (USEC_PER_SEC / HZ) - 1) / (USEC_PER_SEC / HZ);
    }

So with HZ=1000:

    usecs_to_jiffies(333) = (333 + 1000 - 1) / 1000 = 1

Therefore for pvt->timeout = 333 us, the timeout does not collapse to 0
jiffies. The current code evaluates to 2 jiffies, so
wait_for_completion_timeout() remains blocking rather than becoming a
non-blocking check.

> > 
> > [ ... ]
> >> +static int eic7700_pvt_hwmon_read(struct device *dev,
> >> +				  enum hwmon_sensor_types type, u32 attr,
> >> +				  int ch, long *val)
> >> +{
> >> +	struct pvt_hwmon *pvt = dev_get_drvdata(dev);
> > 
> > The driver registers the temperature sensor with the thermal subsystem via
> > the HWMON_C_REGISTER_TZ flag.
> > 
> > If thermal zone background polling directly invokes this read callback while
> > a user concurrently reads a hwmon sysfs attribute, is there a race condition?
> > Does this need an internal mutex to prevent concurrent hardware state
> > corruption?
> > 
> 
> False positive. Polling is through the hwmon subsystem, which protects such
> accesses with the hwmon subsystem lock.
> 
> 
> The other concerns seem real. Please address.
> 
> Thanks,
> Guenter
> 
> > [ ... ]
> >> +static struct pvt_hwmon *eic7700_pvt_create_data(struct platform_device *pdev)
> >> +{
> > [ ... ]
> >> +	ret = devm_add_action(dev, pvt_clear_data, pvt);
> > 
> > Because devres actions execute in reverse order of registration (LIFO), will
> > this action run after the hwmon device is unregistered?
> > 
> > Since hwmon unregistration blocks until all active sysfs read operations
> > finish, the reader would have already completed before pvt_clear_data runs,
> > which might make it ineffective for unblocking sysfs readers.

I will remove pvt_clear_data() and the associated devm_add_action() call.

> > 
> > [ ... ]
> >> +static void eic7700_pvt_disable_pm_runtime(void *data)
> >> +{
> >> +	struct pvt_hwmon *pvt = data;
> >> +
> >> +	pm_runtime_dont_use_autosuspend(pvt->dev);
> >> +	pm_runtime_force_suspend(pvt->dev);
> >> +
> >> +	if (!IS_ENABLED(CONFIG_PM))
> >> +		clk_disable_unprepare(pvt->clk);
> >> +}
> > 
> > Should this custom devres cleanup action call pm_runtime_disable()?
> > 
> > If the module is unloaded and then re-probed, could the unbalanced
> > disable_depth cause an "Unbalanced pm_runtime_enable!" warning and corrupt
> > the power management state?

This looks like a false positive.

The cleanup path already calls pm_runtime_force_suspend(), and
pm_runtime_force_suspend() itself invokes pm_runtime_disable() in the PM
core.
Adding another explicit pm_runtime_disable() after
pm_runtime_force_suspend() would over-increment disable_depth and break
the reprobe path.

> > 
> >> +
> >> +static int eic7700_pvt_probe(struct platform_device *pdev)
> >> +{
> > [ ... ]
> >> +	if (IS_ENABLED(CONFIG_PM))
> >> +		clk_disable_unprepare(pvt->clk);
> >> +
> >> +	ret = eic7700_pvt_request_irq(pvt);
> >> +	if (ret)
> >> +		return ret;
> > 
> > If CONFIG_PM is disabled, the clock remains enabled. If
> > eic7700_pvt_request_irq() fails here, does the function return without
> > disabling the clock? This could permanently leak the clock reference because
> > the devres cleanup action is not yet registered.

This concern is valid.

When CONFIG_PM is disabled, the clock remains enabled after probe-time
initialization. If eic7700_pvt_request_irq() fails before the devres PM cleanup
action is registered, the clock can indeed be leaked.

I will fix this by explicitly disabling the clock on the IRQ request error path
when CONFIG_PM is disabled.

The probe will be changed along these lines:

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

> > 
> >> +
> >> +	pm_runtime_enable(&pdev->dev);
> > 
> > Since the IRQ is unmasked before pm_runtime_enable() is called, what happens
> > if the hardware asserts an interrupt in this window?
> > 
> > If eic7700_pvt_hard_isr() fires, pm_runtime_get_if_active() would return
> > -EINVAL and the handler would return IRQ_NONE without clearing the interrupt.
> > Since the interrupt is level-triggered (IRQF_TRIGGER_HIGH), could this cause
> > an infinite IRQ storm that locks up the CPU?

My understanding is that eic7700_pvt_init_iface() already clears
the interrupt status and disables PVT_ENA_EN before the IRQ is requested,
so no new interrupt is expected in the request_irq() -> pm_runtime_enable()
window.

Given that, do you think an additional change is still needed here?

Best regards,
Huan He