| Message ID | 20210728003126.1425028-12-anitha.chrisanthus@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/14] drm/kmb: Enable LCD DMA for low TVDDCV | expand |
Hi Anitha, On Tue, Jul 27, 2021 at 05:31:24PM -0700, Anitha Chrisanthus wrote: > If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer. > This can potentially result in kernel panic when kmb_dsi_host_unregister is > called. > > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display") > Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver") > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > --- > drivers/gpu/drm/kmb/kmb_drv.c | 9 ++++++--- > drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++---- > drivers/gpu/drm/kmb/kmb_dsi.h | 3 ++- > 3 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c > index bb7eca9e13ae..12f35c43d838 100644 > --- a/drivers/gpu/drm/kmb/kmb_drv.c > +++ b/drivers/gpu/drm/kmb/kmb_drv.c > @@ -454,8 +454,9 @@ static int kmb_remove(struct platform_device *pdev) > dev_set_drvdata(dev, NULL); > > /* Unregister DSI host */ > - kmb_dsi_host_unregister(kmb->kmb_dsi); > + kmb_dsi_host_unregister(); > drm_atomic_helper_shutdown(drm); > + drm_dev_put(drm); > return 0; > } > > @@ -519,7 +520,7 @@ static int kmb_probe(struct platform_device *pdev) > if (IS_ERR(kmb->kmb_dsi)) { > drm_err(&kmb->drm, "failed to initialize DSI\n"); > ret = PTR_ERR(kmb->kmb_dsi); > - goto err_free1; > + goto err_free2; > } > > kmb->kmb_dsi->dev = &dsi_pdev->dev; > @@ -555,8 +556,10 @@ static int kmb_probe(struct platform_device *pdev) > drm_crtc_cleanup(&kmb->crtc); > drm_mode_config_cleanup(&kmb->drm); > err_free1: > + kmb_dsi_clk_disable(kmb->kmb_dsi); > + err_free2: > dev_set_drvdata(dev, NULL); > - kmb_dsi_host_unregister(kmb->kmb_dsi); > + kmb_dsi_host_unregister(); > This really looks like a step backward. There should not be a eed to call unregister if kmb_dsi is not a valid pointer in the first place. Also drn_dev_put() should not be needed with the use of drmm infrastructure. > return ret; > } > diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c > index 1cca0fe6f35f..a500172ada87 100644 > --- a/drivers/gpu/drm/kmb/kmb_dsi.c > +++ b/drivers/gpu/drm/kmb/kmb_dsi.c > @@ -172,17 +172,17 @@ mipi_hs_freq_range[MIPI_DPHY_DEFAULT_BIT_RATES] = { > {.default_bit_rate_mbps = 2500, .hsfreqrange_code = 0x49} > }; > > -static void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi) > +void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi) > { > clk_disable_unprepare(kmb_dsi->clk_mipi); > clk_disable_unprepare(kmb_dsi->clk_mipi_ecfg); > clk_disable_unprepare(kmb_dsi->clk_mipi_cfg); > } > > -void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi) > +void kmb_dsi_host_unregister(void) > { > - kmb_dsi_clk_disable(kmb_dsi); > - mipi_dsi_host_unregister(kmb_dsi->host); > + if (dsi_host) > + mipi_dsi_host_unregister(dsi_host); > } I thought we had killed the global dsi_host variable?? Seems some cleanup is till needed here. Sam
On Tue, Jul 27, 2021 at 05:31:24PM -0700, Anitha Chrisanthus wrote: > If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer. > This can potentially result in kernel panic when kmb_dsi_host_unregister is > called. > > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display") > Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver") > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > --- > drivers/gpu/drm/kmb/kmb_drv.c | 9 ++++++--- > drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++---- > drivers/gpu/drm/kmb/kmb_dsi.h | 3 ++- > 3 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c > index bb7eca9e13ae..12f35c43d838 100644 > --- a/drivers/gpu/drm/kmb/kmb_drv.c > +++ b/drivers/gpu/drm/kmb/kmb_drv.c > @@ -454,8 +454,9 @@ static int kmb_remove(struct platform_device *pdev) > dev_set_drvdata(dev, NULL); > > /* Unregister DSI host */ > - kmb_dsi_host_unregister(kmb->kmb_dsi); > + kmb_dsi_host_unregister(); > drm_atomic_helper_shutdown(drm); > + drm_dev_put(drm); > return 0; > } > > @@ -519,7 +520,7 @@ static int kmb_probe(struct platform_device *pdev) > if (IS_ERR(kmb->kmb_dsi)) { > drm_err(&kmb->drm, "failed to initialize DSI\n"); > ret = PTR_ERR(kmb->kmb_dsi); > - goto err_free1; > + goto err_free2; Don't use numberred labels. The label names should say what the goto does just like a function name says what calling the function does. The existing label names in this function mostly use "Come From" label style which is not useful either. When you're writing probe function keep track in your head of the most recent successful allocation and then when an error occurs that's what you have to free. a = alloc(); if (!a) return; <-- nothing to free b = alloc(); if (!b) goto free_a; <-- good name. a is the most recent. c = alloc(); if (!c) goto free_b; return 0; free_b: free(b); free_a: free(a); Then copy and past the error handling and add a free(c) to create the release function: void release(struct foo *p) { free(c); free(b); free(a); } regards, dan carpenter
Hi Sam, > -----Original Message----- > From: Sam Ravnborg <sam@ravnborg.org> > Sent: Wednesday, July 28, 2021 12:27 AM > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com> > Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J > <edmund.j.dea@intel.com>; Dan Carpenter <dan.carpenter@oracle.com> > Subject: Re: [PATCH 12/14] drm/kmb: Fix possible oops in error handling > > Hi Anitha, > > On Tue, Jul 27, 2021 at 05:31:24PM -0700, Anitha Chrisanthus wrote: > > If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer. > > This can potentially result in kernel panic when kmb_dsi_host_unregister is > > called. > > > > Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display") > > Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver") > > Cc: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > --- > > drivers/gpu/drm/kmb/kmb_drv.c | 9 ++++++--- > > drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++---- > > drivers/gpu/drm/kmb/kmb_dsi.h | 3 ++- > > 3 files changed, 13 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/kmb/kmb_drv.c > b/drivers/gpu/drm/kmb/kmb_drv.c > > index bb7eca9e13ae..12f35c43d838 100644 > > --- a/drivers/gpu/drm/kmb/kmb_drv.c > > +++ b/drivers/gpu/drm/kmb/kmb_drv.c > > @@ -454,8 +454,9 @@ static int kmb_remove(struct platform_device > *pdev) > > dev_set_drvdata(dev, NULL); > > > > /* Unregister DSI host */ > > - kmb_dsi_host_unregister(kmb->kmb_dsi); > > + kmb_dsi_host_unregister(); > > drm_atomic_helper_shutdown(drm); > > + drm_dev_put(drm); > > return 0; > > } > > > > @@ -519,7 +520,7 @@ static int kmb_probe(struct platform_device *pdev) > > if (IS_ERR(kmb->kmb_dsi)) { > > drm_err(&kmb->drm, "failed to initialize DSI\n"); > > ret = PTR_ERR(kmb->kmb_dsi); > > - goto err_free1; > > + goto err_free2; > > } > > > > kmb->kmb_dsi->dev = &dsi_pdev->dev; > > @@ -555,8 +556,10 @@ static int kmb_probe(struct platform_device *pdev) > > drm_crtc_cleanup(&kmb->crtc); > > drm_mode_config_cleanup(&kmb->drm); > > err_free1: > > + kmb_dsi_clk_disable(kmb->kmb_dsi); > > + err_free2: > > dev_set_drvdata(dev, NULL); > > - kmb_dsi_host_unregister(kmb->kmb_dsi); > > + kmb_dsi_host_unregister(); > > > > This really looks like a step backward. There should not be a eed to > call unregister if kmb_dsi is not a valid pointer in the first place. > Also drn_dev_put() should not be needed with the use of drmm > infrastructure. Agree, I was trying to address issues with Dan's original patch. I will keep the original code, with only this change if (IS_ERR(kmb->kmb_dsi)) { drm_err(&kmb->drm, "failed to initialize DSI\n"); - ret = PTR_ERR(kmb->kmb_dsi); - goto err_free2; + dev_set_drvdata(dev, NULL); + return PTR_ERR(kmb->kmb_dsi); Will send v2 > > > > > return ret; > > } > > diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c > b/drivers/gpu/drm/kmb/kmb_dsi.c > > index 1cca0fe6f35f..a500172ada87 100644 > > --- a/drivers/gpu/drm/kmb/kmb_dsi.c > > +++ b/drivers/gpu/drm/kmb/kmb_dsi.c > > @@ -172,17 +172,17 @@ > mipi_hs_freq_range[MIPI_DPHY_DEFAULT_BIT_RATES] = { > > {.default_bit_rate_mbps = 2500, .hsfreqrange_code = 0x49} > > }; > > > > -static void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi) > > +void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi) > > { > > clk_disable_unprepare(kmb_dsi->clk_mipi); > > clk_disable_unprepare(kmb_dsi->clk_mipi_ecfg); > > clk_disable_unprepare(kmb_dsi->clk_mipi_cfg); > > } > > > > -void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi) > > +void kmb_dsi_host_unregister(void) > > { > > - kmb_dsi_clk_disable(kmb_dsi); > > - mipi_dsi_host_unregister(kmb_dsi->host); > > + if (dsi_host) > > + mipi_dsi_host_unregister(dsi_host); > > } > I thought we had killed the global dsi_host variable?? > Seems some cleanup is till needed here. > > Sam
diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c index bb7eca9e13ae..12f35c43d838 100644 --- a/drivers/gpu/drm/kmb/kmb_drv.c +++ b/drivers/gpu/drm/kmb/kmb_drv.c @@ -454,8 +454,9 @@ static int kmb_remove(struct platform_device *pdev) dev_set_drvdata(dev, NULL); /* Unregister DSI host */ - kmb_dsi_host_unregister(kmb->kmb_dsi); + kmb_dsi_host_unregister(); drm_atomic_helper_shutdown(drm); + drm_dev_put(drm); return 0; } @@ -519,7 +520,7 @@ static int kmb_probe(struct platform_device *pdev) if (IS_ERR(kmb->kmb_dsi)) { drm_err(&kmb->drm, "failed to initialize DSI\n"); ret = PTR_ERR(kmb->kmb_dsi); - goto err_free1; + goto err_free2; } kmb->kmb_dsi->dev = &dsi_pdev->dev; @@ -555,8 +556,10 @@ static int kmb_probe(struct platform_device *pdev) drm_crtc_cleanup(&kmb->crtc); drm_mode_config_cleanup(&kmb->drm); err_free1: + kmb_dsi_clk_disable(kmb->kmb_dsi); + err_free2: dev_set_drvdata(dev, NULL); - kmb_dsi_host_unregister(kmb->kmb_dsi); + kmb_dsi_host_unregister(); return ret; } diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c index 1cca0fe6f35f..a500172ada87 100644 --- a/drivers/gpu/drm/kmb/kmb_dsi.c +++ b/drivers/gpu/drm/kmb/kmb_dsi.c @@ -172,17 +172,17 @@ mipi_hs_freq_range[MIPI_DPHY_DEFAULT_BIT_RATES] = { {.default_bit_rate_mbps = 2500, .hsfreqrange_code = 0x49} }; -static void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi) +void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi) { clk_disable_unprepare(kmb_dsi->clk_mipi); clk_disable_unprepare(kmb_dsi->clk_mipi_ecfg); clk_disable_unprepare(kmb_dsi->clk_mipi_cfg); } -void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi) +void kmb_dsi_host_unregister(void) { - kmb_dsi_clk_disable(kmb_dsi); - mipi_dsi_host_unregister(kmb_dsi->host); + if (dsi_host) + mipi_dsi_host_unregister(dsi_host); } /* @@ -229,6 +229,7 @@ int kmb_dsi_host_bridge_init(struct device *dev) dsi_device = kzalloc(sizeof(*dsi_device), GFP_KERNEL); if (!dsi_device) { kfree(dsi_host); + dsi_host = NULL; return -ENOMEM; } } diff --git a/drivers/gpu/drm/kmb/kmb_dsi.h b/drivers/gpu/drm/kmb/kmb_dsi.h index 66b7c500d9bc..89e85c993609 100644 --- a/drivers/gpu/drm/kmb/kmb_dsi.h +++ b/drivers/gpu/drm/kmb/kmb_dsi.h @@ -378,7 +378,8 @@ static inline void kmb_clr_bit_mipi(struct kmb_dsi *kmb_dsi, int kmb_dsi_host_bridge_init(struct device *dev); struct kmb_dsi *kmb_dsi_init(struct platform_device *pdev); -void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi); +void kmb_dsi_host_unregister(void); +void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi); int kmb_dsi_mode_set(struct kmb_dsi *kmb_dsi, struct drm_display_mode *mode, int sys_clk_mhz); int kmb_dsi_map_mmio(struct kmb_dsi *kmb_dsi);
If kmb_dsi_init() fails the "kmb->kmb_dsi" variable is an error pointer. This can potentially result in kernel panic when kmb_dsi_host_unregister is called. Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display") Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver") Cc: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> --- drivers/gpu/drm/kmb/kmb_drv.c | 9 ++++++--- drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++---- drivers/gpu/drm/kmb/kmb_dsi.h | 3 ++- 3 files changed, 13 insertions(+), 8 deletions(-)