RE: [RESEND PATCH] drm/exynos: dp: drop driver owner initialization
Hi Krzysztof,
> -Original Message-
> From: Krzysztof Kozlowski
> Sent: Tuesday, June 4, 2024 10:11 PM
> To: Jingoo Han ; Inki Dae ;
> Seung-Woo Kim ; Kyungmin Park
> ; David Airlie ; Daniel
> Vetter ; Krzysztof Kozlowski ; Alim
> Akhtar ; dri-devel@lists.freedesktop.org; linux-
> arm-ker...@lists.infradead.org; linux-samsung-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Cc: Krzysztof Kozlowski
> Subject: [RESEND PATCH] drm/exynos: dp: drop driver owner initialization
>
> Core in platform_driver_register() already sets the .owner, so driver
> does not need to. Whatever is set here will be anyway overwritten by
> main driver calling platform_driver_register().
>
> Signed-off-by: Krzysztof Kozlowski
Applied.
Thanks,
Inki Dae
> ---
> drivers/gpu/drm/exynos/exynos_dp.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c
> b/drivers/gpu/drm/exynos/exynos_dp.c
> index f48c4343f469..3e6d4c6aa877 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -285,7 +285,6 @@ struct platform_driver dp_driver = {
> .remove_new = exynos_dp_remove,
> .driver = {
> .name = "exynos-dp",
> - .owner = THIS_MODULE,
> .pm = pm_ptr(_dp_pm_ops),
> .of_match_table = exynos_dp_match,
> },
> --
> 2.43.0
>
RE: [PATCH 00/11] drm/exynos: drop driver owner initialization
Hi Krzysztof, > -Original Message- > From: Krzysztof Kozlowski > Sent: Sunday, March 31, 2024 5:33 AM > To: Inki Dae ; Seung-Woo Kim > ; Kyungmin Park ; David > Airlie ; Daniel Vetter ; Krzysztof > Kozlowski ; Alim Akhtar > > Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; > linux-samsung-...@vger.kernel.org; linux-ker...@vger.kernel.org > Subject: [PATCH 00/11] drm/exynos: drop driver owner initialization > > Simplify the code by dropping unnecessary .owner initialization in the > driver. Applied. Thanks. :) Inki Dae > > Best regards, > Krzysztof > > --- > Krzysztof Kozlowski (11): > drm/exynos: fimc: drop driver owner initialization > drm/exynos: fimd: drop driver owner initialization > drm/exynos: dsi: drop driver owner initialization > drm/exynos: g2d: drop driver owner initialization > drm/exynos: gsc: drop driver owner initialization > drm/exynos: mic: drop driver owner initialization > drm/exynos: rotator: drop driver owner initialization > drm/exynos: scaler: drop driver owner initialization > drm/exynos: vidi: drop driver owner initialization > drm/exynos: hdmi: drop driver owner initialization > drm/exynos: mixer: drop driver owner initialization > > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 1 - > drivers/gpu/drm/exynos/exynos_drm_fimc.c| 1 - > drivers/gpu/drm/exynos/exynos_drm_fimd.c| 1 - > drivers/gpu/drm/exynos/exynos_drm_g2d.c | 1 - > drivers/gpu/drm/exynos/exynos_drm_gsc.c | 1 - > drivers/gpu/drm/exynos/exynos_drm_mic.c | 1 - > drivers/gpu/drm/exynos/exynos_drm_rotator.c | 1 - > drivers/gpu/drm/exynos/exynos_drm_scaler.c | 1 - > drivers/gpu/drm/exynos/exynos_drm_vidi.c| 1 - > drivers/gpu/drm/exynos/exynos_hdmi.c| 1 - > drivers/gpu/drm/exynos/exynos_mixer.c | 1 - > 11 files changed, 11 deletions(-) > --- > base-commit: 7fdcff3312e16ba8d1419f8a18f465c5cc235ecf > change-id: 20240330-b4-module-owner-drm-exynos-d2f1b2d48af3 > > Best regards, > -- > Krzysztof Kozlowski >
RE: [PATCH RESEND] drm/exynos: dpi: Change connector type to DPI
Hi Paul,
> -Original Message-
> From: Paul Cercueil
> Sent: Thursday, December 7, 2023 7:19 AM
> To: Inki Dae ; Seung-Woo Kim
> ; Kyungmin Park ;
> Krzysztof Kozlowski ; Alim Akhtar
>
> Cc: David Airlie ; Daniel Vetter ;
> dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org;
> linux-samsung-...@vger.kernel.org; linux-ker...@vger.kernel.org; Paul
> Cercueil
> Subject: [PATCH RESEND] drm/exynos: dpi: Change connector type to DPI
>
> When exynos_drm_dpi.c was written, DRM_MODE_CONNECTOR_DPI did not exist
> yet and I guess that's the reason why DRM_MODE_CONNECTOR_VGA was used as
> the connector type.
>
> However, now it makes more sense to use DRM_MODE_CONNECTOR_DPI as the
> connector type.
Sorry for late. Merged.
Thanks,
Inki Dae
>
> Signed-off-by: Paul Cercueil
> ---
> drivers/gpu/drm/exynos/exynos_drm_dpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> index 378e5381978f..0dc36df6ada3 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
> @@ -101,7 +101,7 @@ static int exynos_dpi_create_connector(struct
> drm_encoder *encoder)
>
> ret = drm_connector_init(encoder->dev, connector,
>_dpi_connector_funcs,
> - DRM_MODE_CONNECTOR_VGA);
> + DRM_MODE_CONNECTOR_DPI);
> if (ret) {
> DRM_DEV_ERROR(ctx->dev,
> "failed to initialize connector with drm\n");
> --
> 2.42.0
>
RE: [PATCH v3 08/16] drm/exynos: Convert to platform remove callback returning void
Hello Uwe Kleine-König, > -Original Message- > From: Uwe Kleine-König > Sent: Wednesday, November 29, 2023 1:55 AM > To: Inki Dae > Cc: linux-samsung-...@vger.kernel.org; Daniel Vetter ; > Jingoo Han ; Seung-Woo Kim ; > Kyungmin Park ; DRI mailing list de...@lists.freedesktop.org>; Krzysztof Kozlowski > ; ker...@pengutronix.de; Alim Akhtar > ; David Airlie ; linux-arm- > ker...@lists.infradead.org > Subject: Re: [PATCH v3 08/16] drm/exynos: Convert to platform remove > callback returning void > > Hello Inki, > > On Wed, Nov 08, 2023 at 08:54:54AM +0100, Uwe Kleine-König wrote: > > Hello Inki, > > > > On Wed, Nov 08, 2023 at 01:16:18PM +0900, Inki Dae wrote: > > > Sorry for late. There was a merge conflict so I fixed it manually and > > > merged. And seems your patch description is duplicated so dropped > > > duplicated one. > > > > Ah. I have a template that generates one patch per driver. I guess this > > is the result of using squash instead of fixup while putting all exynos > > changes into a single patch. > > This patch didn't make it into next yet even though it's included in > your exynos-drm-next branch at > https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git. > > Is this on purpose? drm-exynos tree is not included in the next tree. It was previously included, but it has been removed. drm-exynos tree is merged into the mainline through the drm-next tree, but when the drm-next is synchronized to the next tree, a conflict occurred between the exynos-drm tree and the drm-next tree. Therefore, I had requested that drm-exynos tree be removed from the next. Perhaps I was inexperienced in managing the git tree at that time. :) Thanks, Inki Dae > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König| > Industrial Linux Solutions | > https://protect2.fireeye.com/v1/url?k=421afdc5-23671746-421b768a- > 74fe48600158-d04d6f7cf9d099c0=1=0938fb20-be8e-4a27-af6c- > 9e745ec738c6=https%3A%2F%2Fwww.pengutronix.de%2F |
RE: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect
Hi All , Please review the reply comment. Regards, Ramya SR -Original Message- From: Ramya SR Sent: Thursday, September 28, 2023 7:55 AM To: 'Alex Deucher' ; 'imre.d...@intel.com' Cc: 'Lyude Paul' ; 'Jani Nikula' ; 'Jeff Layton' ; 'linux-ker...@vger.kernel.org' ; 'dri-devel@lists.freedesktop.org' ; 'Wayne Lin' ; 'Alex Deucher' ; Ramya SR (QUIC) Subject: RE: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect Reminder. Please review the reply comment. Thanks and Regards, Ramya SR -Original Message- From: Ramya SR Sent: Tuesday, September 26, 2023 4:12 PM To: Alex Deucher ; imre.d...@intel.com Cc: Lyude Paul ; Jani Nikula ; Jeff Layton ; linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Wayne Lin ; Alex Deucher ; Ramya SR (QUIC) Subject: RE: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect -Original Message- From: Alex Deucher Sent: Monday, September 25, 2023 8:27 PM To: imre.d...@intel.com Cc: Lyude Paul ; Jani Nikula ; Jeff Layton ; linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Wayne Lin ; Alex Deucher ; Ramya SR (QUIC) Subject: Re: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. On Fri, Sep 22, 2023 at 3:22 PM Imre Deak wrote: > > On Fri, Sep 22, 2023 at 03:02:23PM -0400, Lyude Paul wrote: > > > > Oh! wow thank you for catching this: > > > > Reviewed-by: Lyude Paul > > > > I will go and push this to drm-misc-next in just a moment > > > > On Fri, 2023-09-15 at 10:24 +0530, Ramya SR wrote: > > > Modeset mutex unlock is missing in drm_dp_mst_detect_port function. > > > This will lead to deadlock if calling the function multiple times > > > in an atomic operation, for example, getting imultiple MST ports > > > status for a DP MST bonding scenario. > > > > > > Signed-off-by: Ramya SR > > > --- > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > index ed96cfc..d6512c4 100644 > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > @@ -4154,7 +4154,7 @@ drm_dp_mst_detect_port(struct drm_connector > > > *connector, > > > > > > ret = drm_modeset_lock(>base.lock, ctx); > > > if (ret) > > > - goto out; > > > + goto fail; > > > > > > ret = connector_status_disconnected; > > > > > > @@ -4181,6 +4181,8 @@ drm_dp_mst_detect_port(struct drm_connector > > > *connector, > > > break; > > > } > > > out: > > > + drm_modeset_unlock(>base.lock); > > Isn't this supposed to be unlocked only by > drm_helper_probe_detect_ctx() / drm_helper_probe_detect() ? Maybe adding a comment to that effect would be helpful for the future. Alex >this is different lock, above function mentioned is locking/unlocking the >global connection_mutex, that is required always locked during the atomic >>check/commit. Here we are talking about MST topology manager mutex >"mgr->base.lock". >For normal non-bond MST, it's ok, the calling sequence for detecting >MST connector status is dp_mst_connector_detect >->mst->mst_fw_cbs->detect_port_ctx = dp_mst_detect_port >->drm_dp_mst_detect_port (where mgr->base.lock is locked) > Then for the bond MST case, to figure out if the sibling connectors > are also connected, so that the bonding is possible, we need detect > two or more connectors >in single dp_mst_connector_detect call >dp_mst_connector_detect ->mst->mst_fw_cbs->detect_port_ctx = >dp_mst_detect_port ->drm_dp_mst_detect_port (where mgr->base.lock is >locked) dp_mst_find_sibling_connector >->mst->mst_fw_cbs->detect_port_ctx = dp_mst_detect_port >->drm_dp_mst_detect_port (blocked by mgr->base.lock) > > > > +fail: > > > drm_dp_mst_topology_put_port(port); > > > return ret; > > > } > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > >
RE: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect
Reminder. Please review the reply comment. Thanks and Regards, Ramya SR -Original Message- From: Ramya SR Sent: Tuesday, September 26, 2023 4:12 PM To: Alex Deucher ; imre.d...@intel.com Cc: Lyude Paul ; Jani Nikula ; Jeff Layton ; linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Wayne Lin ; Alex Deucher ; Ramya SR (QUIC) Subject: RE: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect -Original Message- From: Alex Deucher Sent: Monday, September 25, 2023 8:27 PM To: imre.d...@intel.com Cc: Lyude Paul ; Jani Nikula ; Jeff Layton ; linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Wayne Lin ; Alex Deucher ; Ramya SR (QUIC) Subject: Re: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. On Fri, Sep 22, 2023 at 3:22 PM Imre Deak wrote: > > On Fri, Sep 22, 2023 at 03:02:23PM -0400, Lyude Paul wrote: > > > > Oh! wow thank you for catching this: > > > > Reviewed-by: Lyude Paul > > > > I will go and push this to drm-misc-next in just a moment > > > > On Fri, 2023-09-15 at 10:24 +0530, Ramya SR wrote: > > > Modeset mutex unlock is missing in drm_dp_mst_detect_port function. > > > This will lead to deadlock if calling the function multiple times > > > in an atomic operation, for example, getting imultiple MST ports > > > status for a DP MST bonding scenario. > > > > > > Signed-off-by: Ramya SR > > > --- > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > index ed96cfc..d6512c4 100644 > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > @@ -4154,7 +4154,7 @@ drm_dp_mst_detect_port(struct drm_connector > > > *connector, > > > > > > ret = drm_modeset_lock(>base.lock, ctx); > > > if (ret) > > > - goto out; > > > + goto fail; > > > > > > ret = connector_status_disconnected; > > > > > > @@ -4181,6 +4181,8 @@ drm_dp_mst_detect_port(struct drm_connector > > > *connector, > > > break; > > > } > > > out: > > > + drm_modeset_unlock(>base.lock); > > Isn't this supposed to be unlocked only by > drm_helper_probe_detect_ctx() / drm_helper_probe_detect() ? Maybe adding a comment to that effect would be helpful for the future. Alex >this is different lock, above function mentioned is locking/unlocking the >global connection_mutex, that is required always locked during the atomic >>check/commit. Here we are talking about MST topology manager mutex >"mgr->base.lock". >For normal non-bond MST, it's ok, the calling sequence for detecting >MST connector status is dp_mst_connector_detect >->mst->mst_fw_cbs->detect_port_ctx = dp_mst_detect_port >->drm_dp_mst_detect_port (where mgr->base.lock is locked) > Then for the bond MST case, to figure out if the sibling connectors > are also connected, so that the bonding is possible, we need detect > two or more connectors >in single dp_mst_connector_detect call >dp_mst_connector_detect ->mst->mst_fw_cbs->detect_port_ctx = >dp_mst_detect_port ->drm_dp_mst_detect_port (where mgr->base.lock is >locked) dp_mst_find_sibling_connector >->mst->mst_fw_cbs->detect_port_ctx = dp_mst_detect_port >->drm_dp_mst_detect_port (blocked by mgr->base.lock) > > > > +fail: > > > drm_dp_mst_topology_put_port(port); > > > return ret; > > > } > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > >
RE: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect
-Original Message- From: Alex Deucher Sent: Monday, September 25, 2023 8:27 PM To: imre.d...@intel.com Cc: Lyude Paul ; Jani Nikula ; Jeff Layton ; linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Wayne Lin ; Alex Deucher ; Ramya SR (QUIC) Subject: Re: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. On Fri, Sep 22, 2023 at 3:22 PM Imre Deak wrote: > > On Fri, Sep 22, 2023 at 03:02:23PM -0400, Lyude Paul wrote: > > > > Oh! wow thank you for catching this: > > > > Reviewed-by: Lyude Paul > > > > I will go and push this to drm-misc-next in just a moment > > > > On Fri, 2023-09-15 at 10:24 +0530, Ramya SR wrote: > > > Modeset mutex unlock is missing in drm_dp_mst_detect_port function. > > > This will lead to deadlock if calling the function multiple times > > > in an atomic operation, for example, getting imultiple MST ports > > > status for a DP MST bonding scenario. > > > > > > Signed-off-by: Ramya SR > > > --- > > > drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > index ed96cfc..d6512c4 100644 > > > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > > > @@ -4154,7 +4154,7 @@ drm_dp_mst_detect_port(struct drm_connector > > > *connector, > > > > > > ret = drm_modeset_lock(>base.lock, ctx); > > > if (ret) > > > - goto out; > > > + goto fail; > > > > > > ret = connector_status_disconnected; > > > > > > @@ -4181,6 +4181,8 @@ drm_dp_mst_detect_port(struct drm_connector > > > *connector, > > > break; > > > } > > > out: > > > + drm_modeset_unlock(>base.lock); > > Isn't this supposed to be unlocked only by > drm_helper_probe_detect_ctx() / drm_helper_probe_detect() ? Maybe adding a comment to that effect would be helpful for the future. Alex >this is different lock, above function mentioned is locking/unlocking the >global connection_mutex, that is required always locked during the atomic >>check/commit. Here we are talking about MST topology manager mutex >"mgr->base.lock". >For normal non-bond MST, it's ok, the calling sequence for detecting MST >connector status is > dp_mst_connector_detect ->mst->mst_fw_cbs->detect_port_ctx = > dp_mst_detect_port ->drm_dp_mst_detect_port (where mgr->base.lock is locked) > Then for the bond MST case, to figure out if the sibling connectors are also > connected, so that the bonding is possible, we need detect two or more > connectors >in single dp_mst_connector_detect call >dp_mst_connector_detect ->mst->mst_fw_cbs->detect_port_ctx = >dp_mst_detect_port ->drm_dp_mst_detect_port (where mgr->base.lock is locked) >dp_mst_find_sibling_connector ->mst->mst_fw_cbs->detect_port_ctx = >dp_mst_detect_port ->drm_dp_mst_detect_port (blocked by mgr->base.lock) > > > > +fail: > > > drm_dp_mst_topology_put_port(port); > > > return ret; > > > } > > > > -- > > Cheers, > > Lyude Paul (she/her) > > Software Engineer at Red Hat > >
RE: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect
Gentle Reminder. Please review the commit. -Original Message- From: Ramya SR (QUIC) Sent: Friday, September 15, 2023 10:25 AM To: David Airlie ; Daniel Vetter ; Lyude Paul ; Wayne Lin ; Jani Nikula ; Imre Deak ; Alex Deucher ; Jeff Layton ; dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org Cc: Ramya SR (QUIC) Subject: [PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect Modeset mutex unlock is missing in drm_dp_mst_detect_port function. This will lead to deadlock if calling the function multiple times in an atomic operation, for example, getting imultiple MST ports status for a DP MST bonding scenario. Signed-off-by: Ramya SR --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index ed96cfc..d6512c4 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -4154,7 +4154,7 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = drm_modeset_lock(>base.lock, ctx); if (ret) - goto out; + goto fail; ret = connector_status_disconnected; @@ -4181,6 +4181,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector, break; } out: + drm_modeset_unlock(>base.lock); +fail: drm_dp_mst_topology_put_port(port); return ret; } -- 2.7.4
RE: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock
> -Original Message-
> From: Michael Tretter
> Sent: Friday, September 8, 2023 6:48 PM
> To: 대인기/Tizen Platform Lab(SR)/삼성전자
> Cc: 'Inki Dae' ; 'Neil Armstrong'
> ; 'Robert Foss' ; 'Jonas
> Karlman' ; dri-devel@lists.freedesktop.org; linux-
> ker...@vger.kernel.org; 'Jernej Skrabec' ;
> 'Laurent Pinchart' ; 'Andrzej Hajda'
> ; 'Marek Szyprowski' ;
> ker...@pengutronix.de; 'Jagan Teki'
> Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference
> clock
>
> On Tue, 05 Sep 2023 18:06:06 +0900, 대인기/Tizen Platform Lab(SR)/삼성전자
> wrote:
> >
> >
> > > -Original Message-
> > > From: dri-devel On Behalf Of
> > > Michael Tretter
> > > Sent: Monday, September 4, 2023 8:15 PM
> > > To: Inki Dae
> > > Cc: Neil Armstrong ; Robert Foss
> > > ; Jonas Karlman ; dri-
> > > de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Jernej
> Skrabec
> > > ; Laurent Pinchart
> > > ; Andrzej Hajda
> > > ; Marek Szyprowski ;
> > > ker...@pengutronix.de; Jagan Teki
> > > Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL
> reference
> > > clock
> > >
> > > On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> > > > 2023년 8월 29일 (화) 오전 12:59, Michael Tretter
>
> > > 님이 작성:
> > > > >
> > > > > The PLL requires a clock between 2 MHz and 30 MHz after the pre-
> > > divider.
> > > > > The reference clock for the PLL may change due to changes to it's
> > > parent
> > > > > clock. Thus, the frequency may be out of range or unsuited for
> > > > > generating the high speed clock for MIPI DSI.
> > > > >
> > > > > Try to keep the pre-devider small, and set the reference clock
> close
> > > to
> > > > > 30 MHz before recalculating the PLL configuration. Use a divider
> with
> > > a
> > > > > power of two for the reference clock as this seems to work best in
> > > > > my tests.
> > > >
> > > > Clock frequency is specific to SoC architecture so we have to handle
> > > > it carefully because samsung-dsim.c is a common module for I.MX and
> > > > Exynos series.
> > > > You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
> > > > pre-divider", and the clock means that fin_pll - PFD input frequency
> -
> > > > which can be calculated with oscillator clock frequency / P value?
> > > > According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.
> > > >
> > > > For example,
> > > > In case of Exyhos, we use 24MHz as oscillator clock frequency, so
> > > > fin_pll frequency, 8MHz = 24MHz / P(3).
> > > >
> > > > Can you tell me the source of the constraint that clocks must be
> > > > between 2MHz and 30MHz?
> > >
> > > The source is the i.MX8M Nano reference manual, Table 13-40. DPHY PLL
> > > Parameters. It documents that the P divider frequency (fin_pll) has a
> > > frequency range of 2 MHz to 30 MHz. According to the same table, the
> input
>
> Small clarification: These are the corrected limits of TRM rev. 1. In TRM
> rev.
> 0 the limits are incorrectly specified as 6 MHz to 12 MHz.
>
> >
> > In case of Exynos, the range is from 6MHz to 12MHz according to
> Exynos4212 reference manual, Table 1-5.
> >
> > > frequency (fin) has a range of 6 MHz to 300 MHz.
> >
> > In case of Exynos, the range is from 6MHz to 200MHz.
> >
> > >
> > > Is the table incorrect?
> >
>
> > It's correct for I.MX but incorrect for Exynos. I think it would mean
> that
> > the valid range depends on SoC architecture. So I'd say that this patch
> is
> > specific to I.MX.
>
> I understand.
>
> > This was one of what I concerted about when trying to move
> > samsung-dsim.c module to bridge directory for common use between I.MX
> and
> > Exynos Platforms, and this will be what we have to solve together - I.MX
> and
> > Exynos engineers. How about using platform specific data -
> > samsung_dsim_driver_data structure?
> >
> > I.e,
> >
> > static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
> > ...
> > .min_fin_pll = 2,
> > .max_fin_pll = 30,
> > ...
> > };
> >
> > static const struct samsung_dsim_driver_data exynos_dsi_driver_data
> = {
> > ...
> > .min_fin_pll = 6,
> >
[PATCH v1] drm/dp/mst: fix missing modeset unlock for MST port detect
Modeset mutex unlock is missing in drm_dp_mst_detect_port function. This will lead to deadlock if calling the function multiple times in an atomic operation, for example, getting imultiple MST ports status for a DP MST bonding scenario. Signed-off-by: Ramya SR --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index ed96cfc..d6512c4 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -4154,7 +4154,7 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = drm_modeset_lock(>base.lock, ctx); if (ret) - goto out; + goto fail; ret = connector_status_disconnected; @@ -4181,6 +4181,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector, break; } out: + drm_modeset_unlock(>base.lock); +fail: drm_dp_mst_topology_put_port(port); return ret; } -- 2.7.4
RE: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference clock
> -Original Message-
> From: dri-devel On Behalf Of
> Michael Tretter
> Sent: Monday, September 4, 2023 8:15 PM
> To: Inki Dae
> Cc: Neil Armstrong ; Robert Foss
> ; Jonas Karlman ; dri-
> de...@lists.freedesktop.org; linux-ker...@vger.kernel.org; Jernej Skrabec
> ; Laurent Pinchart
> ; Andrzej Hajda
> ; Marek Szyprowski ;
> ker...@pengutronix.de; Jagan Teki
> Subject: Re: [PATCH 3/5] drm/bridge: samsung-dsim: update PLL reference
> clock
>
> On Mon, 04 Sep 2023 14:44:41 +0900, Inki Dae wrote:
> > 2023년 8월 29일 (화) 오전 12:59, Michael Tretter
> 님이 작성:
> > >
> > > The PLL requires a clock between 2 MHz and 30 MHz after the pre-
> divider.
> > > The reference clock for the PLL may change due to changes to it's
> parent
> > > clock. Thus, the frequency may be out of range or unsuited for
> > > generating the high speed clock for MIPI DSI.
> > >
> > > Try to keep the pre-devider small, and set the reference clock close
> to
> > > 30 MHz before recalculating the PLL configuration. Use a divider with
> a
> > > power of two for the reference clock as this seems to work best in
> > > my tests.
> >
> > Clock frequency is specific to SoC architecture so we have to handle
> > it carefully because samsung-dsim.c is a common module for I.MX and
> > Exynos series.
> > You mentioned "The PLL requires a clock between 2 MHz and 3MHz after
> > pre-divider", and the clock means that fin_pll - PFD input frequency -
> > which can be calculated with oscillator clock frequency / P value?
> > According to Exynos datasheet, the fin_pll should be 6 ~ 12Mhz.
> >
> > For example,
> > In case of Exyhos, we use 24MHz as oscillator clock frequency, so
> > fin_pll frequency, 8MHz = 24MHz / P(3).
> >
> > Can you tell me the source of the constraint that clocks must be
> > between 2MHz and 30MHz?
>
> The source is the i.MX8M Nano reference manual, Table 13-40. DPHY PLL
> Parameters. It documents that the P divider frequency (fin_pll) has a
> frequency range of 2 MHz to 30 MHz. According to the same table, the input
In case of Exynos, the range is from 6MHz to 12MHz according to Exynos4212
reference manual, Table 1-5.
> frequency (fin) has a range of 6 MHz to 300 MHz.
In case of Exynos, the range is from 6MHz to 200MHz.
>
> Is the table incorrect?
It's correct for I.MX but incorrect for Exynos. I think it would mean that the
valid range depends on SoC architecture. So I'd say that this patch is specific
to I.MX. This was one of what I concerted about when trying to move
samsung-dsim.c module to bridge directory for common use between I.MX and
Exynos Platforms, and this will be what we have to solve together - I.MX and
Exynos engineers. How about using platform specific data -
samsung_dsim_driver_data structure?
I.e,
static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
...
.min_fin_pll = 2,
.max_fin_pll = 30,
...
};
static const struct samsung_dsim_driver_data exynos_dsi_driver_data = {
...
.min_fin_pll = 6,
.max_fin_pll = 12,
...
};
And then,
while (fin > driver_data->max_fin_pll * MHZ)
fin = fin / 2;
>
> I also tried to always set the reference clock to 24 MHz, but depending on
> the
> display clock this isn't always possible.
According to dt binding, if samsung,pll-clock-frequency exists then we have to
use it first. I'm not sure but could we check if the pll_clk_rate is valid or
not depending on the given display clock in advance? If so then maybe we could
update the pll_clk_rate correctly by reading the pll_clk frequency again.
Thanks,
Inki Dae
>
> Michael
>
> >
> > To other I.MX and Exynos engineers, please do not merge this patch
> > until two SoC platforms are tested correctly.
> >
> > Thanks,
> > Inki Dae
> >
> > >
> > > Signed-off-by: Michael Tretter
> > > ---
> > > drivers/gpu/drm/bridge/samsung-dsim.c | 15 +--
> > > 1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index da90c2038042..4de6e4f116db 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -611,10 +611,21 @@ static unsigned long samsung_dsim_set_pll(struct
> samsung_dsim *dsi,
> > > u16 m;
> > > u32 reg;
> > >
> > > - if (dsi->pll_clk)
> > > + if (dsi->pll_clk) {
> > > + /*
> > > +* Ensure that the reference clock is generated with a
> power of
> > > +* two divider from its parent, but close to the PLLs
> upper
> > > +* limit of the valid range of 2 MHz to 30 MHz.
> > > +*/
> > > + fin = clk_get_rate(clk_get_parent(dsi->pll_clk));
> > > + while (fin > 30 * MHZ)
> > > + fin = fin / 2;
> > > + clk_set_rate(dsi->pll_clk, fin);
> > > +
> > > fin = clk_get_rate(dsi->pll_clk);
> > > - else
> > > +
RE: [PATCH] drm/exynos: fix a possible null-pointer dereference due to data race in exynos_drm_crtc_atomic_disable()
> -Original Message-
> From: Tuo Li
> Sent: Friday, June 30, 2023 11:19 AM
> To: inki@samsung.com; sw0312@samsung.com;
> kyungmin.p...@samsung.com; airl...@gmail.com; dan...@ffwll.ch;
> krzysztof.kozlow...@linaro.org; alim.akh...@samsung.com
> Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org;
> linux-samsung-...@vger.kernel.org; linux-ker...@vger.kernel.org;
> baijiaju1...@outlook.com; Tuo Li ; BassCheck
>
> Subject: [PATCH] drm/exynos: fix a possible null-pointer dereference due
> to data race in exynos_drm_crtc_atomic_disable()
>
> The variable crtc->state->event is often protected by the lock
> crtc->dev->event_lock when is accessed. However, it is accessed as a
> condition of an if statement in exynos_drm_crtc_atomic_disable() without
> holding the lock:
>
> if (crtc->state->event && !crtc->state->active)
>
> However, if crtc->state->event is changed to NULL by another thread right
> after the conditions of the if statement is checked to be true, a
> null-pointer dereference can occur in drm_crtc_send_vblank_event():
>
> e->pipe = pipe;
>
> To fix this possible null-pointer dereference caused by data race, the
> spin lock coverage is extended to protect the if statement as well as the
> function call to drm_crtc_send_vblank_event().
>
> Reported-by: BassCheck
> Signed-off-by: Tuo Li
Applied.
Thanks,
Inki Dae
> ---
> drivers/gpu/drm/exynos/exynos_drm_crtc.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 4153f302de7c..d19e796c2061 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -39,13 +39,12 @@ static void exynos_drm_crtc_atomic_disable(struct
> drm_crtc *crtc,
> if (exynos_crtc->ops->atomic_disable)
> exynos_crtc->ops->atomic_disable(exynos_crtc);
>
> + spin_lock_irq(>dev->event_lock);
> if (crtc->state->event && !crtc->state->active) {
> - spin_lock_irq(>dev->event_lock);
> drm_crtc_send_vblank_event(crtc, crtc->state->event);
> - spin_unlock_irq(>dev->event_lock);
> -
> crtc->state->event = NULL;
> }
> + spin_unlock_irq(>dev->event_lock);
> }
>
> static int exynos_crtc_atomic_check(struct drm_crtc *crtc,
> --
> 2.34.1
RE: [PATCH v2 06/11] drm/exynos: Set fbdev FBINFO_VIRTFB flag
Hi, > -Original Message- > From: Thomas Zimmermann > Sent: Thursday, July 6, 2023 9:47 PM > To: javi...@redhat.com; maarten.lankho...@linux.intel.com; > mrip...@kernel.org > Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; > linux-samsung-...@vger.kernel.org; linux-te...@vger.kernel.org; linux- > fb...@vger.kernel.org; Thomas Zimmermann ; Inki Dae > ; Seung-Woo Kim ; Kyungmin > Park ; Krzysztof Kozlowski > ; Alim Akhtar > Subject: [PATCH v2 06/11] drm/exynos: Set fbdev FBINFO_VIRTFB flag > > Mark the framebuffer with FBINFO_VIRTFB. The framebuffer range is > in DMA-able memory and should be accessed with the CPU's regular > memory ops. > > v2: > * drop FBINFO_FLAG_DEFAULT > > Signed-off-by: Thomas Zimmermann > Reviewed-by: Javier Martinez Canillas > Acked-by: Maxime Ripard Acked-by : Inki Dae Thanks, Inki Dae > Cc: Inki Dae > Cc: Seung-Woo Kim > Cc: Kyungmin Park > Cc: Krzysztof Kozlowski > Cc: Alim Akhtar > --- > drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > index 7ca3424b59ce..828318de8529 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c > @@ -79,6 +79,7 @@ static int exynos_drm_fbdev_update(struct drm_fb_helper > *helper, > offset = fbi->var.xoffset * fb->format->cpp[0]; > offset += fbi->var.yoffset * fb->pitches[0]; > > + fbi->flags |= FBINFO_VIRTFB; > fbi->screen_buffer = exynos_gem->kvaddr + offset; > fbi->screen_size = size; > fbi->fix.smem_len = size; > -- > 2.41.0
RE: [PATCH 05/10] drm/exynos: Use fbdev DMA helpers
Hi,
> -Original Message-
> From: Thomas Zimmermann
> Sent: Wednesday, July 5, 2023 12:50 AM
> To: javi...@redhat.com; maarten.lankho...@linux.intel.com;
> mrip...@kernel.org
> Cc: dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org;
> linux-samsung-...@vger.kernel.org; linux-te...@vger.kernel.org; linux-
> fb...@vger.kernel.org; Thomas Zimmermann ; Inki Dae
> ; Seung-Woo Kim ; Kyungmin
> Park ; Krzysztof Kozlowski
> ; Alim Akhtar
> Subject: [PATCH 05/10] drm/exynos: Use fbdev DMA helpers
>
> Use fbdev's DMA helpers for fbdev emulation. They drivers previously
> used the I/O-memory helpers, while allocating DMA-able system memory.
> This could (in theory) result in bus errors from accessing the memory
> range.
>
> This bug has been present since the exynos driver was first added.
Acked-by : Inki Dae
Thanks,
Inki Dae
>
> Signed-off-by: Thomas Zimmermann
> Fixes: 1c248b7d2960 ("DRM: add DRM Driver for Samsung SoC EXYNOS4210.")
> Cc: Inki Dae
> Cc: Seung-Woo Kim
> Cc: Kyungmin Park
> Cc: Krzysztof Kozlowski
> Cc: Alim Akhtar
> ---
> drivers/gpu/drm/exynos/Kconfig| 2 +-
> drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/Kconfig
> b/drivers/gpu/drm/exynos/Kconfig
> index 7ca7e1dab52c..661b42ad4873 100644
> --- a/drivers/gpu/drm/exynos/Kconfig
> +++ b/drivers/gpu/drm/exynos/Kconfig
> @@ -7,7 +7,7 @@ config DRM_EXYNOS
> select DRM_DISPLAY_HELPER if DRM_EXYNOS_DP
> select DRM_KMS_HELPER
> select VIDEOMODE_HELPERS
> - select FB_IO_HELPERS if DRM_FBDEV_EMULATION
> + select FB_DMA_HELPERS if DRM_FBDEV_EMULATION
> select SND_SOC_HDMI_CODEC if SND_SOC
> help
> Choose this option if you have a Samsung SoC Exynos chipset.
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index fdf65587f1fe..7ca3424b59ce 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -49,9 +49,9 @@ static void exynos_drm_fb_destroy(struct fb_info *info)
>
> static const struct fb_ops exynos_drm_fb_ops = {
> .owner = THIS_MODULE,
> - __FB_DEFAULT_IO_OPS_RDWR,
> + __FB_DEFAULT_DMA_OPS_RDWR,
> DRM_FB_HELPER_DEFAULT_OPS,
> - __FB_DEFAULT_IO_OPS_DRAW,
> + __FB_DEFAULT_DMA_OPS_DRAW,
> .fb_mmap= exynos_drm_fb_mmap,
> .fb_destroy = exynos_drm_fb_destroy,
> };
> --
> 2.41.0
RE: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
Hi,
> -Original Message-
> From: dri-devel On Behalf Of
> Paul Cercueil
> Sent: Tuesday, July 4, 2023 6:47 AM
> To: Krzysztof Kozlowski ; Rob Herring
> ; Conor Dooley ; Alim Akhtar
> ; Neil Armstrong ; Sam
> Ravnborg
> Cc: devicet...@vger.kernel.org; linux-samsung-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Paul Cercueil
> ; linux-arm-ker...@lists.infradead.org
> Subject: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
>
> Register a backlight device to be able to switch between all the gamma
> levels.
>
> Signed-off-by: Paul Cercueil
> ---
> drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> index 7fd9444b42c5..b4f87d6244cb 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> @@ -8,6 +8,7 @@
> * Andrzej Hajda
> */
>
> +#include
> #include
> #include
> #include
> @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx)
> return 0;
> }
>
> +static int ld9040_bl_update_status(struct backlight_device *dev)
> +{
> + struct ld9040 *ctx = dev_get_drvdata(>dev);
> +
> + ctx->brightness = dev->props.brightness;
> + ld9040_brightness_set(ctx);
> +
> + return 0;
> +}
> +
> +static int ld9040_bl_get_intensity(struct backlight_device *dev)
> +{
> + if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
fb_blank member is deprecated according to the description of backlight.h
file so you could drop above condition I think.
Thanks,
Inki Dae
RE: [PATCH] drm/exynos: fix race condition UAF in exynos_g2d_exec_ioctl
Andi~ :)
> -Original Message-
> From: Andi Shyti
> Sent: Thursday, June 1, 2023 5:29 PM
> To: 대인기/Tizen Platform Lab(SR)/삼성전자
> Cc: 'lm0963' ; sw0312@samsung.com;
> kyungmin.p...@samsung.com; airl...@gmail.com; dan...@ffwll.ch;
> krzysztof.kozlow...@linaro.org; alim.akh...@samsung.com; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> samsung-...@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] drm/exynos: fix race condition UAF in
> exynos_g2d_exec_ioctl
>
> Hi Inki,
>
> > > > > > > > If it is async, runqueue_node is freed in
> g2d_runqueue_worker on
> > > another
> > > > > > > > worker thread. So in extreme cases, if g2d_runqueue_worker
> runs
> > > first, and
> > > > > > > > then executes the following if statement, there will be use-
> > > after-free.
> > > > > > > >
> > > > > > > > Signed-off-by: Min Li
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/exynos/exynos_drm_g2d.c | 2 +-
> > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > > b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > > > > > > > index ec784e58da5c..414e585ec7dd 100644
> > > > > > > > --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > > > > > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > > > > > > > @@ -1335,7 +1335,7 @@ int exynos_g2d_exec_ioctl(struct
> > > drm_device *drm_dev, void *data,
> > > > > > > > /* Let the runqueue know that there is work to do. */
> > > > > > > > queue_work(g2d->g2d_workq, >runqueue_work);
> > > > > > > >
> > > > > > > > - if (runqueue_node->async)
> > > > > > > > + if (req->async)
> > > > > > >
> > > > > > > did you actually hit this? If you did, then the fix is not OK.
> > > > > >
> > > > > > No, I didn't actually hit this. I found it through code review.
> This
> > > > > > is only a theoretical issue that can only be triggered in
> extreme
> > > > > > cases.
> > > > >
> > > > > first of all runqueue is used again two lines below this, which
> > > > > means that if you don't hit the uaf here you will hit it
> > > > > immediately after.
> > > >
> > > > No, if async is true, then it will goto out, which will directly
> return.
> > > >
> > > > if (runqueue_node->async)
> > > > goto out; // here, go to out, will directly return
> > > >
> > > > wait_for_completion(_node->complete); // not hit
> > > > g2d_free_runqueue_node(g2d, runqueue_node);
> > > >
> > > > out:
> > > > return 0;
> > >
> > > that's right, sorry, I misread it.
> > >
> > > > > Second, if runqueue is freed, than we need to remove the part
> > > > > where it's freed because it doesn't make sense to free runqueue
> > > > > at this stage.
> > > >
> > > > It is freed by g2d_free_runqueue_node in g2d_runqueue_worker
> > > >
> > > > static void g2d_runqueue_worker(struct work_struct *work)
> > > > {
> > > > ..
> > > > if (runqueue_node) {
> > > > pm_runtime_mark_last_busy(g2d->dev);
> > > > pm_runtime_put_autosuspend(g2d->dev);
> > > >
> > > > complete(_node->complete);
> > > > if (runqueue_node->async)
> > > > g2d_free_runqueue_node(g2d, runqueue_node);//
freed
> here
> > >
> > > this is what I'm wondering: is it correct to free a resource
> > > here? The design looks to me a bit fragile and prone to mistakes.
> >
> > This question seems to deviate from the purpose of this patch. If you
> are providing additional opinions for code quality improvement unrelated
> to this patch, it would be more appropriate for me to answer instead of
> him.
>
> It's not deviating as the question was already made in my first
> review. It just looks strange to me that a piece of data shared
> amongst processes can be freed up w
RE: [PATCH] drm/exynos: fix race condition UAF in exynos_g2d_exec_ioctl
Hi Andi,
> -Original Message-
> From: Andi Shyti
> Sent: Wednesday, May 31, 2023 9:06 PM
> To: lm0963
> Cc: inki@samsung.com; sw0312@samsung.com;
> kyungmin.p...@samsung.com; airl...@gmail.com; dan...@ffwll.ch;
> krzysztof.kozlow...@linaro.org; alim.akh...@samsung.com; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> samsung-...@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] drm/exynos: fix race condition UAF in
> exynos_g2d_exec_ioctl
>
> Hi Min,
>
> On Wed, May 31, 2023 at 06:54:34PM +0800, lm0963 wrote:
> > Hi Andi,
> >
> > On Wed, May 31, 2023 at 4:19 PM Andi Shyti wrote:
> > >
> > > Hi Min,
> > >
> > > > > > If it is async, runqueue_node is freed in g2d_runqueue_worker on
> another
> > > > > > worker thread. So in extreme cases, if g2d_runqueue_worker runs
> first, and
> > > > > > then executes the following if statement, there will be use-
> after-free.
> > > > > >
> > > > > > Signed-off-by: Min Li
> > > > > > ---
> > > > > > drivers/gpu/drm/exynos/exynos_drm_g2d.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > > > > > index ec784e58da5c..414e585ec7dd 100644
> > > > > > --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > > > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> > > > > > @@ -1335,7 +1335,7 @@ int exynos_g2d_exec_ioctl(struct
> drm_device *drm_dev, void *data,
> > > > > > /* Let the runqueue know that there is work to do. */
> > > > > > queue_work(g2d->g2d_workq, >runqueue_work);
> > > > > >
> > > > > > - if (runqueue_node->async)
> > > > > > + if (req->async)
> > > > >
> > > > > did you actually hit this? If you did, then the fix is not OK.
> > > >
> > > > No, I didn't actually hit this. I found it through code review. This
> > > > is only a theoretical issue that can only be triggered in extreme
> > > > cases.
> > >
> > > first of all runqueue is used again two lines below this, which
> > > means that if you don't hit the uaf here you will hit it
> > > immediately after.
> >
> > No, if async is true, then it will goto out, which will directly return.
> >
> > if (runqueue_node->async)
> > goto out; // here, go to out, will directly return
> >
> > wait_for_completion(_node->complete); // not hit
> > g2d_free_runqueue_node(g2d, runqueue_node);
> >
> > out:
> > return 0;
>
> that's right, sorry, I misread it.
>
> > > Second, if runqueue is freed, than we need to remove the part
> > > where it's freed because it doesn't make sense to free runqueue
> > > at this stage.
> >
> > It is freed by g2d_free_runqueue_node in g2d_runqueue_worker
> >
> > static void g2d_runqueue_worker(struct work_struct *work)
> > {
> > ..
> > if (runqueue_node) {
> > pm_runtime_mark_last_busy(g2d->dev);
> > pm_runtime_put_autosuspend(g2d->dev);
> >
> > complete(_node->complete);
> > if (runqueue_node->async)
> > g2d_free_runqueue_node(g2d, runqueue_node);// freed here
>
> this is what I'm wondering: is it correct to free a resource
> here? The design looks to me a bit fragile and prone to mistakes.
This question seems to deviate from the purpose of this patch. If you are
providing additional opinions for code quality improvement unrelated to this
patch, it would be more appropriate for me to answer instead of him.
The runqueue node - which contains command list for g2d rendering - is
generated when the user calls the ioctl system call. Therefore, if the
user-requested command list is rendered by g2d device then there is no longer a
reason to keep it. :)
>
> The patch per se is OK. It doesn't make much difference to me
> where you actually read async, although this patch looks a bit
> safer:
>
> Reviewed-by: Andi Shyti
Thanks,
Inki Dae
>
> However some refactoring might be needed to make it a bit more
> robust.
>
> Thanks,
> Andi
>
> > }
> >
> > >
> > > Finally, can you elaborate on the code review that you did so
> > > that we all understand it?
> >
> > queue_work(g2d->g2d_workq, >runqueue_work);
> > msleep(100);// add sleep here to let g2d_runqueue_worker run first
> > if (runqueue_node->async)
> > goto out;
> >
> >
> > >
> > > Andi
> >
> >
> >
> > --
> > Min Li
RE: [PATCH] drm/exynos: fix race condition UAF in exynos_g2d_exec_ioctl
Hi, > -Original Message- > From: Min Li > Sent: Friday, May 26, 2023 10:02 PM > To: inki@samsung.com > Cc: sw0312@samsung.com; kyungmin.p...@samsung.com; airl...@gmail.com; > dan...@ffwll.ch; krzysztof.kozlow...@linaro.org; alim.akh...@samsung.com; > dri-devel@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; > linux-samsung-...@vger.kernel.org; linux-ker...@vger.kernel.org > Subject: [PATCH] drm/exynos: fix race condition UAF in > exynos_g2d_exec_ioctl > > If it is async, runqueue_node is freed in g2d_runqueue_worker on another > worker thread. So in extreme cases, if g2d_runqueue_worker runs first, and > then executes the following if statement, there will be use-after-free. > I received a report about the related issue from a white hacker before. Thanks for contribution. :) > Signed-off-by: Min Li > --- > drivers/gpu/drm/exynos/exynos_drm_g2d.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c > b/drivers/gpu/drm/exynos/exynos_drm_g2d.c > index ec784e58da5c..414e585ec7dd 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c > @@ -1335,7 +1335,7 @@ int exynos_g2d_exec_ioctl(struct drm_device *drm_dev, > void *data, > /* Let the runqueue know that there is work to do. */ > queue_work(g2d->g2d_workq, >runqueue_work); > > - if (runqueue_node->async) > + if (req->async) > goto out; > > wait_for_completion(_node->complete); > -- > 2.34.1
RE: [PATCH v15 00/16] drm: Add Samsung MIPI DSIM bridge
Hi Jagan, > -Original Message- > From: Jagan Teki > Sent: Friday, March 3, 2023 11:51 PM > To: Andrzej Hajda ; Inki Dae ; > Marek Szyprowski ; Neil Armstrong > ; Marek Vasut ; Maxime Ripard > > Cc: Seung-Woo Kim ; Kyungmin Park > ; Frieder Schrempf ; > Tim Harvey ; Adam Ford ; Matteo > Lisi ; dri-devel@lists.freedesktop.org; linux- > samsung-...@vger.kernel.org; linux-amarula amar...@amarulasolutions.com>; Jagan Teki > Subject: [PATCH v15 00/16] drm: Add Samsung MIPI DSIM bridge > > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > Inki Dae: please note that this series added on top of exynos-drm-next > since few exynos dsi changes are not been part of drm-misc-next. > Request you to pick these via exynos-drm-next, or let me know if you > have any comments? Seems some issue Marek found on testing. If fixed then I will try to pick this patch series up. To Andrzej, Neil, Robert and Laurent. Could you give me any comment to below patch, [drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge] : https://www.spinics.net/lists/dri-devel/msg385731.html If you could kindly provide me with an Acked-by or Signed-off-by after review, it would greatly assist me in picking up this patch series for upstream. Thanks, Inki Dae > > Patch 0001 - 0002: find child DSI bridge and panel > > Patch 0003 - 0004: optional PHY, PMS_P offset > > Patch 0005 : introduce hw_type > > Patch 0006 : fixing host init > > Patch 0007 : atomic_check > > Patch 0008 : input_bus_flags > > Patch 0009 : atomic_get_input_bus_fmts > > Patch 0010 - 0011: component vs bridge > > Patch 0012 : DSIM bridge > > Patch 0013 - 0014: i.MX8M Mini/Nano > > Patch 0015 - 0016: i.MX8M Plus > > Changes for v15: > - drop drm_of helpers > - re-added find DSI bridge/Panel helper in dsim > - collect RB from Marek V > - fixed leading underscore in function names > - commit messages updated > - rebased on exynos-drm-next > > Changes for v13: > - remove devm call for DSI panel or bridge finding > - rebased on drm-misc-next > > Changes for v12: > - collect RB from Marek V > - add te_irq_handler hook > - fix comments from Marek V > - update atomic_get_input_bus_fmts logic > > Changes for v11: > - collect RB from Frieder Schrempf > - collect ACK from Rob > - collect ACK from Robert > - fix BIT macro replacements > - fix checkpatch --strict warnings > - fix unneeded commit text > - drop extra lines > > Changes for v10: > - rebase on drm-misc-next > - add drm_of_dsi_find_panel_or_bridge > - add devm_drm_of_dsi_get_bridge > - fix host initialization (Thanks to Marek Szyprowski) > - rearrange the tiny patches for easy to review > - update simple names for enum hw_type > - add is_hw_exynos macro > - rework on commit messages > > Changes for v9: > - rebase on drm-misc-next > - drop drm bridge attach fix for Exynos > - added prepare_prev_first flag > - added pre_enable_prev_first flag > - fix bridge chain order for exynos > - added fix for Exynos host init for first DSI transfer > - added MEDIA_BUS_FMT_FIXED > - return MEDIA_BUS_FMT_RGB888_1X24 output_fmt if supported output_fmt > list is unsupported. > - added MEDIA_BUS_FMT_YUYV10_1X20 > - added MEDIA_BUS_FMT_YUYV12_1X24 > > Changes for v8: > * fixed comment lines > * fixed commit messages > * fixed video mode bits > * collect Marek Ack > * fixed video mode bit names > * update input formats logic > * added imx8mplus support > > Changes for v7: > * fix the drm bridge attach chain for exynos drm dsi driver > * fix the hw_type checking logic > > Changes for v6: > * handle previous bridge for exynos dsi while attaching bridge > > Changes for v5: > * bridge changes to support multi-arch > * updated and clear commit messages > * add hw_type via plat data > * removed unneeded quirk > * rebased on linux-next > > Changes for v4: > * include Inki Dae in MAINTAINERS > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build > * update init handling to ensure host init done on first cmd transfer > > Changes for v3: > * fix the mult-arch build > * fix dsi host init > * updated commit messages > > Changes for v2: > * fix bridge handling > * fix dsi host init > * correct the commit messages > > Tested in Engicam i.Core MX8M Mini SoM. > > Repo: > https://protect2.fireeye.com/v1/url?k=ba7a5cbf-dbf1498c-ba7bd7f0-000babff9bb7- > 64eb01b9825d1cad=1=55e2423e-1b57-4c22-a4a8- > 3dfcebdd3d9d=https%3A%2F%2Fgithub.com%2Fopenedev%2Fkernel%2Ftree%2Fimx8mm- > dsi-v15 > > v13: > https://lore.kernel.org/all/20230227113925.875425-1- > ja...@amarulasolutions.com/ > > Any inputs? > Jagan. > > Jagan Teki (14): > drm: exynos: dsi: Drop explicit call to bridge detach > drm: exynos: dsi: Lookup OF-graph or Child node devices > drm: exynos: dsi: Mark PHY as optional > drm: exynos: dsi: Add platform PLL_P (PMS_P) offset >
RE: [PATCH v11 0/3] drm: exynos: dsi: Restore the bridge chain
Hi Jagan Teki, > -Original Message- > From: Jagan Teki > Sent: Friday, January 20, 2023 3:19 AM > To: 대인기/Tizen Platform Lab(SR)/삼성전자 > Cc: Marek Szyprowski ; Seung-Woo Kim > ; Kyungmin Park ; Neil > Armstrong ; Robert Foss ; > Andrzej Hajda ; Sam Ravnborg ; > thierry.red...@gmail.com; Marek Vasut ; linux-samsung- > s...@vger.kernel.org; dri-devel@lists.freedesktop.org; linux-amarula amar...@amarulasolutions.com> > Subject: Re: [PATCH v11 0/3] drm: exynos: dsi: Restore the bridge chain > > Hi Inki Dae, > > On Thu, Jan 12, 2023 at 7:56 AM 대인기/Tizen Platform Lab(SR)/삼성전자 > wrote: > > > > Hi Jagan Teki, > > > > Sorry for late. > > > > > -Original Message- > > > From: Jagan Teki > > > Sent: Saturday, January 7, 2023 2:56 AM > > > To: Marek Szyprowski ; Inki Dae > > > ; Seung-Woo Kim ; Kyungmin > Park > > > ; Neil Armstrong ; > Robert > > > Foss ; Andrzej Hajda ; > Sam > > > Ravnborg > > > Cc: Marek Vasut ; linux-samsung-...@vger.kernel.org; dri- > > > de...@lists.freedesktop.org; linux-amarula > > amar...@amarulasolutions.com> > > > Subject: Re: [PATCH v11 0/3] drm: exynos: dsi: Restore the bridge chain > > > > > > On Mon, Dec 12, 2022 at 11:59 PM Jagan Teki > wrote: > > > > > > > > Split the Exynos DSI bridge chain update patches from Samsung DSIM > > > > bridge driver for easy to apply. > > > > > > > > Changes for v11: > > > > - enable bridge.pre_enable_prev_first > > > > Changes for v10: > > > > - collect Marek.V Review tag > > > > > > Any update on this? > > > > > > > Added Thierry Reding who is a maintainer of DRM panel drivers. > > > > I will pick this patch series - including panel and bride patches - up. > > > > Thierry and Andrzej, please let me know if any problem. > > Any further update on this? Already merged to exynos-drm-next so this patch series will go to -next as long as no feedback from Thierry and Andrzej. I will request a PR this or next week. Thanks, Inki Dae > > Thanks, > Jagan.
RE: [PATCH v11 0/3] drm: exynos: dsi: Restore the bridge chain
Hi Jagan Teki, Sorry for late. > -Original Message- > From: Jagan Teki > Sent: Saturday, January 7, 2023 2:56 AM > To: Marek Szyprowski ; Inki Dae > ; Seung-Woo Kim ; Kyungmin Park > ; Neil Armstrong ; Robert > Foss ; Andrzej Hajda ; Sam > Ravnborg > Cc: Marek Vasut ; linux-samsung-...@vger.kernel.org; dri- > de...@lists.freedesktop.org; linux-amarula amar...@amarulasolutions.com> > Subject: Re: [PATCH v11 0/3] drm: exynos: dsi: Restore the bridge chain > > On Mon, Dec 12, 2022 at 11:59 PM Jagan Teki > wrote: > > > > Split the Exynos DSI bridge chain update patches from Samsung DSIM > > bridge driver for easy to apply. > > > > Changes for v11: > > - enable bridge.pre_enable_prev_first > > Changes for v10: > > - collect Marek.V Review tag > > Any update on this? > Added Thierry Reding who is a maintainer of DRM panel drivers. I will pick this patch series - including panel and bride patches - up. Thierry and Andrzej, please let me know if any problem. Thanks, Inki Dae > Jagan.
RE: [PATCH 1/2] drm/exynos: MAINTAINERS: move Joonyoung Shim to credits
Hi Rob,
> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: Friday, July 1, 2022 8:18 AM
> To: ���α�/Tizen Platform Lab(SR)/�Ｚ
> Cc: 'Krzysztof Kozlowski' ; 'Seung-Woo Kim'
> ; 'Kyungmin Park'
> ; 'David Airlie' ; 'Daniel
> Vetter' ; 'Krzysztof Kozlowski'
> ; 'Alim Akhtar'
> ; 'Kishon Vijay Abraham I' ;
> 'Vinod Koul' ; linux-ker...@vger.kernel.org; dri-
> de...@lists.freedesktop.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-samsung-...@vger.kernel.org; linux-
> p...@lists.infradead.org
> Subject: Re: [PATCH 1/2] drm/exynos: MAINTAINERS: move Joonyoung Shim
> to credits
>
> On Thu, Jun 30, 2022 at 01:16:14PM +0900, ���α�/Tizen Platform Lab(SR)/�
> Ｚ wrote:
> >
> >
> > > -Original Message-
> > > From: Krzysztof Kozlowski [mailto:krzysztof.kozlow...@linaro.org]
> > > Sent: Monday, June 27, 2022 1:33 AM
> > > To: Inki Dae ; Seung-Woo Kim
> > > ; Kyungmin Park
> ; David
> > > Airlie ; Daniel Vetter ; Rob Herring
> > > ; Krzysztof Kozlowski
> > > ; Alim Akhtar
> > ;
> > > Kishon Vijay Abraham I ; Vinod Koul ;
> > > linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org;
> > > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
> > > samsung-...@vger.kernel.org; linux-...@lists.infradead.org
> > > Cc: Krzysztof Kozlowski
> > > Subject: [PATCH 1/2] drm/exynos: MAINTAINERS: move Joonyoung Shim
> to
> > > credits
> > >
> > > Emails to Joonyoung Shim bounce ("550 5.1.1 Recipient address rejected:
> > > User unknown"), so move him to credits file.
> > >
> >
> > Applied.
>
> Both patches or just this one?
Ah, didn't check it. I will take other one too. Please let me know if there is
any problem.
Thanks,
Inki Dae
RE: [PATCH 1/2] drm/exynos: MAINTAINERS: move Joonyoung Shim to credits
> -Original Message-
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlow...@linaro.org]
> Sent: Monday, June 27, 2022 1:33 AM
> To: Inki Dae ; Seung-Woo Kim
> ; Kyungmin Park ; David
> Airlie ; Daniel Vetter ; Rob Herring
> ; Krzysztof Kozlowski
> ; Alim Akhtar
;
> Kishon Vijay Abraham I ; Vinod Koul ;
> linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org;
> devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
> samsung-...@vger.kernel.org; linux-...@lists.infradead.org
> Cc: Krzysztof Kozlowski
> Subject: [PATCH 1/2] drm/exynos: MAINTAINERS: move Joonyoung Shim to
> credits
>
> Emails to Joonyoung Shim bounce ("550 5.1.1 Recipient address rejected:
> User unknown"), so move him to credits file.
>
Applied.
Thanks,
Inki Dae
> Signed-off-by: Krzysztof Kozlowski
> ---
> CREDITS | 4
> MAINTAINERS | 1 -
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/CREDITS b/CREDITS
> index 7e85a53b6a88..91a564c17012 100644
> --- a/CREDITS
> +++ b/CREDITS
> @@ -3491,6 +3491,10 @@ D: wd33c93 SCSI driver (linux-m68k)
> S: San Jose, California
> S: USA
>
> +N: Joonyoung Shim
> +E: y0922.s...@samsung.com
> +D: Samsung Exynos DRM drivers
> +
> N: Robert Siemer
> E: robert.sie...@gmx.de
> P: 2048/C99A4289 2F DC 17 2E 56 62 01 C8 3D F2 AC 09 F2 E5 DD EE
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 19875f60ebb1..d208bf3b6f11 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6695,7 +6695,6 @@ F: drivers/gpu/drm/bridge/
>
> DRM DRIVERS FOR EXYNOS
> M: Inki Dae
> -M: Joonyoung Shim
> M: Seung-Woo Kim
> M: Kyungmin Park
> L: dri-devel@lists.freedesktop.org
> --
> 2.34.1
