[PATCH v2 0/4] drm: rcar-du: Improve device_link handling

[PATCH v2 0/4] drm: rcar-du: Improve device_link handling

[Laurent Pinchart]

Hello,

This small patch series came out of the review of [1].

I noticed that the DU driver enforces suspend/resume ordering with the
CMMs but not with the VSPs, which patch 1/4 fixes. This in turn made me
notice that the driver leaks the device_link instances to the CMM.
Patches 2/4 and 3/4 refactor CMM handling a bit, to prepare for 4/4 that
fixes the leak.

Changes compared to v1 are minor, see individual patches for details.

[1] https://lore.kernel.org/r/02669d4630e04fe24c17dd2576ec8b27ded458f0.1765541401.git.tommaso.merciai.xr@bp.renesas.com

Laurent Pinchart (4):
  drm: rcar-du: Ensure correct suspend/resume ordering with VSP
  drm: rcar-du: Store CMM device pointer instead of platform_device
  drm: rcar-du: Use __free() to simplify device_node handling
  drm: rcar-du: Don't leak device_link to CMM

 drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c    | 26 +++++-----
 drivers/gpu/drm/renesas/rcar-du/rcar_cmm.h    | 18 +++----
 .../gpu/drm/renesas/rcar-du/rcar_du_crtc.c    | 16 +++---
 .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  3 +-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h |  8 ++-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 52 +++++++++----------
 drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c | 16 ++++++
 drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.h |  2 +
 8 files changed, 81 insertions(+), 60 deletions(-)


base-commit: 25854131c04a5aa25a41cf527aab269aadb86699
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/4] drm: rcar-du: Ensure correct suspend/resume ordering with VSP

[Laurent Pinchart]

The VSP serves as an interface to memory and a compositor to the DU. It
therefore needs to be suspended after and resumed before the DU, to be
properly stopped and restarted in a controlled fashion driven by the DU
driver. This currently works by chance. Avoid relying on luck by
enforcing the correct suspend/resume ordering with device links.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Reviewed-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
---
 drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c | 16 ++++++++++++++++
 drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c
index 94c22d2db197..a4a49dcd8233 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.c
@@ -20,6 +20,7 @@
 #include <drm/drm_vblank.h>
 
 #include <linux/bitops.h>
+#include <linux/device.h>
 #include <linux/dma-mapping.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
@@ -458,6 +459,9 @@ static void rcar_du_vsp_cleanup(struct drm_device *dev, void *res)
 
 	kfree(vsp->planes);
 
+	if (vsp->link)
+		device_link_del(vsp->link);
+
 	put_device(vsp->vsp);
 }
 
@@ -482,6 +486,18 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * Enforce suspend/resume ordering between the DU (consumer) and the
+	 * VSP (supplier). The DU will be suspended before and resume after the
+	 * VSP.
+	 */
+	vsp->link = device_link_add(rcdu->dev, vsp->vsp, DL_FLAG_STATELESS);
+	if (!vsp->link) {
+		dev_err(rcdu->dev, "Failed to create device link to VSP %s\n",
+			dev_name(vsp->vsp));
+		return -EINVAL;
+	}
+
 	ret = vsp1_du_init(vsp->vsp);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.h
index 67630f0b6599..a6731249db34 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_vsp.h
@@ -12,6 +12,7 @@
 
 #include <drm/drm_plane.h>
 
+struct device_link;
 struct drm_framebuffer;
 struct rcar_du_format_info;
 struct rcar_du_vsp;
@@ -26,6 +27,7 @@ struct rcar_du_vsp_plane {
 struct rcar_du_vsp {
 	unsigned int index;
 	struct device *vsp;
+	struct device_link *link;
 	struct rcar_du_device *dev;
 	struct rcar_du_vsp_plane *planes;
 	unsigned int num_planes;
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/4] drm: rcar-du: Store CMM device pointer instead of platform_device

[Laurent Pinchart]

The DU driver stores the CMM devices as pointers to struct
platform_device, and passes them to the API exposed by the CMM driver.
This is similar to how the VSP is handled, except that the VSP uses
struct device pointers. Replace the CMM platform_device pointers with
device pointers for consistency.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v1:

- Renamed function arguments from pdev to dev
---
 drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c    | 26 +++++++++----------
 drivers/gpu/drm/renesas/rcar-du/rcar_cmm.h    | 18 ++++++-------
 .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  2 +-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h |  2 +-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c |  6 ++---
 5 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c b/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c
index 93ba115d654f..5bced9d778e8 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.c
@@ -59,7 +59,7 @@ static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
 
 /*
  * rcar_cmm_setup() - Configure the CMM unit
- * @pdev: The platform device associated with the CMM instance
+ * @dev: The device associated with the CMM instance
  * @config: The CMM unit configuration
  *
  * Configure the CMM unit with the given configuration. Currently enabling,
@@ -73,10 +73,10 @@ static void rcar_cmm_lut_write(struct rcar_cmm *rcmm,
  * TODO: Add support for LUT double buffer operations to avoid updating the
  * LUT table entries while a frame is being displayed.
  */
-int rcar_cmm_setup(struct platform_device *pdev,
+int rcar_cmm_setup(struct device *dev,
 		   const struct rcar_cmm_config *config)
 {
-	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+	struct rcar_cmm *rcmm = dev_get_drvdata(dev);
 
 	/* Disable LUT if no table is provided. */
 	if (!config->lut.table) {
@@ -102,7 +102,7 @@ EXPORT_SYMBOL_GPL(rcar_cmm_setup);
 
 /*
  * rcar_cmm_enable() - Enable the CMM unit
- * @pdev: The platform device associated with the CMM instance
+ * @dev: The device associated with the CMM instance
  *
  * When the output of the corresponding DU channel is routed to the CMM unit,
  * the unit shall be enabled before the DU channel is started, and remain
@@ -113,11 +113,11 @@ EXPORT_SYMBOL_GPL(rcar_cmm_setup);
  * It is an error to attempt to enable an already enabled CMM unit, or to
  * attempt to disable a disabled unit.
  */
-int rcar_cmm_enable(struct platform_device *pdev)
+int rcar_cmm_enable(struct device *dev)
 {
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
 		return ret;
 
@@ -127,7 +127,7 @@ EXPORT_SYMBOL_GPL(rcar_cmm_enable);
 
 /*
  * rcar_cmm_disable() - Disable the CMM unit
- * @pdev: The platform device associated with the CMM instance
+ * @dev: The device associated with the CMM instance
  *
  * See rcar_cmm_enable() for usage information.
  *
@@ -135,27 +135,27 @@ EXPORT_SYMBOL_GPL(rcar_cmm_enable);
  * state shall thus be restored with rcar_cmm_setup() when re-enabling the CMM
  * unit after the next rcar_cmm_enable() call.
  */
-void rcar_cmm_disable(struct platform_device *pdev)
+void rcar_cmm_disable(struct device *dev)
 {
-	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+	struct rcar_cmm *rcmm = dev_get_drvdata(dev);
 
 	rcar_cmm_write(rcmm, CM2_LUT_CTRL, 0);
 	rcmm->lut.enabled = false;
 
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 }
 EXPORT_SYMBOL_GPL(rcar_cmm_disable);
 
 /*
  * rcar_cmm_init() - Initialize the CMM unit
- * @pdev: The platform device associated with the CMM instance
+ * @dev: The device associated with the CMM instance
  *
  * Return: 0 on success, -EPROBE_DEFER if the CMM is not available yet,
  *         -ENODEV if the DRM_RCAR_CMM config option is disabled
  */
-int rcar_cmm_init(struct platform_device *pdev)
+int rcar_cmm_init(struct device *dev)
 {
-	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+	struct rcar_cmm *rcmm = dev_get_drvdata(dev);
 
 	if (!rcmm)
 		return -EPROBE_DEFER;
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.h b/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.h
index 628072acc98b..c420113430b9 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_cmm.h
@@ -10,8 +10,8 @@
 
 #define CM2_LUT_SIZE		256
 
+struct device;
 struct drm_color_lut;
-struct platform_device;
 
 /**
  * struct rcar_cmm_config - CMM configuration
@@ -26,29 +26,29 @@ struct rcar_cmm_config {
 };
 
 #if IS_ENABLED(CONFIG_DRM_RCAR_CMM)
-int rcar_cmm_init(struct platform_device *pdev);
+int rcar_cmm_init(struct device *dev);
 
-int rcar_cmm_enable(struct platform_device *pdev);
-void rcar_cmm_disable(struct platform_device *pdev);
+int rcar_cmm_enable(struct device *dev);
+void rcar_cmm_disable(struct device *dev);
 
-int rcar_cmm_setup(struct platform_device *pdev,
+int rcar_cmm_setup(struct device *dev,
 		   const struct rcar_cmm_config *config);
 #else
-static inline int rcar_cmm_init(struct platform_device *pdev)
+static inline int rcar_cmm_init(struct device *dev)
 {
 	return -ENODEV;
 }
 
-static inline int rcar_cmm_enable(struct platform_device *pdev)
+static inline int rcar_cmm_enable(struct device *dev)
 {
 	return 0;
 }
 
-static inline void rcar_cmm_disable(struct platform_device *pdev)
+static inline void rcar_cmm_disable(struct device *dev)
 {
 }
 
-static inline int rcar_cmm_setup(struct platform_device *pdev,
+static inline int rcar_cmm_setup(struct device *dev,
 				 const struct rcar_cmm_config *config)
 {
 	return 0;
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
index d0f38a8b3561..07a40b305be8 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
@@ -65,7 +65,7 @@ struct rcar_du_crtc {
 	unsigned int vblank_count;
 
 	struct rcar_du_group *group;
-	struct platform_device *cmm;
+	struct device *cmm;
 	struct rcar_du_vsp *vsp;
 	unsigned int vsp_pipe;
 
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
index 5cfa2bb7ad93..9e160dede4e6 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
@@ -106,7 +106,7 @@ struct rcar_du_device {
 	unsigned int num_crtcs;
 
 	struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
-	struct platform_device *cmms[RCAR_DU_MAX_CRTCS];
+	struct device *cmms[RCAR_DU_MAX_CRTCS];
 	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
 	struct drm_bridge *lvds[RCAR_DU_MAX_LVDS];
 	struct drm_bridge *dsi[RCAR_DU_MAX_DSI];
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
index 60e6f43b8ab2..f38e45d38ad2 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
@@ -806,13 +806,13 @@ static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
 		 * -ENODEV is used to report that the CMM config option is
 		 * disabled: return 0 and let the DU continue probing.
 		 */
-		ret = rcar_cmm_init(pdev);
+		ret = rcar_cmm_init(&pdev->dev);
 		if (ret) {
 			platform_device_put(pdev);
 			return ret == -ENODEV ? 0 : ret;
 		}
 
-		rcdu->cmms[i] = pdev;
+		rcdu->cmms[i] = &pdev->dev;
 
 		/*
 		 * Enforce suspend/resume ordering by making the CMM a provider
@@ -835,7 +835,7 @@ static void rcar_du_modeset_cleanup(struct drm_device *dev, void *res)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(rcdu->cmms); ++i)
-		platform_device_put(rcdu->cmms[i]);
+		put_device(rcdu->cmms[i]);
 }
 
 int rcar_du_modeset_init(struct rcar_du_device *rcdu)
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/4] drm: rcar-du: Use __free() to simplify device_node handling

[Laurent Pinchart]

Replace manual of_node_put() calls with __free(). This simplifies error
handling code and makes it less bug-prone.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 25 ++++++-------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
index f38e45d38ad2..9a53b5a86c82 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
@@ -19,6 +19,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/dma-buf.h>
 #include <linux/of.h>
@@ -573,7 +574,7 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
 				     enum rcar_du_output output,
 				     struct of_endpoint *ep)
 {
-	struct device_node *entity;
+	struct device_node *entity __free(device_node) = NULL;
 	int ret;
 
 	/* Locate the connected entity and initialize the encoder. */
@@ -588,7 +589,6 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
 		dev_dbg(rcdu->dev,
 			"connected entity %pOF is disabled, skipping\n",
 			entity);
-		of_node_put(entity);
 		return -ENODEV;
 	}
 
@@ -598,15 +598,13 @@ static int rcar_du_encoders_init_one(struct rcar_du_device *rcdu,
 			 "failed to initialize encoder %pOF on output %s (%d), skipping\n",
 			 entity, rcar_du_output_name(output), ret);
 
-	of_node_put(entity);
-
 	return ret;
 }
 
 static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 {
+	struct device_node *ep_node __free(device_node) = NULL;
 	struct device_node *np = rcdu->dev->of_node;
-	struct device_node *ep_node;
 	unsigned int num_encoders = 0;
 
 	/*
@@ -620,10 +618,8 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 		int ret;
 
 		ret = of_graph_parse_endpoint(ep_node, &ep);
-		if (ret < 0) {
-			of_node_put(ep_node);
+		if (ret < 0)
 			return ret;
-		}
 
 		/* Find the output route corresponding to the port number. */
 		for (i = 0; i < RCAR_DU_OUTPUT_MAX; ++i) {
@@ -644,10 +640,8 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 		/* Process the output pipeline. */
 		ret = rcar_du_encoders_init_one(rcdu, output, &ep);
 		if (ret < 0) {
-			if (ret == -EPROBE_DEFER) {
-				of_node_put(ep_node);
+			if (ret == -EPROBE_DEFER)
 				return ret;
-			}
 
 			continue;
 		}
@@ -775,9 +769,9 @@ static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
 	}
 
 	for (i = 0; i < cells; ++i) {
+		struct device_node *cmm __free(device_node) = NULL;
 		struct platform_device *pdev;
 		struct device_link *link;
-		struct device_node *cmm;
 		int ret;
 
 		cmm = of_parse_phandle(np, "renesas,cmms", i);
@@ -787,21 +781,16 @@ static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
 			return -EINVAL;
 		}
 
-		if (!of_device_is_available(cmm)) {
+		if (!of_device_is_available(cmm))
 			/* It's fine to have a phandle to a non-enabled CMM. */
-			of_node_put(cmm);
 			continue;
-		}
 
 		pdev = of_find_device_by_node(cmm);
 		if (!pdev) {
 			dev_err(rcdu->dev, "No device found for CMM%u\n", i);
-			of_node_put(cmm);
 			return -EINVAL;
 		}
 
-		of_node_put(cmm);
-
 		/*
 		 * -ENODEV is used to report that the CMM config option is
 		 * disabled: return 0 and let the DU continue probing.
-- 
Regards,

Laurent Pinchart

[PATCH v2 4/4] drm: rcar-du: Don't leak device_link to CMM

[Laurent Pinchart]

The DU driver creates device_link instances between the DU and CMMs, but
never deletes them. Fix it by introducing a rcar_du_cmm structure to
group the CMM device and device_link, and deleting the links at cleanup
time.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../gpu/drm/renesas/rcar-du/rcar_du_crtc.c    | 16 +++++-----
 .../gpu/drm/renesas/rcar-du/rcar_du_crtc.h    |  3 +-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h |  8 ++++-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 29 ++++++++++++-------
 4 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
index 28a5aa5a14d8..7c36c30a75b6 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.c
@@ -513,13 +513,13 @@ static void rcar_du_cmm_setup(struct drm_crtc *crtc)
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct rcar_cmm_config cmm_config = {};
 
-	if (!rcrtc->cmm)
+	if (!rcrtc->cmm->dev)
 		return;
 
 	if (drm_lut)
 		cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
 
-	rcar_cmm_setup(rcrtc->cmm, &cmm_config);
+	rcar_cmm_setup(rcrtc->cmm->dev, &cmm_config);
 }
 
 /* -----------------------------------------------------------------------------
@@ -667,8 +667,8 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_disable(rcrtc);
 
-	if (rcrtc->cmm)
-		rcar_cmm_disable(rcrtc->cmm);
+	if (rcrtc->cmm->dev)
+		rcar_cmm_disable(rcrtc->cmm->dev);
 
 	/*
 	 * Select switch sync mode. This stops display operation and configures
@@ -726,8 +726,8 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
 	struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state);
 	struct rcar_du_device *rcdu = rcrtc->dev;
 
-	if (rcrtc->cmm)
-		rcar_cmm_enable(rcrtc->cmm);
+	if (rcrtc->cmm->dev)
+		rcar_cmm_enable(rcrtc->cmm->dev);
 	rcar_du_crtc_get(rcrtc);
 
 	/*
@@ -1300,8 +1300,8 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
 		return ret;
 
 	/* CMM might be disabled for this CRTC. */
-	if (rcdu->cmms[swindex]) {
-		rcrtc->cmm = rcdu->cmms[swindex];
+	if (rcdu->cmms[swindex].dev) {
+		rcrtc->cmm = &rcdu->cmms[swindex];
 		rgrp->cmms_mask |= BIT(hwindex % 2);
 
 		drm_mode_crtc_set_gamma_size(crtc, CM2_LUT_SIZE);
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
index 07a40b305be8..8857926e109a 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_crtc.h
@@ -19,6 +19,7 @@
 
 #include <media/vsp1.h>
 
+struct rcar_du_cmm;
 struct rcar_du_group;
 struct rcar_du_vsp;
 
@@ -65,7 +66,7 @@ struct rcar_du_crtc {
 	unsigned int vblank_count;
 
 	struct rcar_du_group *group;
-	struct device *cmm;
+	struct rcar_du_cmm *cmm;
 	struct rcar_du_vsp *vsp;
 	unsigned int vsp_pipe;
 
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
index 9e160dede4e6..de9c6617a2d4 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_drv.h
@@ -22,6 +22,7 @@
 
 struct clk;
 struct device;
+struct device_link;
 struct drm_bridge;
 struct drm_property;
 struct rcar_du_device;
@@ -88,6 +89,11 @@ struct rcar_du_device_info {
 	unsigned int lvds_clk_mask;
 };
 
+struct rcar_du_cmm {
+	struct device *dev;
+	struct device_link *link;
+};
+
 #define RCAR_DU_MAX_CRTCS		4
 #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
 #define RCAR_DU_MAX_VSPS		4
@@ -106,7 +112,7 @@ struct rcar_du_device {
 	unsigned int num_crtcs;
 
 	struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
-	struct device *cmms[RCAR_DU_MAX_CRTCS];
+	struct rcar_du_cmm cmms[RCAR_DU_MAX_CRTCS];
 	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
 	struct drm_bridge *lvds[RCAR_DU_MAX_LVDS];
 	struct drm_bridge *dsi[RCAR_DU_MAX_DSI];
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
index 9a53b5a86c82..b2d0e4651e35 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
@@ -769,23 +769,23 @@ static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
 	}
 
 	for (i = 0; i < cells; ++i) {
-		struct device_node *cmm __free(device_node) = NULL;
+		struct device_node *cmm_node __free(device_node) = NULL;
+		struct rcar_du_cmm *cmm = &rcdu->cmms[i];
 		struct platform_device *pdev;
-		struct device_link *link;
 		int ret;
 
-		cmm = of_parse_phandle(np, "renesas,cmms", i);
-		if (!cmm) {
+		cmm_node = of_parse_phandle(np, "renesas,cmms", i);
+		if (!cmm_node) {
 			dev_err(rcdu->dev,
 				"Failed to parse 'renesas,cmms' property\n");
 			return -EINVAL;
 		}
 
-		if (!of_device_is_available(cmm))
+		if (!of_device_is_available(cmm_node))
 			/* It's fine to have a phandle to a non-enabled CMM. */
 			continue;
 
-		pdev = of_find_device_by_node(cmm);
+		pdev = of_find_device_by_node(cmm_node);
 		if (!pdev) {
 			dev_err(rcdu->dev, "No device found for CMM%u\n", i);
 			return -EINVAL;
@@ -801,14 +801,15 @@ static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
 			return ret == -ENODEV ? 0 : ret;
 		}
 
-		rcdu->cmms[i] = &pdev->dev;
+		cmm->dev = &pdev->dev;
 
 		/*
 		 * Enforce suspend/resume ordering by making the CMM a provider
 		 * of the DU: CMM is suspended after and resumed before the DU.
 		 */
-		link = device_link_add(rcdu->dev, &pdev->dev, DL_FLAG_STATELESS);
-		if (!link) {
+		cmm->link = device_link_add(rcdu->dev, cmm->dev,
+					    DL_FLAG_STATELESS);
+		if (!cmm->link) {
 			dev_err(rcdu->dev,
 				"Failed to create device link to CMM%u\n", i);
 			return -EINVAL;
@@ -823,8 +824,14 @@ static void rcar_du_modeset_cleanup(struct drm_device *dev, void *res)
 	struct rcar_du_device *rcdu = to_rcar_du_device(dev);
 	unsigned int i;
 
-	for (i = 0; i < ARRAY_SIZE(rcdu->cmms); ++i)
-		put_device(rcdu->cmms[i]);
+	for (i = 0; i < ARRAY_SIZE(rcdu->cmms); ++i) {
+		struct rcar_du_cmm *cmm = &rcdu->cmms[i];
+
+		if (cmm->link)
+			device_link_del(cmm->link);
+
+		put_device(cmm->dev);
+	}
 }
 
 int rcar_du_modeset_init(struct rcar_du_device *rcdu)
-- 
Regards,

Laurent Pinchart

Claude review: drm: rcar-du: Improve device_link handling

[Claude Code Review Bot]

Overall Series Review

Subject: drm: rcar-du: Improve device_link handling
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Patches: 5
Reviewed: 2026-03-25T07:42:39.397669

---

This is a clean, well-structured 4-patch series that addresses two real issues in the rcar-du driver: missing device_link for VSP suspend/resume ordering, and leaked device_link instances for CMM. The preparatory refactoring (patches 2-3) is logical and makes the final fix straightforward. The code is generally well-written by an experienced maintainer.

However, **patch 4 introduces a NULL pointer dereference bug** that must be fixed before merging. There is also a minor coding style concern in patch 3.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: rcar-du: Ensure correct suspend/resume ordering with VSP

[Claude Code Review Bot]

Patch Review

**Status: Good**

This patch correctly adds a `DL_FLAG_STATELESS` device link between the DU (consumer) and VSP (supplier) to enforce proper suspend/resume ordering, mirroring what was already done for CMM.

The implementation is clean:
- The link is stored in `vsp->link` and properly deleted in `rcar_du_vsp_cleanup()` with a NULL guard
- Cleanup ordering is correct — `device_link_del()` before `put_device()`
- The error message is informative
- Already has Reviewed-by and Tested-by tags

No issues found.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: rcar-du: Store CMM device pointer instead of platform_device

[Claude Code Review Bot]

Patch Review

**Status: Good**

Straightforward mechanical refactoring changing CMM API from `struct platform_device *` to `struct device *` for consistency with VSP handling. All callers updated correctly:
- `platform_get_drvdata()` → `dev_get_drvdata()`
- `pm_runtime_resume_and_get(&pdev->dev)` → `pm_runtime_resume_and_get(dev)`
- `platform_device_put()` → `put_device()`
- Storage type updated in `rcar_du_crtc.h` and `rcar_du_drv.h`

No issues found.

---
Generated by Claude Code Patch Reviewer

Claude review: drm: rcar-du: Use __free() to simplify device_node handling

[Claude Code Review Bot]

Patch Review

**Status: Good, one minor style nit**

The `__free(device_node)` usage is correct in all three cases:

1. **`entity` in `rcar_du_encoders_init_one()`**: Simple function-scoped lifetime, replaces two `of_node_put()` calls.

2. **`ep_node` in `rcar_du_encoders_init()`**: Used with `for_each_endpoint_of_node()`. This works correctly because:
   - On early `return`, `__free` puts the current node reference (previously needed manual `of_node_put(ep_node)`)
   - On normal loop completion, `ep_node` is NULL, and `of_node_put(NULL)` is a no-op
   - On `continue`, the macro's next iteration handles the put internally

3. **`cmm` in `rcar_du_cmm_init()`**: Simple loop-scoped lifetime, replaces three `of_node_put()` calls.

**Minor style nit:** After patch 3, the following pattern appears:

```c
if (!of_device_is_available(cmm))
    /* It's fine to have a phandle to a non-enabled CMM. */
    continue;
```

This places a comment between an `if` condition and its body without braces. While technically valid C, the kernel coding style (and common sense) would prefer braces here since the comment makes it look like two statements under the `if`. The original code had braces. Consider keeping them:

```c
if (!of_device_is_available(cmm)) {
    /* It's fine to have a phandle to a non-enabled CMM. */
    continue;
}
```

---
Generated by Claude Code Patch Reviewer

Claude review: drm: rcar-du: Don't leak device_link to CMM

[Claude Code Review Bot]

Patch Review

**Status: Bug — NULL pointer dereference**

The core idea is sound: introduce `struct rcar_du_cmm` to group the device and link, and delete links at cleanup. However, this patch introduces a NULL pointer dereference.

In `rcar_du_crtc_create()`, `rcrtc->cmm` is only assigned when the CMM device exists:

```c
if (rcdu->cmms[swindex].dev) {
    rcrtc->cmm = &rcdu->cmms[swindex];
    ...
}
```

If CMM is not available for a CRTC, `rcrtc->cmm` remains **NULL**. But the patch changes checks from `if (!rcrtc->cmm)` to `if (!rcrtc->cmm->dev)` in three places (`rcar_du_cmm_setup`, `rcar_du_crtc_stop`, `rcar_du_crtc_atomic_enable`), which will dereference the NULL pointer:

```c
// rcar_du_crtc.c, rcar_du_cmm_setup():
if (!rcrtc->cmm->dev)    // BUG: rcrtc->cmm is NULL when no CMM
    return;

// rcar_du_crtc.c, rcar_du_crtc_stop():
if (rcrtc->cmm->dev)     // BUG: same issue
    rcar_cmm_disable(rcrtc->cmm->dev);

// rcar_du_crtc.c, rcar_du_crtc_atomic_enable():
if (rcrtc->cmm->dev)     // BUG: same issue
    rcar_cmm_enable(rcrtc->cmm->dev);
```

**Fix options:**
1. Always set `rcrtc->cmm = &rcdu->cmms[swindex]` (even when `.dev` is NULL), so the pointer is always valid and only `.dev` needs checking. This is the simplest fix.
2. Add a two-level NULL check: `if (rcrtc->cmm && rcrtc->cmm->dev)` — but this is ugly and defeats the purpose of the refactoring.

Option 1 would look like:

```c
/* In rcar_du_crtc_create(): */
rcrtc->cmm = &rcdu->cmms[swindex];
if (rcrtc->cmm->dev) {
    rgrp->cmms_mask |= BIT(hwindex % 2);
    drm_mode_crtc_set_gamma_size(crtc, CM2_LUT_SIZE);
    drm_crtc_enable_color_mgmt(crtc, 0, false, CM2_LUT_SIZE);
}
```

---
Generated by Claude Code Patch Reviewer