diff mbox series

[v3] drm/ast: astdp: fix loop timeout check

Message ID 1ba8da25-2d09-4924-a4ff-c0714bfbb192@stanley.mountain (mailing list archive)
State New, archived
Headers show
Series [v3] drm/ast: astdp: fix loop timeout check | expand

Commit Message

Dan Carpenter Aug. 12, 2024, 8:29 a.m. UTC
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@linaro.org>
---
v3: V2 had the same bug but just without the always true if (i) statement.
    Remove the final sleep.
v2: In V1, 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 | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Thomas Zimmermann Aug. 12, 2024, 10:43 a.m. UTC | #1
Hi

Am 12.08.24 um 10:29 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@linaro.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks a lot for the fix. I'll merge it during the week if no other 
comments come in.

Best regards
Thomas

> ---
> v3: V2 had the same bug but just without the always true if (i) statement.
>      Remove the final sleep.
> v2: In V1, 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 | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 5d07678b502c..ca022c287785 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -146,18 +146,19 @@ 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--) {
> -		u8 vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
> +	for (i = 0; i < 10; i++) {
> +		u8 vgacrdc;
>   
> -		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)
> -			break;
>   		if (i)
>   			msleep(100);
> +
> +		vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
> +		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)
> +			return;
>   	}
> -	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)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
index 5d07678b502c..ca022c287785 100644
--- a/drivers/gpu/drm/ast/ast_dp.c
+++ b/drivers/gpu/drm/ast/ast_dp.c
@@ -146,18 +146,19 @@  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--) {
-		u8 vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
+	for (i = 0; i < 10; i++) {
+		u8 vgacrdc;
 
-		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)
-			break;
 		if (i)
 			msleep(100);
+
+		vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
+		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)
+			return;
 	}
-	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)