[PATCH 0/7] drm/msm: probe deferral fixes

[PATCH 0/7] drm/msm: probe deferral fixes

[Johan Hovold]

The MSM DRM is currently broken in multiple ways with respect to probe
deferral. Not only does the driver currently fail to probe again after a
late deferral, but due to a related use-after-free bug this also
triggers NULL-pointer dereferences.

These bugs are not new but have become critical with the release of
5.19 where probe is deferred in case the aux-bus EP panel driver has not
yet been loaded.

The underlying problem is lifetime issues due to careless use of
device-managed resources.

Specifically, device-managed resources allocated post component bind
must be tied to the lifetime of the aggregate DRM device or they will
not necessarily be released when binding of the aggregate device is
deferred.

The following call chain and pseudo code serves as an illustration of
the problem:

 - platform_probe(pdev1)
   - dp_display_probe()
     - component_add()

 - platform_probe(pdev2) 				// last component
   - dp_display_probe()					// d0
       - component_add()
         - try_to_bring_up_aggregate_device()
	   - devres_open_group(adev->parent)		// d1

	   - msm_drm_bind()
	     - msm_drm_init()
	       - component_bind_all()
	         - for_each_component()
		   - component_bind()
		     - devres_open_group(&pdev->dev)	// d2
	             - dp_display_bind()
		       - devm_kzalloc(&pdev->dev)	// a1, OK
		     - devres_close_group(&pdev->dev)	// d3

	       - dpu_kms_hw_init()
	         - for_each_panel()
	           - msm_dp_modeset_init()
		     - dp_display_request_irq()
		       - devm_request_irq(&pdev->dev)	// a2, BUG
		     - if (pdev == pdev2 && condition)
		       - return -EPROBE_DEFER;

	      - if (error)
 	        - component_unbind_all()
	          - for_each_component()
		    - component_unbind()
		      - dp_display_unbind()
		      - devres_release_group(&pdev->dev) // d4, only a1 is freed

           - if (error)
	     - devres_release_group(adev->parent)	// d5

The device-managed allocation a2 is buggy as its lifetime is tied to the
component platform device and will not be released when the aggregate
device bind fails (e.g. due to a probe deferral).

When pdev2 is later probed again, the attempt to allocate the IRQ a
second time will fail for pdev1 (which is still bound to its platform
driver).

This series fixes the lifetime issues by tying the lifetime of a2 (and
similar allocations) to the lifetime of the aggregate device so that a2
is released at d5.

In some cases, such has for the DP IRQ, the above situation can also be
avoided by moving the allocation in question to the platform driver
probe (d0) or component bind (between d2 and d3). But as doing so is not
a general fix, this can be done later as a cleanup/optimisation.

Johan


Johan Hovold (7):
  drm/msm: fix use-after-free on probe deferral
  drm/msm: fix memory corruption with too many bridges
  drm/msm/dp: fix IRQ lifetime
  drm/msm/dp: fix aux-bus EP lifetime
  drm/msm/dp: fix bridge lifetime
  drm/msm/hdmi: fix IRQ lifetime
  drm/msm: drop modeset sanity checks

 drivers/gpu/drm/bridge/parade-ps8640.c   |  2 +-
 drivers/gpu/drm/display/drm_dp_aux_bus.c |  5 +++--
 drivers/gpu/drm/msm/dp/dp_display.c      | 16 +++++++++-------
 drivers/gpu/drm/msm/dp/dp_parser.c       |  6 +++---
 drivers/gpu/drm/msm/dp/dp_parser.h       |  5 +++--
 drivers/gpu/drm/msm/dsi/dsi.c            |  9 +++++----
 drivers/gpu/drm/msm/hdmi/hdmi.c          |  7 ++++++-
 drivers/gpu/drm/msm/msm_drv.c            |  1 +
 include/drm/display/drm_dp_aux_bus.h     |  6 +++---
 9 files changed, 34 insertions(+), 23 deletions(-)

-- 
2.35.1

[PATCH 1/7] drm/msm: fix use-after-free on probe deferral

[Johan Hovold]

The bridge counter was never reset when tearing down the DRM device so
that stale pointers to deallocated structures would be accessed on the
next tear down (e.g. after a second late bind deferral).

Given enough bridges and a few probe deferrals this could currently also
lead to data beyond the bridge array being corrupted.

Fixes: d28ea556267c ("drm/msm: properly add and remove internal bridges")
Cc: stable@vger.kernel.org      # 5.19
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 391d86b54ded..d254fe2507ec 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -241,6 +241,7 @@ static int msm_drm_uninit(struct device *dev)
 
 	for (i = 0; i < priv->num_bridges; i++)
 		drm_bridge_remove(priv->bridges[i]);
+	priv->num_bridges = 0;
 
 	pm_runtime_get_sync(dev);
 	msm_irq_uninstall(ddev);
-- 
2.35.1

[PATCH 2/7] drm/msm: fix memory corruption with too many bridges

[Johan Hovold]

Add the missing sanity checks on the bridge counter to avoid corrupting
data beyond the fixed-sized bridge array in case there are ever more
than eight bridges.

a3376e3ec81c ("drm/msm: convert to drm_bridge")
ab5b0107ccf3 ("drm/msm: Initial add eDP support in msm drm driver (v5)")
a689554ba6ed ("drm/msm: Initial add DSI connector support")
Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable")
Cc: stable@vger.kernel.org	# 3.12
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++
 drivers/gpu/drm/msm/dsi/dsi.c       | 6 ++++++
 drivers/gpu/drm/msm/hdmi/hdmi.c     | 5 +++++
 3 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3e284fed8d30..fbe950edaefe 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1604,6 +1604,12 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 		return -EINVAL;
 
 	priv = dev->dev_private;
+
+	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
+		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
+		return -ENOSPC;
+	}
+
 	dp_display->drm_dev = dev;
 
 	dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 39bbabb5daf6..8a95c744972a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -218,6 +218,12 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 		return -EINVAL;
 
 	priv = dev->dev_private;
+
+	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
+		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
+		return -ENOSPC;
+	}
+
 	msm_dsi->dev = dev;
 
 	ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 93fe61b86967..a0ed6aa8e4e1 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -300,6 +300,11 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 	struct platform_device *pdev = hdmi->pdev;
 	int ret;
 
+	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
+		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
+		return -ENOSPC;
+	}
+
 	hdmi->dev = dev;
 	hdmi->encoder = encoder;
 
-- 
2.35.1

[PATCH 3/7] drm/msm/dp: fix IRQ lifetime

[Johan Hovold]

Device-managed resources allocated post component bind must be tied to
the lifetime of the aggregate DRM device or they will not necessarily be
released when binding of the aggregate device is deferred.

This is specifically true for the DP IRQ, which will otherwise remain
requested so that the next bind attempt fails when requesting the IRQ a
second time.

Since commit c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
this can happen when the aux-bus panel driver has not yet been loaded so
that probe is deferred.

Fix this by tying the device-managed lifetime of the DP IRQ to the DRM
device so that it is released when bind fails.

Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Cc: stable@vger.kernel.org      # 5.10
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index fbe950edaefe..ba557328710a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1258,7 +1258,7 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 		return -EINVAL;
 	}
 
-	rc = devm_request_irq(&dp->pdev->dev, dp->irq,
+	rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
 			dp_display_irq_handler,
 			IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
 	if (rc < 0) {
-- 
2.35.1

[PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[Johan Hovold]

Device-managed resources allocated post component bind must be tied to
the lifetime of the aggregate DRM device or they will not necessarily be
released when binding of the aggregate device is deferred.

This can lead resource leaks or failure to bind the aggregate device
when binding is later retried and a second attempt to allocate the
resources is made.

For the DP aux-bus, an attempt to populate the bus a second time will
simply fail ("DP AUX EP device already populated").

Fix this by amending the DP aux interface and tying the lifetime of the
EP device to the DRM device rather than DP controller platform device.

Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
Cc: stable@vger.kernel.org      # 5.19
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/bridge/parade-ps8640.c   | 2 +-
 drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
 drivers/gpu/drm/msm/dp/dp_display.c      | 3 ++-
 include/drm/display/drm_dp_aux_bus.h     | 6 +++---
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index d7483c13c569..6127979370cb 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -719,7 +719,7 @@ static int ps8640_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
+	ret = devm_of_dp_aux_populate_bus(dev, &ps_bridge->aux, ps8640_bridge_link_panel);
 
 	/*
 	 * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
index f5741b45ca07..2706f2cf82f7 100644
--- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
+++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
@@ -322,6 +322,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
 
 /**
  * devm_of_dp_aux_populate_bus() - devm wrapper for of_dp_aux_populate_bus()
+ * @dev: Device to tie the lifetime of the EP devices to
  * @aux: The AUX channel whose device we want to populate
  * @done_probing: Callback functions to call after EP device finishes probing.
  *                Will not be called if there are no EP devices and this
@@ -333,7 +334,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
  *         no children. The done_probing() function won't be called in that
  *         case.
  */
-int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
 				int (*done_probing)(struct drm_dp_aux *aux))
 {
 	int ret;
@@ -342,7 +343,7 @@ int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
 	if (ret)
 		return ret;
 
-	return devm_add_action_or_reset(aux->dev,
+	return devm_add_action_or_reset(dev,
 					of_dp_aux_depopulate_bus_void, aux);
 }
 EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index ba557328710a..e1aa6355bbf6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1559,7 +1559,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 		 * panel driver is probed asynchronously but is the best we
 		 * can do without a bigger driver reorganization.
 		 */
-		rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+		rc = devm_of_dp_aux_populate_ep_devices(dp->drm_dev->dev,
+							dp_priv->aux);
 		of_node_put(aux_bus);
 		if (rc)
 			goto error;
diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h
index 8a0a486383c5..a4063aa7fc40 100644
--- a/include/drm/display/drm_dp_aux_bus.h
+++ b/include/drm/display/drm_dp_aux_bus.h
@@ -47,7 +47,7 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr
 int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
 			   int (*done_probing)(struct drm_dp_aux *aux));
 void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
-int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
+int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
 				int (*done_probing)(struct drm_dp_aux *aux));
 
 /* Deprecated versions of the above functions. To be removed when no callers. */
@@ -61,11 +61,11 @@ static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
 	return (ret != -ENODEV) ? ret : 0;
 }
 
-static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
+static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
 {
 	int ret;
 
-	ret = devm_of_dp_aux_populate_bus(aux, NULL);
+	ret = devm_of_dp_aux_populate_bus(dev, aux, NULL);
 
 	/* New API returns -ENODEV for no child case; adapt to old assumption */
 	return (ret != -ENODEV) ? ret : 0;
-- 
2.35.1

[PATCH 5/7] drm/msm/dp: fix bridge lifetime

[Johan Hovold]

Device-managed resources allocated post component bind must be tied to
the lifetime of the aggregate DRM device or they will not necessarily be
released when binding of the aggregate device is deferred.

This can lead resource leaks or failure to bind the aggregate device
when binding is later retried and a second attempt to allocate the
resources is made.

For the DP bridges, previously allocated bridges will leak on probe
deferral.

Fix this by amending the DP parser interface and tying the lifetime of
the bridge device to the DRM device rather than DP platform device.

Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
Cc: stable@vger.kernel.org      # 5.19
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
 drivers/gpu/drm/msm/dp/dp_parser.c  | 6 +++---
 drivers/gpu/drm/msm/dp/dp_parser.h  | 5 +++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index e1aa6355bbf6..393af1ea9ed8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1576,7 +1576,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 	 * For DisplayPort interfaces external bridges are optional, so
 	 * silently ignore an error if one is not present (-ENODEV).
 	 */
-	rc = dp_parser_find_next_bridge(dp_priv->parser);
+	rc = devm_dp_parser_find_next_bridge(dp->drm_dev->dev, dp_priv->parser);
 	if (!dp->is_edp && rc == -ENODEV)
 		return 0;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index dd732215d55b..dcbe893d66d7 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -240,12 +240,12 @@ static int dp_parser_clock(struct dp_parser *parser)
 	return 0;
 }
 
-int dp_parser_find_next_bridge(struct dp_parser *parser)
+int devm_dp_parser_find_next_bridge(struct device *dev, struct dp_parser *parser)
 {
-	struct device *dev = &parser->pdev->dev;
+	struct platform_device *pdev = parser->pdev;
 	struct drm_bridge *bridge;
 
-	bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+	bridge = devm_drm_of_get_bridge(dev, pdev->dev.of_node, 1, 0);
 	if (IS_ERR(bridge))
 		return PTR_ERR(bridge);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 866c1a82bf1a..d30ab773db46 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -138,8 +138,9 @@ struct dp_parser {
 struct dp_parser *dp_parser_get(struct platform_device *pdev);
 
 /**
- * dp_parser_find_next_bridge() - find an additional bridge to DP
+ * devm_dp_parser_find_next_bridge() - find an additional bridge to DP
  *
+ * @dev: device to tie bridge lifetime to
  * @parser: dp_parser data from client
  *
  * This function is used to find any additional bridge attached to
@@ -147,6 +148,6 @@ struct dp_parser *dp_parser_get(struct platform_device *pdev);
  *
  * Return: 0 if able to get the bridge, otherwise negative errno for failure.
  */
-int dp_parser_find_next_bridge(struct dp_parser *parser);
+int devm_dp_parser_find_next_bridge(struct device *dev, struct dp_parser *parser);
 
 #endif
-- 
2.35.1

[PATCH 6/7] drm/msm/hdmi: fix IRQ lifetime

[Johan Hovold]

Device-managed resources allocated post component bind must be tied to
the lifetime of the aggregate DRM device or they will not necessarily be
released when binding of the aggregate device is deferred.

This is specifically true for the HDMI IRQ, which will otherwise remain
requested so that the next bind attempt fails when requesting the IRQ a
second time.

Fix this by tying the device-managed lifetime of the HDMI IRQ to the DRM
device so that it is released when bind fails.

Fixes: 067fef372c73 ("drm/msm/hdmi: refactor bind/init")
Cc: stable@vger.kernel.org      # 3.19
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index a0ed6aa8e4e1..f28fb21e3891 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -344,7 +344,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 		goto fail;
 	}
 
-	ret = devm_request_irq(&pdev->dev, hdmi->irq,
+	ret = devm_request_irq(dev->dev, hdmi->irq,
 			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
 			"hdmi_isr", hdmi);
 	if (ret < 0) {
-- 
2.35.1

[PATCH 7/7] drm/msm: drop modeset sanity checks

[Johan Hovold]

Drop the overly defensive modeset sanity checks of function parameters
which have already been checked or used by the callers.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 7 +------
 drivers/gpu/drm/msm/dsi/dsi.c       | 7 +------
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 393af1ea9ed8..8ad28bf81abe 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1597,15 +1597,10 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder)
 {
-	struct msm_drm_private *priv;
+	struct msm_drm_private *priv = dev->dev_private;
 	struct dp_display_private *dp_priv;
 	int ret;
 
-	if (WARN_ON(!encoder) || WARN_ON(!dp_display) || WARN_ON(!dev))
-		return -EINVAL;
-
-	priv = dev->dev_private;
-
 	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
 		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
 		return -ENOSPC;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 8a95c744972a..31fdee2052be 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -211,14 +211,9 @@ void __exit msm_dsi_unregister(void)
 int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 			 struct drm_encoder *encoder)
 {
-	struct msm_drm_private *priv;
+	struct msm_drm_private *priv = dev->dev_private;
 	int ret;
 
-	if (WARN_ON(!encoder) || WARN_ON(!msm_dsi) || WARN_ON(!dev))
-		return -EINVAL;
-
-	priv = dev->dev_private;
-
 	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
 		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
 		return -ENOSPC;
-- 
2.35.1

Re: [PATCH 0/7] drm/msm: probe deferral fixes

[Abhinav Kumar]

Adding kuogee to this series

Hi Johan

Thanks for posting this.

We will take a look at this, re-validate and give our reviews/tested-bys.

Thanks

Abhinav
On 9/12/2022 8:40 AM, Johan Hovold wrote:
> The MSM DRM is currently broken in multiple ways with respect to probe
> deferral. Not only does the driver currently fail to probe again after a
> late deferral, but due to a related use-after-free bug this also
> triggers NULL-pointer dereferences.
> 
> These bugs are not new but have become critical with the release of
> 5.19 where probe is deferred in case the aux-bus EP panel driver has not
> yet been loaded.
> 
> The underlying problem is lifetime issues due to careless use of
> device-managed resources.
> 
> Specifically, device-managed resources allocated post component bind
> must be tied to the lifetime of the aggregate DRM device or they will
> not necessarily be released when binding of the aggregate device is
> deferred.
> 
> The following call chain and pseudo code serves as an illustration of
> the problem:
> 
>   - platform_probe(pdev1)
>     - dp_display_probe()
>       - component_add()
> 
>   - platform_probe(pdev2) 				// last component
>     - dp_display_probe()					// d0
>         - component_add()
>           - try_to_bring_up_aggregate_device()
> 	   - devres_open_group(adev->parent)		// d1
> 
> 	   - msm_drm_bind()
> 	     - msm_drm_init()
> 	       - component_bind_all()
> 	         - for_each_component()
> 		   - component_bind()
> 		     - devres_open_group(&pdev->dev)	// d2
> 	             - dp_display_bind()
> 		       - devm_kzalloc(&pdev->dev)	// a1, OK
> 		     - devres_close_group(&pdev->dev)	// d3
> 
> 	       - dpu_kms_hw_init()
> 	         - for_each_panel()
> 	           - msm_dp_modeset_init()
> 		     - dp_display_request_irq()
> 		       - devm_request_irq(&pdev->dev)	// a2, BUG
> 		     - if (pdev == pdev2 && condition)
> 		       - return -EPROBE_DEFER;
> 
> 	      - if (error)
>   	        - component_unbind_all()
> 	          - for_each_component()
> 		    - component_unbind()
> 		      - dp_display_unbind()
> 		      - devres_release_group(&pdev->dev) // d4, only a1 is freed
> 
>             - if (error)
> 	     - devres_release_group(adev->parent)	// d5
> 
> The device-managed allocation a2 is buggy as its lifetime is tied to the
> component platform device and will not be released when the aggregate
> device bind fails (e.g. due to a probe deferral).
> 
> When pdev2 is later probed again, the attempt to allocate the IRQ a
> second time will fail for pdev1 (which is still bound to its platform
> driver).
> 
> This series fixes the lifetime issues by tying the lifetime of a2 (and
> similar allocations) to the lifetime of the aggregate device so that a2
> is released at d5.
> 
> In some cases, such has for the DP IRQ, the above situation can also be
> avoided by moving the allocation in question to the platform driver
> probe (d0) or component bind (between d2 and d3). But as doing so is not
> a general fix, this can be done later as a cleanup/optimisation.
> 
> Johan
> 
> 
> Johan Hovold (7):
>    drm/msm: fix use-after-free on probe deferral
>    drm/msm: fix memory corruption with too many bridges
>    drm/msm/dp: fix IRQ lifetime
>    drm/msm/dp: fix aux-bus EP lifetime
>    drm/msm/dp: fix bridge lifetime
>    drm/msm/hdmi: fix IRQ lifetime
>    drm/msm: drop modeset sanity checks
> 
>   drivers/gpu/drm/bridge/parade-ps8640.c   |  2 +-
>   drivers/gpu/drm/display/drm_dp_aux_bus.c |  5 +++--
>   drivers/gpu/drm/msm/dp/dp_display.c      | 16 +++++++++-------
>   drivers/gpu/drm/msm/dp/dp_parser.c       |  6 +++---
>   drivers/gpu/drm/msm/dp/dp_parser.h       |  5 +++--
>   drivers/gpu/drm/msm/dsi/dsi.c            |  9 +++++----
>   drivers/gpu/drm/msm/hdmi/hdmi.c          |  7 ++++++-
>   drivers/gpu/drm/msm/msm_drv.c            |  1 +
>   include/drm/display/drm_dp_aux_bus.h     |  6 +++---
>   9 files changed, 34 insertions(+), 23 deletions(-)
> 

Re: [PATCH 1/7] drm/msm: fix use-after-free on probe deferral

[Dmitry Baryshkov]

On 12/09/2022 18:40, Johan Hovold wrote:
> The bridge counter was never reset when tearing down the DRM device so
> that stale pointers to deallocated structures would be accessed on the
> next tear down (e.g. after a second late bind deferral).
> 
> Given enough bridges and a few probe deferrals this could currently also
> lead to data beyond the bridge array being corrupted.
> 
> Fixes: d28ea556267c ("drm/msm: properly add and remove internal bridges")
> Cc: stable@vger.kernel.org      # 5.19

Fixes: a3376e3ec81c ("drm/msm: convert to drm_bridge")
Cc: stable@vger.kernel.org # 3.12

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

> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 391d86b54ded..d254fe2507ec 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -241,6 +241,7 @@ static int msm_drm_uninit(struct device *dev)
>   
>   	for (i = 0; i < priv->num_bridges; i++)
>   		drm_bridge_remove(priv->bridges[i]);
> +	priv->num_bridges = 0;
>   
>   	pm_runtime_get_sync(dev);
>   	msm_irq_uninstall(ddev);

-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm: fix memory corruption with too many bridges

[Dmitry Baryshkov]

On 12/09/2022 18:40, Johan Hovold wrote:
> Add the missing sanity checks on the bridge counter to avoid corrupting
> data beyond the fixed-sized bridge array in case there are ever more
> than eight bridges.
> 
> a3376e3ec81c ("drm/msm: convert to drm_bridge")
> ab5b0107ccf3 ("drm/msm: Initial add eDP support in msm drm driver (v5)")
> a689554ba6ed ("drm/msm: Initial add DSI connector support")

Most probably you've missed the Fixes: here.

> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable")
> Cc: stable@vger.kernel.org	# 3.12
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++
>   drivers/gpu/drm/msm/dsi/dsi.c       | 6 ++++++
>   drivers/gpu/drm/msm/hdmi/hdmi.c     | 5 +++++

Could you please split this into respective dp/dsi/hdmi patches. This 
will assist both the review and the stable team.

Otherwise LGTM.

>   3 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 3e284fed8d30..fbe950edaefe 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1604,6 +1604,12 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>   		return -EINVAL;
>   
>   	priv = dev->dev_private;
> +
> +	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
> +		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
> +		return -ENOSPC;
> +	}
> +
>   	dp_display->drm_dev = dev;
>   
>   	dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 39bbabb5daf6..8a95c744972a 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -218,6 +218,12 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>   		return -EINVAL;
>   
>   	priv = dev->dev_private;
> +
> +	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
> +		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
> +		return -ENOSPC;
> +	}
> +
>   	msm_dsi->dev = dev;
>   
>   	ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 93fe61b86967..a0ed6aa8e4e1 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -300,6 +300,11 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   	struct platform_device *pdev = hdmi->pdev;
>   	int ret;
>   
> +	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
> +		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
> +		return -ENOSPC;
> +	}
> +
>   	hdmi->dev = dev;
>   	hdmi->encoder = encoder;
>   

-- 
With best wishes
Dmitry

Re: [PATCH 6/7] drm/msm/hdmi: fix IRQ lifetime

[Dmitry Baryshkov]

On 12/09/2022 18:40, Johan Hovold wrote:
> Device-managed resources allocated post component bind must be tied to
> the lifetime of the aggregate DRM device or they will not necessarily be
> released when binding of the aggregate device is deferred.
> 
> This is specifically true for the HDMI IRQ, which will otherwise remain
> requested so that the next bind attempt fails when requesting the IRQ a
> second time.
> 
> Fix this by tying the device-managed lifetime of the HDMI IRQ to the DRM
> device so that it is released when bind fails.
> 
> Fixes: 067fef372c73 ("drm/msm/hdmi: refactor bind/init")
> Cc: stable@vger.kernel.org      # 3.19
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

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

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index a0ed6aa8e4e1..f28fb21e3891 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -344,7 +344,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   		goto fail;
>   	}
>   
> -	ret = devm_request_irq(&pdev->dev, hdmi->irq,
> +	ret = devm_request_irq(dev->dev, hdmi->irq,
>   			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>   			"hdmi_isr", hdmi);
>   	if (ret < 0) {

-- 
With best wishes
Dmitry

Re: [PATCH 7/7] drm/msm: drop modeset sanity checks

[Dmitry Baryshkov]

On 12/09/2022 18:40, Johan Hovold wrote:
> Drop the overly defensive modeset sanity checks of function parameters
> which have already been checked or used by the callers.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Again, please split into dp and dsi patches. After that:

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

> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 7 +------
>   drivers/gpu/drm/msm/dsi/dsi.c       | 7 +------
>   2 files changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 393af1ea9ed8..8ad28bf81abe 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1597,15 +1597,10 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>   			struct drm_encoder *encoder)
>   {
> -	struct msm_drm_private *priv;
> +	struct msm_drm_private *priv = dev->dev_private;
>   	struct dp_display_private *dp_priv;
>   	int ret;
>   
> -	if (WARN_ON(!encoder) || WARN_ON(!dp_display) || WARN_ON(!dev))
> -		return -EINVAL;
> -
> -	priv = dev->dev_private;
> -
>   	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>   		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>   		return -ENOSPC;
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index 8a95c744972a..31fdee2052be 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -211,14 +211,9 @@ void __exit msm_dsi_unregister(void)
>   int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>   			 struct drm_encoder *encoder)
>   {
> -	struct msm_drm_private *priv;
> +	struct msm_drm_private *priv = dev->dev_private;
>   	int ret;
>   
> -	if (WARN_ON(!encoder) || WARN_ON(!msm_dsi) || WARN_ON(!dev))
> -		return -EINVAL;
> -
> -	priv = dev->dev_private;
> -
>   	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>   		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>   		return -ENOSPC;

-- 
With best wishes
Dmitry

Re: [PATCH 3/7] drm/msm/dp: fix IRQ lifetime

[Dmitry Baryshkov]

On 12/09/2022 18:40, Johan Hovold wrote:
> Device-managed resources allocated post component bind must be tied to
> the lifetime of the aggregate DRM device or they will not necessarily be
> released when binding of the aggregate device is deferred.
> 
> This is specifically true for the DP IRQ, which will otherwise remain
> requested so that the next bind attempt fails when requesting the IRQ a
> second time.
> 
> Since commit c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
> this can happen when the aux-bus panel driver has not yet been loaded so
> that probe is deferred.
> 
> Fix this by tying the device-managed lifetime of the DP IRQ to the DRM
> device so that it is released when bind fails.
> 
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Cc: stable@vger.kernel.org      # 5.10
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

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

> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index fbe950edaefe..ba557328710a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1258,7 +1258,7 @@ int dp_display_request_irq(struct msm_dp *dp_display)
>   		return -EINVAL;
>   	}
>   
> -	rc = devm_request_irq(&dp->pdev->dev, dp->irq,
> +	rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
>   			dp_display_irq_handler,
>   			IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
>   	if (rc < 0) {

-- 
With best wishes
Dmitry

Re: [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[Dmitry Baryshkov]

On 12/09/2022 18:40, Johan Hovold wrote:
> Device-managed resources allocated post component bind must be tied to
> the lifetime of the aggregate DRM device or they will not necessarily be
> released when binding of the aggregate device is deferred.
> 
> This can lead resource leaks or failure to bind the aggregate device
> when binding is later retried and a second attempt to allocate the
> resources is made.
> 
> For the DP aux-bus, an attempt to populate the bus a second time will
> simply fail ("DP AUX EP device already populated").
> 
> Fix this by amending the DP aux interface and tying the lifetime of the
> EP device to the DRM device rather than DP controller platform device.

Doug, could you please take a look?

For me this is another reminder/pressure point that we should populate 
the AUX BUS from the probe(), before binding the components together.

> 
> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
> Cc: stable@vger.kernel.org      # 5.19
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/gpu/drm/bridge/parade-ps8640.c   | 2 +-
>   drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
>   drivers/gpu/drm/msm/dp/dp_display.c      | 3 ++-
>   include/drm/display/drm_dp_aux_bus.h     | 6 +++---
>   4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index d7483c13c569..6127979370cb 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -719,7 +719,7 @@ static int ps8640_probe(struct i2c_client *client)
>   	if (ret)
>   		return ret;
>   
> -	ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
> +	ret = devm_of_dp_aux_populate_bus(dev, &ps_bridge->aux, ps8640_bridge_link_panel);
>   
>   	/*
>   	 * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> index f5741b45ca07..2706f2cf82f7 100644
> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
> @@ -322,6 +322,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
>   
>   /**
>    * devm_of_dp_aux_populate_bus() - devm wrapper for of_dp_aux_populate_bus()
> + * @dev: Device to tie the lifetime of the EP devices to
>    * @aux: The AUX channel whose device we want to populate
>    * @done_probing: Callback functions to call after EP device finishes probing.
>    *                Will not be called if there are no EP devices and this
> @@ -333,7 +334,7 @@ static void of_dp_aux_depopulate_bus_void(void *data)
>    *         no children. The done_probing() function won't be called in that
>    *         case.
>    */
> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> +int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
>   				int (*done_probing)(struct drm_dp_aux *aux))
>   {
>   	int ret;
> @@ -342,7 +343,7 @@ int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>   	if (ret)
>   		return ret;
>   
> -	return devm_add_action_or_reset(aux->dev,
> +	return devm_add_action_or_reset(dev,
>   					of_dp_aux_depopulate_bus_void, aux);
>   }
>   EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ba557328710a..e1aa6355bbf6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1559,7 +1559,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>   		 * panel driver is probed asynchronously but is the best we
>   		 * can do without a bigger driver reorganization.
>   		 */
> -		rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> +		rc = devm_of_dp_aux_populate_ep_devices(dp->drm_dev->dev,
> +							dp_priv->aux);
>   		of_node_put(aux_bus);
>   		if (rc)
>   			goto error;
> diff --git a/include/drm/display/drm_dp_aux_bus.h b/include/drm/display/drm_dp_aux_bus.h
> index 8a0a486383c5..a4063aa7fc40 100644
> --- a/include/drm/display/drm_dp_aux_bus.h
> +++ b/include/drm/display/drm_dp_aux_bus.h
> @@ -47,7 +47,7 @@ static inline struct dp_aux_ep_driver *to_dp_aux_ep_drv(struct device_driver *dr
>   int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>   			   int (*done_probing)(struct drm_dp_aux *aux));
>   void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
> +int devm_of_dp_aux_populate_bus(struct device *dev, struct drm_dp_aux *aux,
>   				int (*done_probing)(struct drm_dp_aux *aux));
>   
>   /* Deprecated versions of the above functions. To be removed when no callers. */
> @@ -61,11 +61,11 @@ static inline int of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
>   	return (ret != -ENODEV) ? ret : 0;
>   }
>   
> -static inline int devm_of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
> +static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
>   {
>   	int ret;
>   
> -	ret = devm_of_dp_aux_populate_bus(aux, NULL);
> +	ret = devm_of_dp_aux_populate_bus(dev, aux, NULL);
>   
>   	/* New API returns -ENODEV for no child case; adapt to old assumption */
>   	return (ret != -ENODEV) ? ret : 0;

-- 
With best wishes
Dmitry

Re: [PATCH 5/7] drm/msm/dp: fix bridge lifetime

[Dmitry Baryshkov]

On 12/09/2022 18:40, Johan Hovold wrote:
> Device-managed resources allocated post component bind must be tied to
> the lifetime of the aggregate DRM device or they will not necessarily be
> released when binding of the aggregate device is deferred.
> 
> This can lead resource leaks or failure to bind the aggregate device
> when binding is later retried and a second attempt to allocate the
> resources is made.
> 
> For the DP bridges, previously allocated bridges will leak on probe
> deferral.
> 
> Fix this by amending the DP parser interface and tying the lifetime of
> the bridge device to the DRM device rather than DP platform device.
> 
> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
> Cc: stable@vger.kernel.org      # 5.19
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Seems logical enough.

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

> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
>   drivers/gpu/drm/msm/dp/dp_parser.c  | 6 +++---
>   drivers/gpu/drm/msm/dp/dp_parser.h  | 5 +++--
>   3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e1aa6355bbf6..393af1ea9ed8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1576,7 +1576,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>   	 * For DisplayPort interfaces external bridges are optional, so
>   	 * silently ignore an error if one is not present (-ENODEV).
>   	 */
> -	rc = dp_parser_find_next_bridge(dp_priv->parser);
> +	rc = devm_dp_parser_find_next_bridge(dp->drm_dev->dev, dp_priv->parser);
>   	if (!dp->is_edp && rc == -ENODEV)
>   		return 0;
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index dd732215d55b..dcbe893d66d7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -240,12 +240,12 @@ static int dp_parser_clock(struct dp_parser *parser)
>   	return 0;
>   }
>   
> -int dp_parser_find_next_bridge(struct dp_parser *parser)
> +int devm_dp_parser_find_next_bridge(struct device *dev, struct dp_parser *parser)
>   {
> -	struct device *dev = &parser->pdev->dev;
> +	struct platform_device *pdev = parser->pdev;
>   	struct drm_bridge *bridge;
>   
> -	bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> +	bridge = devm_drm_of_get_bridge(dev, pdev->dev.of_node, 1, 0);
>   	if (IS_ERR(bridge))
>   		return PTR_ERR(bridge);
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 866c1a82bf1a..d30ab773db46 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -138,8 +138,9 @@ struct dp_parser {
>   struct dp_parser *dp_parser_get(struct platform_device *pdev);
>   
>   /**
> - * dp_parser_find_next_bridge() - find an additional bridge to DP
> + * devm_dp_parser_find_next_bridge() - find an additional bridge to DP
>    *
> + * @dev: device to tie bridge lifetime to
>    * @parser: dp_parser data from client
>    *
>    * This function is used to find any additional bridge attached to
> @@ -147,6 +148,6 @@ struct dp_parser *dp_parser_get(struct platform_device *pdev);
>    *
>    * Return: 0 if able to get the bridge, otherwise negative errno for failure.
>    */
> -int dp_parser_find_next_bridge(struct dp_parser *parser);
> +int devm_dp_parser_find_next_bridge(struct device *dev, struct dp_parser *parser);
>   
>   #endif

-- 
With best wishes
Dmitry

Re: [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[Steev Klimaszewski]

On 9/12/22 1:10 PM, Dmitry Baryshkov wrote:
> On 12/09/2022 18:40, Johan Hovold wrote:
>> Device-managed resources allocated post component bind must be tied to
>> the lifetime of the aggregate DRM device or they will not necessarily be
>> released when binding of the aggregate device is deferred.
>>
>> This can lead resource leaks or failure to bind the aggregate device
>> when binding is later retried and a second attempt to allocate the
>> resources is made.
>>
>> For the DP aux-bus, an attempt to populate the bus a second time will
>> simply fail ("DP AUX EP device already populated").
>>
>> Fix this by amending the DP aux interface and tying the lifetime of the
>> EP device to the DRM device rather than DP controller platform device.
>
> Doug, could you please take a look?
>
> For me this is another reminder/pressure point that we should populate 
> the AUX BUS from the probe(), before binding the components together.
>
>>
>> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
>> Cc: stable@vger.kernel.org      # 5.19
>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>> ---
>>   drivers/gpu/drm/bridge/parade-ps8640.c   | 2 +-
>>   drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
>>   drivers/gpu/drm/msm/dp/dp_display.c      | 3 ++-
>>   include/drm/display/drm_dp_aux_bus.h     | 6 +++---
>>   4 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c 
>> b/drivers/gpu/drm/bridge/parade-ps8640.c
>> index d7483c13c569..6127979370cb 100644
>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>> @@ -719,7 +719,7 @@ static int ps8640_probe(struct i2c_client *client)
>>       if (ret)
>>           return ret;
>>   -    ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, 
>> ps8640_bridge_link_panel);
>> +    ret = devm_of_dp_aux_populate_bus(dev, &ps_bridge->aux, 
>> ps8640_bridge_link_panel);
>>         /*
>>        * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's 
>> up to
>> diff --git a/drivers/gpu/drm/display/drm_dp_aux_bus.c 
>> b/drivers/gpu/drm/display/drm_dp_aux_bus.c
>> index f5741b45ca07..2706f2cf82f7 100644
>> --- a/drivers/gpu/drm/display/drm_dp_aux_bus.c
>> +++ b/drivers/gpu/drm/display/drm_dp_aux_bus.c
>> @@ -322,6 +322,7 @@ static void of_dp_aux_depopulate_bus_void(void 
>> *data)
>>     /**
>>    * devm_of_dp_aux_populate_bus() - devm wrapper for 
>> of_dp_aux_populate_bus()
>> + * @dev: Device to tie the lifetime of the EP devices to
>>    * @aux: The AUX channel whose device we want to populate
>>    * @done_probing: Callback functions to call after EP device 
>> finishes probing.
>>    *                Will not be called if there are no EP devices and 
>> this
>> @@ -333,7 +334,7 @@ static void of_dp_aux_depopulate_bus_void(void 
>> *data)
>>    *         no children. The done_probing() function won't be called 
>> in that
>>    *         case.
>>    */
>> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>> +int devm_of_dp_aux_populate_bus(struct device *dev, struct 
>> drm_dp_aux *aux,
>>                   int (*done_probing)(struct drm_dp_aux *aux))
>>   {
>>       int ret;
>> @@ -342,7 +343,7 @@ int devm_of_dp_aux_populate_bus(struct drm_dp_aux 
>> *aux,
>>       if (ret)
>>           return ret;
>>   -    return devm_add_action_or_reset(aux->dev,
>> +    return devm_add_action_or_reset(dev,
>>                       of_dp_aux_depopulate_bus_void, aux);
>>   }
>>   EXPORT_SYMBOL_GPL(devm_of_dp_aux_populate_bus);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index ba557328710a..e1aa6355bbf6 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1559,7 +1559,8 @@ static int dp_display_get_next_bridge(struct 
>> msm_dp *dp)
>>            * panel driver is probed asynchronously but is the best we
>>            * can do without a bigger driver reorganization.
>>            */
>> -        rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> +        rc = devm_of_dp_aux_populate_ep_devices(dp->drm_dev->dev,
>> +                            dp_priv->aux);
>>           of_node_put(aux_bus);
>>           if (rc)
>>               goto error;
>> diff --git a/include/drm/display/drm_dp_aux_bus.h 
>> b/include/drm/display/drm_dp_aux_bus.h
>> index 8a0a486383c5..a4063aa7fc40 100644
>> --- a/include/drm/display/drm_dp_aux_bus.h
>> +++ b/include/drm/display/drm_dp_aux_bus.h
>> @@ -47,7 +47,7 @@ static inline struct dp_aux_ep_driver 
>> *to_dp_aux_ep_drv(struct device_driver *dr
>>   int of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>>                  int (*done_probing)(struct drm_dp_aux *aux));
>>   void of_dp_aux_depopulate_bus(struct drm_dp_aux *aux);
>> -int devm_of_dp_aux_populate_bus(struct drm_dp_aux *aux,
>> +int devm_of_dp_aux_populate_bus(struct device *dev, struct 
>> drm_dp_aux *aux,
>>                   int (*done_probing)(struct drm_dp_aux *aux));
>>     /* Deprecated versions of the above functions. To be removed when 
>> no callers. */
>> @@ -61,11 +61,11 @@ static inline int 
>> of_dp_aux_populate_ep_devices(struct drm_dp_aux *aux)
>>       return (ret != -ENODEV) ? ret : 0;
>>   }
>>   -static inline int devm_of_dp_aux_populate_ep_devices(struct 
>> drm_dp_aux *aux)
>> +static inline int devm_of_dp_aux_populate_ep_devices(struct device 
>> *dev, struct drm_dp_aux *aux)
>>   {
>>       int ret;
>>   -    ret = devm_of_dp_aux_populate_bus(aux, NULL);
>> +    ret = devm_of_dp_aux_populate_bus(dev, aux, NULL);
>>         /* New API returns -ENODEV for no child case; adapt to old 
>> assumption */
>>       return (ret != -ENODEV) ? ret : 0;
>
This breaks builds which have ti-sn65dsi86 included:

drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:50: error: passing argument 1 
of 'devm_of_dp_aux_populate_ep_devices' from incompatible argument type.

As well,

drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:15: error: too few arguments 
to function 'devm_of_dp_aux_populate_ep_devices'


--steev

Re: [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[kernel test robot]

Hi Johan,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20220912]
[also build test ERROR on v6.0-rc5]
[cannot apply to drm-misc/drm-misc-next drm/drm-next drm-intel/for-linux-next drm-tip/drm-tip linus/master v6.0-rc5 v6.0-rc4 v6.0-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/drm-msm-probe-deferral-fixes/20220912-234351
base:    044b771be9c5de9d817dfafb829d2f049c71c3b4
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220913/202209130930.yrI8pQGL-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/458c96e19570036b3dd6e48d91f0bf6f67b996fa
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Johan-Hovold/drm-msm-probe-deferral-fixes/20220912-234351
        git checkout 458c96e19570036b3dd6e48d91f0bf6f67b996fa
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/bridge/ti-sn65dsi86.c: In function 'ti_sn_aux_probe':
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:632:50: error: passing argument 1 of 'devm_of_dp_aux_populate_ep_devices' from incompatible pointer type [-Werror=incompatible-pointer-types]
     632 |         ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
         |                                                  ^~~~~~~~~~~
         |                                                  |
         |                                                  struct drm_dp_aux *
   In file included from drivers/gpu/drm/bridge/ti-sn65dsi86.c:26:
   include/drm/display/drm_dp_aux_bus.h:64:69: note: expected 'struct device *' but argument is of type 'struct drm_dp_aux *'
      64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
         |                                                      ~~~~~~~~~~~~~~~^~~
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:632:15: error: too few arguments to function 'devm_of_dp_aux_populate_ep_devices'
     632 |         ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/drm/display/drm_dp_aux_bus.h:64:19: note: declared here
      64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   drivers/gpu/drm/bridge/analogix/anx7625.c: In function 'anx7625_i2c_probe':
>> drivers/gpu/drm/bridge/analogix/anx7625.c:2654:44: error: passing argument 1 of 'devm_of_dp_aux_populate_ep_devices' from incompatible pointer type [-Werror=incompatible-pointer-types]
    2654 |         devm_of_dp_aux_populate_ep_devices(&platform->aux);
         |                                            ^~~~~~~~~~~~~~
         |                                            |
         |                                            struct drm_dp_aux *
   In file included from drivers/gpu/drm/bridge/analogix/anx7625.c:24:
   include/drm/display/drm_dp_aux_bus.h:64:69: note: expected 'struct device *' but argument is of type 'struct drm_dp_aux *'
      64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
         |                                                      ~~~~~~~~~~~~~~~^~~
>> drivers/gpu/drm/bridge/analogix/anx7625.c:2654:9: error: too few arguments to function 'devm_of_dp_aux_populate_ep_devices'
    2654 |         devm_of_dp_aux_populate_ep_devices(&platform->aux);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/drm/display/drm_dp_aux_bus.h:64:19: note: declared here
      64 | static inline int devm_of_dp_aux_populate_ep_devices(struct device *dev, struct drm_dp_aux *aux)
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/devm_of_dp_aux_populate_ep_devices +632 drivers/gpu/drm/bridge/ti-sn65dsi86.c

77674e722f4b27 Laurent Pinchart 2021-06-24  620  
77674e722f4b27 Laurent Pinchart 2021-06-24  621  static int ti_sn_aux_probe(struct auxiliary_device *adev,
77674e722f4b27 Laurent Pinchart 2021-06-24  622  			   const struct auxiliary_device_id *id)
77674e722f4b27 Laurent Pinchart 2021-06-24  623  {
77674e722f4b27 Laurent Pinchart 2021-06-24  624  	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
77674e722f4b27 Laurent Pinchart 2021-06-24  625  	int ret;
77674e722f4b27 Laurent Pinchart 2021-06-24  626  
77674e722f4b27 Laurent Pinchart 2021-06-24  627  	pdata->aux.name = "ti-sn65dsi86-aux";
77674e722f4b27 Laurent Pinchart 2021-06-24  628  	pdata->aux.dev = &adev->dev;
77674e722f4b27 Laurent Pinchart 2021-06-24  629  	pdata->aux.transfer = ti_sn_aux_transfer;
77674e722f4b27 Laurent Pinchart 2021-06-24  630  	drm_dp_aux_init(&pdata->aux);
77674e722f4b27 Laurent Pinchart 2021-06-24  631  
77674e722f4b27 Laurent Pinchart 2021-06-24 @632  	ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
77674e722f4b27 Laurent Pinchart 2021-06-24  633  	if (ret)
77674e722f4b27 Laurent Pinchart 2021-06-24  634  		return ret;
77674e722f4b27 Laurent Pinchart 2021-06-24  635  
77674e722f4b27 Laurent Pinchart 2021-06-24  636  	/*
77674e722f4b27 Laurent Pinchart 2021-06-24  637  	 * The eDP to MIPI bridge parts don't work until the AUX channel is
77674e722f4b27 Laurent Pinchart 2021-06-24  638  	 * setup so we don't add it in the main driver probe, we add it now.
77674e722f4b27 Laurent Pinchart 2021-06-24  639  	 */
77674e722f4b27 Laurent Pinchart 2021-06-24  640  	return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
77674e722f4b27 Laurent Pinchart 2021-06-24  641  }
77674e722f4b27 Laurent Pinchart 2021-06-24  642  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[Doug Anderson]

Hi,

On Mon, Sep 12, 2022 at 7:10 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 12/09/2022 18:40, Johan Hovold wrote:
> > Device-managed resources allocated post component bind must be tied to
> > the lifetime of the aggregate DRM device or they will not necessarily be
> > released when binding of the aggregate device is deferred.
> >
> > This can lead resource leaks or failure to bind the aggregate device
> > when binding is later retried and a second attempt to allocate the
> > resources is made.
> >
> > For the DP aux-bus, an attempt to populate the bus a second time will
> > simply fail ("DP AUX EP device already populated").
> >
> > Fix this by amending the DP aux interface and tying the lifetime of the
> > EP device to the DRM device rather than DP controller platform device.
>
> Doug, could you please take a look?
>
> For me this is another reminder/pressure point that we should populate
> the AUX BUS from the probe(), before binding the components together.

Aside from the kernel robot complaints, I'm not necessarily convinced.
I think we know that the AUX DP stuff in MSM-DP is fragile right now
and Qualcomm has promised to clean it up. This really feels like a
band-aid and is really a sign that we're populating the AUX DP bus in
the wrong place in Qualcomm's code. As you said, if we moved this to
probe(), which is the plan in the promised cleanup, then it wouldn't
be a problem.

As far as I know Qualcomm has queued this cleanup behind their current
PSR work (though it's never been clear why both can't be worked on at
the same time) and the PSR work was stalled because they couldn't
figure out what caused the glitching I reported. It's still on my nag
list that I bring up with them every week...

In any case, if a band-aid is urgent, maybe you could just call
of_dp_aux_populate_bus() directly in Qualcomm code and you could add
your own devm_add_action_or_reset() on the DRM device.

Re: [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[Johan Hovold]

On Tue, Sep 13, 2022 at 07:35:15AM +0100, Doug Anderson wrote:
> Hi,
> 
> On Mon, Sep 12, 2022 at 7:10 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On 12/09/2022 18:40, Johan Hovold wrote:
> > > Device-managed resources allocated post component bind must be tied to
> > > the lifetime of the aggregate DRM device or they will not necessarily be
> > > released when binding of the aggregate device is deferred.
> > >
> > > This can lead resource leaks or failure to bind the aggregate device
> > > when binding is later retried and a second attempt to allocate the
> > > resources is made.
> > >
> > > For the DP aux-bus, an attempt to populate the bus a second time will
> > > simply fail ("DP AUX EP device already populated").
> > >
> > > Fix this by amending the DP aux interface and tying the lifetime of the
> > > EP device to the DRM device rather than DP controller platform device.
> >
> > Doug, could you please take a look?
> >
> > For me this is another reminder/pressure point that we should populate
> > the AUX BUS from the probe(), before binding the components together.
> 
> Aside from the kernel robot complaints, I'm not necessarily convinced.
> I think we know that the AUX DP stuff in MSM-DP is fragile right now
> and Qualcomm has promised to clean it up. This really feels like a
> band-aid and is really a sign that we're populating the AUX DP bus in
> the wrong place in Qualcomm's code. As you said, if we moved this to
> probe(), which is the plan in the promised cleanup, then it wouldn't
> be a problem.

Right, but that appears to be non-trivial judging from the discussions
you had back when the offending patch was merged:

	https://lore.kernel.org/lkml/CAD=FV=X+QvjwoT2zGP82KW4kD0oMUY6ZgCizSikNX_Uj8dNDqA@mail.gmail.com/t/#u

> As far as I know Qualcomm has queued this cleanup behind their current
> PSR work (though it's never been clear why both can't be worked on at
> the same time) and the PSR work was stalled because they couldn't
> figure out what caused the glitching I reported. It's still on my nag
> list that I bring up with them every week...
> 
> In any case, if a band-aid is urgent, maybe you could just call
> of_dp_aux_populate_bus() directly in Qualcomm code and you could add
> your own devm_add_action_or_reset() on the DRM device.

Yeah, that's probably better. I apparently missed a bunch of users of
devm_of_dp_aux_populate_ep_devices() after searching for
devm_of_dp_aux_populate_bus() instead. Judging from a quick glance all
of these populate the bus at probe, so Qualcomm indeed appears to be
the odd bird here.

But the bug is real, in mainline and needs to be fixed, so rolling a
custom devm action indeed should to be the right thing to do here (if
only to have a smaller fix).

Johan

Re: [PATCH 4/7] drm/msm/dp: fix aux-bus EP lifetime

[Johan Hovold]

On Mon, Sep 12, 2022 at 04:55:58PM -0500, Steev Klimaszewski wrote:
> 
> On 9/12/22 1:10 PM, Dmitry Baryshkov wrote:
> > On 12/09/2022 18:40, Johan Hovold wrote:
> >> Device-managed resources allocated post component bind must be tied to
> >> the lifetime of the aggregate DRM device or they will not necessarily be
> >> released when binding of the aggregate device is deferred.
> >>
> >> This can lead resource leaks or failure to bind the aggregate device
> >> when binding is later retried and a second attempt to allocate the
> >> resources is made.
> >>
> >> For the DP aux-bus, an attempt to populate the bus a second time will
> >> simply fail ("DP AUX EP device already populated").
> >>
> >> Fix this by amending the DP aux interface and tying the lifetime of the
> >> EP device to the DRM device rather than DP controller platform device.
> >
> > Doug, could you please take a look?
> >
> > For me this is another reminder/pressure point that we should populate 
> > the AUX BUS from the probe(), before binding the components together.
> >
> >>
> >> Fixes: c3bf8e21b38a ("drm/msm/dp: Add eDP support via aux_bus")
> >> Cc: stable@vger.kernel.org      # 5.19
> >> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >> ---
> >>   drivers/gpu/drm/bridge/parade-ps8640.c   | 2 +-
> >>   drivers/gpu/drm/display/drm_dp_aux_bus.c | 5 +++--
> >>   drivers/gpu/drm/msm/dp/dp_display.c      | 3 ++-
> >>   include/drm/display/drm_dp_aux_bus.h     | 6 +++---
> >>   4 files changed, 9 insertions(+), 7 deletions(-)

> This breaks builds which have ti-sn65dsi86 included:
> 
> drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:50: error: passing argument 1 
> of 'devm_of_dp_aux_populate_ep_devices' from incompatible argument type.
> 
> As well,
> 
> drivers/gpu/drm/bridge/ti-sn65dsi86.c:628:15: error: too few arguments 
> to function 'devm_of_dp_aux_populate_ep_devices'

Thanks for reporting this. I messed up and apparently only grepped for
devm_of_dp_aux_populate_bus() and not the
devm_of_dp_aux_populate_ep_devices() wrapper when searching for users.

Johan

Re: [PATCH 1/7] drm/msm: fix use-after-free on probe deferral

[Johan Hovold]

On Mon, Sep 12, 2022 at 08:52:44PM +0300, Dmitry Baryshkov wrote:
> On 12/09/2022 18:40, Johan Hovold wrote:
> > The bridge counter was never reset when tearing down the DRM device so
> > that stale pointers to deallocated structures would be accessed on the
> > next tear down (e.g. after a second late bind deferral).
> > 
> > Given enough bridges and a few probe deferrals this could currently also
> > lead to data beyond the bridge array being corrupted.
> > 
> > Fixes: d28ea556267c ("drm/msm: properly add and remove internal bridges")
> > Cc: stable@vger.kernel.org      # 5.19
> 
> Fixes: a3376e3ec81c ("drm/msm: convert to drm_bridge")
> Cc: stable@vger.kernel.org # 3.12

The use after free was introduced in 5.19, and the next patch takes care
of the possible overflow of the bridges array that has been around since
3.12.

But sure, this oversight has been there since 3.12. I'll reconsider
adding the other Fixes tag. The stable team struggles with context
changes apparently so not sure it's worth backporting, though.

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/gpu/drm/msm/msm_drv.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 391d86b54ded..d254fe2507ec 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -241,6 +241,7 @@ static int msm_drm_uninit(struct device *dev)
> >   
> >   	for (i = 0; i < priv->num_bridges; i++)
> >   		drm_bridge_remove(priv->bridges[i]);
> > +	priv->num_bridges = 0;
> >   
> >   	pm_runtime_get_sync(dev);
> >   	msm_irq_uninstall(ddev);

Johan

Re: [PATCH 2/7] drm/msm: fix memory corruption with too many bridges

[Johan Hovold]

On Mon, Sep 12, 2022 at 08:55:55PM +0300, Dmitry Baryshkov wrote:
> On 12/09/2022 18:40, Johan Hovold wrote:
> > Add the missing sanity checks on the bridge counter to avoid corrupting
> > data beyond the fixed-sized bridge array in case there are ever more
> > than eight bridges.
> > 
> > a3376e3ec81c ("drm/msm: convert to drm_bridge")
> > ab5b0107ccf3 ("drm/msm: Initial add eDP support in msm drm driver (v5)")
> > a689554ba6ed ("drm/msm: Initial add DSI connector support")
> 
> Most probably you've missed the Fixes: here.

Indeed, thanks for catching that.

> > Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable")
> > Cc: stable@vger.kernel.org	# 3.12
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++
> >   drivers/gpu/drm/msm/dsi/dsi.c       | 6 ++++++
> >   drivers/gpu/drm/msm/hdmi/hdmi.c     | 5 +++++
> 
> Could you please split this into respective dp/dsi/hdmi patches. This 
> will assist both the review and the stable team.

Yeah, you're right, that should help the stable team if nothing else.

> Otherwise LGTM.

Johan

Re: [PATCH 7/7] drm/msm: drop modeset sanity checks

[Johan Hovold]

On Mon, Sep 12, 2022 at 09:06:28PM +0300, Dmitry Baryshkov wrote:
> On 12/09/2022 18:40, Johan Hovold wrote:
> > Drop the overly defensive modeset sanity checks of function parameters
> > which have already been checked or used by the callers.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> Again, please split into dp and dsi patches. After that:

Sure, if you prefer. But without the stable-tree argument I think
there's little point in splitting.

> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> > ---
> >   drivers/gpu/drm/msm/dp/dp_display.c | 7 +------
> >   drivers/gpu/drm/msm/dsi/dsi.c       | 7 +------
> >   2 files changed, 2 insertions(+), 12 deletions(-)

Johan