[PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Markus Elfring]

Date: Sat, 18 Mar 2023 16:21:32 +0100

The label “cleanup” was used to jump to another pointer check despite of
the detail in the implementation of the function “dm_validate_stream_and_context”
that it was determined already that corresponding variables contained
still null pointers.

1. Thus return directly if
   * a null pointer was passed for the function parameter “stream”
     or
   * a call of the function “dc_create_plane_state” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Fixes: 5468c36d628524effbb89a9503eb1a2318804759 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index eeaeca8b51f4..3086613f5f5d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6426,19 +6426,19 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 						struct dc_stream_state *stream)
 {
 	enum dc_status dc_result = DC_ERROR_UNEXPECTED;
-	struct dc_plane_state *dc_plane_state = NULL;
-	struct dc_state *dc_state = NULL;
+	struct dc_plane_state *dc_plane_state;
+	struct dc_state *dc_state;

 	if (!stream)
-		goto cleanup;
+		return dc_result;

 	dc_plane_state = dc_create_plane_state(dc);
 	if (!dc_plane_state)
-		goto cleanup;
+		return dc_result;

 	dc_state = dc_create_state(dc);
 	if (!dc_state)
-		goto cleanup;
+		goto release_plane_state;

 	/* populate stream to plane */
 	dc_plane_state->src_rect.height  = stream->src.height;
@@ -6475,13 +6475,9 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 	if (dc_result == DC_OK)
 		dc_result = dc_validate_global_state(dc, dc_state, true);

-cleanup:
-	if (dc_state)
-		dc_release_state(dc_state);
-
-	if (dc_plane_state)
-		dc_plane_state_release(dc_plane_state);
-
+	dc_release_state(dc_state);
+release_plane_state:
+	dc_plane_state_release(dc_plane_state);
 	return dc_result;
 }

--
2.40.0

Re: [PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Hamza Mahfooz]

Hi Markus,

On 3/24/23 11:42, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 16:21:32 +0100
> 
> The label “cleanup” was used to jump to another pointer check despite of
> the detail in the implementation of the function “dm_validate_stream_and_context”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Thus return directly if
>     * a null pointer was passed for the function parameter “stream”
>       or
>     * a call of the function “dc_create_plane_state” failed.
> 
> 2. Use a more appropriate label instead.
> 
> 3. Delete two questionable checks.
> 
> 4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
>     which became unnecessary with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 5468c36d628524effbb89a9503eb1a2318804759 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")

Please truncate the hash to 12 characters.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Also please ensure that your Signed-off-by matches the "From:" entry in 
the email.

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++-----------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index eeaeca8b51f4..3086613f5f5d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6426,19 +6426,19 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>   						struct dc_stream_state *stream)
>   {
>   	enum dc_status dc_result = DC_ERROR_UNEXPECTED;
> -	struct dc_plane_state *dc_plane_state = NULL;
> -	struct dc_state *dc_state = NULL;
> +	struct dc_plane_state *dc_plane_state;
> +	struct dc_state *dc_state;
> 
>   	if (!stream)
> -		goto cleanup;
> +		return dc_result;
> 
>   	dc_plane_state = dc_create_plane_state(dc);
>   	if (!dc_plane_state)
> -		goto cleanup;
> +		return dc_result;
> 
>   	dc_state = dc_create_state(dc);
>   	if (!dc_state)
> -		goto cleanup;
> +		goto release_plane_state;
> 
>   	/* populate stream to plane */
>   	dc_plane_state->src_rect.height  = stream->src.height;
> @@ -6475,13 +6475,9 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>   	if (dc_result == DC_OK)
>   		dc_result = dc_validate_global_state(dc, dc_state, true);
> 
> -cleanup:
> -	if (dc_state)
> -		dc_release_state(dc_state);
> -
> -	if (dc_plane_state)
> -		dc_plane_state_release(dc_plane_state);
> -
> +	dc_release_state(dc_state);
> +release_plane_state:
> +	dc_plane_state_release(dc_plane_state);
>   	return dc_result;
>   }
> 
> --
> 2.40.0
> 

-- 
Hamza

Re: [PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Markus Elfring]

>> The label “cleanup” was used to jump to another pointer check despite of
>> the detail in the implementation of the function “dm_validate_stream_and_context”
>> that it was determined already that corresponding variables contained
>> still null pointers.
>>
>> 1. Thus return directly if
>>    * a null pointer was passed for the function parameter “stream”
>>      or
>>    * a call of the function “dc_create_plane_state” failed.
>>
>> 2. Use a more appropriate label instead.
>>
>> 3. Delete two questionable checks.
>>
>> 4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
>>    which became unnecessary with this refactoring.
>>
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: 5468c36d628524effbb89a9503eb1a2318804759 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")
>
> Please truncate the hash to 12 characters.

May longer identifiers (or even the complete SHA-1 ID) occasionally also
be tolerated for the tag “Fixes”?

How do you think about the proposed change possibilities?

Regards,
Markus

Re: [PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Hamza Mahfooz]

On 3/24/23 14:19, Markus Elfring wrote:
> May longer identifiers (or even the complete SHA-1 ID) occasionally also
> be tolerated for the tag “Fixes”?
> 
> How do you think about the proposed change possibilities?

Well, the hash length is restricted by convention (see
./scripts/checkpatch.pl in the kernel tree), so to propose that change
it would have done more broadly than just for amd-gfx.

> 
> Regards,
> Markus

-- 
Hamza

[PATCH v2] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 9 Jun 2025 08:21:16 +0200

The label “cleanup” was used to jump to another pointer check despite of
the detail in the implementation of the function “dm_validate_stream_and_context”
that it was determined already that corresponding variables contained
still null pointers.

1. Thus return directly if
   * a null pointer was passed for the function parameter “stream”
     or
   * a call of the function “dc_create_plane_state” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Fixes: 5468c36d6285 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

V2:
* The change suggestion was rebased on source files of
  the software “Linux next-20250606”.

* Recipient lists were adjusted accordingly.


 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 78816712afbb..f8aa1c541678 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7473,19 +7473,19 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 						struct dc_stream_state *stream)
 {
 	enum dc_status dc_result = DC_ERROR_UNEXPECTED;
-	struct dc_plane_state *dc_plane_state = NULL;
-	struct dc_state *dc_state = NULL;
+	struct dc_plane_state *dc_plane_state;
+	struct dc_state *dc_state;
 
 	if (!stream)
-		goto cleanup;
+		return dc_result;
 
 	dc_plane_state = dc_create_plane_state(dc);
 	if (!dc_plane_state)
-		goto cleanup;
+		return dc_result;
 
 	dc_state = dc_state_create(dc, NULL);
 	if (!dc_state)
-		goto cleanup;
+		goto release_plane_state;
 
 	/* populate stream to plane */
 	dc_plane_state->src_rect.height  = stream->src.height;
@@ -7522,13 +7522,9 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 	if (dc_result == DC_OK)
 		dc_result = dc_validate_global_state(dc, dc_state, DC_VALIDATE_MODE_ONLY);
 
-cleanup:
-	if (dc_state)
-		dc_state_release(dc_state);
-
-	if (dc_plane_state)
-		dc_plane_state_release(dc_plane_state);
-
+	dc_release_state(dc_state);
+release_plane_state:
+	dc_plane_state_release(dc_plane_state);
 	return dc_result;
 }
 
-- 
2.49.0

Re: [PATCH v2] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[kernel test robot]

Hi Markus,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20250606]
[also build test ERROR on v6.16-rc1]
[cannot apply to drm-exynos/exynos-drm-next linus/master drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm/drm-next drm-misc/drm-misc-next v6.16-rc1 v6.15 v6.15-rc7]
[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/Markus-Elfring/drm-amd-display-Fix-exception-handling-in-dm_validate_stream_and_context/20250609-151039
base:   next-20250606
patch link:    https://lore.kernel.org/r/da489521-7786-4716-8fb8-d79b3c08d93c%40web.de
patch subject: [PATCH v2] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()
config: x86_64-buildonly-randconfig-005-20250609 (https://download.01.org/0day-ci/archive/20250610/202506100312.Ms4XgAzW-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250610/202506100312.Ms4XgAzW-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/202506100312.Ms4XgAzW-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:7525:2: error: call to undeclared function 'dc_release_state'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    7525 |         dc_release_state(dc_state);
         |         ^
   1 error generated.


vim +/dc_release_state +7525 drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c

  7471	
  7472	static enum dc_status dm_validate_stream_and_context(struct dc *dc,
  7473							struct dc_stream_state *stream)
  7474	{
  7475		enum dc_status dc_result = DC_ERROR_UNEXPECTED;
  7476		struct dc_plane_state *dc_plane_state;
  7477		struct dc_state *dc_state;
  7478	
  7479		if (!stream)
  7480			return dc_result;
  7481	
  7482		dc_plane_state = dc_create_plane_state(dc);
  7483		if (!dc_plane_state)
  7484			return dc_result;
  7485	
  7486		dc_state = dc_state_create(dc, NULL);
  7487		if (!dc_state)
  7488			goto release_plane_state;
  7489	
  7490		/* populate stream to plane */
  7491		dc_plane_state->src_rect.height  = stream->src.height;
  7492		dc_plane_state->src_rect.width   = stream->src.width;
  7493		dc_plane_state->dst_rect.height  = stream->src.height;
  7494		dc_plane_state->dst_rect.width   = stream->src.width;
  7495		dc_plane_state->clip_rect.height = stream->src.height;
  7496		dc_plane_state->clip_rect.width  = stream->src.width;
  7497		dc_plane_state->plane_size.surface_pitch = ((stream->src.width + 255) / 256) * 256;
  7498		dc_plane_state->plane_size.surface_size.height = stream->src.height;
  7499		dc_plane_state->plane_size.surface_size.width  = stream->src.width;
  7500		dc_plane_state->plane_size.chroma_size.height  = stream->src.height;
  7501		dc_plane_state->plane_size.chroma_size.width   = stream->src.width;
  7502		dc_plane_state->format = SURFACE_PIXEL_FORMAT_GRPH_ARGB8888;
  7503		dc_plane_state->tiling_info.gfx9.swizzle = DC_SW_UNKNOWN;
  7504		dc_plane_state->rotation = ROTATION_ANGLE_0;
  7505		dc_plane_state->is_tiling_rotated = false;
  7506		dc_plane_state->tiling_info.gfx8.array_mode = DC_ARRAY_LINEAR_GENERAL;
  7507	
  7508		dc_result = dc_validate_stream(dc, stream);
  7509		if (dc_result == DC_OK)
  7510			dc_result = dc_validate_plane(dc, dc_plane_state);
  7511	
  7512		if (dc_result == DC_OK)
  7513			dc_result = dc_state_add_stream(dc, dc_state, stream);
  7514	
  7515		if (dc_result == DC_OK && !dc_state_add_plane(
  7516							dc,
  7517							stream,
  7518							dc_plane_state,
  7519							dc_state))
  7520			dc_result = DC_FAIL_ATTACH_SURFACES;
  7521	
  7522		if (dc_result == DC_OK)
  7523			dc_result = dc_validate_global_state(dc, dc_state, DC_VALIDATE_MODE_ONLY);
  7524	
> 7525		dc_release_state(dc_state);
  7526	release_plane_state:
  7527		dc_plane_state_release(dc_plane_state);
  7528		return dc_result;
  7529	}
  7530	

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

[PATCH v3] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 10 Jun 2025 07:42:40 +0200

The label “cleanup” was used to jump to another pointer check despite of
the detail in the implementation of the function “dm_validate_stream_and_context”
that it was determined already that corresponding variables contained
still null pointers.

1. Thus return directly if
   * a null pointer was passed for the function parameter “stream”
     or
   * a call of the function “dc_create_plane_state” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202506100312.Ms4XgAzW-lkp@intel.com/
Fixes: 5468c36d6285 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

V3:
* Another function call was renamed.

* Recipient lists were adjusted once more.

V2:
* The change suggestion was rebased on source files of
  the software “Linux next-20250606”.

* Recipient lists were adjusted accordingly.


 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 78816712afbb..7dc80b2fbd30 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7473,19 +7473,19 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 						struct dc_stream_state *stream)
 {
 	enum dc_status dc_result = DC_ERROR_UNEXPECTED;
-	struct dc_plane_state *dc_plane_state = NULL;
-	struct dc_state *dc_state = NULL;
+	struct dc_plane_state *dc_plane_state;
+	struct dc_state *dc_state;
 
 	if (!stream)
-		goto cleanup;
+		return dc_result;
 
 	dc_plane_state = dc_create_plane_state(dc);
 	if (!dc_plane_state)
-		goto cleanup;
+		return dc_result;
 
 	dc_state = dc_state_create(dc, NULL);
 	if (!dc_state)
-		goto cleanup;
+		goto release_plane_state;
 
 	/* populate stream to plane */
 	dc_plane_state->src_rect.height  = stream->src.height;
@@ -7522,13 +7522,9 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
 	if (dc_result == DC_OK)
 		dc_result = dc_validate_global_state(dc, dc_state, DC_VALIDATE_MODE_ONLY);
 
-cleanup:
-	if (dc_state)
-		dc_state_release(dc_state);
-
-	if (dc_plane_state)
-		dc_plane_state_release(dc_plane_state);
-
+	dc_state_release(dc_state);
+release_plane_state:
+	dc_plane_state_release(dc_plane_state);
 	return dc_result;
 }
 
-- 
2.49.0

Re: [PATCH v3] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Melissa Wen]

On 06/10, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 10 Jun 2025 07:42:40 +0200
> 
> The label “cleanup” was used to jump to another pointer check despite of
> the detail in the implementation of the function “dm_validate_stream_and_context”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Thus return directly if
>    * a null pointer was passed for the function parameter “stream”
>      or
>    * a call of the function “dc_create_plane_state” failed.
> 
> 2. Use a more appropriate label instead.
> 
> 3. Delete two questionable checks.
> 
> 4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
>    which became unnecessary with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 

Hi Markus,

Thanks for working on this improvement.
Overall, LGTM. Some nits below.

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202506100312.Ms4XgAzW-lkp@intel.com/

As the patch wasn't merged yet, don't add these two kernel-bot-related lines.

You only need to add these lines "If you fix the issue in a separate
patch/commit (i.e. not just a new version of the same patch/commit)"

> Fixes: 5468c36d6285 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> V3:
> * Another function call was renamed.
> 
> * Recipient lists were adjusted once more.
> 
> V2:
> * The change suggestion was rebased on source files of
>   the software “Linux next-20250606”.
> 
> * Recipient lists were adjusted accordingly.
> 
> 
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++-----------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 78816712afbb..7dc80b2fbd30 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7473,19 +7473,19 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>  						struct dc_stream_state *stream)
>  {
>  	enum dc_status dc_result = DC_ERROR_UNEXPECTED;
> -	struct dc_plane_state *dc_plane_state = NULL;
> -	struct dc_state *dc_state = NULL;
> +	struct dc_plane_state *dc_plane_state;
> +	struct dc_state *dc_state;
>  
>  	if (!stream)
> -		goto cleanup;
> +		return dc_result;
>  
>  	dc_plane_state = dc_create_plane_state(dc);
>  	if (!dc_plane_state)
> -		goto cleanup;
> +		return dc_result;
>  
>  	dc_state = dc_state_create(dc, NULL);
>  	if (!dc_state)
> -		goto cleanup;
> +		goto release_plane_state;
>  
>  	/* populate stream to plane */
>  	dc_plane_state->src_rect.height  = stream->src.height;
> @@ -7522,13 +7522,9 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>  	if (dc_result == DC_OK)
>  		dc_result = dc_validate_global_state(dc, dc_state, DC_VALIDATE_MODE_ONLY);
>  
> -cleanup:
> -	if (dc_state)
> -		dc_state_release(dc_state);
> -
> -	if (dc_plane_state)
> -		dc_plane_state_release(dc_plane_state);
> -
> +	dc_state_release(dc_state);

For readability, I would add an extra line here...

> +release_plane_state:
> +	dc_plane_state_release(dc_plane_state);

and here.

With that, you can add my

Reviewed-by: Melissa Wen <mwen@igalia.com>

>  	return dc_result;
>  }
>  
> -- 
> 2.49.0
> 

Re: [PATCH v3] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Dan Carpenter]

On Thu, Jun 12, 2025 at 11:08:10AM -0300, Melissa Wen wrote:
> On 06/10, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Tue, 10 Jun 2025 07:42:40 +0200
> > 
> > The label “cleanup” was used to jump to another pointer check despite of
> > the detail in the implementation of the function “dm_validate_stream_and_context”
> > that it was determined already that corresponding variables contained
> > still null pointers.
> > 
> > 1. Thus return directly if
> >    * a null pointer was passed for the function parameter “stream”
> >      or
> >    * a call of the function “dc_create_plane_state” failed.
> > 
> > 2. Use a more appropriate label instead.
> > 
> > 3. Delete two questionable checks.
> > 
> > 4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
> >    which became unnecessary with this refactoring.
> > 
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> 
> Hi Markus,
> 
> Thanks for working on this improvement.
> Overall, LGTM. Some nits below.
> 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202506100312.Ms4XgAzW-lkp@intel.com/
> 
> As the patch wasn't merged yet, don't add these two kernel-bot-related lines.
> 
> You only need to add these lines "If you fix the issue in a separate
> patch/commit (i.e. not just a new version of the same patch/commit)"
> 

If you're going to fold the fix into the original commit then it
doesn't matter what the commit message says since it will be gone
in the end either way.

regards,
dan carpenter

Re: [PATCH v3] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Alex Hung]

On 6/10/25 00:10, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 10 Jun 2025 07:42:40 +0200
> 
> The label “cleanup” was used to jump to another pointer check despite of
> the detail in the implementation of the function “dm_validate_stream_and_context”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Thus return directly if
>     * a null pointer was passed for the function parameter “stream”
>       or
>     * a call of the function “dc_create_plane_state” failed.
> 
> 2. Use a more appropriate label instead.
> 
> 3. Delete two questionable checks.
> 
> 4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
>     which became unnecessary with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202506100312.Ms4XgAzW-lkp@intel.com/
> Fixes: 5468c36d6285 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> V3:
> * Another function call was renamed.
> 
> * Recipient lists were adjusted once more.
> 
> V2:
> * The change suggestion was rebased on source files of
>    the software “Linux next-20250606”.
> 
> * Recipient lists were adjusted accordingly.
> 
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++-----------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 78816712afbb..7dc80b2fbd30 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7473,19 +7473,19 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>   						struct dc_stream_state *stream)
>   {
>   	enum dc_status dc_result = DC_ERROR_UNEXPECTED;
> -	struct dc_plane_state *dc_plane_state = NULL;
> -	struct dc_state *dc_state = NULL;
> +	struct dc_plane_state *dc_plane_state;
> +	struct dc_state *dc_state;
>   
>   	if (!stream)
> -		goto cleanup;
> +		return dc_result;
>   
>   	dc_plane_state = dc_create_plane_state(dc);
>   	if (!dc_plane_state)
> -		goto cleanup;
> +		return dc_result;

I think the two early returns look fine, but the rest of the changes 
reduces the readability and reusability.

>   
>   	dc_state = dc_state_create(dc, NULL);
>   	if (!dc_state)
> -		goto cleanup;
> +		goto release_plane_state;
>   
>   	/* populate stream to plane */
>   	dc_plane_state->src_rect.height  = stream->src.height;
> @@ -7522,13 +7522,9 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>   	if (dc_result == DC_OK)
>   		dc_result = dc_validate_global_state(dc, dc_state, DC_VALIDATE_MODE_ONLY);
>   
> -cleanup:
> -	if (dc_state)
> -		dc_state_release(dc_state);
> -
> -	if (dc_plane_state)
> -		dc_plane_state_release(dc_plane_state);
> -
> +	dc_state_release(dc_state);
> +release_plane_state:
> +	dc_plane_state_release(dc_plane_state);

This clean was intended to be reused for now and for future changes, and 
the changes here remove the reusability. Also "cleanup" is commonly used 
already.

>   	return dc_result;
>   }
>   

I guess the intention was to reduce goto statements. If that's the case, 
it would be better to eliminate all goto and then to remove cleanup + 
two checks.

On the other hand, I don't see anything wrong with goto/cleanup approach 
either. Multiple exits in a function do not hurt if managed correctly.

Re: [v3] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Markus Elfring]

…
>> 1. Thus return directly if
…
> I guess the intention was to reduce goto statements.
…

Partly, yes.

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.16-rc2#n532

Regards,
Markus