| Message ID | 1594260156-8316-2-git-send-email-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v2,1/2] drm/bridge: dw-hdmi: Don't cleanup i2c adapter and ddc ptr in __dw_hdmi_probe() bailout path | expand |
On Thu, Jul 09, 2020 at 10:02:36AM +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(). A counterpart movement > for drm_bridge_remove() is also needed then. Moreover, since drm_bridge_add() > initializes &bridge->hpd_mutex, this may help those platform specific > dw-hdmi drivers(based on the component framework) avoid accessing the > uninitialized mutex in drm_bridge_hpd_notify() which is called in > dw_hdmi_irq(). Putting drm_bridge_add() in __dw_hdmi_probe() just before > it returns successfully should bring no logic change for platforms based > on the DRM bridge API, which is a good choice from safety point of view. > Also, __dw_hdmi_probe() is renamed to dw_hdmi_probe() since dw_hdmi_probe() > does nothing else but calling __dw_hdmi_probe(). Similar renaming applies > to the __dw_hdmi_remove()/dw_hdmi_remove() pair. Hmm, it took me some attempts to read this. Anyway, applied as-is to drm-misc-next. Sam > > Fixes: ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation optional") > 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: Sam Ravnborg <sam@ravnborg.org> > Cc: dri-devel@lists.freedesktop.org > Cc: NXP Linux Team <linux-imx@nxp.com> > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v1->v2: > * Put drm_bridge_add() in __dw_hdmi_probe() just before it returns > successfully so that the bridge shows up in the global bridge list > late enough to avoid potential race condition with other display > drivers. (Laurent) > * Rename __dw_hdmi_probe/remove() to dw_hdmi_probe/remove() > respectively. (Laurent) > * Cc'ed Sam. > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44 +++++++++---------------------- > 1 file changed, 13 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 137b6eb..748df1c 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -3179,9 +3179,11 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi) > hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > } > > -static struct dw_hdmi * > -__dw_hdmi_probe(struct platform_device *pdev, > - const struct dw_hdmi_plat_data *plat_data) > +/* ----------------------------------------------------------------------------- > + * Probe/remove API, used from platforms based on the DRM bridge API. > + */ > +struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, > + const struct dw_hdmi_plat_data *plat_data) > { > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > @@ -3438,6 +3440,8 @@ __dw_hdmi_probe(struct platform_device *pdev, > hdmi->cec = platform_device_register_full(&pdevinfo); > } > > + drm_bridge_add(&hdmi->bridge); > + > return hdmi; > > err_iahb: > @@ -3451,9 +3455,12 @@ __dw_hdmi_probe(struct platform_device *pdev, > > return ERR_PTR(ret); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_probe); > > -static void __dw_hdmi_remove(struct dw_hdmi *hdmi) > +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)) > @@ -3472,31 +3479,6 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) > else > i2c_put_adapter(hdmi->ddc); > } > - > -/* ----------------------------------------------------------------------------- > - * Probe/remove API, used from platforms based on the DRM bridge API. > - */ > -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; > -} > -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); > > /* ----------------------------------------------------------------------------- > @@ -3509,7 +3491,7 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, > struct dw_hdmi *hdmi; > int ret; > > - hdmi = __dw_hdmi_probe(pdev, plat_data); > + hdmi = dw_hdmi_probe(pdev, plat_data); > if (IS_ERR(hdmi)) > return hdmi; > > @@ -3526,7 +3508,7 @@ EXPORT_SYMBOL_GPL(dw_hdmi_bind); > > void dw_hdmi_unbind(struct dw_hdmi *hdmi) > { > - __dw_hdmi_remove(hdmi); > + dw_hdmi_remove(hdmi); > } > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, 2020-07-10 at 19:32 +0200, Sam Ravnborg wrote: > On Thu, Jul 09, 2020 at 10:02:36AM +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(). A counterpart > > movement > > for drm_bridge_remove() is also needed then. Moreover, since > > drm_bridge_add() > > initializes &bridge->hpd_mutex, this may help those platform > > specific > > dw-hdmi drivers(based on the component framework) avoid accessing > > the > > uninitialized mutex in drm_bridge_hpd_notify() which is called in > > dw_hdmi_irq(). Putting drm_bridge_add() in __dw_hdmi_probe() just > > before > > it returns successfully should bring no logic change for platforms > > based > > on the DRM bridge API, which is a good choice from safety point of > > view. > > Also, __dw_hdmi_probe() is renamed to dw_hdmi_probe() since > > dw_hdmi_probe() > > does nothing else but calling __dw_hdmi_probe(). Similar renaming > > applies > > to the __dw_hdmi_remove()/dw_hdmi_remove() pair. > > Hmm, it took me some attempts to read this. > Anyway, applied as-is to drm-misc-next. Thank you, Sam. Liu Ying > > Sam > > > > > Fixes: ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation > > optional") > > 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: Sam Ravnborg <sam@ravnborg.org> > > Cc: dri-devel@lists.freedesktop.org > > Cc: NXP Linux Team <linux-imx@nxp.com> > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > v1->v2: > > * Put drm_bridge_add() in __dw_hdmi_probe() just before it returns > > successfully so that the bridge shows up in the global bridge > > list > > late enough to avoid potential race condition with other display > > drivers. (Laurent) > > * Rename __dw_hdmi_probe/remove() to dw_hdmi_probe/remove() > > respectively. (Laurent) > > * Cc'ed Sam. > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44 +++++++++-------- > > -------------- > > 1 file changed, 13 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > index 137b6eb..748df1c 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -3179,9 +3179,11 @@ static void dw_hdmi_init_hw(struct dw_hdmi > > *hdmi) > > hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > > } > > > > -static struct dw_hdmi * > > -__dw_hdmi_probe(struct platform_device *pdev, > > - const struct dw_hdmi_plat_data *plat_data) > > +/* ------------------------------------------------------------- > > ---------------- > > + * Probe/remove API, used from platforms based on the DRM bridge > > API. > > + */ > > +struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, > > + const struct dw_hdmi_plat_data > > *plat_data) > > { > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > @@ -3438,6 +3440,8 @@ __dw_hdmi_probe(struct platform_device *pdev, > > hdmi->cec = platform_device_register_full(&pdevinfo); > > } > > > > + drm_bridge_add(&hdmi->bridge); > > + > > return hdmi; > > > > err_iahb: > > @@ -3451,9 +3455,12 @@ __dw_hdmi_probe(struct platform_device > > *pdev, > > > > return ERR_PTR(ret); > > } > > +EXPORT_SYMBOL_GPL(dw_hdmi_probe); > > > > -static void __dw_hdmi_remove(struct dw_hdmi *hdmi) > > +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)) > > @@ -3472,31 +3479,6 @@ static void __dw_hdmi_remove(struct dw_hdmi > > *hdmi) > > else > > i2c_put_adapter(hdmi->ddc); > > } > > - > > -/* ------------------------------------------------------------- > > ---------------- > > - * Probe/remove API, used from platforms based on the DRM bridge > > API. > > - */ > > -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; > > -} > > -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); > > > > /* ------------------------------------------------------------- > > ---------------- > > @@ -3509,7 +3491,7 @@ struct dw_hdmi *dw_hdmi_bind(struct > > platform_device *pdev, > > struct dw_hdmi *hdmi; > > int ret; > > > > - hdmi = __dw_hdmi_probe(pdev, plat_data); > > + hdmi = dw_hdmi_probe(pdev, plat_data); > > if (IS_ERR(hdmi)) > > return hdmi; > > > > @@ -3526,7 +3508,7 @@ EXPORT_SYMBOL_GPL(dw_hdmi_bind); > > > > void dw_hdmi_unbind(struct dw_hdmi *hdmi) > > { > > - __dw_hdmi_remove(hdmi); > > + dw_hdmi_remove(hdmi); > > } > > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > > > -- > > 2.7.4 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cvictor.liu%40nxp.com%7C0f923c54af014f54175608d824f72c5d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637299991302447959&sdata=kgoboL8o%2Ft0qvvlQw4nyrHlcusib5Ynuc%2BY%2Fn70fu14%3D&reserved=0
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 137b6eb..748df1c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -3179,9 +3179,11 @@ static void dw_hdmi_init_hw(struct dw_hdmi *hdmi) hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); } -static struct dw_hdmi * -__dw_hdmi_probe(struct platform_device *pdev, - const struct dw_hdmi_plat_data *plat_data) +/* ----------------------------------------------------------------------------- + * Probe/remove API, used from platforms based on the DRM bridge API. + */ +struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, + const struct dw_hdmi_plat_data *plat_data) { struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; @@ -3438,6 +3440,8 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->cec = platform_device_register_full(&pdevinfo); } + drm_bridge_add(&hdmi->bridge); + return hdmi; err_iahb: @@ -3451,9 +3455,12 @@ __dw_hdmi_probe(struct platform_device *pdev, return ERR_PTR(ret); } +EXPORT_SYMBOL_GPL(dw_hdmi_probe); -static void __dw_hdmi_remove(struct dw_hdmi *hdmi) +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)) @@ -3472,31 +3479,6 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) else i2c_put_adapter(hdmi->ddc); } - -/* ----------------------------------------------------------------------------- - * Probe/remove API, used from platforms based on the DRM bridge API. - */ -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; -} -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); /* ----------------------------------------------------------------------------- @@ -3509,7 +3491,7 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, struct dw_hdmi *hdmi; int ret; - hdmi = __dw_hdmi_probe(pdev, plat_data); + hdmi = dw_hdmi_probe(pdev, plat_data); if (IS_ERR(hdmi)) return hdmi; @@ -3526,7 +3508,7 @@ EXPORT_SYMBOL_GPL(dw_hdmi_bind); void dw_hdmi_unbind(struct dw_hdmi *hdmi) { - __dw_hdmi_remove(hdmi); + dw_hdmi_remove(hdmi); } EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
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(). A counterpart movement for drm_bridge_remove() is also needed then. Moreover, since drm_bridge_add() initializes &bridge->hpd_mutex, this may help those platform specific dw-hdmi drivers(based on the component framework) avoid accessing the uninitialized mutex in drm_bridge_hpd_notify() which is called in dw_hdmi_irq(). Putting drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully should bring no logic change for platforms based on the DRM bridge API, which is a good choice from safety point of view. Also, __dw_hdmi_probe() is renamed to dw_hdmi_probe() since dw_hdmi_probe() does nothing else but calling __dw_hdmi_probe(). Similar renaming applies to the __dw_hdmi_remove()/dw_hdmi_remove() pair. Fixes: ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation optional") 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: Sam Ravnborg <sam@ravnborg.org> Cc: dri-devel@lists.freedesktop.org Cc: NXP Linux Team <linux-imx@nxp.com> Signed-off-by: Liu Ying <victor.liu@nxp.com> --- v1->v2: * Put drm_bridge_add() in __dw_hdmi_probe() just before it returns successfully so that the bridge shows up in the global bridge list late enough to avoid potential race condition with other display drivers. (Laurent) * Rename __dw_hdmi_probe/remove() to dw_hdmi_probe/remove() respectively. (Laurent) * Cc'ed Sam. drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 44 +++++++++---------------------- 1 file changed, 13 insertions(+), 31 deletions(-)