| Message ID | 20170624005628.5896-1-shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Shuah, On 24.06.2017 02:56, Shuah Khan wrote: > Fix to call of_node_put() right after of_drm_find_bridge() instead of > holding on to it until exynos_dsi_remove(). I think the current implementation is OK, node is get in probe and put in remove. There could be many bind/unbind during lifetime of the bound driver. For example, there is possible sequence: 1. probe - 2. bind 3. unbind 4. bind With this patch on 2nd bind (point 4) driver will call of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). Regards Andrzej > > Suggested-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index e337cd2..7513b88 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, > > if (dsi->bridge_node) { > bridge = of_drm_find_bridge(dsi->bridge_node); > + of_node_put(dsi->bridge_node); > if (bridge) > drm_bridge_attach(encoder, bridge, NULL); > } > @@ -1807,10 +1808,6 @@ static int exynos_dsi_probe(struct platform_device *pdev) > > static int exynos_dsi_remove(struct platform_device *pdev) > { > - struct exynos_dsi *dsi = platform_get_drvdata(pdev); > - > - of_node_put(dsi->bridge_node); > - > pm_runtime_disable(&pdev->dev); > > component_del(&pdev->dev, &exynos_dsi_component_ops); -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrzej, 2017년 06월 26일 16:02에 Andrzej Hajda 이(가) 쓴 글: > Hi Shuah, > > > On 24.06.2017 02:56, Shuah Khan wrote: >> Fix to call of_node_put() right after of_drm_find_bridge() instead of >> holding on to it until exynos_dsi_remove(). > > I think the current implementation is OK, node is get in probe and put > in remove. > There could be many bind/unbind during lifetime of the bound driver. > For example, there is possible sequence: > 1. probe - > 2. bind > 3. unbind > 4. bind > > With this patch on 2nd bind (point 4) driver will call > of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). > Right. This is a problem. How about moving of_drm_find_bridge function call to probe and keeping drm_bridge_attach call in bind for cleanup? Seems it doesn't need to call of_drm_find_bridge function every time bind callback is called. Thanks, Inki Dae > Regards > Andrzej > > >> >> Suggested-by: Inki Dae <inki.dae@samsung.com> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> index e337cd2..7513b88 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >> >> if (dsi->bridge_node) { >> bridge = of_drm_find_bridge(dsi->bridge_node); >> + of_node_put(dsi->bridge_node); >> if (bridge) >> drm_bridge_attach(encoder, bridge, NULL); >> } >> @@ -1807,10 +1808,6 @@ static int exynos_dsi_probe(struct platform_device *pdev) >> >> static int exynos_dsi_remove(struct platform_device *pdev) >> { >> - struct exynos_dsi *dsi = platform_get_drvdata(pdev); >> - >> - of_node_put(dsi->bridge_node); >> - >> pm_runtime_disable(&pdev->dev); >> >> component_del(&pdev->dev, &exynos_dsi_component_ops); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27.06.2017 11:13, Inki Dae wrote: > Hi Andrzej, > > 2017년 06월 26일 16:02에 Andrzej Hajda 이(가) 쓴 글: >> Hi Shuah, >> >> >> On 24.06.2017 02:56, Shuah Khan wrote: >>> Fix to call of_node_put() right after of_drm_find_bridge() instead of >>> holding on to it until exynos_dsi_remove(). >> I think the current implementation is OK, node is get in probe and put >> in remove. >> There could be many bind/unbind during lifetime of the bound driver. >> For example, there is possible sequence: >> 1. probe - >> 2. bind >> 3. unbind >> 4. bind >> >> With this patch on 2nd bind (point 4) driver will call >> of_drm_find_bridge on dsi->bridge_node which was put earlier (point 2.). >> > Right. This is a problem. > > How about moving of_drm_find_bridge function call to probe and keeping drm_bridge_attach call in bind for cleanup? > Seems it doesn't need to call of_drm_find_bridge function every time bind callback is called. I think the reason to call of_drm_find_bridge in bind is to avoid probe deferring, and with current code it will probably defer forever, explanation below: The only bridge connected to DSI is bridge provided by MIC driver, and MIC driver is componentized, but it registers his bridge in bind callback. So with drm_bridge_attach moved to probe of DSI initialization would never succeed because: 1. MIC->bind will be called only if all DRM components are present. 2. DSI driver will defer probing because the bridge is not present, thus DSI component will never be present. So the current code looks better to me, but there are two thing which could be improved: A. Maybe it would be good thing to move MIC bridge registration to probe, I see no reason to postpone it to bind phase, this way it should be safer if for some reason DSI->bind callback would be called before MIC->bind. B. Add error checking to of_drm_find_bridge in DSI driver, I know MIC is generally optional, but if the graph link is present (ie. dsi->bridge_node is valid) MIC bridge should be present, if it is not, it indicates some problem. Regards Andrzej > > Thanks, > Inki Dae > >> Regards >> Andrzej >> >> >>> Suggested-by: Inki Dae <inki.dae@samsung.com> >>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- >>> 1 file changed, 1 insertion(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> index e337cd2..7513b88 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>> @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, >>> >>> if (dsi->bridge_node) { >>> bridge = of_drm_find_bridge(dsi->bridge_node); >>> + of_node_put(dsi->bridge_node); >>> if (bridge) >>> drm_bridge_attach(encoder, bridge, NULL); >>> } >>> @@ -1807,10 +1808,6 @@ static int exynos_dsi_probe(struct platform_device *pdev) >>> >>> static int exynos_dsi_remove(struct platform_device *pdev) >>> { >>> - struct exynos_dsi *dsi = platform_get_drvdata(pdev); >>> - >>> - of_node_put(dsi->bridge_node); >>> - >>> pm_runtime_disable(&pdev->dev); >>> >>> component_del(&pdev->dev, &exynos_dsi_component_ops); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e337cd2..7513b88 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1689,6 +1689,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, if (dsi->bridge_node) { bridge = of_drm_find_bridge(dsi->bridge_node); + of_node_put(dsi->bridge_node); if (bridge) drm_bridge_attach(encoder, bridge, NULL); } @@ -1807,10 +1808,6 @@ static int exynos_dsi_probe(struct platform_device *pdev) static int exynos_dsi_remove(struct platform_device *pdev) { - struct exynos_dsi *dsi = platform_get_drvdata(pdev); - - of_node_put(dsi->bridge_node); - pm_runtime_disable(&pdev->dev); component_del(&pdev->dev, &exynos_dsi_component_ops);
Fix to call of_node_put() right after of_drm_find_bridge() instead of holding on to it until exynos_dsi_remove(). Suggested-by: Inki Dae <inki.dae@samsung.com> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)