RE: [PATCH 12/14] drm/kmb: Fix possible oops in error handling
Hi Sam,
> -Original Message-
> From: Sam Ravnborg
> Sent: Wednesday, July 28, 2021 12:27 AM
> To: Chrisanthus, Anitha
> Cc: dri-devel@lists.freedesktop.org; Dea, Edmund J
> ; Dan Carpenter
> 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
> > Signed-off-by: Anitha Chrisanthus
> > ---
> > 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(>drm, "failed to initialize DSI\n");
> > ret = PTR_ERR(kmb->kmb_dsi);
> > - goto err_free1;
> > + goto err_free2;
> > }
> >
> > kmb->kmb_dsi->dev = _pdev->dev;
> > @@ -555,8 +556,10 @@ static int kmb_probe(struct platform_device *pdev)
> > drm_crtc_cleanup(>crtc);
> > drm_mode_config_cleanup(>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(>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
Re: [PATCH 12/14] drm/kmb: Fix possible oops in error handling
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
> Signed-off-by: Anitha Chrisanthus
> ---
> 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(>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
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
> Signed-off-by: Anitha Chrisanthus
> ---
> 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(>drm, "failed to initialize DSI\n");
> ret = PTR_ERR(kmb->kmb_dsi);
> - goto err_free1;
> + goto err_free2;
> }
>
> kmb->kmb_dsi->dev = _pdev->dev;
> @@ -555,8 +556,10 @@ static int kmb_probe(struct platform_device *pdev)
> drm_crtc_cleanup(>crtc);
> drm_mode_config_cleanup(>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
