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

[Huan He]

Hi Guenter,

Thank you very much for your detailed review and valuable feedback. I
apologize for the delayed response.

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

Regarding the Runtime PM issue potentially causing clock refcount
imbalance, we have investigated it and will address this in the v3 patch.

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

For the spurious interrupt concern: after confirming with the hardware
team, the PVT_ENA register has no independent interrupt enable, and
PVT_INT does not support masking. In the current implementation, enabling
the device during check_pwr may generate an interrupt, but the driver
subsequently disables the PVT module (PVT_ENA_EN = 0) and clears the
interrupt status by writing PVT_INT_CLR. In practice, no issues have been
observed.

Could you please confirm whether it is acceptable to keep the current
implementation under these conditions?
 
Best regards,
Huan He