| Message ID | 1592298292-17634-2-git-send-email-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path | expand |
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);
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); > >
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);
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(-)