Re: [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list
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
> > Cc: Neil Armstrong
> > Cc: Laurent Pinchart
> > Cc: Jonas Karlman
> > Cc: Jernej Skrabec
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Boris Brezillon
> > Cc: Jerome Brunet
> > Cc: Cheng-Yi Chiang
> > Cc: Dariusz Marcinkiewicz
> > Cc: Archit Taneja
> > Cc: Jose Abreu
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: NXP Linux Team
> > Signed-off-by: Liu Ying
> > ---
> > 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_
Re: [PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list
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
> Cc: Neil Armstrong
> Cc: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Boris Brezillon
> Cc: Jerome Brunet
> Cc: Cheng-Yi Chiang
> Cc: Dariusz Marcinkiewicz
> Cc: Archit Taneja
> Cc: Jose Abreu
> Cc: dri-devel@lists.freedesktop.org
> Cc: NXP Linux Team
> Signed-off-by: Liu Ying
> ---
> 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-deve
[PATCH 2/2] drm/bridge: dw-hdmi: Always add the bridge in the global bridge list
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
Cc: Neil Armstrong
Cc: Laurent Pinchart
Cc: Jonas Karlman
Cc: Jernej Skrabec
Cc: David Airlie
Cc: Daniel Vetter
Cc: Boris Brezillon
Cc: Jerome Brunet
Cc: Cheng-Yi Chiang
Cc: Dariusz Marcinkiewicz
Cc: Archit Taneja
Cc: Jose Abreu
Cc: dri-devel@lists.freedesktop.org
Cc: NXP Linux Team
Signed-off-by: Liu Ying
---
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
