Re: [PATCH 1/2] PM: runtime: Add a general runtime get sync operation to deal with usage counter

["Rafael J. Wysocki" <rafael@kernel.org>]

On Mon, Nov 9, 2020 at 2:46 PM zhangqilong <zhangqilong3@huawei.com> wrote:
>
> Hi,
>
> >
> > On Mon, Nov 9, 2020 at 2:24 PM zhangqilong <zhangqilong3@huawei.com>
> > wrote:
> > >
> > > Hi
> > > >
> > > > On Mon, Nov 9, 2020 at 9:05 AM Zhang Qilong
> > > > <zhangqilong3@huawei.com>
> > > > wrote:
> > > > >
> > > > > In many case, we need to check return value of
> > > > > pm_runtime_get_sync, but it brings a trouble to the usage counter
> > > > > processing. Many callers forget to decrease the usage counter when
> > > > > it failed. It has been discussed a lot[0][1]. So we add a function
> > > > > to deal with the usage counter for better coding.
> > > > >
> > > > > [0]https://lkml.org/lkml/2020/6/14/88
> > > > > [1]https://patchwork.ozlabs.org/project/linux-tegra/patch/20200520
> > > > > 0951 48.10995-1-dinghao.liu@zju.edu.cn/
> > > > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> > > > > ---
> > > > >  include/linux/pm_runtime.h | 32
> > ++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/pm_runtime.h
> > > > > b/include/linux/pm_runtime.h index 4b708f4e8eed..2b0af5b1dffd
> > > > > 100644
> > > > > --- a/include/linux/pm_runtime.h
> > > > > +++ b/include/linux/pm_runtime.h
> > > > > @@ -386,6 +386,38 @@ static inline int pm_runtime_get_sync(struct
> > > > > device
> > > > *dev)
> > > > >         return __pm_runtime_resume(dev, RPM_GET_PUT);  }
> > > > >
> > > > > +/**
> > > > > + * gene_pm_runtime_get_sync - Bump up usage counter of a device
> > > > > +and
> > > > resume it.
> > > > > + * @dev: Target device.
> > > >
> > > > The force argument is not documented.
> > >
> > > (1) Good catch, I will add it in next version.
> > >
> > > >
> > > > > + *
> > > > > + * Increase runtime PM usage counter of @dev first, and carry out
> > > > > + runtime-resume
> > > > > + * of it synchronously. If __pm_runtime_resume return negative
> > > > > + value(device is in
> > > > > + * error state) or return positive value(the runtime of device is
> > > > > + already active)
> > > > > + * with force is true, it need decrease the usage counter of the
> > > > > + device when
> > > > > + * return.
> > > > > + *
> > > > > + * The possible return values of this function is zero or negative value.
> > > > > + * zero:
> > > > > + *    - it means success and the status will store the resume operation
> > > > status
> > > > > + *      if needed, the runtime PM usage counter of @dev remains
> > > > incremented.
> > > > > + * negative:
> > > > > + *    - it means failure and the runtime PM usage counter of @dev has
> > > > been
> > > > > + *      decreased.
> > > > > + * positive:
> > > > > + *    - it means the runtime of the device is already active before that.
> > If
> > > > > + *      caller set force to true, we still need to decrease the usage
> > > > counter.
> > > >
> > > > Why is this needed?
> > >
> > > (2) If caller set force, it means caller will return even the device
> > > has already been active (__pm_runtime_resume return positive value)
> > > after calling gene_pm_runtime_get_sync, we still need to decrease the
> > usage count.
> >
> > But who needs this?
> >
> > I don't think that it is a good idea to complicate the API this way.
>
> The callers like:
> ret = pm_runtime_get_sync(dev);
> if (ret) {
>         ...
>         return (xxx);
> }

Which isn't correct really, is it?

If ret is greater than 0, the error should not be returned in the
first place, so you may want the new wrapper to return zero in that
case instead.

> drivers/spi/spi-img-spfi.c:734 img_spfi_resume() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/mfd/arizona-core.c:49 arizona_clk32k_enable() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/usb/dwc3/dwc3-pci.c:212 dwc3_pci_resume_work() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/input/keyboard/omap4-keypad.c:279 omap4_keypad_probe() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/gpu/drm/vc4/vc4_dsi.c:839 vc4_dsi_encoder_enable() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/gpu/drm/i915/selftests/mock_gem_device.c:157 mock_gem_device() warn: 'pm_runtime_get_sync(&pdev->dev)' returns positive and negative
> drivers/watchdog/rti_wdt.c:230 rti_wdt_probe() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/media/platform/exynos4-is/mipi-csis.c:513 s5pcsis_s_stream() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c:89 mtk_vcodec_dec_pw_on() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/media/platform/ti-vpe/cal.c:794 cal_probe() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/media/platform/ti-vpe/vpe.c:2478 vpe_runtime_get() warn: pm_runtime_get_sync() also returns 1 on success
> drivers/media/i2c/smiapp/smiapp-core.c:1529 smiapp_pm_get_init() warn: pm_runtime_get_sync() also returns 1 on success
> ...
> they need it to simplify the function.
>
> If we only want to simplify like
> ret = pm_runtime_get_sync(dev);
> if (ret < 0) {
>         ...
>         Return (xxx)
> }
> The parameter force could be removed.

Which is exactly my point.