Re: [PATCH] Revert "venus: pm_helpers: Fix error check in vcodec_domains_get()"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux