On 7/10/2023 1:33 PM, Dan Carpenter wrote:
> On Mon, Jul 10, 2023 at 12:59:22PM +0530, Vikash Garodia wrote:
>> Hi Dan,
>>
>> On 7/3/2023 9:00 PM, Dan Carpenter wrote:
>>> This reverts commit 0f6e8d8c94a82e85e1b9b62a7671990740dc6f70.
>>>
>>> The reverted commit was based on static analysis and a misunderstanding
>>> of how PTR_ERR() and NULLs are supposed to work. When a function
>>> returns both pointer errors and NULL then normally the NULL means
>>> "continue operating without a feature because it was deliberately
>>> turned off". The NULL should not be treated as a failure. If a driver
>>> cannot work when that feature is disabled then the KConfig should
>>> enforce that the function cannot return NULL. We should not need to
>>> test for it.
>>>
>>> In this code, the patch breaks the venus driver if CONFIG_PM is
>>> disabled.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>>> ---
>>> This patch is also based on static analysis and review so probably best
>>> to be cautious. My guess is that very few people disable CONFIG_PM
>>> these days so that's why the bug wasn't caught.
>>>
>>> drivers/media/platform/qcom/venus/pm_helpers.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> index 48c9084bb4db..c93d2906e4c7 100644
>>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>>> @@ -869,8 +869,8 @@ static int vcodec_domains_get(struct venus_core *core)
>>> for (i = 0; i < res->vcodec_pmdomains_num; i++) {
>>> pd = dev_pm_domain_attach_by_name(dev,
>>> res->vcodec_pmdomains[i]);
>>> - if (IS_ERR_OR_NULL(pd))
>>> - return PTR_ERR(pd) ? : -ENODATA;
>>> + if (IS_ERR(pd))
>>> + return PTR_ERR(pd);
>> Trying to understand if pm domains for Venus (pd) is returned NULL here, how
>> would the driver be able to enable and disable those power domains at [1]. And
>> without that, video usecase would be functional ?
>>
>> [1]
>> https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/qcom/venus/pm_helpers.c#L1043
>
> I don't understand. The CONFIG_PM is Power Management. If you
> deliberately disable power management then then, obviously, power
> domains are not going to be enabled. The __pm_runtime_resume() function
> turns into:
>
> include/linux/pm_runtime.h
> 268 static inline int __pm_runtime_resume(struct device *dev, int rpmflags)
> 269 {
> 270 return 1;
> 271 }
>
> In real life, most people are going to want power management.
>
> This is a video accelerator. I would expect that it could still work
> without power management.
It won't, without those 2 power domains enabled for Venus. Does it work for you
when you disabled the config ?
If it really can't then that should be
> enforced in the Kconfig. Having this code here which says "We can't
> load without CONFIG_PM" is wrong, it should be "We can't compile without
> CONFIG_PM".
Its better enforced in Kconfig. By allowing NULL for
dev_pm_domain_attach_by_name does not still indicate that the functionality is
dependent on CONFIG_PM to be enabled.
Thanks,
Vikash
