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

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

Hi Guenter, Sashiko,

Gentle ping on this.

I am preparing the next revision and would like to confirm whether the
remaining interrupt-ordering concern still needs a code change.

My understanding is that eic7700_pvt_init_iface() 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.

I plan to address the confirmed issues in the next revision, including:
  - replacing pm_runtime_put_noidle() in the ISR with pm_runtime_put()
  - removing pvt_clear_data() and the related devm_add_action()
  - disabling the clock on the IRQ request error path when !CONFIG_PM

Could you please let me know whether you still expect an additional change
for this part?

Thanks for your time and review.

Best regards,
Huan He

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