diff mbox series

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

Message ID 9dbd4d2c-0757-4d5f-aa11-7d9e665e7633@stanley.mountain (mailing list archive)
State New, archived
Headers show
Series [v2] drm/ast: astdp: fix loop timeout check | expand

Commit Message

Dan Carpenter Aug. 12, 2024, 6:42 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>
---
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(-)

Comments

Thomas Zimmermann Aug. 12, 2024, 6:48 a.m. UTC | #1
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@linaro.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)
Dan Carpenter Aug. 12, 2024, 6:52 a.m. UTC | #2
On Mon, Aug 12, 2024 at 09:42:53AM +0300, Dan Carpenter wrote:
> 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>
> ---
> 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.

I mean this version also sleeps on the last failed iteration but at least there
isn't always true if statement to try prevent optimize away the last sleep.
I'm okay with a sleep on the slow path but not with pointless if statements. ;)

regards,
dan carpenter
Dan Carpenter Aug. 12, 2024, 6:54 a.m. UTC | #3
On Mon, Aug 12, 2024 at 08:48:16AM +0200, Thomas Zimmermann wrote:
> 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@linaro.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

I feel like if we really hit this failure path then we won't care about the
tenth msleep().  I can resend if you want, but I'd prefer to just leave it.

regards,
dan carpenter
Thomas Zimmermann Aug. 12, 2024, 7:30 a.m. UTC | #4
Hi

Am 12.08.24 um 08:54 schrieb Dan Carpenter:
> On Mon, Aug 12, 2024 at 08:48:16AM +0200, Thomas Zimmermann wrote:
>> 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@linaro.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
> I feel like if we really hit this failure path then we won't care about the
> tenth msleep().  I can resend if you want, but I'd prefer to just leave it.

Please resend. Even if the link training ultimately fails, the rest of 
DRM keeps running. 100 msec is not so short to shrug it off IMHO.

Best regards
Thomas

>
> regards,
> dan carpenter
>
Dan Carpenter Aug. 12, 2024, 8:24 a.m. UTC | #5
On Mon, Aug 12, 2024 at 09:30:00AM +0200, Thomas Zimmermann wrote:
> > I feel like if we really hit this failure path then we won't care about the
> > tenth msleep().  I can resend if you want, but I'd prefer to just leave it.
> 
> Please resend. Even if the link training ultimately fails, the rest of DRM
> keeps running. 100 msec is not so short to shrug it off IMHO.
> 

Sure.  No problem.

regards,
dan carpenter
diff mbox series

Patch

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);
 	}
-	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)