linux-kernel - Re: [PATCH v2] drm/ast: astdp: fix loop timeout check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c1ad0a1-bbc5-4274-bdf5-fcf2e043a869@suse.de>
Date: Mon, 12 Aug 2024 08:48:16 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Dave Airlie <airlied@...hat.com>, Jocelyn Falempe <jfalempe@...hat.com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, David Airlie <airlied@...il.com>,
 Daniel Vetter <daniel@...ll.ch>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
 Jani Nikula <jani.nikula@...ux.intel.com>
Subject: Re: [PATCH v2] drm/ast: astdp: fix loop timeout check

Hi

Am 12.08.24 um 08:42 schrieb Dan Carpenter:
> This code has an issue because it loops until "i" is set to UINT_MAX but
> the test for failure assumes that "i" is set to zero.  The result is that
> it will only print an error message if we succeed on the very last try.
> Reformat the loop to count forwards instead of backwards.
>
> Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable")
> Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> ---
> v2: In version one, I introduced a bug where it would msleep(100) after failure
>      and that is a pointless thing to do.  Also change the loop to a for loop.
> ---
>   drivers/gpu/drm/ast/ast_dp.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 5d07678b502c..9bc21dd6a54d 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -146,18 +146,16 @@ void ast_dp_power_on_off(struct drm_device *dev, bool on)
>   void ast_dp_link_training(struct ast_device *ast)
>   {
>   	struct drm_device *dev = &ast->base;
> -	unsigned int i = 10;
> +	int i;
>   
> -	while (i--) {
> +	for (i = 0; i < 10; i++) {
>   		u8 vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
>   
>   		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)
> -			break;
> -		if (i)
> -			msleep(100);
> +			return;
> +		msleep(100);

But we don't want to wait during the final iteration of this loop. If 
you want to use the for loop, it should be something like

for (i= 0; i < 10; ++i) {

     if (i)
       msleep(100)

     // now test vgacrdc
}

Best regards
Thomas

>   	}
> -	if (!i)
> -		drm_err(dev, "Link training failed\n");
> +	drm_err(dev, "Link training failed\n");
>   }
>   
>   void ast_dp_set_on_off(struct drm_device *dev, bool on)

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)