[PATCH] drm/vc4: hdmi: Fix defined but not used warning

[PATCH] drm/vc4: hdmi: Fix defined but not used warning

[Maxime Ripard]

On platforms without CONFIG_PM, SET_RUNTIME_PM_OPS will be a nop and the
functions vc4_hdmi_runtime_resume and vc4_hdmi_runtime_suspend will not
be used anywhere leading to

warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]

Since we select CONFIG_PM anyway, vc4_hdmi_runtime_suspend() and
vc4_hdmi_runtime_suspend() will always be used and we can thus always
assign them in struct dev_pm_ops without using the SET_RUNTIME_PM_OPS
macro.

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

I'm not sure how to merge this one, since this commit has been reverted
in Linus tree, and un-reverted in linux-next. Should we wait a bit until
the reworked version of the original commit has been merged again?

Maxime
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 500cdd56b335..5cf3a9aae147 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2411,9 +2411,8 @@ static const struct of_device_id vc4_hdmi_dt_match[] = {
 };
 
 static const struct dev_pm_ops vc4_hdmi_pm_ops = {
-	SET_RUNTIME_PM_OPS(vc4_hdmi_runtime_suspend,
-			   vc4_hdmi_runtime_resume,
-			   NULL)
+	.runtime_resume = vc4_hdmi_runtime_resume,
+	.runtime_suspend = vc4_hdmi_runtime_suspend,
 };
 
 struct platform_driver vc4_hdmi_driver = {
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/vc4: hdmi: Fix defined but not used warning

[Nathan Chancellor]

On Thu, Sep 23, 2021 at 05:57:28PM +0200, Maxime Ripard wrote:
> On platforms without CONFIG_PM, SET_RUNTIME_PM_OPS will be a nop and the
> functions vc4_hdmi_runtime_resume and vc4_hdmi_runtime_suspend will not
> be used anywhere leading to
> 
> warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
> 
> Since we select CONFIG_PM anyway, vc4_hdmi_runtime_suspend() and
> vc4_hdmi_runtime_suspend() will always be used and we can thus always
> assign them in struct dev_pm_ops without using the SET_RUNTIME_PM_OPS
> macro.
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Tested-by: Nathan Chancellor <nathan@kernel.org> # build

> ---
> 
> I'm not sure how to merge this one, since this commit has been reverted
> in Linus tree, and un-reverted in linux-next. Should we wait a bit until
> the reworked version of the original commit has been merged again?
> 
> Maxime
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 500cdd56b335..5cf3a9aae147 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2411,9 +2411,8 @@ static const struct of_device_id vc4_hdmi_dt_match[] = {
>  };
>  
>  static const struct dev_pm_ops vc4_hdmi_pm_ops = {
> -	SET_RUNTIME_PM_OPS(vc4_hdmi_runtime_suspend,
> -			   vc4_hdmi_runtime_resume,
> -			   NULL)
> +	.runtime_resume = vc4_hdmi_runtime_resume,
> +	.runtime_suspend = vc4_hdmi_runtime_suspend,
>  };
>  
>  struct platform_driver vc4_hdmi_driver = {
> -- 
> 2.31.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/vc4: hdmi: Fix defined but not used warning

[Linus Torvalds]

On Thu, Sep 23, 2021 at 8:57 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Since we select CONFIG_PM anyway, vc4_hdmi_runtime_suspend() and
> vc4_hdmi_runtime_suspend() will always be used and we can thus always
> assign them in struct dev_pm_ops without using the SET_RUNTIME_PM_OPS
> macro.

This cannot be true.

If CONFIG_PM is always enabled, then the patch is a no-op, and the
warning you quote cannot happen:

   warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]

So this patch is very obviously broken, the message is misleading, and
the claims in your commit message cannot _possibly_ be true.

Maxime, this kind of "respond to bug reports with random contents"
most not continue.

You need to actually look at what the reporter is reporting, and think
about the code. Because the above fix is broken, broken, broken.

The way people fix this is by either making the function definitions
be conditional on their uses - so that the compiler removes them
entirely - or mark them as __maybe_unused. Then a smart _linker_ can
actually remove the code if people use the smarter linker options.

But responding with a patch that claims something that clearly isn't
true is not a valid response.

                   Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/vc4: hdmi: Fix defined but not used warning

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2284 bytes --]

On Thu, Sep 23, 2021 at 09:54:06AM -0700, Linus Torvalds wrote:
> On Thu, Sep 23, 2021 at 8:57 AM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Since we select CONFIG_PM anyway, vc4_hdmi_runtime_suspend() and
> > vc4_hdmi_runtime_suspend() will always be used and we can thus always
> > assign them in struct dev_pm_ops without using the SET_RUNTIME_PM_OPS
> > macro.
> 
> This cannot be true.
> 
> If CONFIG_PM is always enabled, then the patch is a no-op, and the
> warning you quote cannot happen:
> 
>    warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
> 
> So this patch is very obviously broken, the message is misleading, and
> the claims in your commit message cannot _possibly_ be true.

I guess it could have been worded a bit better.

DRM_VC4 allows compilation through COMPILE_TEST and selects PM. Some
platforms don't define PM at all.

In the latter case, SET_RUNTIME_PM_OPS will be a nop, the functions
won't be used, and we'll get this warning.

> Maxime, this kind of "respond to bug reports with random contents"
> most not continue.

I'm not super familiar with how to deal with those kind of situations,
but it does address the warning on those platforms without affecting the
current operations of the driver. I don't see how it qualifies as
random.

> You need to actually look at what the reporter is reporting, and think
> about the code. Because the above fix is broken, broken, broken.

Like I said, this was a genuine attempt at fixing things. It's clear now
that you don't feel the same way and would prefer some other solution.
That's why we have review in the first place I guess? I fail to see what
that kind of personal comments brings to the discussion though.

> The way people fix this is by either making the function definitions
> be conditional on their uses - so that the compiler removes them
> entirely - or mark them as __maybe_unused. Then a smart _linker_ can
> actually remove the code if people use the smarter linker options.

The initial point of selecting CONFIG_PM was to get rid of the #ifdef,
and for all practical purposes the code will always be used when the
driver will run so __maybe_unused didn't look like a proper solution
either.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel