Re: [PATCH RFC] drm/radeon/ci add comment to document intentionally unreachable code

[Alex Deucher]

On Tue, Jul 19, 2016 at 3:54 PM, Nicholas Mc Guire <hofrat@xxxxxxxxx> wrote:
>  commit d967be9b80a5 ("drm/radeon/ci: disable needless sclk changes")
>  introduces an unreachable if(C != C) conditional code section
>  flagged by coccinelle script bad_conditional.cocci:
>
>  Add a comment to make it clear that this is intentional.
>
>  Fixes: d967be9b80a5 ("drm/radeon/ci: disable needless sclk changes")
>
> Signed-off-by: Nicholas Mc Guire <hofrat@xxxxxxxxx>

Applied.  thanks.

Alex

> ---
>         ./drivers/gpu/drm/radeon/ci_dpm.c:3847 bad/useless conditional
> <snip>
> 3845     } else {
> 3846          /* XXX check display min clock requirements */
> 3847          if (CISLAND_MINIMUM_ENGINE_CLOCK !=
> CISLAND_MINIMUM_ENGINE_CLOCK)
> 3848               pi->need_update_smu7_dpm_table |= DPMTABLE_UPDATE_SCLK;
> 3849     }
> <snip>
>
>  which is unreachable given the condition - as this seems to be an
>  intentional disabling of that code section as stated in the commit
>  message of d967be9b80a5 ("drm/radeon/ci: disable needless sclk changes")
>  that introduced that change it should be at least commented
>
>   in drivers/gpu/drm/amd/amdgpu/ci_dpm.c at line 3988
> 3986    } else {
> 3987        /* XXX check display min clock requirements */
> 3988        if (CISLAND_MINIMUM_ENGINE_CLOCK !=
> CISLAND_MINIMUM_ENGINE_CLOCK)
> 3989             pi->need_update_smu7_dpm_table |= DPMTABLE_UPDATE_SCLK;
> 3990    }
>
>  The below patch adds a possibly suitable comment (taken directly from
>  commit d967be9b80a5 ("drm/radeon/ci: disable needless sclk changes")
>  But IÂm not sure if such constructs really should be in the upstream
>  kernel at all....
>
> Patch was compile tested with x86_64_defconfig + CONFIG_DRM_RADEON=m
>
> Patch is against 4.7.0-rc7 (localversion-next is -next-20160719)
>
>  drivers/gpu/drm/radeon/ci_dpm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c
> index 35e0fc3..7ba4508 100644
> --- a/drivers/gpu/drm/radeon/ci_dpm.c
> +++ b/drivers/gpu/drm/radeon/ci_dpm.c
> @@ -3843,7 +3843,10 @@ static void ci_find_dpm_states_clocks_in_dpm_table(struct radeon_device *rdev,
>         if (i >= sclk_table->count) {
>                 pi->need_update_smu7_dpm_table |= DPMTABLE_OD_UPDATE_SCLK;
>         } else {
> -               /* XXX check display min clock requirements */
> +               /* XXX The current code always reprogrammed the sclk levels,
> +                * but we don't currently handle disp sclk requirements
> +                * so just skip it.
> +                */
>                 if (CISLAND_MINIMUM_ENGINE_CLOCK != CISLAND_MINIMUM_ENGINE_CLOCK)
>                         pi->need_update_smu7_dpm_table |= DPMTABLE_UPDATE_SCLK;
>         }
> --
> 2.1.4
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>