Re: [PATCH] i2c: imx: Fix PM device usage count

[Uwe Kleine-König]

On Sat, Feb 24, 2018 at 11:43:28PM +0100, Tobias Jordan wrote:
> pm_runtime_get_sync() increases the device's usage count even when
> reporting an error, so add a call to pm_runtime_put_noidle() in the
> related error branches.
> 
> Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the
>   performance")
> Signed-off-by: Tobias Jordan <Tobias.Jordan@xxxxxxxxxxxxxx>
> ---
> In i2c_imx_xfer(), one could also move the "out" label up (in front of
> the call to pm_runtime_put_autosuspend()), but I'm not sure what the
> underlying error scenario is; calling _put_noidle() seems to be the
> safer bet.
> 
> This is one of a number of patches for problems found using coccinelle 
> scripting in the SIL2LinuxMP project. The patch has been compile-tested;
> it's based on linux-next-20180223.
> 
> For a discussion of the corresponding issue, see
> https://marc.info/?l=linux-pm&m=151904483924999&w=2

I don't get the original mail, so reply here. In reply to the question I
would have asked here, too:

> Why isn't ...get_sync() directly calling ...put_noidle() but relies on
> the driver implementation to do it? It seems unintuitive for a _get_
> function to increase the usage count although returning an error.

Rafael replied:

> Because ...get_sync() returns an error when runtime PM is disabled and
> we wanted that case to be transparent for the users of it.
> 
> In the majority of cases (if not always) errors returned by
> ...get_sync()
> mean disabled runtime PM or flaky hardware and the latter is much less
> common (and generally there's not much to do about them in the kernel
> anyway).

If pm_runtime_get_sync() should be transparent for the users if PM is
disable, why not simply return success then? Or introduce a return value
convention like:

	<0 (i.e. -ESOMETHIN) is error
	0 success
	1 PM is disabled

(Taking a quick glimpse there seem to be already some cases where 1 or
-ESOMETHING is returned, but I didn't find documentation explaining the
return values.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |