* [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path
@ 2020-06-16 9:04 Liu Ying
2020-06-16 9:04 ` [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list Liu Ying
2020-06-28 8:15 ` [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path Laurent Pinchart
0 siblings, 2 replies; 5+ messages in thread
From: Liu Ying @ 2020-06-16 9:04 UTC (permalink / raw)
To: dri-devel
Cc: joabreu, jernej.skrabec, darekm, jonas, airlied, narmstrong,
architt, a.hajda, boris.brezillon, Laurent.pinchart, linux-imx,
cychiang, jbrunet
It's unnecessary to cleanup the i2c adapter and the ddc pointer in
the bailout path of __dw_hdmi_probe(), since the adapter is not
added and the ddc pointer is not set.
Fixes: a23d6265f033 (drm: bridge: dw-hdmi: Extract PHY interrupt setup to a function)
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Cheng-Yi Chiang <cychiang@chromium.org>
Cc: Dariusz Marcinkiewicz <darekm@google.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: dri-devel@lists.freedesktop.org
Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 30681398c..da84a91 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3358,11 +3358,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
return hdmi;
err_iahb:
- if (hdmi->i2c) {
- i2c_del_adapter(&hdmi->i2c->adap);
- hdmi->ddc = NULL;
- }
-
clk_disable_unprepare(hdmi->iahb_clk);
if (hdmi->cec_clk)
clk_disable_unprepare(hdmi->cec_clk);
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list
2020-06-16 9:04 [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path Liu Ying
@ 2020-06-16 9:04 ` Liu Ying
2020-06-28 8:22 ` Laurent Pinchart
2020-06-28 8:15 ` [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path Laurent Pinchart
1 sibling, 1 reply; 5+ messages in thread
From: Liu Ying @ 2020-06-16 9:04 UTC (permalink / raw)
To: dri-devel
Cc: joabreu, jernej.skrabec, darekm, jonas, airlied, narmstrong,
architt, a.hajda, boris.brezillon, Laurent.pinchart, linux-imx,
cychiang, jbrunet
It doesn't hurt to add the bridge in the global bridge list also for
platform specific dw-hdmi drivers which are based on the component
framework. This can be achieved by moving the drm_bridge_add() function
call from dw_hdmi_probe() to __dw_hdmi_probe(). Moreover, putting the
drm_bridge_add() function call prior to the interrupt registration and
enablement ensures that the mutex hpd_mutex embedded in the structure
drm_bridge can be initialized in drm_bridge_add() beforehand, which
avoids accessing the uninitialized mutex in case people want to call
function drm_bridge_hpd_notify() with the mutex locked internally to
handle hot plug detection event in the interrupt handler dw_hdmi_irq().
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Cheng-Yi Chiang <cychiang@chromium.org>
Cc: Dariusz Marcinkiewicz <darekm@google.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: dri-devel@lists.freedesktop.org
Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
Laurent,
I may see the uninitialized mutex accessing issue with
i.MX dw-hdmi after applying your below patch set[1].
I think patch '[22/27] drm: bridge: dw-hdmi: Make connector creation optional'
triggers the issue.
[1] https://patchwork.kernel.org/cover/11569709/
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 34 ++++++++++++++-----------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index da84a91..4711700 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3247,17 +3247,25 @@ __dw_hdmi_probe(struct platform_device *pdev,
dw_hdmi_init_hw(hdmi);
+ hdmi->bridge.driver_private = hdmi;
+ hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
+#ifdef CONFIG_OF
+ hdmi->bridge.of_node = pdev->dev.of_node;
+#endif
+
+ drm_bridge_add(&hdmi->bridge);
+
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
ret = irq;
- goto err_iahb;
+ goto err_irq;
}
ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq,
dw_hdmi_irq, IRQF_SHARED,
dev_name(dev), hdmi);
if (ret)
- goto err_iahb;
+ goto err_irq;
/*
* To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
@@ -3290,12 +3298,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
hdmi->ddc = NULL;
}
- hdmi->bridge.driver_private = hdmi;
- hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
-#ifdef CONFIG_OF
- hdmi->bridge.of_node = pdev->dev.of_node;
-#endif
-
if (hdmi->version >= 0x200a)
hdmi->connector.ycbcr_420_allowed =
hdmi->plat_data->ycbcr_420_allowed;
@@ -3357,6 +3359,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
return hdmi;
+err_irq:
+ drm_bridge_remove(&hdmi->bridge);
err_iahb:
clk_disable_unprepare(hdmi->iahb_clk);
if (hdmi->cec_clk)
@@ -3371,6 +3375,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
{
+ drm_bridge_remove(&hdmi->bridge);
+
if (hdmi->audio && !IS_ERR(hdmi->audio))
platform_device_unregister(hdmi->audio);
if (!IS_ERR(hdmi->cec))
@@ -3396,22 +3402,12 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
const struct dw_hdmi_plat_data *plat_data)
{
- struct dw_hdmi *hdmi;
-
- hdmi = __dw_hdmi_probe(pdev, plat_data);
- if (IS_ERR(hdmi))
- return hdmi;
-
- drm_bridge_add(&hdmi->bridge);
-
- return hdmi;
+ return __dw_hdmi_probe(pdev, plat_data);
}
EXPORT_SYMBOL_GPL(dw_hdmi_probe);
void dw_hdmi_remove(struct dw_hdmi *hdmi)
{
- drm_bridge_remove(&hdmi->bridge);
-
__dw_hdmi_remove(hdmi);
}
EXPORT_SYMBOL_GPL(dw_hdmi_remove);
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path
2020-06-16 9:04 [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path Liu Ying
2020-06-16 9:04 ` [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list Liu Ying
@ 2020-06-28 8:15 ` Laurent Pinchart
1 sibling, 0 replies; 5+ messages in thread
From: Laurent Pinchart @ 2020-06-28 8:15 UTC (permalink / raw)
To: Liu Ying
Cc: joabreu, jernej.skrabec, darekm, jonas, airlied, narmstrong,
architt, dri-devel, a.hajda, boris.brezillon, linux-imx,
cychiang, jbrunet
Hi Liu,
Thank you for the patch.
On Tue, Jun 16, 2020 at 05:04:51PM +0800, Liu Ying wrote:
> It's unnecessary to cleanup the i2c adapter and the ddc pointer in
> the bailout path of __dw_hdmi_probe(), since the adapter is not
> added and the ddc pointer is not set.
Indeed. The code doesn't hurt, but is unnecessary.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Fixes: a23d6265f033 (drm: bridge: dw-hdmi: Extract PHY interrupt setup to a function)
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Cheng-Yi Chiang <cychiang@chromium.org>
> Cc: Dariusz Marcinkiewicz <darekm@google.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Jose Abreu <joabreu@synopsys.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 30681398c..da84a91 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3358,11 +3358,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
> return hdmi;
>
> err_iahb:
> - if (hdmi->i2c) {
> - i2c_del_adapter(&hdmi->i2c->adap);
> - hdmi->ddc = NULL;
> - }
> -
> clk_disable_unprepare(hdmi->iahb_clk);
> if (hdmi->cec_clk)
> clk_disable_unprepare(hdmi->cec_clk);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list
2020-06-16 9:04 ` [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list Liu Ying
@ 2020-06-28 8:22 ` Laurent Pinchart
2020-06-29 2:44 ` Liu Ying
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2020-06-28 8:22 UTC (permalink / raw)
To: Liu Ying
Cc: joabreu, jernej.skrabec, darekm, jonas, airlied, narmstrong,
dri-devel, a.hajda, boris.brezillon, linux-imx, cychiang,
Sam Ravnborg, jbrunet
Hi Liu,
(CC'ing Sam)
Thank you for the patch.
On Tue, Jun 16, 2020 at 05:04:52PM +0800, Liu Ying wrote:
> It doesn't hurt to add the bridge in the global bridge list also for
> platform specific dw-hdmi drivers which are based on the component
> framework. This can be achieved by moving the drm_bridge_add() function
> call from dw_hdmi_probe() to __dw_hdmi_probe(). Moreover, putting the
> drm_bridge_add() function call prior to the interrupt registration and
> enablement ensures that the mutex hpd_mutex embedded in the structure
> drm_bridge can be initialized in drm_bridge_add() beforehand, which
> avoids accessing the uninitialized mutex in case people want to call
> function drm_bridge_hpd_notify() with the mutex locked internally to
> handle hot plug detection event in the interrupt handler dw_hdmi_irq().
>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Cheng-Yi Chiang <cychiang@chromium.org>
> Cc: Dariusz Marcinkiewicz <darekm@google.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Jose Abreu <joabreu@synopsys.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> Laurent,
>
> I may see the uninitialized mutex accessing issue with
> i.MX dw-hdmi after applying your below patch set[1].
> I think patch '[22/27] drm: bridge: dw-hdmi: Make connector creation optional'
> triggers the issue.
>
> [1] https://patchwork.kernel.org/cover/11569709/
>
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 34 ++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index da84a91..4711700 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3247,17 +3247,25 @@ __dw_hdmi_probe(struct platform_device *pdev,
>
> dw_hdmi_init_hw(hdmi);
>
> + hdmi->bridge.driver_private = hdmi;
> + hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
> +#ifdef CONFIG_OF
> + hdmi->bridge.of_node = pdev->dev.of_node;
> +#endif
> +
> + drm_bridge_add(&hdmi->bridge);
This would introduce a race condition where a display driver could get
hold of the bridge before it is fully initialized.
I fear the right fix for this may be to add a drm_bridge_init() function
to move mutex initialization away from drm_bridge_add(). That's a rather
intrusive change I'm afraid :-(
> +
> irq = platform_get_irq(pdev, 0);
> if (irq < 0) {
> ret = irq;
> - goto err_iahb;
> + goto err_irq;
> }
>
> ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq,
> dw_hdmi_irq, IRQF_SHARED,
> dev_name(dev), hdmi);
> if (ret)
> - goto err_iahb;
> + goto err_irq;
>
> /*
> * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
> @@ -3290,12 +3298,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
> hdmi->ddc = NULL;
> }
>
> - hdmi->bridge.driver_private = hdmi;
> - hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
> -#ifdef CONFIG_OF
> - hdmi->bridge.of_node = pdev->dev.of_node;
> -#endif
> -
> if (hdmi->version >= 0x200a)
> hdmi->connector.ycbcr_420_allowed =
> hdmi->plat_data->ycbcr_420_allowed;
> @@ -3357,6 +3359,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
>
> return hdmi;
>
> +err_irq:
> + drm_bridge_remove(&hdmi->bridge);
> err_iahb:
> clk_disable_unprepare(hdmi->iahb_clk);
> if (hdmi->cec_clk)
> @@ -3371,6 +3375,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
>
> static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
> {
> + drm_bridge_remove(&hdmi->bridge);
> +
> if (hdmi->audio && !IS_ERR(hdmi->audio))
> platform_device_unregister(hdmi->audio);
> if (!IS_ERR(hdmi->cec))
> @@ -3396,22 +3402,12 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
> const struct dw_hdmi_plat_data *plat_data)
> {
> - struct dw_hdmi *hdmi;
> -
> - hdmi = __dw_hdmi_probe(pdev, plat_data);
> - if (IS_ERR(hdmi))
> - return hdmi;
> -
> - drm_bridge_add(&hdmi->bridge);
> -
> - return hdmi;
> + return __dw_hdmi_probe(pdev, plat_data);
> }
> EXPORT_SYMBOL_GPL(dw_hdmi_probe);
Do we need to keep __dw_hdmi_probe() and dw_hdmi_probe(), can't we
rename __dw_hdmi_probe() to dw_hdmi_probe() ? Same for the remove
functions.
>
> void dw_hdmi_remove(struct dw_hdmi *hdmi)
> {
> - drm_bridge_remove(&hdmi->bridge);
> -
> __dw_hdmi_remove(hdmi);
> }
> EXPORT_SYMBOL_GPL(dw_hdmi_remove);
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list
2020-06-28 8:22 ` Laurent Pinchart
@ 2020-06-29 2:44 ` Liu Ying
0 siblings, 0 replies; 5+ messages in thread
From: Liu Ying @ 2020-06-29 2:44 UTC (permalink / raw)
To: Laurent Pinchart
Cc: joabreu, jernej.skrabec, darekm, jonas, airlied, narmstrong,
dri-devel, a.hajda, boris.brezillon, linux-imx, cychiang,
Sam Ravnborg, jbrunet
Hi Laurent,
On Sun, 2020-06-28 at 11:22 +0300, Laurent Pinchart wrote:
> Hi Liu,
>
> (CC'ing Sam)
>
> Thank you for the patch.
Thanks for your review.
>
> On Tue, Jun 16, 2020 at 05:04:52PM +0800, Liu Ying wrote:
> > It doesn't hurt to add the bridge in the global bridge list also
> > for
> > platform specific dw-hdmi drivers which are based on the component
> > framework. This can be achieved by moving the drm_bridge_add()
> > function
> > call from dw_hdmi_probe() to __dw_hdmi_probe(). Moreover, putting
> > the
> > drm_bridge_add() function call prior to the interrupt registration
> > and
> > enablement ensures that the mutex hpd_mutex embedded in the
> > structure
> > drm_bridge can be initialized in drm_bridge_add() beforehand, which
> > avoids accessing the uninitialized mutex in case people want to
> > call
> > function drm_bridge_hpd_notify() with the mutex locked internally
> > to
> > handle hot plug detection event in the interrupt handler
> > dw_hdmi_irq().
> >
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > Cc: Cheng-Yi Chiang <cychiang@chromium.org>
> > Cc: Dariusz Marcinkiewicz <darekm@google.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> > Cc: Jose Abreu <joabreu@synopsys.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> > Laurent,
> >
> > I may see the uninitialized mutex accessing issue with
> > i.MX dw-hdmi after applying your below patch set[1].
> > I think patch '[22/27] drm: bridge: dw-hdmi: Make connector
> > creation optional'
> > triggers the issue.
> >
> > [1]
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11569709%2F&data=02%7C01%7Cvictor.liu%40nxp.com%7Cca86b38a5fbc49a44b1c08d81b3c5cde%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637289293354715359&sdata=C7kz8HONVSNMYkQGb4h9uVcdZHqJVSmtwgnN4J2cKws%3D&reserved=0
> >
> > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 34 ++++++++++++++-----
> > ------------
> > 1 file changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index da84a91..4711700 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -3247,17 +3247,25 @@ __dw_hdmi_probe(struct platform_device
> > *pdev,
> >
> > dw_hdmi_init_hw(hdmi);
> >
> > + hdmi->bridge.driver_private = hdmi;
> > + hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
> > +#ifdef CONFIG_OF
> > + hdmi->bridge.of_node = pdev->dev.of_node;
> > +#endif
> > +
> > + drm_bridge_add(&hdmi->bridge);
>
> This would introduce a race condition where a display driver could
> get
> hold of the bridge before it is fully initialized.
Yes, it seems it's a bit too early to add the bridge in the global
bridge list.
>
> I fear the right fix for this may be to add a drm_bridge_init()
> function
> to move mutex initialization away from drm_bridge_add(). That's a
> rather
> intrusive change I'm afraid :-(
Looking into the issue more closely, it may be solved by moving
drm_bridge_add() from dw_hdmi_probe() to __dw_hdmi_probe() just before
__dw_hdmi_probe() returns successfully and a counterpart movement for
drm_bridge_remove(). The key is that hdmi->bridge.dev must be !NULL
when drm_bridge_hpd_notify() is called in dw_hdmi_irq() and
hdmi->bridge.dev is set in drm_bridge_attach() after drm_bridge_add()
is called.
This looks more safe because there is no logic change as
dw_hdmi_probe()/dw_hdmi_remove() see and just an additional
drm_bridge_add()/drm_bridge_remove() call as
dw_hdmi_bind()/dw_hdmi_unbind() see.
I plan to test this with i.MX dw-hdmi tomorrow.
>
> > +
> > irq = platform_get_irq(pdev, 0);
> > if (irq < 0) {
> > ret = irq;
> > - goto err_iahb;
> > + goto err_irq;
> > }
> >
> > ret = devm_request_threaded_irq(dev, irq, dw_hdmi_hardirq,
> > dw_hdmi_irq, IRQF_SHARED,
> > dev_name(dev), hdmi);
> > if (ret)
> > - goto err_iahb;
> > + goto err_irq;
> >
> > /*
> > * To prevent overflows in HDMI_IH_FC_STAT2, set the clk
> > regenerator
> > @@ -3290,12 +3298,6 @@ __dw_hdmi_probe(struct platform_device
> > *pdev,
> > hdmi->ddc = NULL;
> > }
> >
> > - hdmi->bridge.driver_private = hdmi;
> > - hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
> > -#ifdef CONFIG_OF
> > - hdmi->bridge.of_node = pdev->dev.of_node;
> > -#endif
> > -
> > if (hdmi->version >= 0x200a)
> > hdmi->connector.ycbcr_420_allowed =
> > hdmi->plat_data->ycbcr_420_allowed;
> > @@ -3357,6 +3359,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
> >
> > return hdmi;
> >
> > +err_irq:
> > + drm_bridge_remove(&hdmi->bridge);
> > err_iahb:
> > clk_disable_unprepare(hdmi->iahb_clk);
> > if (hdmi->cec_clk)
> > @@ -3371,6 +3375,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
> >
> > static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
> > {
> > + drm_bridge_remove(&hdmi->bridge);
> > +
> > if (hdmi->audio && !IS_ERR(hdmi->audio))
> > platform_device_unregister(hdmi->audio);
> > if (!IS_ERR(hdmi->cec))
> > @@ -3396,22 +3402,12 @@ static void __dw_hdmi_remove(struct dw_hdmi
> > *hdmi)
> > struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
> > const struct dw_hdmi_plat_data
> > *plat_data)
> > {
> > - struct dw_hdmi *hdmi;
> > -
> > - hdmi = __dw_hdmi_probe(pdev, plat_data);
> > - if (IS_ERR(hdmi))
> > - return hdmi;
> > -
> > - drm_bridge_add(&hdmi->bridge);
> > -
> > - return hdmi;
> > + return __dw_hdmi_probe(pdev, plat_data);
> > }
> > EXPORT_SYMBOL_GPL(dw_hdmi_probe);
>
> Do we need to keep __dw_hdmi_probe() and dw_hdmi_probe(), can't we
> rename __dw_hdmi_probe() to dw_hdmi_probe() ? Same for the remove
> functions.
Yes, the renaming makes sense. Will do that in V2 if the above new
solution stands.
Regards,
Liu Ying
>
> >
> > void dw_hdmi_remove(struct dw_hdmi *hdmi)
> > {
> > - drm_bridge_remove(&hdmi->bridge);
> > -
> > __dw_hdmi_remove(hdmi);
> > }
> > EXPORT_SYMBOL_GPL(dw_hdmi_remove);
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-29 6:51 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16 9:04 [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path Liu Ying
2020-06-16 9:04 ` [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list Liu Ying
2020-06-28 8:22 ` Laurent Pinchart
2020-06-29 2:44 ` Liu Ying
2020-06-28 8:15 ` [PATCH 1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).