| Message ID | 1596046674-29155-1-git-send-email-hyun.kwon@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] drm: xlnx: zynqmp: Stop the loop at lowest link rate without check | expand |
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
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
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;
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(-)