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@xxxxxxxxxxx> > > Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx> > > Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx> > > Cc: Jonas Karlman <jonas@xxxxxxxxx> > > Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxx> > > Cc: David Airlie <airlied@xxxxxxxx> > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> > > Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx> > > Cc: Cheng-Yi Chiang <cychiang@xxxxxxxxxxxx> > > Cc: Dariusz Marcinkiewicz <darekm@xxxxxxxxxx> > > Cc: Archit Taneja <architt@xxxxxxxxxxxxxx> > > Cc: Jose Abreu <joabreu@xxxxxxxxxxxx> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > Cc: NXP Linux Team <linux-imx@xxxxxxx> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
