drm/ast: astdp: fix pre-op vs post-op bug

[PATCH] drm/ast: astdp: fix pre-op vs post-op bug

Posted by Dan Carpenter 1 year, 6 months ago

The test for "Link training failed" expect the loop to exit with "i"
set to zero but it exits when "i" is set to -1.  Change this from a
post-op to a pre-op so that it exits with "i" set to zero.  This
changes the number of iterations from 10 to 9 but probably that's
okay.

Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/gpu/drm/ast/ast_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
index 5d07678b502c..4329ab680f62 100644
--- a/drivers/gpu/drm/ast/ast_dp.c
+++ b/drivers/gpu/drm/ast/ast_dp.c
@@ -148,7 +148,7 @@ void ast_dp_link_training(struct ast_device *ast)
 	struct drm_device *dev = &ast->base;
 	unsigned int i = 10;
 
-	while (i--) {
+	while (--i) {
 		u8 vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
 
 		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)
-- 
2.43.0

Re: [PATCH] drm/ast: astdp: fix pre-op vs post-op bug

Posted by Thomas Zimmermann 1 year, 6 months ago

Hi,

thanks a lot for the bugfix.

Am 09.08.24 um 14:33 schrieb Dan Carpenter:
> The test for "Link training failed" expect the loop to exit with "i"
> set to zero but it exits when "i" is set to -1.  Change this from a
> post-op to a pre-op so that it exits with "i" set to zero.  This
> changes the number of iterations from 10 to 9 but probably that's
> okay.

Yes, that's ok.

>
> Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>   drivers/gpu/drm/ast/ast_dp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 5d07678b502c..4329ab680f62 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -148,7 +148,7 @@ void ast_dp_link_training(struct ast_device *ast)
>   	struct drm_device *dev = &ast->base;
>   	unsigned int i = 10;
>   
> -	while (i--) {
> +	while (--i) {

If this loop ever starts with i = 0, it would break again. Can we use

while (i) {
   --i;
    ...
}

instead?

Best regards
Thomas

>   		u8 vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
>   
>   		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)

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

Re: [PATCH] drm/ast: astdp: fix pre-op vs post-op bug

Posted by Jani Nikula 1 year, 6 months ago

On Fri, 09 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi,
>
> thanks a lot for the bugfix.
>
> Am 09.08.24 um 14:33 schrieb Dan Carpenter:
>> The test for "Link training failed" expect the loop to exit with "i"
>> set to zero but it exits when "i" is set to -1.  Change this from a
>> post-op to a pre-op so that it exits with "i" set to zero.  This
>> changes the number of iterations from 10 to 9 but probably that's
>> okay.
>
> Yes, that's ok.
>
>>
>> Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable")
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> ---
>>   drivers/gpu/drm/ast/ast_dp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
>> index 5d07678b502c..4329ab680f62 100644
>> --- a/drivers/gpu/drm/ast/ast_dp.c
>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>> @@ -148,7 +148,7 @@ void ast_dp_link_training(struct ast_device *ast)
>>   	struct drm_device *dev = &ast->base;
>>   	unsigned int i = 10;
>>   
>> -	while (i--) {
>> +	while (--i) {
>
> If this loop ever starts with i = 0, it would break again. Can we use
>
> while (i) {
>    --i;
>     ...
> }
>
> instead?

FWIW, I personally *always* use for loops when there isn't a compelling
reason to do otherwise. You know at a glance that

	for (i = 0; i < N; i++)

gets run N times and what i is going to be afterwards.

Sure, you may have to restructure other things, but I think it's almost
always worth it.

BR,
Jani.



>
> Best regards
> Thomas
>
>>   		u8 vgacrdc = ast_get_index_reg(ast, AST_IO_VGACRI, 0xdc);
>>   
>>   		if (vgacrdc & AST_IO_VGACRDC_LINK_SUCCESS)

-- 
Jani Nikula, Intel

Re: [PATCH] drm/ast: astdp: fix pre-op vs post-op bug

Posted by Dan Carpenter 1 year, 6 months ago

On Fri, Aug 09, 2024 at 04:43:51PM +0300, Jani Nikula wrote:
> On Fri, 09 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > Hi,
> >
> > thanks a lot for the bugfix.
> >
> > Am 09.08.24 um 14:33 schrieb Dan Carpenter:
> >> The test for "Link training failed" expect the loop to exit with "i"
> >> set to zero but it exits when "i" is set to -1.  Change this from a
> >> post-op to a pre-op so that it exits with "i" set to zero.  This
> >> changes the number of iterations from 10 to 9 but probably that's
> >> okay.
> >
> > Yes, that's ok.
> >
> >>
> >> Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >> ---
> >>   drivers/gpu/drm/ast/ast_dp.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> >> index 5d07678b502c..4329ab680f62 100644
> >> --- a/drivers/gpu/drm/ast/ast_dp.c
> >> +++ b/drivers/gpu/drm/ast/ast_dp.c
> >> @@ -148,7 +148,7 @@ void ast_dp_link_training(struct ast_device *ast)
> >>   	struct drm_device *dev = &ast->base;
> >>   	unsigned int i = 10;
> >>   
> >> -	while (i--) {
> >> +	while (--i) {
> >
> > If this loop ever starts with i = 0, it would break again. Can we use
> >
> > while (i) {
> >    --i;
> >     ...
> > }
> >
> > instead?
> 
> FWIW, I personally *always* use for loops when there isn't a compelling
> reason to do otherwise. You know at a glance that
> 
> 	for (i = 0; i < N; i++)
> 
> gets run N times and what i is going to be afterwards.
> 
> Sure, you may have to restructure other things, but I think it's almost
> always worth it.

A for statement works here.  I need to resend the patch anyway because
the if (i) msleep() code doesn't make sense now.

regards,
dan carpenter

Re: [PATCH] drm/ast: astdp: fix pre-op vs post-op bug

Posted by Thomas Zimmermann 1 year, 5 months ago

Hi

Am 09.08.24 um 19:06 schrieb Dan Carpenter:
> On Fri, Aug 09, 2024 at 04:43:51PM +0300, Jani Nikula wrote:
>> On Fri, 09 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Hi,
>>>
>>> thanks a lot for the bugfix.
>>>
>>> Am 09.08.24 um 14:33 schrieb Dan Carpenter:
>>>> The test for "Link training failed" expect the loop to exit with "i"
>>>> set to zero but it exits when "i" is set to -1.  Change this from a
>>>> post-op to a pre-op so that it exits with "i" set to zero.  This
>>>> changes the number of iterations from 10 to 9 but probably that's
>>>> okay.
>>> Yes, that's ok.
>>>
>>>> Fixes: 2281475168d2 ("drm/ast: astdp: Perform link training during atomic_enable")
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/ast/ast_dp.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
>>>> index 5d07678b502c..4329ab680f62 100644
>>>> --- a/drivers/gpu/drm/ast/ast_dp.c
>>>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>>>> @@ -148,7 +148,7 @@ void ast_dp_link_training(struct ast_device *ast)
>>>>    	struct drm_device *dev = &ast->base;
>>>>    	unsigned int i = 10;
>>>>    
>>>> -	while (i--) {
>>>> +	while (--i) {
>>> If this loop ever starts with i = 0, it would break again. Can we use
>>>
>>> while (i) {
>>>     --i;
>>>      ...
>>> }
>>>
>>> instead?
>> FWIW, I personally *always* use for loops when there isn't a compelling
>> reason to do otherwise. You know at a glance that
>>
>> 	for (i = 0; i < N; i++)
>>
>> gets run N times and what i is going to be afterwards.
>>
>> Sure, you may have to restructure other things, but I think it's almost
>> always worth it.
> A for statement works here.  I need to resend the patch anyway because
> the if (i) msleep() code doesn't make sense now.

Why? The loop counts downwards and does not wait if the final iteration 
(i == 0) fails.

Personally, I prefer while for counting downwards. But if you do the for 
loop as mentioned, you have to adapt the loop body.

Best regards
Thomas

>
> regards,
> dan carpenter
>

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