drm/msm: fix bind error handling

This reverts commit 643b7d0869cc7f1f7a5ac7ca6bd25d88f54e31d0.

A recent patch that tried to fix up the msm_drm_init() paths with respect to the workqueue but only ended up making things worse:

First, the newly added calls to msm_drm_uninit() on early errors would trigger NULL-pointer dereferences, for example, as the kms pointer would not have been initialised. (Note that these paths were also modified by a second broken error handling patch which in effect cancelled out this part when merged.)

Second, the newly added allocation sanity check would still leak the previously allocated drm device.

Instead of trying to salvage what was badly broken (and clearly not tested), let's revert the bad commit so that clean and backportable fixes can be added in its place.

Fixes 643b7d0869cc ("drm/msm: Add missing check and destroy for alloc_ordered_workqueue")

Cc Jiasheng Jiang <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 2 --
  1 file changed, 2 deletions(-)

This reverts commit 8636500300a01740d92b345c680b036b94555b1b.

A recent commit tried to address a drm device leak in the early msm_drm_uninit() error paths but ended up making things worse.

Specifically, it moved the drm device reference put in msm_drm_uninit() to msm_drm_init() which means that the drm would now be leaked on normal unbind.

For reasons that were never spelled out, it also added kms NULL pointer checks to a couple of helper functions that had nothing to do with the paths modified by the patch.

Instead of trying to salvage this incrementally, let's revert the bad commit so that clean and backportable fixes can be added in its place.

Fixes 8636500300a0 ("drm/msm: Fix failure paths in msm_drm_init()")

Cc Akhil P Oommen <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c |  3 ---
  drivers/gpu/drm/msm/msm_drv.c                | 11 ++++-------
  2 files changed, 4 insertions(+), 10 deletions(-)

drivers/gpu/drm/msm/disp/msm_disp_snapshot.c +0 -3

index b73031cd48e4..e75b97127c0d 100644
@@ -129,9 +129,6 @@ void msm_disp_snapshot_destroy(struct drm_device *drm_dev)
  	}
 
  	priv = drm_dev->dev_private;
-	if (!priv->kms)
-		return;
-
  	kms = priv->kms;
 
  	if (kms->dump_worker)

drivers/gpu/drm/msm/msm_drv.c +4 -7

index b7f5a78eadd4..9ded384acba4 100644
@@ -150,9 +150,6 @@ static void msm_irq_uninstall(struct drm_device *dev)
  	struct msm_drm_private *priv = dev->dev_private;
  	struct msm_kms *kms = priv->kms;
 
-	if (!priv->kms)
-		return;
-
  	kms->funcs->irq_uninstall(kms);
  	if (kms->irq_requested)
  		free_irq(kms->irq, dev);

@@ -270,6 +267,8 @@ static int msm_drm_uninit(struct device *dev)
  	component_unbind_all(dev, ddev);
 
  	ddev->dev_private = NULL;
+	drm_dev_put(ddev);
+
  	destroy_workqueue(priv->wq);
 
  	return 0;

@@ -442,12 +441,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
  	ret = msm_init_vram(ddev);
  	if (ret)
-		goto err_drm_dev_put;
+		return ret;
 
  	/* Bind all our sub-components: */
  	ret = component_bind_all(dev, ddev);
  	if (ret)
-		goto err_drm_dev_put;
+		return ret;
 
  	dma_set_max_seg_size(dev, UINT_MAX);
 

@@ -542,8 +541,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
  err_msm_uninit:
  	msm_drm_uninit(dev);
-err_drm_dev_put:
-	drm_dev_put(ddev);
  	return ret;
  }
 

In case of early initialisation errors and on platforms that do not use the DPU controller, the deinitilisation code can be called with the kms pointer set to NULL.

Fixes 98659487b845 ("drm/msm: add support to take dpu snapshot")

Cc [email protected] # 5.14

Cc Abhinav Kumar <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/msm_drv.c +2 -1

index 9ded384acba4..17a59d73fe01 100644
@@ -242,7 +242,8 @@ static int msm_drm_uninit(struct device *dev)
  		msm_fbdev_free(ddev);
  #endif
 
-	msm_disp_snapshot_destroy(ddev);
+	if (kms)
+		msm_disp_snapshot_destroy(ddev);
 
  	drm_mode_config_cleanup(ddev);
 

In case of early initialisation errors and on platforms that do not use the DPU controller, the deinitilisation code can be called with the kms pointer set to NULL.

Fixes f026e431cf86 ("drm/msm: Convert to Linux IRQ interfaces")

Cc [email protected] # 5.14

Cc Thomas Zimmermann <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 8 +++++---
  1 file changed, 5 insertions(+), 3 deletions(-)

drivers/gpu/drm/msm/msm_drv.c +5 -3

index 17a59d73fe01..2f2bcdb671d2 100644
@@ -251,9 +251,11 @@ static int msm_drm_uninit(struct device *dev)
  		drm_bridge_remove(priv->bridges[i]);
  	priv->num_bridges = 0;
 
-	pm_runtime_get_sync(dev);
-	msm_irq_uninstall(ddev);
-	pm_runtime_put_sync(dev);
+	if (kms) {
+		pm_runtime_get_sync(dev);
+		msm_irq_uninstall(ddev);
+		pm_runtime_put_sync(dev);
+	}
 
  	if (kms && kms->funcs)
  		kms->funcs->destroy(kms);

Make sure to free the DRM device also in case of early errors during bind().

Fixes 2027e5b3413d ("drm/msm: Initialize MDSS irq domain at probe time")

Cc [email protected] # 5.17

Cc Dmitry Baryshkov <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)

drivers/gpu/drm/msm/msm_drv.c +8 -2

index 2f2bcdb671d2..89634159ad75 100644
@@ -444,12 +444,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
  	ret = msm_init_vram(ddev);
  	if (ret)
-		return ret;
+		goto err_put_dev;
 
  	/* Bind all our sub-components: */
  	ret = component_bind_all(dev, ddev);
  	if (ret)
-		return ret;
+		goto err_put_dev;
 
  	dma_set_max_seg_size(dev, UINT_MAX);
 

@@ -544,6 +544,12 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
  err_msm_uninit:
  	msm_drm_uninit(dev);
+
+	return ret;
+
+err_put_dev:
+	drm_dev_put(ddev);
+
  	return ret;
  }
 

Quoted 7 lines · Johan Hovold

Make sure to free the DRM device also in case of early errors during bind().

Fixes 2027e5b3413d ("drm/msm: Initialize MDSS irq domain at probe time")

Cc [email protected] # 5.17

Cc Dmitry Baryshkov <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

Can we migrate to devm_drm_dev_alloc instead() ? Will it make code simpler and/or easier to handle?

Quoted 9 lines · Dmitry Baryshkov

Quoted 7 lines · Johan Hovold

Make sure to free the DRM device also in case of early errors during bind().

Fixes 2027e5b3413d ("drm/msm: Initialize MDSS irq domain at probe time")

Cc [email protected] # 5.17

Cc Dmitry Baryshkov <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

Quoted 2 lines

Can we migrate to devm_drm_dev_alloc instead() ? Will it make code simpler and/or easier to handle?

I'm just fixing the bugs here. Cleanups/rework like that can be done on top but should not be backported as it risks introducing new issues.

Make sure to release the VRAM buffer also in a case a subcomponent fails to bind.

Fixes d863f0c7b536 ("drm/msm: Call msm_init_vram before binding the gpu")

Cc [email protected] # 5.11

Cc Craig Tatlor <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

Add the missing sanity check to handle workqueue allocation failures.

Fixes c8afe684c95c ("drm/msm: basic KMS driver for snapdragon")

Cc [email protected] # 3.12

Cc Rob Clark <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 4 ++++
  1 file changed, 4 insertions(+)

Make sure to destroy the workqueue also in case of early errors during bind (e.g. a subcomponent failing to bind).

Since commit c3b790ea07a1 ("drm: Manage drm_mode_config_init with drmm_") the mode config will be freed when the drm device is released also when using the legacy interface, but add an explicit cleanup for consistency and to facilitate backporting.

Fixes 060530f1ea67 ("drm/msm: use componentised device support")

Cc [email protected] # 3.15

Cc Rob Clark <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

Switch to using drmm_mode_config_init() so that the mode config is released when the last reference to the DRM device is dropped rather than unconditionally at unbind() (which may be too soon).

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 11 +++++------
  1 file changed, 5 insertions(+), 6 deletions(-)

drivers/gpu/drm/msm/msm_drv.c +5 -6

index 73c597565f99..ade17947d1e5 100644
@@ -247,8 +247,6 @@ static int msm_drm_uninit(struct device *dev)
  	if (kms)
  		msm_disp_snapshot_destroy(ddev);
 
-	drm_mode_config_cleanup(ddev);
-
  	for (i = 0; i < priv->num_bridges; i++)
  		drm_bridge_remove(priv->bridges[i]);
  	priv->num_bridges = 0;

@@ -454,11 +452,13 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
  	might_lock(&priv->lru.lock);
  	fs_reclaim_release(GFP_KERNEL);
 
-	drm_mode_config_init(ddev);
+	ret = drmm_mode_config_init(ddev);
+	if (ret)
+		goto err_destroy_wq;
 
  	ret = msm_init_vram(ddev);
  	if (ret)
-		goto err_cleanup_mode_config;
+		goto err_destroy_wq;
 
  	/* Bind all our sub-components: */
  	ret = component_bind_all(dev, ddev);

@@ -563,8 +563,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
  err_deinit_vram:
  	msm_deinit_vram(ddev);
-err_cleanup_mode_config:
-	drm_mode_config_cleanup(ddev);
+err_destroy_wq:
  	destroy_workqueue(priv->wq);
  err_put_dev:
  	drm_dev_put(ddev);

Quoted 3 lines · Johan Hovold

Switch to using drmm_mode_config_init() so that the mode config is released when the last reference to the DRM device is dropped rather than unconditionally at unbind() (which may be too soon).

This also means that drm_bridge_detach() might be called at some point after unbind(), which might be too late.

Move the include of of_address.h to the top of the file where it belongs.

Signed-off-by Johan Hovold <[email protected]>

---
  drivers/gpu/drm/msm/msm_drv.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

I had reasons to look closer at the MSM DRM driver error handling and realised that it had suffered from a fair amount of bit rot over the years.

Unfortunately, I started fixing this in my 6.2 branch and failed to notice two partial and, as it turned out, broken attempts to address this that are now in 6.3-rc1.

Instead of trying to salvage this incrementally, I'm reverting the two broken commits so that clean and backportable fixes can be added in their place.

Included are also two related cleanups.

Johan Hovold (10):
Revert "drm/msm: Add missing check and destroy for
alloc_ordered_workqueue"
Revert "drm/msm: Fix failure paths in msm_drm_init()" drm/msm: fix NULL-deref on snapshot tear down drm/msm: fix NULL-deref on irq uninstall drm/msm: fix drm device leak on bind errors drm/msm: fix vram leak on bind errors drm/msm: fix missing wq allocation error handling drm/msm: fix workqueue leak on bind errors drm/msm: use drmm_mode_config_init() drm/msm: move include directive

 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c |  3 -
 drivers/gpu/drm/msm/msm_drv.c                | 67 +++++++++++++-------
 2 files changed, 44 insertions(+), 26 deletions(-)

Quoted 12 lines · Johan Hovold

I had reasons to look closer at the MSM DRM driver error handling and realised that it had suffered from a fair amount of bit rot over the years.

Unfortunately, I started fixing this in my 6.2 branch and failed to notice two partial and, as it turned out, broken attempts to address this that are now in 6.3-rc1.

Instead of trying to salvage this incrementally, I'm reverting the two broken commits so that clean and backportable fixes can be added in their place.

Quoted 1 line

Included are also two related cleanups.

Any further comments to these patches (except for 9/10, which should be dropped)?

As the patches being reverted here were first added in 6.3-rc1 there is still time to get this into 6.3-rc (e.g. before AUTOSEL starts trying to backport them).

Johan

Quoted 16 lines

Johan Hovold (10):
Revert "drm/msm: Add missing check and destroy for
alloc_ordered_workqueue"
Revert "drm/msm: Fix failure paths in msm_drm_init()" drm/msm: fix NULL-deref on snapshot tear down drm/msm: fix NULL-deref on irq uninstall drm/msm: fix drm device leak on bind errors drm/msm: fix vram leak on bind errors drm/msm: fix missing wq allocation error handling drm/msm: fix workqueue leak on bind errors drm/msm: use drmm_mode_config_init() drm/msm: move include directive

  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c |  3 -
  drivers/gpu/drm/msm/msm_drv.c                | 67 +++++++++++++-------
  2 files changed, 44 insertions(+), 26 deletions(-)

Quoted 13 lines · Johan Hovold

Quoted 12 lines · Johan Hovold

I had reasons to look closer at the MSM DRM driver error handling and realised that it had suffered from a fair amount of bit rot over the years.

Unfortunately, I started fixing this in my 6.2 branch and failed to notice two partial and, as it turned out, broken attempts to address this that are now in 6.3-rc1.

Instead of trying to salvage this incrementally, I'm reverting the two broken commits so that clean and backportable fixes can be added in their place.

Quoted 8 lines

Quoted 1 line

Included are also two related cleanups.

Any further comments to these patches (except for 9/10, which should be dropped)?

As the patches being reverted here were first added in 6.3-rc1 there is still time to get this into 6.3-rc (e.g. before AUTOSEL starts trying to backport them).

I will take a look at the patches. Additional question, as you have been looking into this area. We have plenty of code which is only called under the if (kms) condition. Could you hopefully move these parts to separate functions, so that the error handling is also simpler? If not, I'll put this to my todo list, but it might take some time before I have time for that.

I had reasons to look closer at the MSM DRM driver error handling and realised that it had suffered from a fair amount of bit rot over the years.

Unfortunately, I started fixing this in my 6.2 branch and failed to notice two partial and, as it turned out, broken attempts to address this that are now in 6.3-rc1.

[...]