| Message ID | 1486124625-23454-1-git-send-email-arvind.yadav.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Friday, February 03, 2017 05:53:45 PM Arvind Yadav wrote: > Here, dss_init_ports is not handling return error form > dpi_init_port and sdi_init_port. Now dss_init_ports is returning > always 0. And it's making below code as a dead code. > > static int dss_bind(struct device *dev) > { > . > . > r = dss_init_ports(pdev); //dss_init_ports will return always 0 > if (r)// This condition will always false > goto err_init_ports; //Dead Code > . > . > } > > This change is to handle return error from dpi_init_port and > sdi_init_port. Also, It will remove dead code from function 'dss_bind'. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > --- > drivers/video/fbdev/omap2/omapfb/dss/dss.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c > index 47d7f69..15a0dab 100644 > --- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c > +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c > @@ -946,6 +946,7 @@ static int dss_init_ports(struct platform_device *pdev) > struct device_node *parent = pdev->dev.of_node; > struct device_node *port; > int r; > + int ret = 0; > > if (parent == NULL) > return 0; > @@ -972,17 +973,17 @@ static int dss_init_ports(struct platform_device *pdev) > > switch (port_type) { > case OMAP_DISPLAY_TYPE_DPI: > - dpi_init_port(pdev, port); > + ret = dpi_init_port(pdev, port); > break; > case OMAP_DISPLAY_TYPE_SDI: > - sdi_init_port(pdev, port); > + ret = sdi_init_port(pdev, port); > break; > default: > break; > } > } while ((port = omapdss_of_get_next_port(parent, port)) != NULL); Shouldn't initialization be stopped after the first failure? i.e.: } while (!ret && (port = omapdss_of_get_next_port(parent, port)) != NULL); Also the cleanup of the partial initialization is missing. From looking at the code we should do something like this before returning: if (ret) dss_uninit_ports(pdev); (dpi_uninit_port() & sdi_uninit_port() skip not fully initialized ports) > - return 0; > + return ret; > } > > static void dss_uninit_ports(struct platform_device *pdev) Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, In dss_init_ports, There is no need to add dss_uninit_ports before returning. Because it's already take care in dss_bind. dss_bind is handling dss_uninit_ports in error path. -Arvind On Monday 06 February 2017 08:58 PM, Bartlomiej Zolnierkiewicz wrote: > dpi_uninit_port() & sdi_uninit_port() skip not fully > initialized port -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tuesday, February 07, 2017 05:41:37 PM Arvind Yadav wrote: > Hi, > > In dss_init_ports, There is no need to add dss_uninit_ports before > returning. > Because it's already take care in dss_bind. dss_bind is handling > dss_uninit_ports in error path. It doesn't handle cleanup of partially initialized ports, please look at the code: ... r = dss_init_ports(pdev); if (r) goto err_init_ports; ... err_runtime_get: pm_runtime_disable(&pdev->dev); dss_uninit_ports(pdev); err_init_ports: if (dss.video1_pll) dss_video_pll_uninit(dss.video1_pll); ... dss_uninit_ports() is not called on partially initialized ports (when dss_init_ports() returns an error we go straight into err_init_ports label and skip dss_uninit_ports()) > -Arvind > > > On Monday 06 February 2017 08:58 PM, Bartlomiej Zolnierkiewicz wrote: > > dpi_uninit_port() & sdi_uninit_port() skip not fully > > initialized port Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 08, 2017 11:25:47 AM Bartlomiej Zolnierkiewicz wrote: > > Hi, > > On Tuesday, February 07, 2017 05:41:37 PM Arvind Yadav wrote: > > Hi, > > > > In dss_init_ports, There is no need to add dss_uninit_ports before > > returning. > > Because it's already take care in dss_bind. dss_bind is handling > > dss_uninit_ports in error path. > > It doesn't handle cleanup of partially initialized ports, BTW by "partially initialized ports" here I mean cases like i.e. dpi port fully initialized and sdi one not initialized > please look at the code: > > ... > r = dss_init_ports(pdev); > if (r) > goto err_init_ports; > ... > err_runtime_get: > pm_runtime_disable(&pdev->dev); > dss_uninit_ports(pdev); > err_init_ports: > if (dss.video1_pll) > dss_video_pll_uninit(dss.video1_pll); > ... > > dss_uninit_ports() is not called on partially initialized ditto > ports (when dss_init_ports() returns an error we go straight > into err_init_ports label and skip dss_uninit_ports()) > > > -Arvind > > > > > > On Monday 06 February 2017 08:58 PM, Bartlomiej Zolnierkiewicz wrote: > > > dpi_uninit_port() & sdi_uninit_port() skip not fully > > > initialized port Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Yes, We will have to handle cleanup of partially initialized ports. As per your suggestion, I have done the changes. Thanks -Arvind On Wednesday 08 February 2017 07:51 PM, Bartlomiej Zolnierkiewicz wrote: > On Wednesday, February 08, 2017 11:25:47 AM Bartlomiej Zolnierkiewicz wrote: >> Hi, >> >> On Tuesday, February 07, 2017 05:41:37 PM Arvind Yadav wrote: >>> Hi, >>> >>> In dss_init_ports, There is no need to add dss_uninit_ports before >>> returning. >>> Because it's already take care in dss_bind. dss_bind is handling >>> dss_uninit_ports in error path. >> It doesn't handle cleanup of partially initialized ports, > BTW by "partially initialized ports" here I mean cases like > i.e. dpi port fully initialized and sdi one not initialized > >> please look at the code: >> >> ... >> r = dss_init_ports(pdev); >> if (r) >> goto err_init_ports; >> ... >> err_runtime_get: >> pm_runtime_disable(&pdev->dev); >> dss_uninit_ports(pdev); >> err_init_ports: >> if (dss.video1_pll) >> dss_video_pll_uninit(dss.video1_pll); >> ... >> >> dss_uninit_ports() is not called on partially initialized > ditto > >> ports (when dss_init_ports() returns an error we go straight >> into err_init_ports label and skip dss_uninit_ports()) >> >>> -Arvind >>> >>> >>> On Monday 06 February 2017 08:58 PM, Bartlomiej Zolnierkiewicz wrote: >>>> dpi_uninit_port() & sdi_uninit_port() skip not fully >>>> initialized port > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c index 47d7f69..15a0dab 100644 --- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c @@ -946,6 +946,7 @@ static int dss_init_ports(struct platform_device *pdev) struct device_node *parent = pdev->dev.of_node; struct device_node *port; int r; + int ret = 0; if (parent == NULL) return 0; @@ -972,17 +973,17 @@ static int dss_init_ports(struct platform_device *pdev) switch (port_type) { case OMAP_DISPLAY_TYPE_DPI: - dpi_init_port(pdev, port); + ret = dpi_init_port(pdev, port); break; case OMAP_DISPLAY_TYPE_SDI: - sdi_init_port(pdev, port); + ret = sdi_init_port(pdev, port); break; default: break; } } while ((port = omapdss_of_get_next_port(parent, port)) != NULL); - return 0; + return ret; } static void dss_uninit_ports(struct platform_device *pdev)
Here, dss_init_ports is not handling return error form dpi_init_port and sdi_init_port. Now dss_init_ports is returning always 0. And it's making below code as a dead code. static int dss_bind(struct device *dev) { . . r = dss_init_ports(pdev); //dss_init_ports will return always 0 if (r)// This condition will always false goto err_init_ports; //Dead Code . . } This change is to handle return error from dpi_init_port and sdi_init_port. Also, It will remove dead code from function 'dss_bind'. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- drivers/video/fbdev/omap2/omapfb/dss/dss.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)