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

[sashiko-bot@kernel.org]

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

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

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

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

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515091942.449-1-hehuan1@eswincomputing.com?part=2