Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 57/78] media: exynos4-is: use pm_runtime_resume_and_get()
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     linuxarm@huawei.com, mauro.chehab@huawei.com,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sylwester Nawrocki <s.nawrocki@samsung.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linux-samsung-soc@vger.kernel.org
References: <cover.1619191723.git.mchehab+huawei@kernel.org>
 <091915bb1cbec13b566d129f85ae229fcb92e2e4.1619191723.git.mchehab+huawei@kernel.org>
 <45068e81-8f9b-fea8-b7bc-bdd0443ba7e6@kernel.org>
 <20210426151224.2b677d1b@coco.lan>
From:   Sylwester Nawrocki <snawrocki@kernel.org>
Message-ID: <1a98b4b7-78c2-810d-a4ba-762c1a9576b3@kernel.org>
Date:   Tue, 27 Apr 2021 10:06:31 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <20210426151224.2b677d1b@coco.lan>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 26.04.2021 15:12, Mauro Carvalho Chehab wrote:
> Em Sun, 25 Apr 2021 22:57:25 +0200
> Sylwester Nawrocki <snawrocki@kernel.org> escreveu:
> 
>> On 24.04.2021 08:45, Mauro Carvalho Chehab wrote:
>>> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
>>> added pm_runtime_resume_and_get() in order to automatically handle
>>> dev->power.usage_count decrement on errors.
>>>
>>> Use the new API, in order to cleanup the error check logic.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

>>> diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
>>> index 972d9601d236..bca35866cc74 100644
>>> --- a/drivers/media/platform/exynos4-is/fimc-is.c
>>> +++ b/drivers/media/platform/exynos4-is/fimc-is.c
>>> @@ -828,7 +828,7 @@ static int fimc_is_probe(struct platform_device *pdev)
>>>    			goto err_irq;
>>>    	}
>>>    
>>> -	ret = pm_runtime_get_sync(dev);
>>> +	ret = pm_runtime_resume_and_get(dev);
>>>    	if (ret < 0)
>>>    		goto err_pm;
>>
>> It seems you intended to use err_suspend label here. We don't need
>> a new label though, instead of err_pm we can jump to err_irq when
>> pm_runtime_resume_and_get() fails.
> 
> Thanks! Will fix at the next version.
> 
>> Note that when runtime PM is
>> disabled pm_runtime_resume_and_get() always returns 0.
> 
> Ok, but there are a couple of conditions at rpm_resume() function
> at drivers/base/power/runtime.c (which is the code that actually
> handles those PM macros) that could make it to return errors,
> which are independent on the PM callbacks, like those:
> 
>          if (dev->power.runtime_error)
>                  retval = -EINVAL;
>          else if (dev->power.disable_depth > 0)
>                  retval = -EACCES;
> 
> and more might be added as the PM core changes.

Right, I looked only at !CONFIG_PM case, this is what the "if (!pm_runtime_enabled(dev))"
test and explicit fimc_is_runtime_{resume,suspend} calls were originally for.
Agreed, better not to rely too much on internal implementation as there is
no specific guarantees about return value at the API documentation.

Regards,
Sylwester

>>> @@ -862,6 +862,7 @@ static int fimc_is_probe(struct platform_device *pdev)
>>>    	fimc_is_unregister_subdevs(is);
>>>    err_pm:
>>>    	pm_runtime_put_noidle(dev);
>>> +err_suspend:
>>>    	if (!pm_runtime_enabled(dev))
>>>    		fimc_is_runtime_suspend(dev);
>>>    err_irq:
>>