Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could lead to invalid reference or set 'is_*' flags mistakely.
To fix this, when an entry is found, add a break to exit the loop.
Xiaomeng Tong (5): gma500: fix a missing break in oaktrail_crtc_mode_set gma500: fix a missing break in cdv_intel_crtc_mode_set gma500: fix a missing break in psb_intel_crtc_mode_set gma500: fix a missing break in cdv_intel_dp_set_m_n gma500: fix a missing break in psb_driver_load
drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++ drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 ++ drivers/gpu/drm/gma500/oaktrail_crtc.c | 2 ++ drivers/gpu/drm/gma500/psb_drv.c | 2 ++ drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++ 5 files changed, 10 insertions(+)
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It will certainly lead to a invalid reference to list itereator variable 'connector' after the loop pointing an bogus address at an offset from the list head, and could lead to multiple 'is_*' flags being set with true mistakely too.
The invalid reference to list itereator is here: drm_object_property_get_value(&connector->base,
To fix this, when found the entry, add a break after the switch statement.
Fixes: a69ac9ea85d87 ("drm/gma500: drm_connector_property -> drm_object_property") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- drivers/gpu/drm/gma500/oaktrail_crtc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c index 36c7c2686c90..eb2d79872bd5 100644 --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c @@ -409,6 +409,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc, is_mipi = true; break; } + + break; }
/* Disable the VGA plane that we never use */
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It will certainly lead to a invalid reference to list itereator variable 'connector' after the loop pointing an bogus address at an offset from the list head, and could lead to multiple 'is_*' flags being set with true mistakely too.
The invalid reference to list itereator is here: drm_object_property_get_value(&connector->base,
To fix this, when found the entry, add a break after the switch statement.
Hi, this is already fixed in: commit b1a7d0ddb169774c3db5afe9e64124daea7fdd9f Author: Patrik Jakobsson patrik.r.jakobsson@gmail.com Date: Tue Mar 22 14:17:38 2022 +0100
drm/gma500: Make use of the drm connector iterator
Fixes: a69ac9ea85d87 ("drm/gma500: drm_connector_property -> drm_object_property") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
drivers/gpu/drm/gma500/oaktrail_crtc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c index 36c7c2686c90..eb2d79872bd5 100644 --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c @@ -409,6 +409,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc, is_mipi = true; break; }
break; } /* Disable the VGA plane that we never use */-- 2.17.1
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could lead to a invalid reference to 'ddi_select' after the loop, and could lead to multiple 'is_*' flags being set with true mistakely, too.
The invalid reference to 'ddi_select' is here: cdv_dpll_set_clock_cdv(dev, crtc, &clock, is_lvds, ddi_select);
To fix this, when found the entry, add a break after the switch statement.
Fixes: d66760962d75 ("gma500: Program the DPLL lane based on the selected digitial port") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c b/drivers/gpu/drm/gma500/cdv_intel_display.c index 94ebc48a4349..3e93019b17cb 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_display.c +++ b/drivers/gpu/drm/gma500/cdv_intel_display.c @@ -616,6 +616,8 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc, DRM_ERROR("invalid output type.\n"); return 0; } + + break; }
if (dev_priv->dplla_96mhz)
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could lead to a invalid reference to 'ddi_select' after the loop, and could lead to multiple 'is_*' flags being set with true mistakely, too.
The invalid reference to 'ddi_select' is here: cdv_dpll_set_clock_cdv(dev, crtc, &clock, is_lvds, ddi_select);
To fix this, when found the entry, add a break after the switch statement.
Fixes: d66760962d75 ("gma500: Program the DPLL lane based on the selected digitial port") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
Hi, this one is also already fixed in:
commit b1a7d0ddb169774c3db5afe9e64124daea7fdd9f Author: Patrik Jakobsson patrik.r.jakobsson@gmail.com Date: Tue Mar 22 14:17:38 2022 +0100
drm/gma500: Make use of the drm connector iterator
drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c b/drivers/gpu/drm/gma500/cdv_intel_display.c index 94ebc48a4349..3e93019b17cb 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_display.c +++ b/drivers/gpu/drm/gma500/cdv_intel_display.c @@ -616,6 +616,8 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc, DRM_ERROR("invalid output type.\n"); return 0; }
break; } if (dev_priv->dplla_96mhz)-- 2.17.1
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could result in multiple 'is_*' flags being set with true mistakely.
To fix this, when found the entry, add a break after the switch statement.
Fixes: 89c78134cc54d (" gma500: Add Poulsbo support") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c index 42d1a733e124..85fc61bf333a 100644 --- a/drivers/gpu/drm/gma500/psb_intel_display.c +++ b/drivers/gpu/drm/gma500/psb_intel_display.c @@ -134,6 +134,8 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc, is_tv = true; break; } + + break; }
refclk = 96000;
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could result in multiple 'is_*' flags being set with true mistakely.
To fix this, when found the entry, add a break after the switch statement.
Fixes: 89c78134cc54d (" gma500: Add Poulsbo support") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
This patch doesn't apply for me and needs to be rebased on top of drm-misc-next or drm-tip.
On Poulsbo there should only be one encoder per crtc so this is only a theoretical issue. But it is good practice to exit the loop early if we can so the patch still makes sense.
Also, please use the correct subject prefix: drm/gma500: instead of just gma500:.
Thanks Patrik
drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c index 42d1a733e124..85fc61bf333a 100644 --- a/drivers/gpu/drm/gma500/psb_intel_display.c +++ b/drivers/gpu/drm/gma500/psb_intel_display.c @@ -134,6 +134,8 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc, is_tv = true; break; }
break; } refclk = 96000;-- 2.17.1
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could result in multiple 'is_*' flags being set with true mistakely.
To fix this, when found the entry, add a break after the switch statement.
Fixes: 89c78134cc54d (" gma500: Add Poulsbo support") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
This patch doesn't apply for me and needs to be rebased on top of drm-misc-next or drm-tip.
On Poulsbo there should only be one encoder per crtc so this is only a theoretical issue. But it is good practice to exit the loop early if we can so the patch still makes sense.
Also, please use the correct subject prefix: drm/gma500: instead of just gma500:.
I will resend another patch as you suggested, thank you.
-- Xiaomeng Tong
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could lead to a invalid reference to 'lane_count/bpp' after the loop.
The invalid reference to 'lane_count/bpp' is here: cdv_intel_dp_compute_m_n(bpp, lane_count,
To fix this, when found the entry, add a break after the switch statement.
Fixes: 8695b61294356 ("gma500: Add the support of display port on CDV") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c index ba6ad1466374..e6473b8da296 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_dp.c +++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c @@ -1016,6 +1016,8 @@ cdv_intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, bpp = dev_priv->edp.bpp; break; } + + break; }
/*
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could lead to a invalid reference to 'lane_count/bpp' after the loop.
The invalid reference to 'lane_count/bpp' is here: cdv_intel_dp_compute_m_n(bpp, lane_count,
To fix this, when found the entry, add a break after the switch statement.
Fixes: 8695b61294356 ("gma500: Add the support of display port on CDV") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
This loop already breaks when the desired conditions are met (see the if statements).
drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c index ba6ad1466374..e6473b8da296 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_dp.c +++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c @@ -1016,6 +1016,8 @@ cdv_intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, bpp = dev_priv->edp.bpp; break; }
break; } /*-- 2.17.1
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. To avoid potential executing 'ret = gma_backlight_init(dev);' repeatly, add a break after the switch statement.
Fixes: 5c49fd3aa0ab0 ("gma500: Add the core DRM files and headers") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- drivers/gpu/drm/gma500/psb_drv.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c index 65cf1c79dd7c..d65a68811bf7 100644 --- a/drivers/gpu/drm/gma500/psb_drv.c +++ b/drivers/gpu/drm/gma500/psb_drv.c @@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags) ret = gma_backlight_init(dev); break; } + + break; }
if (ret)
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. To avoid potential executing 'ret = gma_backlight_init(dev);' repeatly, add a break after the switch statement.
Fixes: 5c49fd3aa0ab0 ("gma500: Add the core DRM files and headers") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
This is incorrect. If we always break on the first iteration we will only run gma_backlight_init() if the first connector is LVDS or MIPI. This might not be the case and gma_backlight_init() will never run. The other loops you have been looking at have an "if (xxx != yyy) continue;" statement at the top which skips all the unwanted entries but this loop does not.
drivers/gpu/drm/gma500/psb_drv.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c index 65cf1c79dd7c..d65a68811bf7 100644 --- a/drivers/gpu/drm/gma500/psb_drv.c +++ b/drivers/gpu/drm/gma500/psb_drv.c @@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags) ret = gma_backlight_init(dev); break; }
break; } if (ret)-- 2.17.1
On Fri, 1 Apr 2022 12:10:48 +0200, Patrik Jakobsson wrote:
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. To avoid potential executing 'ret = gma_backlight_init(dev);' repeatly, add a break after the switch statement.
Fixes: 5c49fd3aa0ab0 ("gma500: Add the core DRM files and headers") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
This is incorrect. If we always break on the first iteration we will only run gma_backlight_init() if the first connector is LVDS or MIPI. This might not be the case and gma_backlight_init() will never run. The other loops you have been looking at have an "if (xxx != yyy) continue;" statement at the top which skips all the unwanted entries but this loop does not.
Yes, your are correct. But it still need to break the loop when found it. So it is better to add if(!ret) break; after the switch statment. I will resend another patch if it is necessary.
drivers/gpu/drm/gma500/psb_drv.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c index 65cf1c79dd7c..d65a68811bf7 100644 --- a/drivers/gpu/drm/gma500/psb_drv.c +++ b/drivers/gpu/drm/gma500/psb_drv.c @@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags) ret = gma_backlight_init(dev); break; }
break; } if (ret)-- 2.17.1
-- Xiaomeng Tong
dri-devel@lists.freedesktop.org
-
Patrik Jakobsson -
Xiaomeng Tong