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(+)
--
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
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 <[email protected]>
---
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 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 <[email protected]>
---
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
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 <[email protected]>
---
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 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 <[email protected]>
---
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 <[email protected]>
---
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 Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <[email protected]> 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 <[email protected]>
>
> 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
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <[email protected]> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
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 Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <[email protected]> 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 <[email protected]>
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 <[email protected]>
> ---
> 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
>
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <[email protected]> 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 <[email protected]>
Hi, this one is also already fixed in:
commit b1a7d0ddb169774c3db5afe9e64124daea7fdd9f
Author: Patrik Jakobsson <[email protected]>
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
>
On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <[email protected]> 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 <[email protected]>
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
>
On Fri, 1 Apr 2022 12:10:48 +0200, Patrik Jakobsson wrote:
> On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <[email protected]> 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 <[email protected]>
>
> 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