lkml.org 
[lkml]   [2021]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 57/78] media: exynos4-is: use pm_runtime_resume_and_get()
From
Date
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:
>>

\
 
 \ /
  Last update: 2021-04-27 10:08    [W:0.093 / U:1.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site