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
> core->pmdomains[i] = pd;
> }
>
