Message ID | e1e46cfd-8d95-4792-ac8f-1237d2af7171@moroto.mountain (mailing list archive) |
---|---|
State | New |
Delegated to: | Stanimir Varbanov |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1qGLW2-00BRKd-Vi; Mon, 03 Jul 2023 15:31:06 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230460AbjGCPbB (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 3 Jul 2023 11:31:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60042 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230232AbjGCPbA (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 3 Jul 2023 11:31:00 -0400 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 31A46E44 for <linux-media@vger.kernel.org>; Mon, 3 Jul 2023 08:30:59 -0700 (PDT) Received: by mail-wm1-x332.google.com with SMTP id 5b1f17b1804b1-3fbc656873eso53445005e9.1 for <linux-media@vger.kernel.org>; Mon, 03 Jul 2023 08:30:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1688398257; x=1690990257; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :from:to:cc:subject:date:message-id:reply-to; bh=20ISh7YKa+MZ6pI5W28iEYHp3Vg3aQFVV9k/Gjt0jiQ=; b=N/9fGQLgOcVoQOQm9kIQrdbaxoq0bo1c/QmUYFch1hvqBhi0zjNU+QF2RepJAvzShC pYip+LoFyn2OCqwvMWWMI/vUMMLVXfNuDMZXg2a2Z56QN72znrosVvBYAEaGvg+ppWO7 QECsO5M+iqKtSogbTfeqMd8SjUXO0QXPXt/dw2e1ws/AO7PomB/iy0BK74Le5cEE9wWS XvlZuUWNIDn5x6TeiDndkBJdP/ADl93hG4PH7y6hbbZLG1xad1lHaG4H3xpnF1DAWMVs GlcvgWOSVPH75L1gqsBzpGO0Wun3xXie4QFjCVLDEDa52SMk/IAuUCR//PRAXItpAqp8 41CA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688398257; x=1690990257; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=20ISh7YKa+MZ6pI5W28iEYHp3Vg3aQFVV9k/Gjt0jiQ=; b=fQKG02bKSwRZRWdm3EKlSu+Pn7hDz7wOc1cFNOSezNpFCAF0eNLpv920MhacQwreSd pXw8JV3W1Eo6G3cYYJYkadxQu369/xZiWsvZnl8EKc7R/ejlNntS/9YfviTvSv1kl0Dy sdOJP1wk9evzWS6Yg4VTJkTawNG8SqudeKRVPtzifqYu68r+3ClnYM5Dy7u1QBIa9S+p mvSVogxgriY+FtCqhC+LZudf/yKYz7b2h/j9HhKFOjCrJSZsdFSWQDCXe+coHX41Kixi 49SwM7S08S3f4od5CBnJfFNqaLHR+U3M00aGZNWnCGMh4SeFdklf7YnPUVhlrZEQMf9K z6eg== X-Gm-Message-State: AC+VfDzpAV1No8to5jby1TF93bjvSwDlfY17UVzE0ggau3dUAeOJNG+i DNulGRetgy/1RgmV9W6uZ7NRiw== X-Google-Smtp-Source: ACHHUZ67EaqrXv0FMq7JQdc962nRwUPBZgX3EtIYEig5myyMiOjJPMOdorrnuOo+K6vpSSFSahPtng== X-Received: by 2002:a05:600c:b47:b0:3fb:b618:f7b3 with SMTP id k7-20020a05600c0b4700b003fbb618f7b3mr13974657wmr.21.1688398257530; Mon, 03 Jul 2023 08:30:57 -0700 (PDT) Received: from localhost ([102.36.222.112]) by smtp.gmail.com with ESMTPSA id l6-20020adff486000000b00313fd294d6csm17837630wro.7.2023.07.03.08.30.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Jul 2023 08:30:56 -0700 (PDT) Date: Mon, 3 Jul 2023 18:30:52 +0300 From: Dan Carpenter <dan.carpenter@linaro.org> To: Tang Bin <tangbin@cmss.chinamobile.com>, Stanimir Varbanov <stanimir.k.varbanov@gmail.com> Cc: Vikash Garodia <quic_vgarodia@quicinc.com>, Bryan O'Donoghue <bryan.odonoghue@linaro.org>, Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org Subject: [PATCH] Revert "venus: pm_helpers: Fix error check in vcodec_domains_get()" Message-ID: <e1e46cfd-8d95-4792-ac8f-1237d2af7171@moroto.mountain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
Revert "venus: pm_helpers: Fix error check in vcodec_domains_get()"
|
|
Commit Message
Dan Carpenter
July 3, 2023, 3:30 p.m. UTC
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@linaro.org>
---
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(-)
Comments
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@linaro.org> > --- > 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; > } >
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@linaro.org> > > --- > > 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
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@linaro.org> >>> --- >>> 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
On Mon, Jul 10, 2023 at 01:48:52PM +0530, Vikash Garodia wrote: > > 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 ? I have not tested this, I'm reviewing static checker bugs, as I explained here: > >>> 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. > 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. The way I'm writing it is the correct way. I explain this better in my blog. https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ Currently it looks like the Kconfig does not have a dependency on CONFIG_PM. If we fix the the Kconfig, but leave the "harmless" bug here then it will still show up as a static checker warning when COMPILE_TEST is enabled. We should just do it in the correct way even after we fix the Kconfig. regards, dan carpenter
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); core->pmdomains[i] = pd; }