[bug report] venus: pm_helpers: Fix error check in vcodec_domains_get()

[bug report] venus: pm_helpers: Fix error check in vcodec_domains_get()

[Dan Carpenter]

Hello Tang Bin,

The patch 0f6e8d8c94a8: "venus: pm_helpers: Fix error check in
vcodec_domains_get()" from Sep 13, 2022, leads to the following
Smatch static checker warning:

	drivers/media/platform/qcom/venus/pm_helpers.c:873 vcodec_domains_get()
	warn: passing zero to 'PTR_ERR'

drivers/media/platform/qcom/venus/pm_helpers.c
    857 static int vcodec_domains_get(struct venus_core *core)
    858 {
    859         int ret;
    860         struct device **opp_virt_dev;
    861         struct device *dev = core->dev;
    862         const struct venus_resources *res = core->res;
    863         struct device *pd;
    864         unsigned int i;
    865 
    866         if (!res->vcodec_pmdomains_num)
    867                 goto skip_pmdomains;
    868 
    869         for (i = 0; i < res->vcodec_pmdomains_num; i++) {
    870                 pd = dev_pm_domain_attach_by_name(dev,
    871                                                   res->vcodec_pmdomains[i]);
    872                 if (IS_ERR_OR_NULL(pd))
--> 873                         return PTR_ERR(pd) ? : -ENODATA;

This is wrong.  It should be if (IS_ERR(pd)).  Now venus_probe() will
fail if CONFIG_PM is disabled where before it would succeed.

When a function returns both error pointers and NULL then the NULL
should be treated as success.  If it is not possible to succeed when a
feature is disabled then that should be enforced by the Kconfig
dependencies.  Checking for if CONFIG_PM is disabled and erroring out
at runtime is not user friendly.

Please read my blog for more details.
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

Also this error path should release resources instead of returning
directly.

    874                 core->pmdomains[i] = pd;
    875         }
    876 
    877 skip_pmdomains:
    878         if (!core->res->opp_pmdomain)
    879                 return 0;
    880 
    881         /* Attach the power domain for setting performance state */
    882         ret = devm_pm_opp_attach_genpd(dev, res->opp_pmdomain, &opp_virt_dev);
    883         if (ret)
    884                 goto opp_attach_err;
    885 
    886         core->opp_pmdomain = *opp_virt_dev;
    887         core->opp_dl_venus = device_link_add(dev, core->opp_pmdomain,
    888                                              DL_FLAG_RPM_ACTIVE |
    889                                              DL_FLAG_PM_RUNTIME |
    890                                              DL_FLAG_STATELESS);
    891         if (!core->opp_dl_venus) {
    892                 ret = -ENODEV;
    893                 goto opp_attach_err;
    894         }
    895 
    896         return 0;
    897 
    898 opp_attach_err:
    899         for (i = 0; i < res->vcodec_pmdomains_num; i++) {
    900                 if (IS_ERR_OR_NULL(core->pmdomains[i]))

This one is okay because core->pmdomains[i] could be NULL because it
was never initialized.

    901                         continue;
    902                 dev_pm_domain_detach(core->pmdomains[i], true);
    903         }
    904 
    905         return ret;
    906 }

regards,
dan carpenter