[PATCH 00/10] drm/msm: fix bind error handling

[PATCH 00/10] drm/msm: fix bind error handling

[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.

Included are also two related cleanups.

Johan


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(-)

-- 
2.39.2

[PATCH 01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"

[Johan Hovold]

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 <jiasheng@iscas.ac.cn>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index aca48c868c14..b7f5a78eadd4 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -420,8 +420,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	priv->dev = ddev;
 
 	priv->wq = alloc_ordered_workqueue("msm", 0);
-	if (!priv->wq)
-		return -ENOMEM;
 
 	INIT_LIST_HEAD(&priv->objects);
 	mutex_init(&priv->obj_lock);
-- 
2.39.2

[PATCH 02/10] Revert "drm/msm: Fix failure paths in msm_drm_init()"

[Johan Hovold]

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 <quic_akhilpo@quicinc.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 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(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index b73031cd48e4..e75b97127c0d 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -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)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index b7f5a78eadd4..9ded384acba4 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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;
 }
 
-- 
2.39.2

[PATCH 03/10] drm/msm: fix NULL-deref on snapshot tear down

[Johan Hovold]

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: stable@vger.kernel.org      # 5.14
Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9ded384acba4..17a59d73fe01 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);
 
-- 
2.39.2

[PATCH 04/10] drm/msm: fix NULL-deref on irq uninstall

[Johan Hovold]

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: stable@vger.kernel.org	# 5.14
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 17a59d73fe01..2f2bcdb671d2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);
-- 
2.39.2

[PATCH 05/10] drm/msm: fix drm device leak on bind errors

[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: stable@vger.kernel.org      # 5.17
Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 2f2bcdb671d2..89634159ad75 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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;
 }
 
-- 
2.39.2

[PATCH 06/10] drm/msm: fix vram leak on bind errors

[Johan Hovold]

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: stable@vger.kernel.org      # 5.11
Cc: Craig Tatlor <ctatlor97@gmail.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 89634159ad75..41cc6cd690cd 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -51,6 +51,8 @@
 #define MSM_VERSION_MINOR	10
 #define MSM_VERSION_PATCHLEVEL	0
 
+static void msm_deinit_vram(struct drm_device *ddev);
+
 static const struct drm_mode_config_funcs mode_config_funcs = {
 	.fb_create = msm_framebuffer_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
@@ -260,12 +262,7 @@ static int msm_drm_uninit(struct device *dev)
 	if (kms && kms->funcs)
 		kms->funcs->destroy(kms);
 
-	if (priv->vram.paddr) {
-		unsigned long attrs = DMA_ATTR_NO_KERNEL_MAPPING;
-		drm_mm_takedown(&priv->vram.mm);
-		dma_free_attrs(dev, priv->vram.size, NULL,
-			       priv->vram.paddr, attrs);
-	}
+	msm_deinit_vram(ddev);
 
 	component_unbind_all(dev, ddev);
 
@@ -403,6 +400,19 @@ static int msm_init_vram(struct drm_device *dev)
 	return ret;
 }
 
+static void msm_deinit_vram(struct drm_device *ddev)
+{
+	struct msm_drm_private *priv = ddev->dev_private;
+	unsigned long attrs = DMA_ATTR_NO_KERNEL_MAPPING;
+
+	if (!priv->vram.paddr)
+		return;
+
+	drm_mm_takedown(&priv->vram.mm);
+	dma_free_attrs(ddev->dev, priv->vram.size, NULL, priv->vram.paddr,
+			attrs);
+}
+
 static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
@@ -449,7 +459,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	/* Bind all our sub-components: */
 	ret = component_bind_all(dev, ddev);
 	if (ret)
-		goto err_put_dev;
+		goto err_deinit_vram;
 
 	dma_set_max_seg_size(dev, UINT_MAX);
 
@@ -547,6 +557,8 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
 	return ret;
 
+err_deinit_vram:
+	msm_deinit_vram(ddev);
 err_put_dev:
 	drm_dev_put(ddev);
 
-- 
2.39.2

[PATCH 07/10] drm/msm: fix missing wq allocation error handling

[Johan Hovold]

Add the missing sanity check to handle workqueue allocation failures.

Fixes: c8afe684c95c ("drm/msm: basic KMS driver for snapdragon")
Cc: stable@vger.kernel.org      # 3.12
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 41cc6cd690cd..ac3b77dbfacc 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -432,6 +432,10 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	priv->dev = ddev;
 
 	priv->wq = alloc_ordered_workqueue("msm", 0);
+	if (!priv->wq) {
+		ret = -ENOMEM;
+		goto err_put_dev;
+	}
 
 	INIT_LIST_HEAD(&priv->objects);
 	mutex_init(&priv->obj_lock);
-- 
2.39.2

[PATCH 08/10] drm/msm: fix workqueue leak on bind errors

[Johan Hovold]

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: stable@vger.kernel.org      # 3.15
Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ac3b77dbfacc..73c597565f99 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -458,7 +458,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
 	ret = msm_init_vram(ddev);
 	if (ret)
-		goto err_put_dev;
+		goto err_cleanup_mode_config;
 
 	/* Bind all our sub-components: */
 	ret = component_bind_all(dev, ddev);
@@ -563,6 +563,9 @@ 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);
+	destroy_workqueue(priv->wq);
 err_put_dev:
 	drm_dev_put(ddev);
 
-- 
2.39.2

[PATCH 09/10] drm/msm: use drmm_mode_config_init()

[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).

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 73c597565f99..ade17947d1e5 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);
-- 
2.39.2

[PATCH 10/10] drm/msm: move include directive

[Johan Hovold]

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

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index ade17947d1e5..42ae7575622b 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -8,6 +8,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/fault-inject.h>
 #include <linux/kthread.h>
+#include <linux/of_address.h>
 #include <linux/sched/mm.h>
 #include <linux/uaccess.h>
 #include <uapi/linux/sched/types.h>
@@ -272,8 +273,6 @@ static int msm_drm_uninit(struct device *dev)
 	return 0;
 }
 
-#include <linux/of_address.h>
-
 struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
 {
 	struct msm_gem_address_space *aspace;
-- 
2.39.2

Re: [PATCH 09/10] drm/msm: use drmm_mode_config_init()

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> 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.

> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 73c597565f99..ade17947d1e5 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -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);

-- 
With best wishes
Dmitry

Re: [PATCH 09/10] drm/msm: use drmm_mode_config_init()

[Johan Hovold]

On Mon, Mar 06, 2023 at 02:38:37PM +0200, Dmitry Baryshkov wrote:
> On 06/03/2023 12:07, Johan Hovold wrote:
> > 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.

Indeed.

Please disregard this patch. It's not needed to fix the bind error paths
anyway.

Johan

Re: [PATCH 00/10] drm/msm: fix bind error handling

[Johan Hovold]

On Mon, Mar 06, 2023 at 11:07:12AM +0100, Johan Hovold wrote:
> 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.

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

> 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(-)

Re: [PATCH 10/10] drm/msm: move include directive

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> Move the include of of_address.h to the top of the file where it
> belongs.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

-- 
With best wishes
Dmitry

Re: [PATCH 05/10] drm/msm: fix drm device leak on bind errors

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> 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: stable@vger.kernel.org      # 5.17
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

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

> ---
>   drivers/gpu/drm/msm/msm_drv.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 2f2bcdb671d2..89634159ad75 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -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;
>   }
>   

-- 
With best wishes
Dmitry

Re: [PATCH 03/10] drm/msm: fix NULL-deref on snapshot tear down

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> 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: stable@vger.kernel.org      # 5.14
> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/msm_drv.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 9ded384acba4..17a59d73fe01 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -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);
>   

-- 
With best wishes
Dmitry

Re: [PATCH 04/10] drm/msm: fix NULL-deref on irq uninstall

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> 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: stable@vger.kernel.org	# 5.14
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/msm_drv.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 17a59d73fe01..2f2bcdb671d2 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -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);

-- 
With best wishes
Dmitry

Re: [PATCH 00/10] drm/msm: fix bind error handling

[Dmitry Baryshkov]

On 21/03/2023 15:02, Johan Hovold wrote:
> On Mon, Mar 06, 2023 at 11:07:12AM +0100, Johan Hovold wrote:
>> 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.
> 
> 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.

> 
> Johan
> 
>> 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(-)

-- 
With best wishes
Dmitry

Re: [PATCH 05/10] drm/msm: fix drm device leak on bind errors

[Johan Hovold]

On Tue, Mar 21, 2023 at 04:54:51PM +0200, Dmitry Baryshkov wrote:
> On 06/03/2023 12:07, Johan Hovold wrote:
> > 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: stable@vger.kernel.org      # 5.17
> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> 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.

Johan

Re: [PATCH 00/10] drm/msm: fix bind error handling

[Johan Hovold]

On Tue, Mar 21, 2023 at 05:21:56PM +0200, Dmitry Baryshkov wrote:
> On 21/03/2023 15:02, Johan Hovold wrote:
> > On Mon, Mar 06, 2023 at 11:07:12AM +0100, Johan Hovold wrote:
> >> 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.
> > 
> > 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.

There's definitely room for cleaning up the bind/unbind paths further,
but for this series I focus on correctness while maintaining symmetry
(e.g. if an allocation was done under if (kms), then the release should
be done under the same).

I don't think I will have time to look at this further for a few weeks
either, but I'll add it to my list of future work as well and I'll check
in with you before actually working on it.

Johan

Re: [PATCH 05/10] drm/msm: fix drm device leak on bind errors

[Dmitry Baryshkov]

On Wed, 22 Mar 2023 at 09:46, Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Mar 21, 2023 at 04:54:51PM +0200, Dmitry Baryshkov wrote:
> > On 06/03/2023 12:07, Johan Hovold wrote:
> > > 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: stable@vger.kernel.org      # 5.17
> > > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >
> > 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.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 06/10] drm/msm: fix vram leak on bind errors

[Dmitry Baryshkov]

On Mon, 6 Mar 2023 at 12:09, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> 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: stable@vger.kernel.org      # 5.11
> Cc: Craig Tatlor <ctatlor97@gmail.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

Re: [PATCH 02/10] Revert "drm/msm: Fix failure paths in msm_drm_init()"

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> 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 <quic_akhilpo@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Ok, let's do it this way

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   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(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> index b73031cd48e4..e75b97127c0d 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> @@ -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)
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index b7f5a78eadd4..9ded384acba4 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -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;
>   }
>   

-- 
With best wishes
Dmitry

Re: [PATCH 01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> 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 <jiasheng@iscas.ac.cn>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 2 --
>   1 file changed, 2 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 07/10] drm/msm: fix missing wq allocation error handling

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> Add the missing sanity check to handle workqueue allocation failures.
> 
> Fixes: c8afe684c95c ("drm/msm: basic KMS driver for snapdragon")
> Cc: stable@vger.kernel.org      # 3.12
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 4 ++++
>   1 file changed, 4 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 08/10] drm/msm: fix workqueue leak on bind errors

[Dmitry Baryshkov]

On 06/03/2023 12:07, Johan Hovold wrote:
> 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: stable@vger.kernel.org      # 3.15
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 00/10] drm/msm: fix bind error handling

[Dmitry Baryshkov]

On Mon, 06 Mar 2023 11:07:12 +0100, Johan Hovold wrote:
> 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.
> 
> [...]

Applied, thanks!

[01/10] Revert "drm/msm: Add missing check and destroy for alloc_ordered_workqueue"
        https://gitlab.freedesktop.org/lumag/msm/-/commit/ebf761d6a02f
[02/10] Revert "drm/msm: Fix failure paths in msm_drm_init()"
        https://gitlab.freedesktop.org/lumag/msm/-/commit/35a08e19a1c6
[03/10] drm/msm: fix NULL-deref on snapshot tear down
        https://gitlab.freedesktop.org/lumag/msm/-/commit/d3234fe12b3b
[04/10] drm/msm: fix NULL-deref on irq uninstall
        https://gitlab.freedesktop.org/lumag/msm/-/commit/0b5ffe5be6fd
[05/10] drm/msm: fix drm device leak on bind errors
        https://gitlab.freedesktop.org/lumag/msm/-/commit/6a44f0dbd141
[06/10] drm/msm: fix vram leak on bind errors
        https://gitlab.freedesktop.org/lumag/msm/-/commit/e6091a855649
[07/10] drm/msm: fix missing wq allocation error handling
        https://gitlab.freedesktop.org/lumag/msm/-/commit/9c6027d5a3f4
[08/10] drm/msm: fix workqueue leak on bind errors
        https://gitlab.freedesktop.org/lumag/msm/-/commit/023691129696
[10/10] drm/msm: move include directive
        https://gitlab.freedesktop.org/lumag/msm/-/commit/110fd0d5b032

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>