On Sat, Jun 17, 2017 at 9:00 AM, Zhi, Yong <yong.zhi@xxxxxxxxx> wrote: > Hi, Andy, > >> -----Original Message----- >> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx] >> Sent: Friday, June 16, 2017 3:59 PM >> To: Zhi, Yong <yong.zhi@xxxxxxxxx> >> Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; >> sakari.ailus@xxxxxxxxxxxxxxx; Zheng, Jian Xu <jian.xu.zheng@xxxxxxxxx>; >> tfiga@xxxxxxxxxxxx; Mani, Rajmohan <rajmohan.mani@xxxxxxxxx>; >> Toivonen, Tuukka <tuukka.toivonen@xxxxxxxxx> >> Subject: Re: [PATCH v2 12/12] intel-ipu3: imgu top level pci device >> >> On Thu, Jun 15, 2017 at 1:19 AM, Yong Zhi <yong.zhi@xxxxxxxxx> wrote: >> >> Commit message. >> >> > Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx> >> >> > + /* Set Power */ >> > + r = pm_runtime_get_sync(dev); >> > + if (r < 0) { >> > + dev_err(dev, "failed to set imgu power\n"); >> >> > + pm_runtime_put(dev); >> >> I'm not sure it's a right thing to do. >> How did you test runtime PM counters in this case? >> >> > + return r; >> > + } >> > > Actually I have not tested the error case, what the right way to do in your opinion? there is no checking of this function return in lot of the driver code, or simply returning the error code, I also saw examples to call either pm_runtime_put() or pm_runtime_put_noidle() in this case. Instead of speculating, if we inspect pm_runtime_get_sync() [1], we can see that it always causes the runtime PM counter to increment, but it never decrements it, even in case of error. So to keep things balanced, you need to call pm_runtime_put() in error path. It shouldn't matter if it's pm_runtime_put() or pm_runtime_put_noidle(), because of runtime PM semantics, which are explicitly specified [2] that after an error, no hardware state change is attempted until the state is explicitly reset by the driver with either pm_runtime_set_active() or pm_runtime_set_suspended(). So, as far as I didn't miss some even more obscure bits of the runtime PM framework, current code is fine. [1] http://elixir.free-electrons.com/linux/v4.11.6/source/include/linux/pm_runtime.h#L235 and the main part: http://elixir.free-electrons.com/linux/v4.11.6/source/drivers/base/power/runtime.c#L1027 [2] http://elixir.free-electrons.com/linux/v4.11.6/source/Documentation/power/runtime_pm.txt#L128 Best regards, Tomasz
