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

[Huan He]

Hi Sashiko,

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

> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Clearing the interrupt status before disabling sensor conversions in init_iface() creates a race condition that can lead to an unhandled interrupt storm.
> - [Medium] Missing pm_runtime_disable() in the device cleanup path causes an unbalanced PM runtime state.
> --
> 
> commit 5842ad32271f7d967e9f1e10c13474a5b57da8ad
> Author: Huan He <hehuan1@xxxxxxxxxxxxxxxxxx>
> 
> 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
> > --- /dev/null
> > +++ b/drivers/hwmon/eic7700-pvt.c
> [ ... ]
> > +static int eic7700_pvt_init_iface(struct pvt_hwmon *pvt)
> > +{
> > +	/*
> > +	 * Make sure controller are disabled so not to accidentally have ISR
> > +	 * executed before the driver data is fully initialized. Clear the IRQ
> > +	 * status as well.
> > +	 */
> > +	eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
> > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, 0);
> 
> [Severity: High]
> Could this clear-then-disable sequence cause an unhandled interrupt storm?
> 
> If a bootloader left the sensor active, a conversion might complete in the
> small window between clearing PVT_INT_CLR and disabling PVT_ENA_EN. This
> would assert the IRQ line.
> 
> When the IRQ is later requested in probe(), the ISR eic7700_pvt_hard_isr()
> will fire immediately. However, since the device is suspended,
> pm_runtime_get_if_active() returns <= 0, causing the ISR to return IRQ_NONE
> without clearing the interrupt.
> 
> Because the interrupt is level-triggered (IRQF_TRIGGER_HIGH), would this
> result in a continuous interrupt storm that ultimately forces the kernel
> to permanently disable the IRQ line?
> 
> Should the conversion engine be disabled before the interrupt is cleared?

I will fix this by disabling the conversion engine first, and only then
clearing the pending interrupt status.

The initialization sequence will be changed along these lines:

    eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, 0);
    eic7700_pvt_update(pvt->regs + PVT_INT, PVT_INT_CLR, PVT_INT_CLR);
    readl(pvt->regs + PVT_INT);

> 
> [ ... ]
> > +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);
> > +}
> 
> [Severity: Medium]
> Does this device cleanup path need a call to pm_runtime_disable()?
> 
> In eic7700_pvt_probe(), pm_runtime_enable() is called, which alters the
> PM core's disable_depth count. Since pm_runtime_disable() is missing here,
> if the driver is unbound and later rebound, the next pm_runtime_enable()
> will encounter an unbalanced depth.
> 
> Could this trigger an "Unbalanced pm_runtime_enable!" warning and cause
> inconsistent PM state tracking?

I don't think this is an actual issue.

I re-checked the runtime PM pairing here.

The probe path enables runtime PM with pm_runtime_enable(). The device
cleanup path calls pm_runtime_force_suspend(), and
pm_runtime_force_suspend() already disables runtime PM internally:

int pm_runtime_force_suspend(struct device *dev)
{
    ...
    pm_runtime_disable(dev);
    ...
}

This cleanup path intentionally uses pm_runtime_force_suspend() rather
than a standalone pm_runtime_disable().

A standalone pm_runtime_disable() only disables runtime PM. It does not
force an RPM_ACTIVE device through the driver's runtime_suspend()
callback, so the clock could remain enabled if the driver is unbound
during the autosuspend delay window.

pm_runtime_force_suspend() handles that case by disabling runtime PM
internally and, if the device is still active, invoking the driver's
runtime_suspend() callback so the clock is actually turned off.

Therefore, adding another explicit pm_runtime_disable() after
pm_runtime_force_suspend() would result in an extra disable_depth
increment and could leave the runtime PM state unbalanced.

Please let me know if you would prefer a different runtime PM cleanup
approach here.

Best regards,
Huan He