| Message ID | f7790a38-6b72-44dd-aaeb-550d2de14cf2@stanley.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: astdp: fix pre-op vs post-op bug | expand |
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)
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)
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
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 >
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)
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(-)