[PATCH 1/1] drm: xlnx: zynqmp: Stop the loop at lowest link rate without check

[PATCH 1/1] drm: xlnx: zynqmp: Stop the loop at lowest link rate without check

[Hyun Kwon]

The loop should exit at the lowest link rate, so break the loop
at the lowest link rate without check. The check is always true
because lowest link rate is smaller than current one and maximum
of current display. Otherwise, it may be seen as the loop can
potentially result in negative array offset.

The patch d76271d22694: "drm: xlnx: DRM/KMS driver for Xilinx ZynqMP
DisplayPort Subsystem" from Jul 7, 2018, leads to the following
static checker warning:

	drivers/gpu/drm/xlnx/zynqmp_dp.c:594 zynqmp_dp_mode_configure()
	error: iterator underflow 'bws' (-1)-2

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
---
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index b735072..1be2b19 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -579,7 +579,7 @@ static int zynqmp_dp_mode_configure(struct zynqmp_dp *dp, int pclock,
 		return -EINVAL;
 	}
 
-	for (i = ARRAY_SIZE(bws) - 1; i >= 0; i--) {
+	for (i = ARRAY_SIZE(bws) - 1; i > 0; i--) {
 		if (current_bw && bws[i] >= current_bw)
 			continue;
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] drm: xlnx: zynqmp: Stop the loop at lowest link rate without check

[Daniel Vetter]

On Wed, Jul 29, 2020 at 8:21 PM Hyun Kwon <hyun.kwon@xilinx.com> wrote:
>
> The loop should exit at the lowest link rate, so break the loop
> at the lowest link rate without check. The check is always true
> because lowest link rate is smaller than current one and maximum
> of current display. Otherwise, it may be seen as the loop can
> potentially result in negative array offset.
>
> The patch d76271d22694: "drm: xlnx: DRM/KMS driver for Xilinx ZynqMP
> DisplayPort Subsystem" from Jul 7, 2018, leads to the following
> static checker warning:
>
>         drivers/gpu/drm/xlnx/zynqmp_dp.c:594 zynqmp_dp_mode_configure()
>         error: iterator underflow 'bws' (-1)-2
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index b735072..1be2b19 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -579,7 +579,7 @@ static int zynqmp_dp_mode_configure(struct zynqmp_dp *dp, int pclock,
>                 return -EINVAL;
>         }
>
> -       for (i = ARRAY_SIZE(bws) - 1; i >= 0; i--) {
> +       for (i = ARRAY_SIZE(bws) - 1; i > 0; i--) {

But now we don't go through the lowest element anymore, which also
looks wrong. Or I'm blind.

I think the problem is later on that we should bail out of the loop on
the last iteration (when i == 0) before we decrement, since otherwise
we then look at bws[-1] in the next loop, which is clearly wrong. I
guess your code results in the same, but it's very confusing logic for
me ...
-Daniel

>                 if (current_bw && bws[i] >= current_bw)
>                         continue;
>
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/1] drm: xlnx: zynqmp: Stop the loop at lowest link rate without check

[Hyun Kwon]

Hi Daniel,

Thanks for the review.

On Wed, Jul 29, 2020 at 02:34:16PM -0700, Daniel Vetter wrote:
> On Wed, Jul 29, 2020 at 8:21 PM Hyun Kwon <hyun.kwon@xilinx.com> wrote:
> >
> > The loop should exit at the lowest link rate, so break the loop
> > at the lowest link rate without check. The check is always true
> > because lowest link rate is smaller than current one and maximum
> > of current display. Otherwise, it may be seen as the loop can
> > potentially result in negative array offset.
> >
> > The patch d76271d22694: "drm: xlnx: DRM/KMS driver for Xilinx ZynqMP
> > DisplayPort Subsystem" from Jul 7, 2018, leads to the following
> > static checker warning:
> >
> >         drivers/gpu/drm/xlnx/zynqmp_dp.c:594 zynqmp_dp_mode_configure()
> >         error: iterator underflow 'bws' (-1)-2
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
> > ---
> >  drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > index b735072..1be2b19 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> > @@ -579,7 +579,7 @@ static int zynqmp_dp_mode_configure(struct zynqmp_dp *dp, int pclock,
> >                 return -EINVAL;
> >         }
> >
> > -       for (i = ARRAY_SIZE(bws) - 1; i >= 0; i--) {
> > +       for (i = ARRAY_SIZE(bws) - 1; i > 0; i--) {
> 
> But now we don't go through the lowest element anymore, which also
> looks wrong. Or I'm blind.
> 

Currently, the lowest element always breaks without decrement by the check of
the loop.

> I think the problem is later on that we should bail out of the loop on
> the last iteration (when i == 0) before we decrement, since otherwise
> we then look at bws[-1] in the next loop, which is clearly wrong. I
> guess your code results in the same, but it's very confusing logic for
> me ...

Indeed. I can convert the for loop into switch - case in v2. Hope it makes less
confusing. :)

Thanks,
-hyun

> -Daniel
> 
> >                 if (current_bw && bws[i] >= current_bw)
> >                         continue;
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel