Re: Re: Re: [PATCH v2 0/2] Add driver support for ESWIN EIC7700 PVT controller

[Huan He]

Hi Guenter,

Thank you very much for taking the time to review this patch and for
providing such detailed feedback.

> 
> # Commit 6f4d5698f334 ("hwmon: Add Eswin EIC7700 PVT sensor driver")
> 1.  eic7700-pvt.c:487: ERROR: Unbalanced clock refcount with Runtime PM
>     > +	pvt->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> 
>     Using `devm_clk_get_enabled()` enables the clock and registers a devm action
>     to disable it on removal. However, the driver also uses Runtime PM to manage
>     the same clock:
> 
>     > +static int __maybe_unused eic7700_pvt_runtime_suspend(struct device *dev)
>     > +{
>     > +	struct pvt_hwmon *pvt = dev_get_drvdata(dev);
>     > +
>     > +	clk_disable_unprepare(pvt->clk);
> 
>     If the device is runtime suspended when `remove()` is called:
>     1. `probe()`: `clk_prepare_enable()` (Ref: 1)
>     2. `runtime_suspend()`: `clk_disable_unprepare()` (Ref: 0)
>     3. `remove()` (via devm): `clk_disable_unprepare()` (Ref: -1)
> 
>     This leads to a refcount underflow and warning.
> 
>     Fix: Use `devm_clk_get()` and manually call `clk_prepare_enable()` in probe.
>     Ensure that the manual enable is balanced correctly, or rely on Runtime PM
>     (and `pm_runtime_get_sync` in probe) to handle the clock, ensuring
>     `pm_runtime_put` balances it. Since `check_pwr` needs the clock before
>     Runtime PM is enabled, you should enable it manually and then possibly
>     disable it before enabling Runtime PM, or keep it enabled and let Runtime PM
>     take over (but ensure `remove` doesn't double disable).
> 
>     A common pattern:
>     ```c
>     pvt->clk = devm_clk_get(dev, NULL);
>     ...
>     clk_prepare_enable(pvt->clk);
>     ...
>     pm_runtime_enable(dev);
>     pm_runtime_get_noresume(dev);
>     ...
>     pm_runtime_put(dev);
>     ```
>     And ensure `remove` (or devm action) disables it ONLY if not suspended?
>     Actually, simpler is:
>     Don't use `devm_clk_get_enabled` if you use `runtime_suspend` to disable it.
>     Use `devm_clk_get`.
>     In probe: `clk_prepare_enable`.
>     Register a `devm_add_action` that calls `clk_disable_unprepare` *only if* the
>     driver is not using Runtime PM to control it? No, that's messy.
> 
>     Better fix:
>     Use `devm_clk_get`.
>     In probe: `clk_prepare_enable`.
>     In remove (devm action?): `clk_disable_unprepare` (but this still has the issue).
> 
>     Correct Fix:
>     Do not use `clk_disable_unprepare` in `runtime_suspend` if you used
>     `devm_clk_get_enabled`.
>     OR
>     Don't use `devm_clk_get_enabled`. Use `devm_clk_get`.
>     Manage the clock entirely via Runtime PM.
>     In probe:
>     `clk_prepare_enable(pvt->clk);` (Temporary for check_pwr)
>     `check_pwr...`
>     `clk_disable_unprepare(pvt->clk);`
>     `pm_runtime_enable(dev);`
>     ...

We will reproduce and further analyze the clock refcount imbalance
scenario.

> 
> 2.  eic7700-pvt.c:154: ERROR: Swallowing signal interruption
>     > +	if (ret && (ret != -ERESTARTSYS))
>     > +		return ret;
> 
>     If `wait_for_completion_interruptible()` returns `-ERESTARTSYS` (which is
>     non-zero), the condition `(ret != -ERESTARTSYS)` is false, so the if-statement
>     is skipped. The function then proceeds to return 0 (success) with potentially
>     uninitialized/stale data.
> 
>     Fix:
>     ```c
>     if (ret)
>         return ret;
>     ```

In v3, we will return all non-zero error codes directly, without
special-casing -ERESTARTSYS.

> 
> 3.  eic7700-pvt.c:368: WARN: Spurious interrupts on shared IRQ line
>     > +	eic7700_pvt_update(pvt->regs + PVT_ENA, PVT_ENA_EN, PVT_ENA_EN);
> 
>     `check_pwr` enables the device (and thus potential interrupts) before
>     `request_irq` is called. If the IRQ line is shared and the device asserts
>     an interrupt immediately, the interrupt will be unhandled (spurious) because
>     no handler is registered yet.
> 
>     Fix: Request the IRQ before enabling the device, or ensure interrupts are masked
>     at the controller level (if possible) before enabling the block. Since `check_pwr`
>     relies on polling and ISR clears the status, moving `request_irq` is tricky.
>     Verify if `PVT_ENA` has a separate interrupt enable bit or if `PVT_INT` has a mask.
>     If not, this is a hardware/driver design risk.
> 

Confirmed with the hardware team, the PVT_ENA register has no independent
interrupt enable, and PVT_INT does not support masking.
Enabling the device before request_irq may generate interrupts, but the
driver disables the PVT module (PVT_ENA_EN = 0) and clears interrupts by
writing PVT_INT_CLR. In practice, no issues have been observed.

> 4.  eic7700-pvt.c:147: NOTE: Infinite wait possibility
>     > +	ret = wait_for_completion_interruptible(&pvt->conversion);
> 
>     If the hardware fails to generate an interrupt (e.g., hangs), this will wait
>     indefinitely (unless a signal is sent). It is generally safer to use
>     `wait_for_completion_timeout` in hardware drivers.
> 

In v3, we will replace wait_for_completion_interruptible() with
wait_for_completion_timeout().

Best regards,
Huan He