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. 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".
regards,
dan carpenter
