[PATCH] drm: hdlcd: Unwind the DRM setup on error conditions in the right order.

[PATCH] drm: hdlcd: Unwind the DRM setup on error conditions in the right order.

[Liviu Dudau]

In hdlcd_drm_bind()/hdlcd_drm_unbind() we unwind the DRM setup in the
wrong order (drm_mode_config_cleanup() before connector and encoder
had a chance to cleanup their memory or before drm_dev_unregister()).
The correct order should match in both functions.

Reported-by: Robin Murphy <Robin.Murphy@arm.com>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---

Robin,

I believe this should fix your problems with HDLCD failing to allocate CMA
memory and then crashing.

Best regards,
Liviu

 drivers/gpu/drm/arm/hdlcd_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 3422ca2..2c6eddb 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -382,7 +382,6 @@ static int hdlcd_drm_bind(struct device *dev)
 
 err_fbdev:
 	drm_kms_helper_poll_fini(drm);
-	drm_mode_config_cleanup(drm);
 	drm_vblank_cleanup(drm);
 err_vblank:
 	pm_runtime_disable(drm->dev);
@@ -390,6 +389,7 @@ err_pm_active:
 	component_unbind_all(dev, drm);
 err_unregister:
 	drm_dev_unregister(drm);
+	drm_mode_config_cleanup(drm);
 err_unload:
 	drm_irq_uninstall(drm);
 	of_reserved_mem_device_release(drm->dev);
@@ -410,15 +410,15 @@ static void hdlcd_drm_unbind(struct device *dev)
 		hdlcd->fbdev = NULL;
 	}
 	drm_kms_helper_poll_fini(drm);
-	component_unbind_all(dev, drm);
 	drm_vblank_cleanup(drm);
+	component_unbind_all(dev, drm);
 	pm_runtime_get_sync(drm->dev);
 	drm_irq_uninstall(drm);
 	pm_runtime_put_sync(drm->dev);
 	pm_runtime_disable(drm->dev);
-	of_reserved_mem_device_release(drm->dev);
-	drm_mode_config_cleanup(drm);
 	drm_dev_unregister(drm);
+	drm_mode_config_cleanup(drm);
+	of_reserved_mem_device_release(drm->dev);
 	drm_dev_unref(drm);
 	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
-- 
2.8.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: hdlcd: Unwind the DRM setup on error conditions in the right order.

[Robin Murphy]

On 08/06/16 13:56, Liviu Dudau wrote:
> In hdlcd_drm_bind()/hdlcd_drm_unbind() we unwind the DRM setup in the
> wrong order (drm_mode_config_cleanup() before connector and encoder
> had a chance to cleanup their memory or before drm_dev_unregister()).
> The correct order should match in both functions.
>
> Reported-by: Robin Murphy <Robin.Murphy@arm.com>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
>
> Robin,
>
> I believe this should fix your problems with HDLCD failing to allocate CMA
> memory and then crashing.

Heh, I'm not sure I'd even clocked that there was yet another propblem 
beyond the original gem_free_object() crash. I've just tried 4.7-rc3 
with the relevant fixes for that, and indeed drm_connector_cleanup() 
does go bang, and this patch makes it happy again.

Tested-by: Robin Murphy <robin.murphy@arm.com>

Thanks,
Robin.

>
> Best regards,
> Liviu
>
>   drivers/gpu/drm/arm/hdlcd_drv.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 3422ca2..2c6eddb 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -382,7 +382,6 @@ static int hdlcd_drm_bind(struct device *dev)
>
>   err_fbdev:
>   	drm_kms_helper_poll_fini(drm);
> -	drm_mode_config_cleanup(drm);
>   	drm_vblank_cleanup(drm);
>   err_vblank:
>   	pm_runtime_disable(drm->dev);
> @@ -390,6 +389,7 @@ err_pm_active:
>   	component_unbind_all(dev, drm);
>   err_unregister:
>   	drm_dev_unregister(drm);
> +	drm_mode_config_cleanup(drm);
>   err_unload:
>   	drm_irq_uninstall(drm);
>   	of_reserved_mem_device_release(drm->dev);
> @@ -410,15 +410,15 @@ static void hdlcd_drm_unbind(struct device *dev)
>   		hdlcd->fbdev = NULL;
>   	}
>   	drm_kms_helper_poll_fini(drm);
> -	component_unbind_all(dev, drm);
>   	drm_vblank_cleanup(drm);
> +	component_unbind_all(dev, drm);
>   	pm_runtime_get_sync(drm->dev);
>   	drm_irq_uninstall(drm);
>   	pm_runtime_put_sync(drm->dev);
>   	pm_runtime_disable(drm->dev);
> -	of_reserved_mem_device_release(drm->dev);
> -	drm_mode_config_cleanup(drm);
>   	drm_dev_unregister(drm);
> +	drm_mode_config_cleanup(drm);
> +	of_reserved_mem_device_release(drm->dev);
>   	drm_dev_unref(drm);
>   	drm->dev_private = NULL;
>   	dev_set_drvdata(dev, NULL);
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: hdlcd: Unwind the DRM setup on error conditions in the right order.

[Liviu Dudau]

On Mon, Jun 13, 2016 at 12:02:31PM +0100, Robin Murphy wrote:
> On 08/06/16 13:56, Liviu Dudau wrote:
> >In hdlcd_drm_bind()/hdlcd_drm_unbind() we unwind the DRM setup in the
> >wrong order (drm_mode_config_cleanup() before connector and encoder
> >had a chance to cleanup their memory or before drm_dev_unregister()).
> >The correct order should match in both functions.
> >
> >Reported-by: Robin Murphy <Robin.Murphy@arm.com>
> >Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >---
> >
> >Robin,
> >
> >I believe this should fix your problems with HDLCD failing to allocate CMA
> >memory and then crashing.
> 
> Heh, I'm not sure I'd even clocked that there was yet another propblem
> beyond the original gem_free_object() crash. I've just tried 4.7-rc3 with
> the relevant fixes for that, and indeed drm_connector_cleanup() does go
> bang, and this patch makes it happy again.
> 
> Tested-by: Robin Murphy <robin.murphy@arm.com>

Cheers! I will push it for the next -rc.

Best regards,
Liviu

> 
> Thanks,
> Robin.
> 
> >
> >Best regards,
> >Liviu
> >
> >  drivers/gpu/drm/arm/hdlcd_drv.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> >index 3422ca2..2c6eddb 100644
> >--- a/drivers/gpu/drm/arm/hdlcd_drv.c
> >+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> >@@ -382,7 +382,6 @@ static int hdlcd_drm_bind(struct device *dev)
> >
> >  err_fbdev:
> >  	drm_kms_helper_poll_fini(drm);
> >-	drm_mode_config_cleanup(drm);
> >  	drm_vblank_cleanup(drm);
> >  err_vblank:
> >  	pm_runtime_disable(drm->dev);
> >@@ -390,6 +389,7 @@ err_pm_active:
> >  	component_unbind_all(dev, drm);
> >  err_unregister:
> >  	drm_dev_unregister(drm);
> >+	drm_mode_config_cleanup(drm);
> >  err_unload:
> >  	drm_irq_uninstall(drm);
> >  	of_reserved_mem_device_release(drm->dev);
> >@@ -410,15 +410,15 @@ static void hdlcd_drm_unbind(struct device *dev)
> >  		hdlcd->fbdev = NULL;
> >  	}
> >  	drm_kms_helper_poll_fini(drm);
> >-	component_unbind_all(dev, drm);
> >  	drm_vblank_cleanup(drm);
> >+	component_unbind_all(dev, drm);
> >  	pm_runtime_get_sync(drm->dev);
> >  	drm_irq_uninstall(drm);
> >  	pm_runtime_put_sync(drm->dev);
> >  	pm_runtime_disable(drm->dev);
> >-	of_reserved_mem_device_release(drm->dev);
> >-	drm_mode_config_cleanup(drm);
> >  	drm_dev_unregister(drm);
> >+	drm_mode_config_cleanup(drm);
> >+	of_reserved_mem_device_release(drm->dev);
> >  	drm_dev_unref(drm);
> >  	drm->dev_private = NULL;
> >  	dev_set_drvdata(dev, NULL);
> >
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel