[PATCH] drm/rcar-du: fix comment to rcar_du_group_get()

[PATCH] drm/rcar-du: fix comment to rcar_du_group_get()

[Alexandra Diupina]

rcar_du_group_get() never returns a negative
error code (always returns 0), so change
the comment about returned value

Fixes: cb2025d2509f ("drm/rcar-du: Introduce CRTCs groups")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
 drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
index 2ccd2581f544..499d4e56c32d 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
@@ -200,7 +200,7 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
  *
  * This function must be called with the DRM mode_config lock held.
  *
- * Return 0 in case of success or a negative error code otherwise.
+ * Always return 0.
  */
 int rcar_du_group_get(struct rcar_du_group *rgrp)
 {
-- 
2.30.2

Re: [PATCH] drm/rcar-du: fix comment to rcar_du_group_get()

[Kieran Bingham]

Hi Alexandra

Quoting Alexandra Diupina (2023-09-03 14:37:09)
> rcar_du_group_get() never returns a negative
> error code (always returns 0), so change
> the comment about returned value

If so, then perhaps this may as well become a void return and remove the
return 0.

That could then clean up some redundant error path handling in
drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c too ?

Still, this does correct the documentation to match the implementation
as it stands so... for that ...

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

But removing an unused error path seems like a worthy clean up
opportunity too.

> 
> Fixes: cb2025d2509f ("drm/rcar-du: Introduce CRTCs groups")

Hrm ... well the documented behaviour was the same even before this
commit in rcar_du_get(), so perhaps it was documenting the intent... But
it does seem that the return code has been redundant for quite some time
so perhaps it's just not required.


--
Kieran


> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> ---
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> index 2ccd2581f544..499d4e56c32d 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> @@ -200,7 +200,7 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>   *
>   * This function must be called with the DRM mode_config lock held.
>   *
> - * Return 0 in case of success or a negative error code otherwise.
> + * Always return 0.
>   */
>  int rcar_du_group_get(struct rcar_du_group *rgrp)
>  {
> -- 
> 2.30.2
>

Re: [PATCH] drm/rcar-du: fix comment to rcar_du_group_get()

[Geert Uytterhoeven]

Hi Alexandra,

On Sun, Sep 3, 2023 at 7:10 PM Alexandra Diupina <adiupina@astralinux.ru> wrote:
> rcar_du_group_get() never returns a negative
> error code (always returns 0), so change
> the comment about returned value
>
> Fixes: cb2025d2509f ("drm/rcar-du: Introduce CRTCs groups")
> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>

Thanks for your patch!

> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> @@ -200,7 +200,7 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>   *
>   * This function must be called with the DRM mode_config lock held.
>   *
> - * Return 0 in case of success or a negative error code otherwise.
> + * Always return 0.
>   */
>  int rcar_du_group_get(struct rcar_du_group *rgrp)
>  {

This is debatable: future changes may make it possible for the
function to fail.  In addition, the (single) caller does check the
return value.

If we are sure the function can never fail, and everyone agrees, its
return type should be changed to void, and the caller should be updated.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/rcar-du: fix comment to rcar_du_group_get()

[Kieran Bingham]

Quoting Kieran Bingham (2023-09-03 16:23:46)
> Hi Alexandra
> 
> Quoting Alexandra Diupina (2023-09-03 14:37:09)
> > rcar_du_group_get() never returns a negative
> > error code (always returns 0), so change
> > the comment about returned value
> 
> If so, then perhaps this may as well become a void return and remove the
> return 0.
> 
> That could then clean up some redundant error path handling in
> drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c too ?
> 
> Still, this does correct the documentation to match the implementation
> as it stands so... for that ...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reading Geert's reply here, I'll retract this RB tag, and instead agree
that the emphasis should either be on a full clean up - or that this
does document the intent that if there is an error it will be returned.

--
Kieran


> 
> But removing an unused error path seems like a worthy clean up
> opportunity too.
> 
> > 
> > Fixes: cb2025d2509f ("drm/rcar-du: Introduce CRTCs groups")
> 
> Hrm ... well the documented behaviour was the same even before this
> commit in rcar_du_get(), so perhaps it was documenting the intent... But
> it does seem that the return code has been redundant for quite some time
> so perhaps it's just not required.
> 
> 
> --
> Kieran
> 
> 
> > Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> > ---
> >  drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > index 2ccd2581f544..499d4e56c32d 100644
> > --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> > @@ -200,7 +200,7 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
> >   *
> >   * This function must be called with the DRM mode_config lock held.
> >   *
> > - * Return 0 in case of success or a negative error code otherwise.
> > + * Always return 0.
> >   */
> >  int rcar_du_group_get(struct rcar_du_group *rgrp)
> >  {
> > -- 
> > 2.30.2
> >

[PATCH v2] drm: rcar-du: turn rcar_du_group_get() into void and remove its return value check

[Alexandra Diupina]

rcar_du_group_get() never returns a negative
error code (always returns 0), so change the comment
about returned value, turn function into void (return
code of rcar_du_group_get has been redundant for a
long time, so perhaps it's just not required) and
remove redundant error path handling in rcar_du_crtc_get()

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 0bb63534fdf3 ("drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get()")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
v2: rcar_du_group_get() is turned into void and its return 
value check is removed in rcar_du_crtc_get()
 drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c  | 6 +-----
 drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 5 +----
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
index 7e175dbfd892..2be7c6e64d72 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
@@ -565,17 +565,13 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
 	if (ret < 0)
 		goto error_clock;
 
-	ret = rcar_du_group_get(rcrtc->group);
-	if (ret < 0)
-		goto error_group;
+	rcar_du_group_get(rcrtc->group);
 
 	rcar_du_crtc_setup(rcrtc);
 	rcrtc->initialized = true;
 
 	return 0;
 
-error_group:
-	clk_disable_unprepare(rcrtc->extclock);
 error_clock:
 	clk_disable_unprepare(rcrtc->clock);
 	return ret;
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
index 2ccd2581f544..7113025dabff 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
@@ -199,10 +199,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
  * before accessing any hardware registers.
  *
  * This function must be called with the DRM mode_config lock held.
- *
- * Return 0 in case of success or a negative error code otherwise.
  */
-int rcar_du_group_get(struct rcar_du_group *rgrp)
+void rcar_du_group_get(struct rcar_du_group *rgrp)
 {
 	if (rgrp->use_count)
 		goto done;
@@ -211,7 +209,6 @@ int rcar_du_group_get(struct rcar_du_group *rgrp)
 
 done:
 	rgrp->use_count++;
-	return 0;
 }
 
 /*
-- 
2.30.2

Re: [PATCH v2] drm: rcar-du: turn rcar_du_group_get() into void and remove its return value check

[kernel test robot]

Hi Alexandra,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.6-rc3 next-20230929]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexandra-Diupina/drm-rcar-du-turn-rcar_du_group_get-into-void-and-remove-its-return-value-check/20230927-184651
base:   linus/master
patch link:    https://lore.kernel.org/r/20230927104438.30628-1-adiupina%40astralinux.ru
patch subject: [PATCH v2] drm: rcar-du: turn rcar_du_group_get() into void and remove its return value check
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20230930/202309301402.uR7IRQVj-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230930/202309301402.uR7IRQVj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309301402.uR7IRQVj-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c:203:6: error: conflicting types for 'rcar_du_group_get'; have 'void(struct rcar_du_group *)'
     203 | void rcar_du_group_get(struct rcar_du_group *rgrp)
         |      ^~~~~~~~~~~~~~~~~
   In file included from drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h:20,
                    from drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c:29:
   drivers/gpu/drm/renesas/rcar-du/rcar_du_group.h:57:5: note: previous declaration of 'rcar_du_group_get' with type 'int(struct rcar_du_group *)'
      57 | int rcar_du_group_get(struct rcar_du_group *rgrp);
         |     ^~~~~~~~~~~~~~~~~


vim +203 drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c

   194	
   195	/*
   196	 * rcar_du_group_get - Acquire a reference to the DU channels group
   197	 *
   198	 * Acquiring the first reference setups core registers. A reference must be held
   199	 * before accessing any hardware registers.
   200	 *
   201	 * This function must be called with the DRM mode_config lock held.
   202	 */
 > 203	void rcar_du_group_get(struct rcar_du_group *rgrp)
   204	{
   205		if (rgrp->use_count)
   206			goto done;
   207	
   208		rcar_du_group_setup(rgrp);
   209	
   210	done:
   211		rgrp->use_count++;
   212	}
   213	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] drm: rcar-du: turn rcar_du_group_get() into void and remove its return value check

[Laurent Pinchart]

Hi Alexandra,

On Wed, Sep 27, 2023 at 01:44:38PM +0300, Alexandra Diupina wrote:
> rcar_du_group_get() never returns a negative
> error code (always returns 0), so change the comment
> about returned value, turn function into void (return
> code of rcar_du_group_get has been redundant for a
> long time, so perhaps it's just not required) and
> remove redundant error path handling in rcar_du_crtc_get()
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Using tools to find issues is fine in principle, but not even
compile-testing the resulting patch before submitting it is not.

> Fixes: 0bb63534fdf3 ("drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get()")
> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> ---
> v2: rcar_du_group_get() is turned into void and its return 
> value check is removed in rcar_du_crtc_get()
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c  | 6 +-----
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 5 +----
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
> index 7e175dbfd892..2be7c6e64d72 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
> @@ -565,17 +565,13 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
>  	if (ret < 0)
>  		goto error_clock;
>  
> -	ret = rcar_du_group_get(rcrtc->group);
> -	if (ret < 0)
> -		goto error_group;
> +	rcar_du_group_get(rcrtc->group);
>  
>  	rcar_du_crtc_setup(rcrtc);
>  	rcrtc->initialized = true;
>  
>  	return 0;
>  
> -error_group:
> -	clk_disable_unprepare(rcrtc->extclock);
>  error_clock:
>  	clk_disable_unprepare(rcrtc->clock);
>  	return ret;
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> index 2ccd2581f544..7113025dabff 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
> @@ -199,10 +199,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>   * before accessing any hardware registers.
>   *
>   * This function must be called with the DRM mode_config lock held.
> - *
> - * Return 0 in case of success or a negative error code otherwise.
>   */
> -int rcar_du_group_get(struct rcar_du_group *rgrp)
> +void rcar_du_group_get(struct rcar_du_group *rgrp)
>  {
>  	if (rgrp->use_count)
>  		goto done;
> @@ -211,7 +209,6 @@ int rcar_du_group_get(struct rcar_du_group *rgrp)
>  
>  done:
>  	rgrp->use_count++;
> -	return 0;
>  }
>  
>  /*

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] drm: rcar-du: turn rcar_du_group_get() into void and remove its return value check

[Александра Дюпина]

Hi Laurent,

I'm sorry, my mistake. Thank you for your comment, I will be more 
careful in the future

30.09.2023 17:50, Laurent Pinchart пишет:
> Using tools to find issues is fine in principle, but not even
> compile-testing the resulting patch before submitting it is not.



Alexandra

[PATCH v3] drm: rcar-du: turn rcar_du_group_get() into void and remove its return value check

[Alexandra Diupina]

rcar_du_group_get() never returns a negative
error code (always returns 0), so change the comment
about returned value, turn function into void (return
code of rcar_du_group_get has been redundant for a
long time, so perhaps it's just not required) and
remove redundant error path handling in rcar_du_crtc_get()

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 0bb63534fdf3 ("drm: rcar-du: Perform the initial CRTC setup from rcar_du_crtc_get()")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
v3: fix build errors
v2: rcar_du_group_get() is turned into void and its return 
value check is removed in rcar_du_crtc_get()
 drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c  | 6 +-----
 drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c | 5 +----
 drivers/gpu/drm/renesas/rcar-du/rcar_du_group.h | 2 +-
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
index 7e175dbfd892..2be7c6e64d72 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
@@ -565,17 +565,13 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
 	if (ret < 0)
 		goto error_clock;
 
-	ret = rcar_du_group_get(rcrtc->group);
-	if (ret < 0)
-		goto error_group;
+	rcar_du_group_get(rcrtc->group);
 
 	rcar_du_crtc_setup(rcrtc);
 	rcrtc->initialized = true;
 
 	return 0;
 
-error_group:
-	clk_disable_unprepare(rcrtc->extclock);
 error_clock:
 	clk_disable_unprepare(rcrtc->clock);
 	return ret;
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
index 2ccd2581f544..7113025dabff 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.c
@@ -199,10 +199,8 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
  * before accessing any hardware registers.
  *
  * This function must be called with the DRM mode_config lock held.
- *
- * Return 0 in case of success or a negative error code otherwise.
  */
-int rcar_du_group_get(struct rcar_du_group *rgrp)
+void rcar_du_group_get(struct rcar_du_group *rgrp)
 {
 	if (rgrp->use_count)
 		goto done;
@@ -211,7 +209,6 @@ int rcar_du_group_get(struct rcar_du_group *rgrp)
 
 done:
 	rgrp->use_count++;
-	return 0;
 }
 
 /*
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.h
index 55649ad86a10..5330dc4ce64a 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_group.h
@@ -54,7 +54,7 @@ struct rcar_du_group {
 u32 rcar_du_group_read(struct rcar_du_group *rgrp, u32 reg);
 void rcar_du_group_write(struct rcar_du_group *rgrp, u32 reg, u32 data);
 
-int rcar_du_group_get(struct rcar_du_group *rgrp);
+void rcar_du_group_get(struct rcar_du_group *rgrp);
 void rcar_du_group_put(struct rcar_du_group *rgrp);
 void rcar_du_group_start_stop(struct rcar_du_group *rgrp, bool start);
 void rcar_du_group_restart(struct rcar_du_group *rgrp);
-- 
2.30.2