Re: [PATCH] drm/amd/display: Remove unused-but-set variable hubp from

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The patch creates an inconsistent state by removing a redundant `hubp &&` check at the end of `hubp1_is_flip_pending` but failing to remove the identical redundant check at the beginning of the function, and completely missing `dcn20_hubp.c`.

Pre-existing issues:
- [High] `union large_integer` is incorrectly defined for big-endian architectures, causing 64-bit addresses to be split into the wrong 32-bit registers.
--

commit 48f884fa7d92e532907b613a494b6862ba88449f
Author: Gleb Markov <markov.gi@npc-ksb.ru>

drm/amd/display: Remove unused-but-set variable hubp from
    
The final check of hubp for NULL covers all remaining lines of code, since
the value of hubp does not change until the end of the method.
This check is redundant because hubp1 is already dereferenced within the
macro.
If it were NULL, the program would have already failed to proceed.
Remove the left part of the expression with the logical "&&".
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: be1fb44389ca ("drm/amd/display: Check null pointers before used").

> diff --git a/drivers/gpu/drm/amd/display/dc/hubp/dcn10/dcn10_hubp.c b/drivers/gpu/drm/amd/display/dc/hubp/dcn10/dcn10_hubp.c
> index 7c97a774141ff..d8eb5996b5774 100644
> --- a/drivers/gpu/drm/amd/display/dc/hubp/dcn10/dcn10_hubp.c
> +++ b/drivers/gpu/drm/amd/display/dc/hubp/dcn10/dcn10_hubp.c
> @@ -772,8 +772,7 @@ bool hubp1_is_flip_pending(struct hubp *hubp)
>  	if (flip_pending)
>  		return true;
>  
> -	if (hubp &&
> -	    earliest_inuse_address.grph.addr.quad_part != hubp->request_address.grph.addr.quad_part)
> +	if (earliest_inuse_address.grph.addr.quad_part != hubp->request_address.grph.addr.quad_part)

[Severity: Medium]
Is this fix complete? Earlier in hubp1_is_flip_pending() there is another
redundant check that wasn't removed:

	if (hubp && hubp->power_gated)
		return false;

Leaving this check at the top of the function might be the root cause that
implies hubp can be NULL, which could still confuse static analyzers.

Also, it looks like a similar redundant "hubp &&" check exists in
dcn20_hubp.c:hubp2_is_flip_pending(). Should that be addressed in this patch
as well?

[Severity: High]
This is a pre-existing issue, but while looking at this code, does the use of
quad_part here work correctly on big-endian architectures?

The earlier REG_GET calls read the low and high parts into the address:

	REG_GET(DCSURF_SURFACE_EARLIEST_INUSE,
			SURFACE_EARLIEST_INUSE_ADDRESS, &earliest_inuse_address.grph.addr.low_part);
	REG_GET(DCSURF_SURFACE_EARLIEST_INUSE_HIGH,
			SURFACE_EARLIEST_INUSE_ADDRESS_HIGH, &earliest_inuse_address.grph.addr.high_part);

Since large_integer is defined unconditionally in dc_hw_types.h as:

	union large_integer {
		struct {
			uint32_t low_part;
			int32_t high_part;
		};
		...
	};

This maps low_part to offset 0 and high_part to offset 4 regardless of
endianness. When earliest_inuse_address.grph.addr.quad_part is read as a 64-bit
integer on big-endian architectures, wouldn't the high and low 32-bit halves
be swapped?

Could this lead to incorrect physical address comparisons here, or hardware
registers being programmed with swapped address halves?

>  		return true;
>  
>  	return false;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629130907.124-1-markov.gi@npc-ksb.ru?part=1