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;
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)-2Reported-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)-2Reported-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@lists.freedesktop.org
-
Daniel Vetter -
Hyun Kwon