[PATCH 00/17] DRM: imx spring-cleaning

[PATCH 00/17] DRM: imx spring-cleaning

[Marco Felsch]

Hi all,

the purpose of this patch series is to address bug reported here
[1]. There where two approaches [2,3] to fix this but non of them get
mainline. The issue is caused by the fact that we are using devres
allocation for the driver (pd,ldb,hdmi,tve) state struct which holds
also the 'struct drm_encoder/connector'. 

We need to move the driver state containers containing the drm members
out of the devres memory management into the drm memory management
framework to fix the bug [1]. Therefore we need to split the single
driver state struct into two: one for the drm_connector device and one
for the drm_encoder device.

The series removes some legacy code paths too and removes the useless
imx_drm_encoder_destroy() API.

Pls don't be surprised about the edid memory leak fix patches. I went
this way because those patche can be applied independently of the last
patch which did the conversion from the devres alloc to the non-devres
alloc.

I did the following tests for each component:
 - probe successful
 - correct failure handling during probe
 - bind / unbind (module load/unload)

I also kept a few lines longer than 80char to improve readability.

Other tester are welcome =)

Regards,
  Marco

[1] https://www.spinics.net/lists/dri-devel/msg189388.html
[2] https://lkml.org/lkml/2018/10/16/1148
[3] https://lkml.org/lkml/2019/4/2/612

Marco Felsch (17):
  drm/imx: drop useless best_encoder callback
  drm/imx: parallel-display: fix edid memory leak
  drm/imx: parallel-display: move panel/bridge detection to fail early
  drm/imx: parallel-display: detach panel within drm_encoder destroy
  drm/imx: parallel-display: split encoder and decoder states
  imx/drm: parallel-display: split attach function
  drm/imx: tve: add regulator_disable devm_action
  drm/imx: tve: split global state container
  drm/imx: imx-ldb: remove useless enum
  drm/imx: imx-ldb: fix edid memory leak
  drm/imx: imx-ldb: release ldb-channel resources within encoder destroy
  drm/imx: remove imx_drm_encoder_destroy helper
  drm/imx: imx-ldb: split imx_ldb devres allocation context
  drm/imx: imx-ldb: add ldb_is_dual helper
  drm/imx: imx-ldb: split encoder and decoder states
  drm/imx: imx-ldb: refactor imx_ldb_bind
  drm/imx: fix drm_mode_config_cleanup race condition

 drivers/gpu/drm/imx/dw_hdmi-imx.c      |  28 +-
 drivers/gpu/drm/imx/imx-drm-core.c     |   9 +-
 drivers/gpu/drm/imx/imx-drm.h          |   1 -
 drivers/gpu/drm/imx/imx-ldb.c          | 514 ++++++++++++++-----------
 drivers/gpu/drm/imx/imx-tve.c          | 173 ++++++---
 drivers/gpu/drm/imx/ipuv3-crtc.c       |  28 +-
 drivers/gpu/drm/imx/parallel-display.c | 142 ++++---
 7 files changed, 523 insertions(+), 372 deletions(-)

-- 
2.20.1

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

[PATCH 01/17] drm/imx: drop useless best_encoder callback

[Marco Felsch]

The best_encoder() callback is used by the drm-core to find an encoder
if the connector is connected to multiple encoders but the parallel, tve
and ldb uses always the 1-encoder : 1-connector setup. Such a simple
setup can be handled by the drm-core.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c          | 9 ---------
 drivers/gpu/drm/imx/imx-tve.c          | 9 ---------
 drivers/gpu/drm/imx/parallel-display.c | 9 ---------
 3 files changed, 27 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 8cb2665b2c74..cd399058b00e 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -155,14 +155,6 @@ static int imx_ldb_connector_get_modes(struct drm_connector *connector)
 	return num_modes;
 }
 
-static struct drm_encoder *imx_ldb_connector_best_encoder(
-		struct drm_connector *connector)
-{
-	struct imx_ldb_channel *imx_ldb_ch = con_to_imx_ldb_ch(connector);
-
-	return &imx_ldb_ch->encoder;
-}
-
 static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
 		unsigned long serial_clk, unsigned long di_clk)
 {
@@ -390,7 +382,6 @@ static const struct drm_connector_funcs imx_ldb_connector_funcs = {
 
 static const struct drm_connector_helper_funcs imx_ldb_connector_helper_funcs = {
 	.get_modes = imx_ldb_connector_get_modes,
-	.best_encoder = imx_ldb_connector_best_encoder,
 };
 
 static const struct drm_encoder_funcs imx_ldb_encoder_funcs = {
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 5bbfaa2cd0f4..e113014a1a79 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -259,14 +259,6 @@ static int imx_tve_connector_mode_valid(struct drm_connector *connector,
 	return MODE_BAD;
 }
 
-static struct drm_encoder *imx_tve_connector_best_encoder(
-		struct drm_connector *connector)
-{
-	struct imx_tve *tve = con_to_tve(connector);
-
-	return &tve->encoder;
-}
-
 static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 				     struct drm_display_mode *orig_mode,
 				     struct drm_display_mode *mode)
@@ -344,7 +336,6 @@ static const struct drm_connector_funcs imx_tve_connector_funcs = {
 
 static const struct drm_connector_helper_funcs imx_tve_connector_helper_funcs = {
 	.get_modes = imx_tve_connector_get_modes,
-	.best_encoder = imx_tve_connector_best_encoder,
 	.mode_valid = imx_tve_connector_mode_valid,
 };
 
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 3dca424059f7..b2dbf0bc4cdc 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -81,14 +81,6 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector)
 	return num_modes;
 }
 
-static struct drm_encoder *imx_pd_connector_best_encoder(
-		struct drm_connector *connector)
-{
-	struct imx_parallel_display *imxpd = con_to_imxpd(connector);
-
-	return &imxpd->encoder;
-}
-
 static void imx_pd_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
@@ -136,7 +128,6 @@ static const struct drm_connector_funcs imx_pd_connector_funcs = {
 
 static const struct drm_connector_helper_funcs imx_pd_connector_helper_funcs = {
 	.get_modes = imx_pd_connector_get_modes,
-	.best_encoder = imx_pd_connector_best_encoder,
 };
 
 static const struct drm_encoder_funcs imx_pd_encoder_funcs = {
-- 
2.20.1

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

[PATCH 02/17] drm/imx: parallel-display: fix edid memory leak

[Marco Felsch]

The edid memory is only freed if the component.unbind() is called. This
is okay if the parallel-display was binded but if the bind() fails we
leek the memory.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/parallel-display.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index b2dbf0bc4cdc..1f08fe8337d7 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -202,7 +202,8 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 
 	edidp = of_get_property(np, "edid", &imxpd->edid_len);
 	if (edidp)
-		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
+		imxpd->edid = devm_kmemdup(dev, edidp, imxpd->edid_len,
+					   GFP_KERNEL);
 
 	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
 	if (!ret) {
@@ -240,8 +241,6 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
 
 	if (imxpd->panel)
 		drm_panel_detach(imxpd->panel);
-
-	kfree(imxpd->edid);
 }
 
 static const struct component_ops imx_pd_ops = {
-- 
2.20.1

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

[PATCH 03/17] drm/imx: parallel-display: move panel/bridge detection to fail early

[Marco Felsch]

We do some string parsing and string comparison in front of
drm_of_find_panel_or_bridge(). All this work is useless if the call
fails. Move drm_of_find_panel_or_bridge() infront of the parsing work to
fail early.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/parallel-display.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 1f08fe8337d7..6ee028faa9a8 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -200,6 +200,11 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (!imxpd)
 		return -ENOMEM;
 
+	/* port@1 is the output port */
+	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
+	if (ret && ret != -ENODEV)
+		return ret;
+
 	edidp = of_get_property(np, "edid", &imxpd->edid_len);
 	if (edidp)
 		imxpd->edid = devm_kmemdup(dev, edidp, imxpd->edid_len,
@@ -218,10 +223,6 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	}
 	imxpd->bus_format = bus_format;
 
-	/* port@1 is the output port */
-	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
-	if (ret && ret != -ENODEV)
-		return ret;
 
 	imxpd->dev = dev;
 
-- 
2.20.1

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

[PATCH 04/17] drm/imx: parallel-display: detach panel within drm_encoder destroy

[Marco Felsch]

There is no reason why this should be done within the
component.unbind() call so let the drm-core do the cleanup during a
drm_mode_config_cleanup() call.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/parallel-display.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 6ee028faa9a8..e731b19fd6b2 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -81,6 +81,15 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector)
 	return num_modes;
 }
 
+static void imx_pd_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
+
+	if (imxpd->panel)
+		drm_panel_detach(imxpd->panel);
+	drm_encoder_cleanup(encoder);
+}
+
 static void imx_pd_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_parallel_display *imxpd = enc_to_imxpd(encoder);
@@ -131,7 +140,7 @@ static const struct drm_connector_helper_funcs imx_pd_connector_helper_funcs = {
 };
 
 static const struct drm_encoder_funcs imx_pd_encoder_funcs = {
-	.destroy = imx_drm_encoder_destroy,
+	.destroy = imx_pd_encoder_destroy,
 };
 
 static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = {
@@ -230,23 +239,11 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	dev_set_drvdata(dev, imxpd);
-
 	return 0;
 }
 
-static void imx_pd_unbind(struct device *dev, struct device *master,
-	void *data)
-{
-	struct imx_parallel_display *imxpd = dev_get_drvdata(dev);
-
-	if (imxpd->panel)
-		drm_panel_detach(imxpd->panel);
-}
-
 static const struct component_ops imx_pd_ops = {
 	.bind	= imx_pd_bind,
-	.unbind	= imx_pd_unbind,
 };
 
 static int imx_pd_probe(struct platform_device *pdev)
-- 
2.20.1

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

[PATCH 05/17] drm/imx: parallel-display: split encoder and decoder states

[Marco Felsch]

This commit prepares the driver to fix the race condition reported by
[1]. The goal is to switch from the devres-kmalloc and the component
framework to a 'normal' kmalloc and the drm framework to release the
memory resources. So all acquired memory resoruces are freed by a
drm_mode_config_cleanup() call and the ->destroy() callbacks. For this
purpose we need to split off the drm_connector structure.

[1] https://www.spinics.net/lists/dri-devel/msg189388.html

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/parallel-display.c | 84 ++++++++++++++++----------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index e731b19fd6b2..a05808982f2f 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -22,21 +22,25 @@
 #include "imx-drm.h"
 
 struct imx_parallel_display {
-	struct drm_connector connector;
 	struct drm_encoder encoder;
 	struct device *dev;
-	void *edid;
-	int edid_len;
 	u32 bus_format;
 	u32 bus_flags;
-	struct drm_display_mode mode;
 	struct drm_panel *panel;
 	struct drm_bridge *bridge;
 };
 
-static inline struct imx_parallel_display *con_to_imxpd(struct drm_connector *c)
+struct imx_parallel_connector {
+	struct imx_parallel_display *imxpd;
+	struct drm_connector connector;
+	void *edid;
+	int edid_len;
+	struct drm_display_mode mode;
+};
+
+static inline struct imx_parallel_connector *con_to_imxpc(struct drm_connector *c)
 {
-	return container_of(c, struct imx_parallel_display, connector);
+	return container_of(c, struct imx_parallel_connector, connector);
 }
 
 static inline struct imx_parallel_display *enc_to_imxpd(struct drm_encoder *e)
@@ -44,9 +48,19 @@ static inline struct imx_parallel_display *enc_to_imxpd(struct drm_encoder *e)
 	return container_of(e, struct imx_parallel_display, encoder);
 }
 
+static void imx_pd_connector_destroy(struct drm_connector *connector)
+{
+	struct imx_parallel_connector *imxpc = con_to_imxpc(connector);
+
+	imx_drm_connector_destroy(connector);
+	/* avoid dangling pointer */
+	imxpc->imxpd = NULL;
+}
+
 static int imx_pd_connector_get_modes(struct drm_connector *connector)
 {
-	struct imx_parallel_display *imxpd = con_to_imxpd(connector);
+	struct imx_parallel_connector *imxpc = con_to_imxpc(connector);
+	struct imx_parallel_display *imxpd = imxpc->imxpd;
 	struct device_node *np = imxpd->dev->of_node;
 	int num_modes;
 
@@ -54,9 +68,9 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector)
 	if (num_modes > 0)
 		return num_modes;
 
-	if (imxpd->edid) {
-		drm_connector_update_edid_property(connector, imxpd->edid);
-		num_modes = drm_add_edid_modes(connector, imxpd->edid);
+	if (imxpc->edid) {
+		drm_connector_update_edid_property(connector, imxpc->edid);
+		num_modes = drm_add_edid_modes(connector, imxpc->edid);
 	}
 
 	if (np) {
@@ -66,13 +80,13 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector)
 		if (!mode)
 			return -EINVAL;
 
-		ret = of_get_drm_display_mode(np, &imxpd->mode,
+		ret = of_get_drm_display_mode(np, &imxpc->mode,
 					      &imxpd->bus_flags,
 					      OF_USE_NATIVE_MODE);
 		if (ret)
 			return ret;
 
-		drm_mode_copy(mode, &imxpd->mode);
+		drm_mode_copy(mode, &imxpc->mode);
 		mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
 		drm_mode_probed_add(connector, mode);
 		num_modes++;
@@ -129,7 +143,7 @@ static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder,
 
 static const struct drm_connector_funcs imx_pd_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = imx_drm_connector_destroy,
+	.destroy = imx_pd_connector_destroy,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
@@ -150,7 +164,8 @@ static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = {
 };
 
 static int imx_pd_register(struct drm_device *drm,
-	struct imx_parallel_display *imxpd)
+			   struct imx_parallel_display *imxpd,
+			   struct imx_parallel_connector *imxpc)
 {
 	struct drm_encoder *encoder = &imxpd->encoder;
 	int ret;
@@ -159,27 +174,26 @@ static int imx_pd_register(struct drm_device *drm,
 	if (ret)
 		return ret;
 
-	/* set the connector's dpms to OFF so that
-	 * drm_helper_connector_dpms() won't return
-	 * immediately since the current state is ON
-	 * at this point.
-	 */
-	imxpd->connector.dpms = DRM_MODE_DPMS_OFF;
-
 	drm_encoder_helper_add(encoder, &imx_pd_encoder_helper_funcs);
 	drm_encoder_init(drm, encoder, &imx_pd_encoder_funcs,
 			 DRM_MODE_ENCODER_NONE, NULL);
 
 	if (!imxpd->bridge) {
-		drm_connector_helper_add(&imxpd->connector,
+		/* set the connector's dpms to OFF so that
+		 * drm_helper_connector_dpms() won't return
+		 * immediately since the current state is ON
+		 * at this point.
+		 */
+		imxpc->connector.dpms = DRM_MODE_DPMS_OFF;
+		drm_connector_helper_add(&imxpc->connector,
 				&imx_pd_connector_helper_funcs);
-		drm_connector_init(drm, &imxpd->connector,
+		drm_connector_init(drm, &imxpc->connector,
 				   &imx_pd_connector_funcs,
 				   DRM_MODE_CONNECTOR_DPI);
 	}
 
 	if (imxpd->panel)
-		drm_panel_attach(imxpd->panel, &imxpd->connector);
+		drm_panel_attach(imxpd->panel, &imxpc->connector);
 
 	if (imxpd->bridge) {
 		ret = drm_bridge_attach(encoder, imxpd->bridge, NULL);
@@ -189,7 +203,7 @@ static int imx_pd_register(struct drm_device *drm,
 			return ret;
 		}
 	} else {
-		drm_connector_attach_encoder(&imxpd->connector, encoder);
+		drm_connector_attach_encoder(&imxpc->connector, encoder);
 	}
 
 	return 0;
@@ -201,6 +215,7 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	struct device_node *np = dev->of_node;
 	const u8 *edidp;
 	struct imx_parallel_display *imxpd;
+	struct imx_parallel_connector *imxpc;
 	int ret;
 	u32 bus_format = 0;
 	const char *fmt;
@@ -214,10 +229,18 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (ret && ret != -ENODEV)
 		return ret;
 
-	edidp = of_get_property(np, "edid", &imxpd->edid_len);
-	if (edidp)
-		imxpd->edid = devm_kmemdup(dev, edidp, imxpd->edid_len,
-					   GFP_KERNEL);
+	if (!imxpd->bridge) {
+		imxpc = devm_kzalloc(dev, sizeof(*imxpc), GFP_KERNEL);
+		if (!imxpc)
+			return -ENOMEM;
+
+		imxpc->imxpd = imxpd;
+
+		edidp = of_get_property(np, "edid", &imxpc->edid_len);
+		if (edidp)
+			imxpc->edid = devm_kmemdup(dev, edidp, imxpc->edid_len,
+						   GFP_KERNEL);
+	}
 
 	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
 	if (!ret) {
@@ -232,10 +255,9 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	}
 	imxpd->bus_format = bus_format;
 
-
 	imxpd->dev = dev;
 
-	ret = imx_pd_register(drm, imxpd);
+	ret = imx_pd_register(drm, imxpd, imxpc);
 	if (ret)
 		return ret;
 
-- 
2.20.1

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

[PATCH 06/17] imx/drm: parallel-display: split attach function

[Marco Felsch]

Split the maybe failing attach function from imx_pd_register() so we
can construct simple error paths later on.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/parallel-display.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index a05808982f2f..78703b15c7cf 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -192,6 +192,16 @@ static int imx_pd_register(struct drm_device *drm,
 				   DRM_MODE_CONNECTOR_DPI);
 	}
 
+	return 0;
+}
+static int imx_pd_attach(struct drm_device *drm,
+			 struct imx_parallel_display *imxpd,
+			 struct imx_parallel_connector *imxpc)
+{
+	struct drm_encoder *encoder = &imxpd->encoder;
+	int ret;
+
+
 	if (imxpd->panel)
 		drm_panel_attach(imxpd->panel, &imxpc->connector);
 
@@ -261,7 +271,7 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	return 0;
+	return imx_pd_attach(drm, imxpd, imxpc);
 }
 
 static const struct component_ops imx_pd_ops = {
-- 
2.20.1

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

[PATCH 07/17] drm/imx: tve: add regulator_disable devm_action

[Marco Felsch]

To avoid a dedicated unbind() callback we can use the new devm_action
mechanism. This allows us to drop the error check because the callback
will only be executed if the regulator was registered.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-tve.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index e113014a1a79..3340ba6e0222 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -485,6 +485,13 @@ static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
 	return 0;
 }
 
+static void imx_tve_disable_regulator(void *data)
+{
+	struct imx_tve *tve = data;
+
+	regulator_disable(tve->dac_reg);
+}
+
 static bool imx_tve_readable_reg(struct device *dev, unsigned int reg)
 {
 	return (reg % 4 == 0) && (reg <= 0xdc);
@@ -609,6 +616,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 		ret = regulator_enable(tve->dac_reg);
 		if (ret)
 			return ret;
+		ret = devm_add_action_or_reset(dev, imx_tve_disable_regulator, tve);
+		if (ret)
+			return ret;
 	}
 
 	tve->clk = devm_clk_get(dev, "tve");
@@ -655,18 +665,8 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 }
 
-static void imx_tve_unbind(struct device *dev, struct device *master,
-	void *data)
-{
-	struct imx_tve *tve = dev_get_drvdata(dev);
-
-	if (!IS_ERR(tve->dac_reg))
-		regulator_disable(tve->dac_reg);
-}
-
 static const struct component_ops imx_tve_ops = {
 	.bind	= imx_tve_bind,
-	.unbind	= imx_tve_unbind,
 };
 
 static int imx_tve_probe(struct platform_device *pdev)
-- 
2.20.1

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

[PATCH 08/17] drm/imx: tve: split global state container

[Marco Felsch]

The race condition reported by [1] applies to the tve driver too but
currently no errors reported. We need to split the connector state from
the encoder state. Furthermore we have to switch from the devres-kmalloc
and the component framework to a 'normal' kmalloc and the drm framework
release mechanism for the DRM containers. All other resources should
still be allocated using the devres-* accessors.

This commit prepares the driver for the kmalloc switch and splits the
resource allocation from the .bind() callback. This improves the drm
master device bind() too because most resources are allocated during
probe.

While on it I dropped the imx_drm_encoder_destroy() API and call
drm_encoder_cleanup() directly.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-tve.c | 145 +++++++++++++++++++++++-----------
 1 file changed, 101 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 3340ba6e0222..a7a05c47f68b 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -100,8 +100,6 @@ enum {
 };
 
 struct imx_tve {
-	struct drm_connector connector;
-	struct drm_encoder encoder;
 	struct device *dev;
 	spinlock_t lock;	/* register lock */
 	bool enabled;
@@ -111,21 +109,38 @@ struct imx_tve {
 
 	struct regmap *regmap;
 	struct regulator *dac_reg;
-	struct i2c_adapter *ddc;
 	struct clk *clk;
 	struct clk *di_sel_clk;
 	struct clk_hw clk_hw_di;
 	struct clk *di_clk;
 };
 
-static inline struct imx_tve *con_to_tve(struct drm_connector *c)
+struct imx_tve_encoder {
+	struct drm_encoder encoder;
+	struct imx_tve *tve;
+};
+
+struct imx_tve_connector {
+	struct drm_connector connector;
+	struct imx_tve *tve;
+	struct i2c_adapter *ddc;
+};
+
+static inline struct imx_tve_connector *con_to_tvec(struct drm_connector *c)
+{
+	return container_of(c, struct imx_tve_connector, connector);
+}
+
+static inline struct imx_tve_encoder *enc_to_tvee(struct drm_encoder *e)
 {
-	return container_of(c, struct imx_tve, connector);
+	return container_of(e, struct imx_tve_encoder, encoder);
 }
 
 static inline struct imx_tve *enc_to_tve(struct drm_encoder *e)
 {
-	return container_of(e, struct imx_tve, encoder);
+	struct imx_tve_encoder *tvee = enc_to_tvee(e);
+
+	return tvee->tve;
 }
 
 static void tve_lock(void *__tve)
@@ -218,16 +233,26 @@ static int tve_setup_vga(struct imx_tve *tve)
 				 TVE_TVDAC_TEST_MODE_MASK, 1);
 }
 
+static void imx_tve_connector_destroy(struct drm_connector *connector)
+{
+	struct imx_tve_connector *tvec = con_to_tvec(connector);
+
+	imx_drm_connector_destroy(connector);
+	i2c_put_adapter(tvec->ddc);
+	/* avoid dangling pointers */
+	tvec->tve = NULL;
+}
+
 static int imx_tve_connector_get_modes(struct drm_connector *connector)
 {
-	struct imx_tve *tve = con_to_tve(connector);
+	struct imx_tve_connector *tvec = con_to_tvec(connector);
 	struct edid *edid;
 	int ret = 0;
 
-	if (!tve->ddc)
+	if (!tvec->ddc)
 		return 0;
 
-	edid = drm_get_edid(connector, tve->ddc);
+	edid = drm_get_edid(connector, tvec->ddc);
 	if (edid) {
 		drm_connector_update_edid_property(connector, edid);
 		ret = drm_add_edid_modes(connector, edid);
@@ -240,7 +265,8 @@ static int imx_tve_connector_get_modes(struct drm_connector *connector)
 static int imx_tve_connector_mode_valid(struct drm_connector *connector,
 					struct drm_display_mode *mode)
 {
-	struct imx_tve *tve = con_to_tve(connector);
+	struct imx_tve_connector *tvec = con_to_tvec(connector);
+	struct imx_tve *tve = tvec->tve;
 	unsigned long rate;
 
 	/* pixel clock with 2x oversampling */
@@ -259,6 +285,15 @@ static int imx_tve_connector_mode_valid(struct drm_connector *connector,
 	return MODE_BAD;
 }
 
+static void imx_tve_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct imx_tve_encoder *tvee = enc_to_tvee(encoder);
+
+	drm_encoder_cleanup(encoder);
+	/* avoid dangling pointers */
+	tvee->tve = NULL;
+}
+
 static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
 				     struct drm_display_mode *orig_mode,
 				     struct drm_display_mode *mode)
@@ -328,7 +363,7 @@ static int imx_tve_atomic_check(struct drm_encoder *encoder,
 
 static const struct drm_connector_funcs imx_tve_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = imx_drm_connector_destroy,
+	.destroy = imx_tve_connector_destroy,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
@@ -340,7 +375,7 @@ static const struct drm_connector_helper_funcs imx_tve_connector_helper_funcs =
 };
 
 static const struct drm_encoder_funcs imx_tve_encoder_funcs = {
-	.destroy = imx_drm_encoder_destroy,
+	.destroy = imx_tve_encoder_destroy,
 };
 
 static const struct drm_encoder_helper_funcs imx_tve_encoder_helper_funcs = {
@@ -457,30 +492,31 @@ static int tve_clk_init(struct imx_tve *tve, void __iomem *base)
 	return 0;
 }
 
-static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
+static int imx_tve_register(struct drm_device *drm,
+			    struct imx_tve_encoder *tvee,
+			    struct imx_tve_connector *tvec)
 {
+	struct imx_tve *tve = tvee->tve;
+	struct drm_encoder *encoder = &tvee->encoder;
+	struct drm_connector *connector = &tvec->connector;
 	int encoder_type;
 	int ret;
 
 	encoder_type = tve->mode == TVE_MODE_VGA ?
 				DRM_MODE_ENCODER_DAC : DRM_MODE_ENCODER_TVDAC;
 
-	ret = imx_drm_encoder_parse_of(drm, &tve->encoder, tve->dev->of_node);
+	ret = imx_drm_encoder_parse_of(drm, encoder, tve->dev->of_node);
 	if (ret)
 		return ret;
 
-	drm_encoder_helper_add(&tve->encoder, &imx_tve_encoder_helper_funcs);
-	drm_encoder_init(drm, &tve->encoder, &imx_tve_encoder_funcs,
-			 encoder_type, NULL);
+	drm_encoder_helper_add(encoder, &imx_tve_encoder_helper_funcs);
+	drm_encoder_init(drm, encoder, &imx_tve_encoder_funcs, encoder_type, NULL);
 
-	drm_connector_helper_add(&tve->connector,
-			&imx_tve_connector_helper_funcs);
-	drm_connector_init_with_ddc(drm, &tve->connector,
-				    &imx_tve_connector_funcs,
-				    DRM_MODE_CONNECTOR_VGA,
-				    tve->ddc);
+	drm_connector_helper_add(connector, &imx_tve_connector_helper_funcs);
+	drm_connector_init_with_ddc(drm, connector, &imx_tve_connector_funcs,
+				    DRM_MODE_CONNECTOR_VGA, tvec->ddc);
 
-	drm_connector_attach_encoder(&tve->connector, &tve->encoder);
+	drm_connector_attach_encoder(connector, encoder);
 
 	return 0;
 }
@@ -533,10 +569,50 @@ static const int of_get_tve_mode(struct device_node *np)
 
 static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
+	struct imx_tve *imx_tve = dev_get_drvdata(dev);
 	struct device_node *ddc_node;
+	struct imx_tve_encoder *tvee;
+	struct imx_tve_connector *tvec;
+	int ret;
+
+	tvee = devm_kzalloc(dev, sizeof(*tvee), GFP_KERNEL);
+	if (!tvee)
+		return -ENOMEM;
+
+	tvec = devm_kzalloc(dev, sizeof(*tvec), GFP_KERNEL);
+	if (!tvec)
+		return -ENOMEM;
+
+	tvee->tve = imx_tve;
+	tvec->tve = imx_tve;
+
+	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		tvec->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+	}
+
+	ret = imx_tve_register(drm, tvee, tvec);
+	if (ret)
+		goto err_put_ddc;
+
+	return 0;
+
+err_put_ddc:
+	i2c_put_adapter(tvec->ddc);
+	return ret;
+}
+
+static const struct component_ops imx_tve_ops = {
+	.bind	= imx_tve_bind,
+};
+
+static int imx_tve_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct imx_tve *tve;
 	struct resource *res;
 	void __iomem *base;
@@ -551,12 +627,6 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	tve->dev = dev;
 	spin_lock_init(&tve->lock);
 
-	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
-	if (ddc_node) {
-		tve->ddc = of_find_i2c_adapter_by_node(ddc_node);
-		of_node_put(ddc_node);
-	}
-
 	tve->mode = of_get_tve_mode(np);
 	if (tve->mode != TVE_MODE_VGA) {
 		dev_err(dev, "only VGA mode supported, currently\n");
@@ -656,21 +726,8 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	ret = imx_tve_register(drm, tve);
-	if (ret)
-		return ret;
-
 	dev_set_drvdata(dev, tve);
 
-	return 0;
-}
-
-static const struct component_ops imx_tve_ops = {
-	.bind	= imx_tve_bind,
-};
-
-static int imx_tve_probe(struct platform_device *pdev)
-{
 	return component_add(&pdev->dev, &imx_tve_ops);
 }
 
-- 
2.20.1

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

[PATCH 09/17] drm/imx: imx-ldb: remove useless enum

[Marco Felsch]

Since commit 5e501ed7253b ("drm/imx: imx-ldb: allow to determine bus
format from the connected panel") the enum isn't used anymore. Drop it
to cleanup the code a bit.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index cd399058b00e..92fb85703c73 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -468,11 +468,6 @@ static int imx_ldb_register(struct drm_device *drm,
 	return 0;
 }
 
-enum {
-	LVDS_BIT_MAP_SPWG,
-	LVDS_BIT_MAP_JEIDA
-};
-
 struct imx_ldb_bit_mapping {
 	u32 bus_format;
 	u32 datawidth;
-- 
2.20.1

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

[PATCH 10/17] drm/imx: imx-ldb: fix edid memory leak

[Marco Felsch]

The edid memory is only freed if the component.unbind() is called. This
is okay if the imx-ldb was bound but if the bind() fails we leek the
memory.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 92fb85703c73..06b435f9b6c9 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -552,9 +552,9 @@ static int imx_ldb_panel_ddc(struct device *dev,
 		edidp = of_get_property(child, "edid",
 					&channel->edid_len);
 		if (edidp) {
-			channel->edid = kmemdup(edidp,
-						channel->edid_len,
-						GFP_KERNEL);
+			channel->edid = devm_kmemdup(dev, edidp,
+						     channel->edid_len,
+						     GFP_KERNEL);
 		} else if (!channel->panel) {
 			/* fallback to display-timings node */
 			ret = of_get_drm_display_mode(child,
@@ -711,7 +711,6 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
 		if (channel->panel)
 			drm_panel_detach(channel->panel);
 
-		kfree(channel->edid);
 		i2c_put_adapter(channel->ddc);
 	}
 }
-- 
2.20.1

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

[PATCH 11/17] drm/imx: imx-ldb: release ldb-channel resources within encoder destroy

[Marco Felsch]

There is no reason why the resoruces should be freed manually within the
component.unbind() call instead it is easier to use the drm-core destroy
helper. The destroy helper gets called for each registered encoder
during a drm_mode_config_cleanup() call.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 06b435f9b6c9..87bf659990da 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -155,6 +155,16 @@ static int imx_ldb_connector_get_modes(struct drm_connector *connector)
 	return num_modes;
 }
 
+static void imx_ldb_encoder_destroy(struct drm_encoder *encoder)
+{
+	struct imx_ldb_channel *channel = enc_to_imx_ldb_ch(encoder);
+
+	if (channel->panel)
+		drm_panel_detach(channel->panel);
+	drm_encoder_cleanup(encoder);
+	i2c_put_adapter(channel->ddc);
+}
+
 static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
 		unsigned long serial_clk, unsigned long di_clk)
 {
@@ -385,7 +395,7 @@ static const struct drm_connector_helper_funcs imx_ldb_connector_helper_funcs =
 };
 
 static const struct drm_encoder_funcs imx_ldb_encoder_funcs = {
-	.destroy = imx_drm_encoder_destroy,
+	.destroy = imx_ldb_encoder_destroy,
 };
 
 static const struct drm_encoder_helper_funcs imx_ldb_encoder_helper_funcs = {
@@ -699,25 +709,8 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 	return ret;
 }
 
-static void imx_ldb_unbind(struct device *dev, struct device *master,
-	void *data)
-{
-	struct imx_ldb *imx_ldb = dev_get_drvdata(dev);
-	int i;
-
-	for (i = 0; i < 2; i++) {
-		struct imx_ldb_channel *channel = &imx_ldb->channel[i];
-
-		if (channel->panel)
-			drm_panel_detach(channel->panel);
-
-		i2c_put_adapter(channel->ddc);
-	}
-}
-
 static const struct component_ops imx_ldb_ops = {
 	.bind	= imx_ldb_bind,
-	.unbind	= imx_ldb_unbind,
 };
 
 static int imx_ldb_probe(struct platform_device *pdev)
-- 
2.20.1

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

[PATCH 12/17] drm/imx: remove imx_drm_encoder_destroy helper

[Marco Felsch]

This helper only added an additional layer without adding
simplifications. All former users of this API are converted to call
drm_encoder_cleanup() directly.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 6 ------
 drivers/gpu/drm/imx/imx-drm.h      | 1 -
 2 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index da87c70e413b..9979547ca883 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -42,12 +42,6 @@ void imx_drm_connector_destroy(struct drm_connector *connector)
 }
 EXPORT_SYMBOL_GPL(imx_drm_connector_destroy);
 
-void imx_drm_encoder_destroy(struct drm_encoder *encoder)
-{
-	drm_encoder_cleanup(encoder);
-}
-EXPORT_SYMBOL_GPL(imx_drm_encoder_destroy);
-
 static int imx_drm_atomic_check(struct drm_device *dev,
 				struct drm_atomic_state *state)
 {
diff --git a/drivers/gpu/drm/imx/imx-drm.h b/drivers/gpu/drm/imx/imx-drm.h
index ab9c6f706eb3..c3e1a3f14d30 100644
--- a/drivers/gpu/drm/imx/imx-drm.h
+++ b/drivers/gpu/drm/imx/imx-drm.h
@@ -38,7 +38,6 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	struct drm_encoder *encoder, struct device_node *np);
 
 void imx_drm_connector_destroy(struct drm_connector *connector);
-void imx_drm_encoder_destroy(struct drm_encoder *encoder);
 
 int ipu_planes_assign_pre(struct drm_device *dev,
 			  struct drm_atomic_state *state);
-- 
2.20.1

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

[PATCH 13/17] drm/imx: imx-ldb: split imx_ldb devres allocation context

[Marco Felsch]

This patch prepares the driver for a overall 'make drm/imx
drm_mode_config_cleanup() aware' patch to avoid race conditions like
[1]. The patch also improves the driver memory usage by allocating only
the necessary number of 'struct imx_ldb_channel'.

To make the driver drm_mode_config_cleanup() aware we need to split the
driver state 'struct imx_ldb'. All the hardware settings like clocks and
the regmap can be allocated during the driver probe() and so the devres
context is moved to the probe/remove.

Now upon a component.bind() call only the necessary number of channels
are allocated. The difference is that now the 'struct imx_ldb_channel'
memory is allocated within the bind/unbind devres context.

So now the 'struct imx_ldb' driver state is still available after a
component.unbind() call.

[1] https://www.spinics.net/lists/dri-devel/msg189388.html

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 187 ++++++++++++++++++----------------
 1 file changed, 98 insertions(+), 89 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 87bf659990da..e3fa46e1639d 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -87,7 +87,6 @@ struct bus_mux {
 struct imx_ldb {
 	struct regmap *regmap;
 	struct device *dev;
-	struct imx_ldb_channel channel[2];
 	struct clk *clk[2]; /* our own clock */
 	struct clk *clk_sel[4]; /* parent of display clock */
 	struct clk *clk_parent[4]; /* original parent of clk_sel */
@@ -212,14 +211,14 @@ static void imx_ldb_encoder_enable(struct drm_encoder *encoder)
 		clk_set_parent(ldb->clk_sel[mux], ldb->clk[imx_ldb_ch->chno]);
 	}
 
-	if (imx_ldb_ch == &ldb->channel[0] || dual) {
+	if (imx_ldb_ch->chno == 0 || dual) {
 		ldb->ldb_ctrl &= ~LDB_CH0_MODE_EN_MASK;
 		if (mux == 0 || ldb->lvds_mux)
 			ldb->ldb_ctrl |= LDB_CH0_MODE_EN_TO_DI0;
 		else if (mux == 1)
 			ldb->ldb_ctrl |= LDB_CH0_MODE_EN_TO_DI1;
 	}
-	if (imx_ldb_ch == &ldb->channel[1] || dual) {
+	if (imx_ldb_ch->chno == 1 || dual) {
 		ldb->ldb_ctrl &= ~LDB_CH1_MODE_EN_MASK;
 		if (mux == 1 || ldb->lvds_mux)
 			ldb->ldb_ctrl |= LDB_CH1_MODE_EN_TO_DI1;
@@ -230,9 +229,9 @@ static void imx_ldb_encoder_enable(struct drm_encoder *encoder)
 	if (ldb->lvds_mux) {
 		const struct bus_mux *lvds_mux = NULL;
 
-		if (imx_ldb_ch == &ldb->channel[0])
+		if (imx_ldb_ch->chno == 0)
 			lvds_mux = &ldb->lvds_mux[0];
-		else if (imx_ldb_ch == &ldb->channel[1])
+		else if (imx_ldb_ch->chno == 1)
 			lvds_mux = &ldb->lvds_mux[1];
 
 		regmap_update_bits(ldb->regmap, lvds_mux->reg, lvds_mux->mask,
@@ -278,13 +277,13 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder *encoder,
 	}
 
 	/* FIXME - assumes straight connections DI0 --> CH0, DI1 --> CH1 */
-	if (imx_ldb_ch == &ldb->channel[0] || dual) {
+	if (imx_ldb_ch->chno == 0 || dual) {
 		if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 			ldb->ldb_ctrl |= LDB_DI0_VS_POL_ACT_LOW;
 		else if (mode->flags & DRM_MODE_FLAG_PVSYNC)
 			ldb->ldb_ctrl &= ~LDB_DI0_VS_POL_ACT_LOW;
 	}
-	if (imx_ldb_ch == &ldb->channel[1] || dual) {
+	if (imx_ldb_ch->chno == 1 || dual) {
 		if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 			ldb->ldb_ctrl |= LDB_DI1_VS_POL_ACT_LOW;
 		else if (mode->flags & DRM_MODE_FLAG_PVSYNC)
@@ -309,9 +308,9 @@ static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
 
 	drm_panel_disable(imx_ldb_ch->panel);
 
-	if (imx_ldb_ch == &ldb->channel[0])
+	if (imx_ldb_ch->chno == 0)
 		ldb->ldb_ctrl &= ~LDB_CH0_MODE_EN_MASK;
-	else if (imx_ldb_ch == &ldb->channel[1])
+	else if (imx_ldb_ch->chno == 1)
 		ldb->ldb_ctrl &= ~LDB_CH1_MODE_EN_MASK;
 
 	regmap_write(ldb->regmap, IOMUXC_GPR2, ldb->ldb_ctrl);
@@ -324,16 +323,16 @@ static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
 	if (ldb->lvds_mux) {
 		const struct bus_mux *lvds_mux = NULL;
 
-		if (imx_ldb_ch == &ldb->channel[0])
+		if (imx_ldb_ch->chno == 0)
 			lvds_mux = &ldb->lvds_mux[0];
-		else if (imx_ldb_ch == &ldb->channel[1])
+		else if (imx_ldb_ch->chno == 1)
 			lvds_mux = &ldb->lvds_mux[1];
 
 		regmap_read(ldb->regmap, lvds_mux->reg, &mux);
 		mux &= lvds_mux->mask;
 		mux >>= lvds_mux->shift;
 	} else {
-		mux = (imx_ldb_ch == &ldb->channel[0]) ? 0 : 1;
+		mux = imx_ldb_ch->chno;
 	}
 
 	/* set display clock mux back to original input clock */
@@ -513,31 +512,6 @@ static u32 of_get_bus_format(struct device *dev, struct device_node *np)
 	return -ENOENT;
 }
 
-static struct bus_mux imx6q_lvds_mux[2] = {
-	{
-		.reg = IOMUXC_GPR3,
-		.shift = 6,
-		.mask = IMX6Q_GPR3_LVDS0_MUX_CTL_MASK,
-	}, {
-		.reg = IOMUXC_GPR3,
-		.shift = 8,
-		.mask = IMX6Q_GPR3_LVDS1_MUX_CTL_MASK,
-	}
-};
-
-/*
- * For a device declaring compatible = "fsl,imx6q-ldb", "fsl,imx53-ldb",
- * of_match_device will walk through this list and take the first entry
- * matching any of its compatible values. Therefore, the more generic
- * entries (in this case fsl,imx53-ldb) need to be ordered last.
- */
-static const struct of_device_id imx_ldb_dt_ids[] = {
-	{ .compatible = "fsl,imx6q-ldb", .data = imx6q_lvds_mux, },
-	{ .compatible = "fsl,imx53-ldb", .data = NULL, },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, imx_ldb_dt_ids);
-
 static int imx_ldb_panel_ddc(struct device *dev,
 		struct imx_ldb_channel *channel, struct device_node *child)
 {
@@ -582,59 +556,12 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
-	const struct of_device_id *of_id =
-			of_match_device(imx_ldb_dt_ids, dev);
+	struct imx_ldb *imx_ldb = dev_get_drvdata(dev);
+	int dual = imx_ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
 	struct device_node *child;
-	struct imx_ldb *imx_ldb;
-	int dual;
 	int ret;
 	int i;
 
-	imx_ldb = devm_kzalloc(dev, sizeof(*imx_ldb), GFP_KERNEL);
-	if (!imx_ldb)
-		return -ENOMEM;
-
-	imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
-	if (IS_ERR(imx_ldb->regmap)) {
-		dev_err(dev, "failed to get parent regmap\n");
-		return PTR_ERR(imx_ldb->regmap);
-	}
-
-	/* disable LDB by resetting the control register to POR default */
-	regmap_write(imx_ldb->regmap, IOMUXC_GPR2, 0);
-
-	imx_ldb->dev = dev;
-
-	if (of_id)
-		imx_ldb->lvds_mux = of_id->data;
-
-	dual = of_property_read_bool(np, "fsl,dual-channel");
-	if (dual)
-		imx_ldb->ldb_ctrl |= LDB_SPLIT_MODE_EN;
-
-	/*
-	 * There are three different possible clock mux configurations:
-	 * i.MX53:  ipu1_di0_sel, ipu1_di1_sel
-	 * i.MX6q:  ipu1_di0_sel, ipu1_di1_sel, ipu2_di0_sel, ipu2_di1_sel
-	 * i.MX6dl: ipu1_di0_sel, ipu1_di1_sel, lcdif_sel
-	 * Map them all to di0_sel...di3_sel.
-	 */
-	for (i = 0; i < 4; i++) {
-		char clkname[16];
-
-		sprintf(clkname, "di%d_sel", i);
-		imx_ldb->clk_sel[i] = devm_clk_get(imx_ldb->dev, clkname);
-		if (IS_ERR(imx_ldb->clk_sel[i])) {
-			ret = PTR_ERR(imx_ldb->clk_sel[i]);
-			imx_ldb->clk_sel[i] = NULL;
-			break;
-		}
-
-		imx_ldb->clk_parent[i] = clk_get_parent(imx_ldb->clk_sel[i]);
-	}
-	if (i == 0)
-		return ret;
-
 	for_each_child_of_node(np, child) {
 		struct imx_ldb_channel *channel;
 		int bus_format;
@@ -653,7 +580,10 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 			continue;
 		}
 
-		channel = &imx_ldb->channel[i];
+		channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
+		if (!channel)
+			return -ENOMEM;
+
 		channel->ldb = imx_ldb;
 		channel->chno = i;
 
@@ -700,8 +630,6 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		}
 	}
 
-	dev_set_drvdata(dev, imx_ldb);
-
 	return 0;
 
 free_child:
@@ -713,8 +641,89 @@ static const struct component_ops imx_ldb_ops = {
 	.bind	= imx_ldb_bind,
 };
 
+static struct bus_mux imx6q_lvds_mux[2] = {
+	{
+		.reg = IOMUXC_GPR3,
+		.shift = 6,
+		.mask = IMX6Q_GPR3_LVDS0_MUX_CTL_MASK,
+	}, {
+		.reg = IOMUXC_GPR3,
+		.shift = 8,
+		.mask = IMX6Q_GPR3_LVDS1_MUX_CTL_MASK,
+	}
+};
+
+/*
+ * For a device declaring compatible = "fsl,imx6q-ldb", "fsl,imx53-ldb",
+ * of_match_device will walk through this list and take the first entry
+ * matching any of its compatible values. Therefore, the more generic
+ * entries (in this case fsl,imx53-ldb) need to be ordered last.
+ */
+static const struct of_device_id imx_ldb_dt_ids[] = {
+	{ .compatible = "fsl,imx6q-ldb", .data = imx6q_lvds_mux, },
+	{ .compatible = "fsl,imx53-ldb", .data = NULL, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, imx_ldb_dt_ids);
+
 static int imx_ldb_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id =
+			of_match_device(imx_ldb_dt_ids, dev);
+	struct device_node *np = dev->of_node;
+	struct imx_ldb *imx_ldb;
+	int dual;
+	int ret;
+	int i;
+
+	imx_ldb = devm_kzalloc(dev, sizeof(*imx_ldb), GFP_KERNEL);
+	if (!imx_ldb)
+		return -ENOMEM;
+
+	imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
+	if (IS_ERR(imx_ldb->regmap)) {
+		dev_err(dev, "failed to get parent regmap\n");
+		return PTR_ERR(imx_ldb->regmap);
+	}
+
+	/* disable LDB by resetting the control register to POR default */
+	regmap_write(imx_ldb->regmap, IOMUXC_GPR2, 0);
+
+	imx_ldb->dev = dev;
+
+	if (of_id)
+		imx_ldb->lvds_mux = of_id->data;
+
+	dual = of_property_read_bool(np, "fsl,dual-channel");
+	if (dual)
+		imx_ldb->ldb_ctrl |= LDB_SPLIT_MODE_EN;
+
+	/*
+	 * There are three different possible clock mux configurations:
+	 * i.MX53:  ipu1_di0_sel, ipu1_di1_sel
+	 * i.MX6q:  ipu1_di0_sel, ipu1_di1_sel, ipu2_di0_sel, ipu2_di1_sel
+	 * i.MX6dl: ipu1_di0_sel, ipu1_di1_sel, lcdif_sel
+	 * Map them all to di0_sel...di3_sel.
+	 */
+	for (i = 0; i < 4; i++) {
+		char clkname[16];
+
+		sprintf(clkname, "di%d_sel", i);
+		imx_ldb->clk_sel[i] = devm_clk_get(imx_ldb->dev, clkname);
+		if (IS_ERR(imx_ldb->clk_sel[i])) {
+			ret = PTR_ERR(imx_ldb->clk_sel[i]);
+			imx_ldb->clk_sel[i] = NULL;
+			break;
+		}
+
+		imx_ldb->clk_parent[i] = clk_get_parent(imx_ldb->clk_sel[i]);
+	}
+	if (i == 0)
+		return ret;
+
+	dev_set_drvdata(dev, imx_ldb);
+
 	return component_add(&pdev->dev, &imx_ldb_ops);
 }
 
-- 
2.20.1

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

[PATCH 14/17] drm/imx: imx-ldb: add ldb_is_dual helper

[Marco Felsch]

There where several places where this bit operation is made. Add a
simple helper so we don't need to remember the bit each time.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index e3fa46e1639d..5ef9fcb9ae94 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -95,11 +95,16 @@ struct imx_ldb {
 	const struct bus_mux *lvds_mux;
 };
 
+static inline int imx_ldb_is_dual(struct imx_ldb *ldb)
+{
+	return ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
+}
+
 static void imx_ldb_ch_set_bus_format(struct imx_ldb_channel *imx_ldb_ch,
 				      u32 bus_format)
 {
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
+	int dual = imx_ldb_is_dual(ldb);
 
 	switch (bus_format) {
 	case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
@@ -196,7 +201,7 @@ static void imx_ldb_encoder_enable(struct drm_encoder *encoder)
 {
 	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
+	int dual = imx_ldb_is_dual(ldb);
 	int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
 
 	drm_panel_prepare(imx_ldb_ch->panel);
@@ -251,7 +256,7 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder *encoder,
 	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
+	int dual = imx_ldb_is_dual(ldb);
 	unsigned long serial_clk;
 	unsigned long di_clk = mode->clock * 1000;
 	int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
@@ -315,7 +320,7 @@ static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
 
 	regmap_write(ldb->regmap, IOMUXC_GPR2, ldb->ldb_ctrl);
 
-	if (ldb->ldb_ctrl & LDB_SPLIT_MODE_EN) {
+	if (imx_ldb_is_dual(ldb)) {
 		clk_disable_unprepare(ldb->clk[0]);
 		clk_disable_unprepare(ldb->clk[1]);
 	}
@@ -434,7 +439,7 @@ static int imx_ldb_register(struct drm_device *drm,
 	if (ret)
 		return ret;
 
-	if (ldb->ldb_ctrl & LDB_SPLIT_MODE_EN) {
+	if (imx_ldb_is_dual(ldb)) {
 		ret = imx_ldb_get_clk(ldb, 1);
 		if (ret)
 			return ret;
@@ -557,7 +562,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
 	struct imx_ldb *imx_ldb = dev_get_drvdata(dev);
-	int dual = imx_ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
+	int dual = imx_ldb_is_dual(imx_ldb);
 	struct device_node *child;
 	int ret;
 	int i;
-- 
2.20.1

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

[PATCH 15/17] drm/imx: imx-ldb: split encoder and decoder states

[Marco Felsch]

The race condition reported by [1] applies to the all drm-imx
subcomponent drivers which implements an encoder:connector combination.
The goal is to switch from the devres-kmalloc and the component
framework to a 'normal' kmalloc and the drm framework to release the
memory resources. So all acquired memory resoruces are freed by a
drm_mode_config_cleanup() call and the ->destroy() callbacks. For this
purpose we need to slit the drm_encoder and the drm_connector state.

[1] https://www.spinics.net/lists/dri-devel/msg189388.html

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 106 +++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 5ef9fcb9ae94..0e5a3c84df10 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -50,7 +50,6 @@ struct imx_ldb;
 
 struct imx_ldb_channel {
 	struct imx_ldb *ldb;
-	struct drm_connector connector;
 	struct drm_encoder encoder;
 
 	/* Defines what is connected to the ldb, only one at a time */
@@ -58,19 +57,26 @@ struct imx_ldb_channel {
 	struct drm_bridge *bridge;
 
 	struct device_node *child;
-	struct i2c_adapter *ddc;
 	int chno;
+	u32 bus_format;
+	u32 bus_flags;
+};
+
+struct imx_ldb_connector {
+	struct imx_ldb_channel *ldb_channel;
+	struct drm_connector connector;
+
+	struct i2c_adapter *ddc;
 	void *edid;
 	int edid_len;
+
 	struct drm_display_mode mode;
 	int mode_valid;
-	u32 bus_format;
-	u32 bus_flags;
 };
 
-static inline struct imx_ldb_channel *con_to_imx_ldb_ch(struct drm_connector *c)
+static inline struct imx_ldb_connector *con_to_imx_ldb_con(struct drm_connector *c)
 {
-	return container_of(c, struct imx_ldb_channel, connector);
+	return container_of(c, struct imx_ldb_connector, connector);
 }
 
 static inline struct imx_ldb_channel *enc_to_imx_ldb_ch(struct drm_encoder *e)
@@ -126,31 +132,41 @@ static void imx_ldb_ch_set_bus_format(struct imx_ldb_channel *imx_ldb_ch,
 	}
 }
 
+static void imx_ldb_connector_destroy(struct drm_connector *connector)
+{
+	struct imx_ldb_connector *imx_ldb_con = con_to_imx_ldb_con(connector);
+
+	imx_drm_connector_destroy(connector);
+	i2c_put_adapter(imx_ldb_con->ddc);
+	/* avoid dangling pointers */
+	imx_ldb_con->ldb_channel = NULL;
+}
+
 static int imx_ldb_connector_get_modes(struct drm_connector *connector)
 {
-	struct imx_ldb_channel *imx_ldb_ch = con_to_imx_ldb_ch(connector);
+	struct imx_ldb_connector *imx_ldb_con = con_to_imx_ldb_con(connector);
+	struct imx_ldb_channel *imx_ldb_ch = imx_ldb_con->ldb_channel;
 	int num_modes;
 
 	num_modes = drm_panel_get_modes(imx_ldb_ch->panel, connector);
 	if (num_modes > 0)
 		return num_modes;
 
-	if (!imx_ldb_ch->edid && imx_ldb_ch->ddc)
-		imx_ldb_ch->edid = drm_get_edid(connector, imx_ldb_ch->ddc);
+	if (!imx_ldb_con->edid && imx_ldb_con->ddc)
+		imx_ldb_con->edid = drm_get_edid(connector, imx_ldb_con->ddc);
 
-	if (imx_ldb_ch->edid) {
-		drm_connector_update_edid_property(connector,
-							imx_ldb_ch->edid);
-		num_modes = drm_add_edid_modes(connector, imx_ldb_ch->edid);
+	if (imx_ldb_con->edid) {
+		drm_connector_update_edid_property(connector, imx_ldb_con->edid);
+		num_modes = drm_add_edid_modes(connector, imx_ldb_con->edid);
 	}
 
-	if (imx_ldb_ch->mode_valid) {
+	if (imx_ldb_con->mode_valid) {
 		struct drm_display_mode *mode;
 
 		mode = drm_mode_create(connector->dev);
 		if (!mode)
 			return -EINVAL;
-		drm_mode_copy(mode, &imx_ldb_ch->mode);
+		drm_mode_copy(mode, &imx_ldb_con->mode);
 		mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
 		drm_mode_probed_add(connector, mode);
 		num_modes++;
@@ -166,7 +182,8 @@ static void imx_ldb_encoder_destroy(struct drm_encoder *encoder)
 	if (channel->panel)
 		drm_panel_detach(channel->panel);
 	drm_encoder_cleanup(encoder);
-	i2c_put_adapter(channel->ddc);
+	/* avoid dangling pointers */
+	channel->ldb = NULL;
 }
 
 static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
@@ -388,7 +405,7 @@ static int imx_ldb_encoder_atomic_check(struct drm_encoder *encoder,
 
 static const struct drm_connector_funcs imx_ldb_connector_funcs = {
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = imx_drm_connector_destroy,
+	.destroy = imx_ldb_connector_destroy,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
@@ -424,11 +441,13 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
 	return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]);
 }
 
-static int imx_ldb_register(struct drm_device *drm,
-	struct imx_ldb_channel *imx_ldb_ch)
+static int imx_ldb_register_channel(struct drm_device *drm,
+				    struct imx_ldb_channel *imx_ldb_ch,
+				    struct imx_ldb_connector *imx_ldb_con)
 {
 	struct imx_ldb *ldb = imx_ldb_ch->ldb;
 	struct drm_encoder *encoder = &imx_ldb_ch->encoder;
+	struct drm_connector *connector = &imx_ldb_con->connector;
 	int ret;
 
 	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
@@ -450,8 +469,7 @@ static int imx_ldb_register(struct drm_device *drm,
 			 DRM_MODE_ENCODER_LVDS, NULL);
 
 	if (imx_ldb_ch->bridge) {
-		ret = drm_bridge_attach(&imx_ldb_ch->encoder,
-					imx_ldb_ch->bridge, NULL);
+		ret = drm_bridge_attach(encoder, imx_ldb_ch->bridge, NULL);
 		if (ret) {
 			DRM_ERROR("Failed to initialize bridge with drm\n");
 			return ret;
@@ -463,18 +481,17 @@ static int imx_ldb_register(struct drm_device *drm,
 		 * historical reasons, the ldb driver can also work without
 		 * a panel.
 		 */
-		drm_connector_helper_add(&imx_ldb_ch->connector,
-				&imx_ldb_connector_helper_funcs);
-		drm_connector_init_with_ddc(drm, &imx_ldb_ch->connector,
+		drm_connector_helper_add(connector,
+					 &imx_ldb_connector_helper_funcs);
+		drm_connector_init_with_ddc(drm, connector,
 					    &imx_ldb_connector_funcs,
 					    DRM_MODE_CONNECTOR_LVDS,
-					    imx_ldb_ch->ddc);
-		drm_connector_attach_encoder(&imx_ldb_ch->connector, encoder);
+					    imx_ldb_con->ddc);
+		drm_connector_attach_encoder(connector, encoder);
 	}
 
 	if (imx_ldb_ch->panel) {
-		ret = drm_panel_attach(imx_ldb_ch->panel,
-				       &imx_ldb_ch->connector);
+		ret = drm_panel_attach(imx_ldb_ch->panel, connector);
 		if (ret)
 			return ret;
 	}
@@ -518,7 +535,9 @@ static u32 of_get_bus_format(struct device *dev, struct device_node *np)
 }
 
 static int imx_ldb_panel_ddc(struct device *dev,
-		struct imx_ldb_channel *channel, struct device_node *child)
+			     struct imx_ldb_channel *channel,
+			     struct imx_ldb_connector *connector,
+			     struct device_node *child)
 {
 	struct device_node *ddc_node;
 	const u8 *edidp;
@@ -526,32 +545,32 @@ static int imx_ldb_panel_ddc(struct device *dev,
 
 	ddc_node = of_parse_phandle(child, "ddc-i2c-bus", 0);
 	if (ddc_node) {
-		channel->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		connector->ddc = of_find_i2c_adapter_by_node(ddc_node);
 		of_node_put(ddc_node);
-		if (!channel->ddc) {
+		if (!connector->ddc) {
 			dev_warn(dev, "failed to get ddc i2c adapter\n");
 			return -EPROBE_DEFER;
 		}
 	}
 
-	if (!channel->ddc) {
+	if (!connector->ddc) {
 		/* if no DDC available, fallback to hardcoded EDID */
 		dev_dbg(dev, "no ddc available\n");
 
 		edidp = of_get_property(child, "edid",
-					&channel->edid_len);
+					&connector->edid_len);
 		if (edidp) {
-			channel->edid = devm_kmemdup(dev, edidp,
-						     channel->edid_len,
-						     GFP_KERNEL);
+			connector->edid = devm_kmemdup(dev, edidp,
+						       connector->edid_len,
+						       GFP_KERNEL);
 		} else if (!channel->panel) {
 			/* fallback to display-timings node */
 			ret = of_get_drm_display_mode(child,
-						      &channel->mode,
+						      &connector->mode,
 						      &channel->bus_flags,
 						      OF_USE_NATIVE_MODE);
 			if (!ret)
-				channel->mode_valid = 1;
+				connector->mode_valid = 1;
 		}
 	}
 	return 0;
@@ -569,6 +588,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 
 	for_each_child_of_node(np, child) {
 		struct imx_ldb_channel *channel;
+		struct imx_ldb_connector *connector;
 		int bus_format;
 
 		ret = of_property_read_u32(child, "reg", &i);
@@ -604,9 +624,15 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 
 		/* panel ddc only if there is no bridge */
 		if (!channel->bridge) {
-			ret = imx_ldb_panel_ddc(dev, channel, child);
+			connector = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
+			if (!connector)
+				return -ENOMEM;
+
+			ret = imx_ldb_panel_ddc(dev, channel, connector, child);
 			if (ret)
 				goto free_child;
+
+			connector->ldb_channel = channel;
 		}
 
 		bus_format = of_get_bus_format(dev, child);
@@ -628,7 +654,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		channel->bus_format = bus_format;
 		channel->child = child;
 
-		ret = imx_ldb_register(drm, channel);
+		ret = imx_ldb_register_channel(drm, channel, connector);
 		if (ret) {
 			channel->child = NULL;
 			goto free_child;
-- 
2.20.1

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

[PATCH 16/17] drm/imx: imx-ldb: refactor imx_ldb_bind

[Marco Felsch]

Refactor the function to easaly construct error paths later on. The
error handling gets dirty if we don't refactor the code yet. While on it
I fixed a missing i2c_put_adapter() if the bind() fails.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 246 ++++++++++++++++++----------------
 1 file changed, 132 insertions(+), 114 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 0e5a3c84df10..5e6c1b09dbfa 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -441,64 +441,6 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno)
 	return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]);
 }
 
-static int imx_ldb_register_channel(struct drm_device *drm,
-				    struct imx_ldb_channel *imx_ldb_ch,
-				    struct imx_ldb_connector *imx_ldb_con)
-{
-	struct imx_ldb *ldb = imx_ldb_ch->ldb;
-	struct drm_encoder *encoder = &imx_ldb_ch->encoder;
-	struct drm_connector *connector = &imx_ldb_con->connector;
-	int ret;
-
-	ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child);
-	if (ret)
-		return ret;
-
-	ret = imx_ldb_get_clk(ldb, imx_ldb_ch->chno);
-	if (ret)
-		return ret;
-
-	if (imx_ldb_is_dual(ldb)) {
-		ret = imx_ldb_get_clk(ldb, 1);
-		if (ret)
-			return ret;
-	}
-
-	drm_encoder_helper_add(encoder, &imx_ldb_encoder_helper_funcs);
-	drm_encoder_init(drm, encoder, &imx_ldb_encoder_funcs,
-			 DRM_MODE_ENCODER_LVDS, NULL);
-
-	if (imx_ldb_ch->bridge) {
-		ret = drm_bridge_attach(encoder, imx_ldb_ch->bridge, NULL);
-		if (ret) {
-			DRM_ERROR("Failed to initialize bridge with drm\n");
-			return ret;
-		}
-	} else {
-		/*
-		 * We want to add the connector whenever there is no bridge
-		 * that brings its own, not only when there is a panel. For
-		 * historical reasons, the ldb driver can also work without
-		 * a panel.
-		 */
-		drm_connector_helper_add(connector,
-					 &imx_ldb_connector_helper_funcs);
-		drm_connector_init_with_ddc(drm, connector,
-					    &imx_ldb_connector_funcs,
-					    DRM_MODE_CONNECTOR_LVDS,
-					    imx_ldb_con->ddc);
-		drm_connector_attach_encoder(connector, encoder);
-	}
-
-	if (imx_ldb_ch->panel) {
-		ret = drm_panel_attach(imx_ldb_ch->panel, connector);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 struct imx_ldb_bit_mapping {
 	u32 bus_format;
 	u32 datawidth;
@@ -576,6 +518,136 @@ static int imx_ldb_panel_ddc(struct device *dev,
 	return 0;
 }
 
+static int imx_ldb_setup_channel(struct device *dev,
+				 struct device_node *child,
+				 struct drm_device *drm,
+				 struct imx_ldb *ldb,
+				 int channel_number)
+{
+	struct imx_ldb_channel *channel;
+	struct imx_ldb_connector *imx_ldb_con;
+	struct drm_encoder *encoder;
+	struct drm_connector *connector = NULL;
+	int bus_format;
+	int ret;
+
+	/*
+	 * 1) Parse all available data and alloc needed structs
+	 * 2) Setup the HW
+	 * 3) Register it with the DRM framework
+	 * 4) Attach bridge or connector to encoder
+	 */
+	channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
+	if (!channel)
+		return -ENOMEM;
+
+	channel->ldb = ldb;
+	channel->chno = channel_number;
+	channel->child = child;
+	encoder = &channel->encoder;
+
+	/*
+	 * The output port is port@4 with an external 4-port mux or
+	 * port@2 with the internal 2-port mux.
+	 */
+	ret = drm_of_find_panel_or_bridge(child,
+					  ldb->lvds_mux ? 4 : 2, 0,
+					  &channel->panel, &channel->bridge);
+	if (ret && ret != -ENODEV)
+		return ret;
+
+	/* panel ddc only if there is no bridge */
+	if (!channel->bridge) {
+		imx_ldb_con = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
+		if (!imx_ldb_con)
+			return -ENOMEM;
+
+		ret = imx_ldb_panel_ddc(dev, channel, imx_ldb_con, child);
+		if (ret)
+			return ret;
+
+		imx_ldb_con->ldb_channel = channel;
+		connector = &imx_ldb_con->connector;
+	}
+
+	bus_format = of_get_bus_format(dev, child);
+	if (bus_format == -EINVAL) {
+		/*
+		 * If no bus format was specified in the device tree,
+		 * we can still get it from the connected panel later.
+		 */
+		if (channel->panel && channel->panel->funcs &&
+		    channel->panel->funcs->get_modes)
+			bus_format = 0;
+	}
+	if (bus_format < 0) {
+		dev_err(dev, "could not determine data mapping: %d\n",
+			bus_format);
+		ret = bus_format;
+		goto err_put_ddc;
+	}
+	channel->bus_format = bus_format;
+
+	/* 2) Setup the HW */
+	ret = imx_ldb_get_clk(channel->ldb, channel->chno);
+	if (ret)
+		goto err_put_ddc;
+
+	if (imx_ldb_is_dual(ldb)) {
+		ret = imx_ldb_get_clk(ldb, 1);
+		if (ret)
+			goto err_put_ddc;
+	}
+
+	/* 3) Register it with the DRM framework */
+	ret = imx_drm_encoder_parse_of(drm, encoder, channel->child);
+	if (ret)
+		goto err_put_ddc;
+
+	drm_encoder_helper_add(encoder, &imx_ldb_encoder_helper_funcs);
+	drm_encoder_init(drm, encoder, &imx_ldb_encoder_funcs,
+			 DRM_MODE_ENCODER_LVDS, NULL);
+
+	if (!channel->bridge) {
+		/*
+		 * We want to add the connector whenever there is no bridge
+		 * that brings its own, not only when there is a panel. For
+		 * historical reasons, the ldb driver can also work without
+		 * a panel.
+		 */
+		drm_connector_helper_add(connector,
+					 &imx_ldb_connector_helper_funcs);
+		drm_connector_init_with_ddc(drm, connector,
+					    &imx_ldb_connector_funcs,
+					    DRM_MODE_CONNECTOR_LVDS,
+					    imx_ldb_con->ddc);
+	}
+
+	/* 4) Attach bridge or connector to encoder */
+	if (channel->bridge) {
+		ret = drm_bridge_attach(encoder, channel->bridge, NULL);
+		if (ret) {
+			DRM_ERROR("Failed to initialize bridge with drm\n");
+			goto err_put_ddc;
+		}
+	} else {
+		drm_connector_attach_encoder(connector, encoder);
+	}
+
+	if (channel->panel) {
+		ret = drm_panel_attach(channel->panel, connector);
+		if (ret)
+			goto err_put_ddc;
+	}
+
+	return 0;
+
+err_put_ddc:
+	if (imx_ldb_con)
+		i2c_put_adapter(imx_ldb_con->ddc);
+	return ret;
+}
+
 static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 {
 	struct drm_device *drm = data;
@@ -587,9 +659,6 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 	int i;
 
 	for_each_child_of_node(np, child) {
-		struct imx_ldb_channel *channel;
-		struct imx_ldb_connector *connector;
-		int bus_format;
 
 		ret = of_property_read_u32(child, "reg", &i);
 		if (ret || i < 0 || i > 1) {
@@ -605,60 +674,9 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 			continue;
 		}
 
-		channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
-		if (!channel)
-			return -ENOMEM;
-
-		channel->ldb = imx_ldb;
-		channel->chno = i;
-
-		/*
-		 * The output port is port@4 with an external 4-port mux or
-		 * port@2 with the internal 2-port mux.
-		 */
-		ret = drm_of_find_panel_or_bridge(child,
-						  imx_ldb->lvds_mux ? 4 : 2, 0,
-						  &channel->panel, &channel->bridge);
-		if (ret && ret != -ENODEV)
-			goto free_child;
-
-		/* panel ddc only if there is no bridge */
-		if (!channel->bridge) {
-			connector = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
-			if (!connector)
-				return -ENOMEM;
-
-			ret = imx_ldb_panel_ddc(dev, channel, connector, child);
-			if (ret)
-				goto free_child;
-
-			connector->ldb_channel = channel;
-		}
-
-		bus_format = of_get_bus_format(dev, child);
-		if (bus_format == -EINVAL) {
-			/*
-			 * If no bus format was specified in the device tree,
-			 * we can still get it from the connected panel later.
-			 */
-			if (channel->panel && channel->panel->funcs &&
-			    channel->panel->funcs->get_modes)
-				bus_format = 0;
-		}
-		if (bus_format < 0) {
-			dev_err(dev, "could not determine data mapping: %d\n",
-				bus_format);
-			ret = bus_format;
-			goto free_child;
-		}
-		channel->bus_format = bus_format;
-		channel->child = child;
-
-		ret = imx_ldb_register_channel(drm, channel, connector);
-		if (ret) {
-			channel->child = NULL;
+		ret = imx_ldb_setup_channel(dev, child, drm, imx_ldb, i);
+		if (ret)
 			goto free_child;
-		}
 	}
 
 	return 0;
-- 
2.20.1

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

[PATCH 17/17] drm/imx: fix drm_mode_config_cleanup race condition

[Marco Felsch]

Currently there is a race conditions if the panel can't be probed e.g.
it is not connected [1]. There where several attempts to fix this [2,3]
but non of them made it into mainline.

The problem is the combination of the component framework and the drm
framework, as Philipp already explained [1]. To fix this we need to
drop the devres-kmalloc and move the plain kmalloc to let the drm
framework free the resources upon a drm_mode_config_cleanup(). So we need
to implement a .destroy() callback for each component. We also need to
reorder the master.unbind() callback to ensure that the driver states
are accessible during a component.unbind() call. This reordering also
aligns the master.unbind() with the error-cleanup path during
master.bind().

[1] https://www.spinics.net/lists/dri-devel/msg189388.html
[2] https://lkml.org/lkml/2018/10/16/1148
[3] https://lkml.org/lkml/2019/4/2/612

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c      | 28 ++++++++++++---------
 drivers/gpu/drm/imx/imx-drm-core.c     |  3 ++-
 drivers/gpu/drm/imx/imx-ldb.c          | 34 +++++++++++++++++---------
 drivers/gpu/drm/imx/imx-tve.c          | 15 +++++++++---
 drivers/gpu/drm/imx/ipuv3-crtc.c       | 28 ++++++++++++++++++---
 drivers/gpu/drm/imx/parallel-display.c | 30 ++++++++++++++++-------
 6 files changed, 96 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index f22cfbf9353e..86a62796c151 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -137,6 +137,12 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
 	return 0;
 }
 
+static void dw_hdmi_imx_encoder_destroy(struct drm_encoder *encoder)
+{
+	drm_encoder_cleanup(encoder);
+	kfree(enc_to_imx_hdmi(encoder));
+}
+
 static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
 	.enable     = dw_hdmi_imx_encoder_enable,
 	.disable    = dw_hdmi_imx_encoder_disable,
@@ -144,7 +150,7 @@ static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs =
 };
 
 static const struct drm_encoder_funcs dw_hdmi_imx_encoder_funcs = {
-	.destroy = drm_encoder_cleanup,
+	.destroy = dw_hdmi_imx_encoder_destroy,
 };
 
 static enum drm_mode_status
@@ -212,7 +218,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	if (!pdev->dev.of_node)
 		return -ENODEV;
 
-	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	hdmi = kzalloc(sizeof(*hdmi), GFP_KERNEL);
 	if (!hdmi)
 		return -ENOMEM;
 
@@ -228,12 +234,16 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	 * not been registered yet.  Defer probing, and hope that
 	 * the required CRTC is added later.
 	 */
-	if (encoder->possible_crtcs == 0)
+	if (encoder->possible_crtcs == 0) {
+		kfree(hdmi);
 		return -EPROBE_DEFER;
+	}
 
 	ret = dw_hdmi_imx_parse_dt(hdmi);
-	if (ret < 0)
+	if (ret < 0) {
+		kfree(hdmi);
 		return ret;
+	}
 
 	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
 	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
@@ -242,15 +252,9 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	platform_set_drvdata(pdev, hdmi);
 
 	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
-
-	/*
-	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
-	 * which would have called the encoder cleanup.  Do it manually.
-	 */
-	if (IS_ERR(hdmi->hdmi)) {
+	/* Don't call kfree() here, this is done by the .destroy() handler. */
+	if (IS_ERR(hdmi->hdmi))
 		ret = PTR_ERR(hdmi->hdmi);
-		drm_encoder_cleanup(encoder);
-	}
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 9979547ca883..feab6eb9e7e5 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -275,9 +275,10 @@ static void imx_drm_unbind(struct device *dev)
 
 	drm_kms_helper_poll_fini(drm);
 
+	component_unbind_all(drm->dev, drm);
+
 	drm_mode_config_cleanup(drm);
 
-	component_unbind_all(drm->dev, drm);
 	dev_set_drvdata(dev, NULL);
 
 	drm_dev_put(drm);
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 5e6c1b09dbfa..4a5d31da592a 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -140,6 +140,8 @@ static void imx_ldb_connector_destroy(struct drm_connector *connector)
 	i2c_put_adapter(imx_ldb_con->ddc);
 	/* avoid dangling pointers */
 	imx_ldb_con->ldb_channel = NULL;
+	kfree(imx_ldb_con->edid);
+	kfree(imx_ldb_con);
 }
 
 static int imx_ldb_connector_get_modes(struct drm_connector *connector)
@@ -184,6 +186,7 @@ static void imx_ldb_encoder_destroy(struct drm_encoder *encoder)
 	drm_encoder_cleanup(encoder);
 	/* avoid dangling pointers */
 	channel->ldb = NULL;
+	kfree(channel);
 }
 
 static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
@@ -502,9 +505,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
 		edidp = of_get_property(child, "edid",
 					&connector->edid_len);
 		if (edidp) {
-			connector->edid = devm_kmemdup(dev, edidp,
-						       connector->edid_len,
-						       GFP_KERNEL);
+			connector->edid = kmemdup(edidp, connector->edid_len,
+						  GFP_KERNEL);
 		} else if (!channel->panel) {
 			/* fallback to display-timings node */
 			ret = of_get_drm_display_mode(child,
@@ -525,7 +527,7 @@ static int imx_ldb_setup_channel(struct device *dev,
 				 int channel_number)
 {
 	struct imx_ldb_channel *channel;
-	struct imx_ldb_connector *imx_ldb_con;
+	struct imx_ldb_connector *imx_ldb_con = NULL;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector = NULL;
 	int bus_format;
@@ -537,7 +539,7 @@ static int imx_ldb_setup_channel(struct device *dev,
 	 * 3) Register it with the DRM framework
 	 * 4) Attach bridge or connector to encoder
 	 */
-	channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
+	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
 	if (!channel)
 		return -ENOMEM;
 
@@ -554,17 +556,19 @@ static int imx_ldb_setup_channel(struct device *dev,
 					  ldb->lvds_mux ? 4 : 2, 0,
 					  &channel->panel, &channel->bridge);
 	if (ret && ret != -ENODEV)
-		return ret;
+		goto err_free;
 
 	/* panel ddc only if there is no bridge */
 	if (!channel->bridge) {
-		imx_ldb_con = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
-		if (!imx_ldb_con)
-			return -ENOMEM;
+		imx_ldb_con = kzalloc(sizeof(*connector), GFP_KERNEL);
+		if (!imx_ldb_con) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
 
 		ret = imx_ldb_panel_ddc(dev, channel, imx_ldb_con, child);
 		if (ret)
-			return ret;
+			goto err_free;
 
 		imx_ldb_con->ldb_channel = channel;
 		connector = &imx_ldb_con->connector;
@@ -628,7 +632,7 @@ static int imx_ldb_setup_channel(struct device *dev,
 		ret = drm_bridge_attach(encoder, channel->bridge, NULL);
 		if (ret) {
 			DRM_ERROR("Failed to initialize bridge with drm\n");
-			goto err_put_ddc;
+			goto err_out;
 		}
 	} else {
 		drm_connector_attach_encoder(connector, encoder);
@@ -637,7 +641,7 @@ static int imx_ldb_setup_channel(struct device *dev,
 	if (channel->panel) {
 		ret = drm_panel_attach(channel->panel, connector);
 		if (ret)
-			goto err_put_ddc;
+			goto err_out;
 	}
 
 	return 0;
@@ -645,6 +649,12 @@ static int imx_ldb_setup_channel(struct device *dev,
 err_put_ddc:
 	if (imx_ldb_con)
 		i2c_put_adapter(imx_ldb_con->ddc);
+err_free:
+	if (imx_ldb_con)
+		kfree(imx_ldb_con->edid);
+	kfree(imx_ldb_con);
+	kfree(channel);
+err_out:
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index a7a05c47f68b..15ff5f35ff0e 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -241,6 +241,7 @@ static void imx_tve_connector_destroy(struct drm_connector *connector)
 	i2c_put_adapter(tvec->ddc);
 	/* avoid dangling pointers */
 	tvec->tve = NULL;
+	kfree(tvec);
 }
 
 static int imx_tve_connector_get_modes(struct drm_connector *connector)
@@ -292,6 +293,7 @@ static void imx_tve_encoder_destroy(struct drm_encoder *encoder)
 	drm_encoder_cleanup(encoder);
 	/* avoid dangling pointers */
 	tvee->tve = NULL;
+	kfree(tvee);
 }
 
 static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
@@ -577,13 +579,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	struct imx_tve_connector *tvec;
 	int ret;
 
-	tvee = devm_kzalloc(dev, sizeof(*tvee), GFP_KERNEL);
+	tvee = kzalloc(sizeof(*tvee), GFP_KERNEL);
 	if (!tvee)
 		return -ENOMEM;
 
-	tvec = devm_kzalloc(dev, sizeof(*tvec), GFP_KERNEL);
-	if (!tvec)
-		return -ENOMEM;
+	tvec = kzalloc(sizeof(*tvec), GFP_KERNEL);
+	if (!tvec) {
+		ret = -ENOMEM;
+		goto err_free;
+	}
 
 	tvee->tve = imx_tve;
 	tvec->tve = imx_tve;
@@ -602,6 +606,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 
 err_put_ddc:
 	i2c_put_adapter(tvec->ddc);
+err_free:
+	kfree(tvec);
+	kfree(tvee);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 63c0284f8b3c..2d24677f7fef 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -105,6 +105,12 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
 	spin_unlock_irq(&crtc->dev->event_lock);
 }
 
+static void imx_drm_crtc_destroy(struct drm_crtc *crtc)
+{
+	drm_crtc_cleanup(crtc);
+	kfree(to_ipu_crtc(crtc));
+}
+
 static void imx_drm_crtc_reset(struct drm_crtc *crtc)
 {
 	struct imx_crtc_state *state;
@@ -166,7 +172,7 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
 
 static const struct drm_crtc_funcs ipu_crtc_funcs = {
 	.set_config = drm_atomic_helper_set_config,
-	.destroy = drm_crtc_cleanup,
+	.destroy = imx_drm_crtc_destroy,
 	.page_flip = drm_atomic_helper_page_flip,
 	.reset = imx_drm_crtc_reset,
 	.atomic_duplicate_state = imx_drm_crtc_duplicate_state,
@@ -357,7 +363,8 @@ static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
 }
 
 static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
-	struct ipu_client_platformdata *pdata, struct drm_device *drm)
+			 struct ipu_client_platformdata *pdata,
+			 struct drm_device *drm)
 {
 	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
 	struct drm_crtc *crtc = &ipu_crtc->base;
@@ -437,10 +444,11 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
 {
 	struct ipu_client_platformdata *pdata = dev->platform_data;
 	struct drm_device *drm = data;
+	struct drm_crtc *registered_crtc = NULL;
 	struct ipu_crtc *ipu_crtc;
 	int ret;
 
-	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
+	ipu_crtc = kzalloc(sizeof(*ipu_crtc), GFP_KERNEL);
 	if (!ipu_crtc)
 		return -ENOMEM;
 
@@ -448,11 +456,23 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
 
 	ret = ipu_crtc_init(ipu_crtc, pdata, drm);
 	if (ret)
-		return ret;
+		goto err;
 
 	dev_set_drvdata(dev, ipu_crtc);
 
 	return 0;
+
+err:
+	drm_for_each_crtc(registered_crtc, drm) {
+		/*
+		 * The crtc was registered with the drm core framework if we
+		 * enter here. So let the core .destroy() helper cleanup the
+		 * code.
+		 */
+		return ret;
+	}
+	kfree(ipu_crtc);
+	return ret;
 }
 
 static void ipu_drm_unbind(struct device *dev, struct device *master,
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 78703b15c7cf..3e247383a498 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -55,6 +55,8 @@ static void imx_pd_connector_destroy(struct drm_connector *connector)
 	imx_drm_connector_destroy(connector);
 	/* avoid dangling pointer */
 	imxpc->imxpd = NULL;
+	kfree(imxpc->edid);
+	kfree(imxpc);
 }
 
 static int imx_pd_connector_get_modes(struct drm_connector *connector)
@@ -102,6 +104,7 @@ static void imx_pd_encoder_destroy(struct drm_encoder *encoder)
 	if (imxpd->panel)
 		drm_panel_detach(imxpd->panel);
 	drm_encoder_cleanup(encoder);
+	kfree(imxpd);
 }
 
 static void imx_pd_encoder_enable(struct drm_encoder *encoder)
@@ -225,31 +228,32 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	struct device_node *np = dev->of_node;
 	const u8 *edidp;
 	struct imx_parallel_display *imxpd;
-	struct imx_parallel_connector *imxpc;
+	struct imx_parallel_connector *imxpc = NULL;
 	int ret;
 	u32 bus_format = 0;
 	const char *fmt;
 
-	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
+	imxpd = kzalloc(sizeof(*imxpd), GFP_KERNEL);
 	if (!imxpd)
 		return -ENOMEM;
 
 	/* port@1 is the output port */
 	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
 	if (ret && ret != -ENODEV)
-		return ret;
+		goto err_free;
 
 	if (!imxpd->bridge) {
-		imxpc = devm_kzalloc(dev, sizeof(*imxpc), GFP_KERNEL);
-		if (!imxpc)
-			return -ENOMEM;
+		imxpc = kzalloc(sizeof(*imxpc), GFP_KERNEL);
+		if (!imxpc) {
+			ret = -ENOMEM;
+			goto err_free;
+		}
 
 		imxpc->imxpd = imxpd;
 
 		edidp = of_get_property(np, "edid", &imxpc->edid_len);
 		if (edidp)
-			imxpc->edid = devm_kmemdup(dev, edidp, imxpc->edid_len,
-						   GFP_KERNEL);
+			imxpc->edid = kmemdup(edidp, imxpc->edid_len, GFP_KERNEL);
 	}
 
 	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
@@ -269,9 +273,17 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 
 	ret = imx_pd_register(drm, imxpd, imxpc);
 	if (ret)
-		return ret;
+		goto err_free;
 
 	return imx_pd_attach(drm, imxpd, imxpc);
+
+err_free:
+	if (imxpc)
+		kfree(imxpc->edid);
+	kfree(imxpc);
+	kfree(imxpd);
+
+	return ret;
 }
 
 static const struct component_ops imx_pd_ops = {
-- 
2.20.1

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

Re: [PATCH 17/17] drm/imx: fix drm_mode_config_cleanup race condition

[Daniel Vetter]

On Thu, Feb 27, 2020 at 05:21:25PM +0100, Marco Felsch wrote:
> Currently there is a race conditions if the panel can't be probed e.g.
> it is not connected [1]. There where several attempts to fix this [2,3]
> but non of them made it into mainline.
> 
> The problem is the combination of the component framework and the drm
> framework, as Philipp already explained [1]. To fix this we need to
> drop the devres-kmalloc and move the plain kmalloc to let the drm
> framework free the resources upon a drm_mode_config_cleanup(). So we need
> to implement a .destroy() callback for each component. We also need to
> reorder the master.unbind() callback to ensure that the driver states
> are accessible during a component.unbind() call. This reordering also
> aligns the master.unbind() with the error-cleanup path during
> master.bind().
> 
> [1] https://www.spinics.net/lists/dri-devel/msg189388.html
> [2] https://lkml.org/lkml/2018/10/16/1148
> [3] https://lkml.org/lkml/2019/4/2/612
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

I think this collides quite badly with my managed drm device resources
patch series I'm working on. Plus once we have that, you could use
drmm_kzalloc and wouldn't need to sprinkle kfree() over everything.

I think at least, I haven't rolled much further than just getting the
baseline stuff figured out. So if it's not super-pressing to get this
patch here landed I think it'd be better to base this on top of the drmm
series. I'm working on sending out v3, I'll cc you on the imx parts so
you'll get pinged.

Cheers, Daniel

> ---
>  drivers/gpu/drm/imx/dw_hdmi-imx.c      | 28 ++++++++++++---------
>  drivers/gpu/drm/imx/imx-drm-core.c     |  3 ++-
>  drivers/gpu/drm/imx/imx-ldb.c          | 34 +++++++++++++++++---------
>  drivers/gpu/drm/imx/imx-tve.c          | 15 +++++++++---
>  drivers/gpu/drm/imx/ipuv3-crtc.c       | 28 ++++++++++++++++++---
>  drivers/gpu/drm/imx/parallel-display.c | 30 ++++++++++++++++-------
>  6 files changed, 96 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index f22cfbf9353e..86a62796c151 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -137,6 +137,12 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
>  	return 0;
>  }
>  
> +static void dw_hdmi_imx_encoder_destroy(struct drm_encoder *encoder)
> +{
> +	drm_encoder_cleanup(encoder);
> +	kfree(enc_to_imx_hdmi(encoder));
> +}
> +
>  static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
>  	.enable     = dw_hdmi_imx_encoder_enable,
>  	.disable    = dw_hdmi_imx_encoder_disable,
> @@ -144,7 +150,7 @@ static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs =
>  };
>  
>  static const struct drm_encoder_funcs dw_hdmi_imx_encoder_funcs = {
> -	.destroy = drm_encoder_cleanup,
> +	.destroy = dw_hdmi_imx_encoder_destroy,
>  };
>  
>  static enum drm_mode_status
> @@ -212,7 +218,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	if (!pdev->dev.of_node)
>  		return -ENODEV;
>  
> -	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> +	hdmi = kzalloc(sizeof(*hdmi), GFP_KERNEL);
>  	if (!hdmi)
>  		return -ENOMEM;
>  
> @@ -228,12 +234,16 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	 * not been registered yet.  Defer probing, and hope that
>  	 * the required CRTC is added later.
>  	 */
> -	if (encoder->possible_crtcs == 0)
> +	if (encoder->possible_crtcs == 0) {
> +		kfree(hdmi);
>  		return -EPROBE_DEFER;
> +	}
>  
>  	ret = dw_hdmi_imx_parse_dt(hdmi);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		kfree(hdmi);
>  		return ret;
> +	}
>  
>  	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
>  	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
> @@ -242,15 +252,9 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	platform_set_drvdata(pdev, hdmi);
>  
>  	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> -
> -	/*
> -	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
> -	 * which would have called the encoder cleanup.  Do it manually.
> -	 */
> -	if (IS_ERR(hdmi->hdmi)) {
> +	/* Don't call kfree() here, this is done by the .destroy() handler. */
> +	if (IS_ERR(hdmi->hdmi))
>  		ret = PTR_ERR(hdmi->hdmi);
> -		drm_encoder_cleanup(encoder);
> -	}
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 9979547ca883..feab6eb9e7e5 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -275,9 +275,10 @@ static void imx_drm_unbind(struct device *dev)
>  
>  	drm_kms_helper_poll_fini(drm);
>  
> +	component_unbind_all(drm->dev, drm);
> +
>  	drm_mode_config_cleanup(drm);
>  
> -	component_unbind_all(drm->dev, drm);
>  	dev_set_drvdata(dev, NULL);
>  
>  	drm_dev_put(drm);
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 5e6c1b09dbfa..4a5d31da592a 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -140,6 +140,8 @@ static void imx_ldb_connector_destroy(struct drm_connector *connector)
>  	i2c_put_adapter(imx_ldb_con->ddc);
>  	/* avoid dangling pointers */
>  	imx_ldb_con->ldb_channel = NULL;
> +	kfree(imx_ldb_con->edid);
> +	kfree(imx_ldb_con);
>  }
>  
>  static int imx_ldb_connector_get_modes(struct drm_connector *connector)
> @@ -184,6 +186,7 @@ static void imx_ldb_encoder_destroy(struct drm_encoder *encoder)
>  	drm_encoder_cleanup(encoder);
>  	/* avoid dangling pointers */
>  	channel->ldb = NULL;
> +	kfree(channel);
>  }
>  
>  static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
> @@ -502,9 +505,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
>  		edidp = of_get_property(child, "edid",
>  					&connector->edid_len);
>  		if (edidp) {
> -			connector->edid = devm_kmemdup(dev, edidp,
> -						       connector->edid_len,
> -						       GFP_KERNEL);
> +			connector->edid = kmemdup(edidp, connector->edid_len,
> +						  GFP_KERNEL);
>  		} else if (!channel->panel) {
>  			/* fallback to display-timings node */
>  			ret = of_get_drm_display_mode(child,
> @@ -525,7 +527,7 @@ static int imx_ldb_setup_channel(struct device *dev,
>  				 int channel_number)
>  {
>  	struct imx_ldb_channel *channel;
> -	struct imx_ldb_connector *imx_ldb_con;
> +	struct imx_ldb_connector *imx_ldb_con = NULL;
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector = NULL;
>  	int bus_format;
> @@ -537,7 +539,7 @@ static int imx_ldb_setup_channel(struct device *dev,
>  	 * 3) Register it with the DRM framework
>  	 * 4) Attach bridge or connector to encoder
>  	 */
> -	channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
>  	if (!channel)
>  		return -ENOMEM;
>  
> @@ -554,17 +556,19 @@ static int imx_ldb_setup_channel(struct device *dev,
>  					  ldb->lvds_mux ? 4 : 2, 0,
>  					  &channel->panel, &channel->bridge);
>  	if (ret && ret != -ENODEV)
> -		return ret;
> +		goto err_free;
>  
>  	/* panel ddc only if there is no bridge */
>  	if (!channel->bridge) {
> -		imx_ldb_con = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> -		if (!imx_ldb_con)
> -			return -ENOMEM;
> +		imx_ldb_con = kzalloc(sizeof(*connector), GFP_KERNEL);
> +		if (!imx_ldb_con) {
> +			ret = -ENOMEM;
> +			goto err_free;
> +		}
>  
>  		ret = imx_ldb_panel_ddc(dev, channel, imx_ldb_con, child);
>  		if (ret)
> -			return ret;
> +			goto err_free;
>  
>  		imx_ldb_con->ldb_channel = channel;
>  		connector = &imx_ldb_con->connector;
> @@ -628,7 +632,7 @@ static int imx_ldb_setup_channel(struct device *dev,
>  		ret = drm_bridge_attach(encoder, channel->bridge, NULL);
>  		if (ret) {
>  			DRM_ERROR("Failed to initialize bridge with drm\n");
> -			goto err_put_ddc;
> +			goto err_out;
>  		}
>  	} else {
>  		drm_connector_attach_encoder(connector, encoder);
> @@ -637,7 +641,7 @@ static int imx_ldb_setup_channel(struct device *dev,
>  	if (channel->panel) {
>  		ret = drm_panel_attach(channel->panel, connector);
>  		if (ret)
> -			goto err_put_ddc;
> +			goto err_out;
>  	}
>  
>  	return 0;
> @@ -645,6 +649,12 @@ static int imx_ldb_setup_channel(struct device *dev,
>  err_put_ddc:
>  	if (imx_ldb_con)
>  		i2c_put_adapter(imx_ldb_con->ddc);
> +err_free:
> +	if (imx_ldb_con)
> +		kfree(imx_ldb_con->edid);
> +	kfree(imx_ldb_con);
> +	kfree(channel);
> +err_out:
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index a7a05c47f68b..15ff5f35ff0e 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -241,6 +241,7 @@ static void imx_tve_connector_destroy(struct drm_connector *connector)
>  	i2c_put_adapter(tvec->ddc);
>  	/* avoid dangling pointers */
>  	tvec->tve = NULL;
> +	kfree(tvec);
>  }
>  
>  static int imx_tve_connector_get_modes(struct drm_connector *connector)
> @@ -292,6 +293,7 @@ static void imx_tve_encoder_destroy(struct drm_encoder *encoder)
>  	drm_encoder_cleanup(encoder);
>  	/* avoid dangling pointers */
>  	tvee->tve = NULL;
> +	kfree(tvee);
>  }
>  
>  static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
> @@ -577,13 +579,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
>  	struct imx_tve_connector *tvec;
>  	int ret;
>  
> -	tvee = devm_kzalloc(dev, sizeof(*tvee), GFP_KERNEL);
> +	tvee = kzalloc(sizeof(*tvee), GFP_KERNEL);
>  	if (!tvee)
>  		return -ENOMEM;
>  
> -	tvec = devm_kzalloc(dev, sizeof(*tvec), GFP_KERNEL);
> -	if (!tvec)
> -		return -ENOMEM;
> +	tvec = kzalloc(sizeof(*tvec), GFP_KERNEL);
> +	if (!tvec) {
> +		ret = -ENOMEM;
> +		goto err_free;
> +	}
>  
>  	tvee->tve = imx_tve;
>  	tvec->tve = imx_tve;
> @@ -602,6 +606,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
>  
>  err_put_ddc:
>  	i2c_put_adapter(tvec->ddc);
> +err_free:
> +	kfree(tvec);
> +	kfree(tvee);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 63c0284f8b3c..2d24677f7fef 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -105,6 +105,12 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  }
>  
> +static void imx_drm_crtc_destroy(struct drm_crtc *crtc)
> +{
> +	drm_crtc_cleanup(crtc);
> +	kfree(to_ipu_crtc(crtc));
> +}
> +
>  static void imx_drm_crtc_reset(struct drm_crtc *crtc)
>  {
>  	struct imx_crtc_state *state;
> @@ -166,7 +172,7 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
>  
>  static const struct drm_crtc_funcs ipu_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
> -	.destroy = drm_crtc_cleanup,
> +	.destroy = imx_drm_crtc_destroy,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.reset = imx_drm_crtc_reset,
>  	.atomic_duplicate_state = imx_drm_crtc_duplicate_state,
> @@ -357,7 +363,8 @@ static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
>  }
>  
>  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
> -	struct ipu_client_platformdata *pdata, struct drm_device *drm)
> +			 struct ipu_client_platformdata *pdata,
> +			 struct drm_device *drm)
>  {
>  	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
>  	struct drm_crtc *crtc = &ipu_crtc->base;
> @@ -437,10 +444,11 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
>  {
>  	struct ipu_client_platformdata *pdata = dev->platform_data;
>  	struct drm_device *drm = data;
> +	struct drm_crtc *registered_crtc = NULL;
>  	struct ipu_crtc *ipu_crtc;
>  	int ret;
>  
> -	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
> +	ipu_crtc = kzalloc(sizeof(*ipu_crtc), GFP_KERNEL);
>  	if (!ipu_crtc)
>  		return -ENOMEM;
>  
> @@ -448,11 +456,23 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
>  
>  	ret = ipu_crtc_init(ipu_crtc, pdata, drm);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	dev_set_drvdata(dev, ipu_crtc);
>  
>  	return 0;
> +
> +err:
> +	drm_for_each_crtc(registered_crtc, drm) {
> +		/*
> +		 * The crtc was registered with the drm core framework if we
> +		 * enter here. So let the core .destroy() helper cleanup the
> +		 * code.
> +		 */
> +		return ret;
> +	}
> +	kfree(ipu_crtc);
> +	return ret;
>  }
>  
>  static void ipu_drm_unbind(struct device *dev, struct device *master,
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index 78703b15c7cf..3e247383a498 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -55,6 +55,8 @@ static void imx_pd_connector_destroy(struct drm_connector *connector)
>  	imx_drm_connector_destroy(connector);
>  	/* avoid dangling pointer */
>  	imxpc->imxpd = NULL;
> +	kfree(imxpc->edid);
> +	kfree(imxpc);
>  }
>  
>  static int imx_pd_connector_get_modes(struct drm_connector *connector)
> @@ -102,6 +104,7 @@ static void imx_pd_encoder_destroy(struct drm_encoder *encoder)
>  	if (imxpd->panel)
>  		drm_panel_detach(imxpd->panel);
>  	drm_encoder_cleanup(encoder);
> +	kfree(imxpd);
>  }
>  
>  static void imx_pd_encoder_enable(struct drm_encoder *encoder)
> @@ -225,31 +228,32 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  	struct device_node *np = dev->of_node;
>  	const u8 *edidp;
>  	struct imx_parallel_display *imxpd;
> -	struct imx_parallel_connector *imxpc;
> +	struct imx_parallel_connector *imxpc = NULL;
>  	int ret;
>  	u32 bus_format = 0;
>  	const char *fmt;
>  
> -	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
> +	imxpd = kzalloc(sizeof(*imxpd), GFP_KERNEL);
>  	if (!imxpd)
>  		return -ENOMEM;
>  
>  	/* port@1 is the output port */
>  	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
>  	if (ret && ret != -ENODEV)
> -		return ret;
> +		goto err_free;
>  
>  	if (!imxpd->bridge) {
> -		imxpc = devm_kzalloc(dev, sizeof(*imxpc), GFP_KERNEL);
> -		if (!imxpc)
> -			return -ENOMEM;
> +		imxpc = kzalloc(sizeof(*imxpc), GFP_KERNEL);
> +		if (!imxpc) {
> +			ret = -ENOMEM;
> +			goto err_free;
> +		}
>  
>  		imxpc->imxpd = imxpd;
>  
>  		edidp = of_get_property(np, "edid", &imxpc->edid_len);
>  		if (edidp)
> -			imxpc->edid = devm_kmemdup(dev, edidp, imxpc->edid_len,
> -						   GFP_KERNEL);
> +			imxpc->edid = kmemdup(edidp, imxpc->edid_len, GFP_KERNEL);
>  	}
>  
>  	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
> @@ -269,9 +273,17 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  
>  	ret = imx_pd_register(drm, imxpd, imxpc);
>  	if (ret)
> -		return ret;
> +		goto err_free;
>  
>  	return imx_pd_attach(drm, imxpd, imxpc);
> +
> +err_free:
> +	if (imxpc)
> +		kfree(imxpc->edid);
> +	kfree(imxpc);
> +	kfree(imxpd);
> +
> +	return ret;
>  }
>  
>  static const struct component_ops imx_pd_ops = {
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/17] drm/imx: fix drm_mode_config_cleanup race condition

[Lucas Stach]

Hi Daniel,

On Do, 2020-02-27 at 18:29 +0100, Daniel Vetter wrote:
> On Thu, Feb 27, 2020 at 05:21:25PM +0100, Marco Felsch wrote:
> > Currently there is a race conditions if the panel can't be probed e.g.
> > it is not connected [1]. There where several attempts to fix this [2,3]
> > but non of them made it into mainline.
> > 
> > The problem is the combination of the component framework and the drm
> > framework, as Philipp already explained [1]. To fix this we need to
> > drop the devres-kmalloc and move the plain kmalloc to let the drm
> > framework free the resources upon a drm_mode_config_cleanup(). So we need
> > to implement a .destroy() callback for each component. We also need to
> > reorder the master.unbind() callback to ensure that the driver states
> > are accessible during a component.unbind() call. This reordering also
> > aligns the master.unbind() with the error-cleanup path during
> > master.bind().
> > 
> > [1] https://www.spinics.net/lists/dri-devel/msg189388.html
> > [2] https://lkml.org/lkml/2018/10/16/1148
> > [3] https://lkml.org/lkml/2019/4/2/612
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> I think this collides quite badly with my managed drm device resources
> patch series I'm working on. Plus once we have that, you could use
> drmm_kzalloc and wouldn't need to sprinkle kfree() over everything.
> 
> I think at least, I haven't rolled much further than just getting the
> baseline stuff figured out. So if it's not super-pressing to get this
> patch here landed I think it'd be better to base this on top of the drmm
> series. I'm working on sending out v3, I'll cc you on the imx parts so
> you'll get pinged.

IMO this part of imx-drm has been broken for far too long already, so
we shouldn't delay this fixes series on a complete resource management
rework.

Regards,
Lucas

> Cheers, Daniel
> 
> > ---
> >  drivers/gpu/drm/imx/dw_hdmi-imx.c      | 28 ++++++++++++---------
> >  drivers/gpu/drm/imx/imx-drm-core.c     |  3 ++-
> >  drivers/gpu/drm/imx/imx-ldb.c          | 34 +++++++++++++++++---------
> >  drivers/gpu/drm/imx/imx-tve.c          | 15 +++++++++---
> >  drivers/gpu/drm/imx/ipuv3-crtc.c       | 28 ++++++++++++++++++---
> >  drivers/gpu/drm/imx/parallel-display.c | 30 ++++++++++++++++-------
> >  6 files changed, 96 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > index f22cfbf9353e..86a62796c151 100644
> > --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > @@ -137,6 +137,12 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
> >  	return 0;
> >  }
> >  
> > +static void dw_hdmi_imx_encoder_destroy(struct drm_encoder *encoder)
> > +{
> > +	drm_encoder_cleanup(encoder);
> > +	kfree(enc_to_imx_hdmi(encoder));
> > +}
> > +
> >  static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
> >  	.enable     = dw_hdmi_imx_encoder_enable,
> >  	.disable    = dw_hdmi_imx_encoder_disable,
> > @@ -144,7 +150,7 @@ static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs =
> >  };
> >  
> >  static const struct drm_encoder_funcs dw_hdmi_imx_encoder_funcs = {
> > -	.destroy = drm_encoder_cleanup,
> > +	.destroy = dw_hdmi_imx_encoder_destroy,
> >  };
> >  
> >  static enum drm_mode_status
> > @@ -212,7 +218,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> >  	if (!pdev->dev.of_node)
> >  		return -ENODEV;
> >  
> > -	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> > +	hdmi = kzalloc(sizeof(*hdmi), GFP_KERNEL);
> >  	if (!hdmi)
> >  		return -ENOMEM;
> >  
> > @@ -228,12 +234,16 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> >  	 * not been registered yet.  Defer probing, and hope that
> >  	 * the required CRTC is added later.
> >  	 */
> > -	if (encoder->possible_crtcs == 0)
> > +	if (encoder->possible_crtcs == 0) {
> > +		kfree(hdmi);
> >  		return -EPROBE_DEFER;
> > +	}
> >  
> >  	ret = dw_hdmi_imx_parse_dt(hdmi);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		kfree(hdmi);
> >  		return ret;
> > +	}
> >  
> >  	drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
> >  	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
> > @@ -242,15 +252,9 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> >  	platform_set_drvdata(pdev, hdmi);
> >  
> >  	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> > -
> > -	/*
> > -	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
> > -	 * which would have called the encoder cleanup.  Do it manually.
> > -	 */
> > -	if (IS_ERR(hdmi->hdmi)) {
> > +	/* Don't call kfree() here, this is done by the .destroy() handler. */
> > +	if (IS_ERR(hdmi->hdmi))
> >  		ret = PTR_ERR(hdmi->hdmi);
> > -		drm_encoder_cleanup(encoder);
> > -	}
> >  
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > index 9979547ca883..feab6eb9e7e5 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -275,9 +275,10 @@ static void imx_drm_unbind(struct device *dev)
> >  
> >  	drm_kms_helper_poll_fini(drm);
> >  
> > +	component_unbind_all(drm->dev, drm);
> > +
> >  	drm_mode_config_cleanup(drm);
> >  
> > -	component_unbind_all(drm->dev, drm);
> >  	dev_set_drvdata(dev, NULL);
> >  
> >  	drm_dev_put(drm);
> > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > index 5e6c1b09dbfa..4a5d31da592a 100644
> > --- a/drivers/gpu/drm/imx/imx-ldb.c
> > +++ b/drivers/gpu/drm/imx/imx-ldb.c
> > @@ -140,6 +140,8 @@ static void imx_ldb_connector_destroy(struct drm_connector *connector)
> >  	i2c_put_adapter(imx_ldb_con->ddc);
> >  	/* avoid dangling pointers */
> >  	imx_ldb_con->ldb_channel = NULL;
> > +	kfree(imx_ldb_con->edid);
> > +	kfree(imx_ldb_con);
> >  }
> >  
> >  static int imx_ldb_connector_get_modes(struct drm_connector *connector)
> > @@ -184,6 +186,7 @@ static void imx_ldb_encoder_destroy(struct drm_encoder *encoder)
> >  	drm_encoder_cleanup(encoder);
> >  	/* avoid dangling pointers */
> >  	channel->ldb = NULL;
> > +	kfree(channel);
> >  }
> >  
> >  static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
> > @@ -502,9 +505,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
> >  		edidp = of_get_property(child, "edid",
> >  					&connector->edid_len);
> >  		if (edidp) {
> > -			connector->edid = devm_kmemdup(dev, edidp,
> > -						       connector->edid_len,
> > -						       GFP_KERNEL);
> > +			connector->edid = kmemdup(edidp, connector->edid_len,
> > +						  GFP_KERNEL);
> >  		} else if (!channel->panel) {
> >  			/* fallback to display-timings node */
> >  			ret = of_get_drm_display_mode(child,
> > @@ -525,7 +527,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> >  				 int channel_number)
> >  {
> >  	struct imx_ldb_channel *channel;
> > -	struct imx_ldb_connector *imx_ldb_con;
> > +	struct imx_ldb_connector *imx_ldb_con = NULL;
> >  	struct drm_encoder *encoder;
> >  	struct drm_connector *connector = NULL;
> >  	int bus_format;
> > @@ -537,7 +539,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> >  	 * 3) Register it with the DRM framework
> >  	 * 4) Attach bridge or connector to encoder
> >  	 */
> > -	channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> > +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> >  	if (!channel)
> >  		return -ENOMEM;
> >  
> > @@ -554,17 +556,19 @@ static int imx_ldb_setup_channel(struct device *dev,
> >  					  ldb->lvds_mux ? 4 : 2, 0,
> >  					  &channel->panel, &channel->bridge);
> >  	if (ret && ret != -ENODEV)
> > -		return ret;
> > +		goto err_free;
> >  
> >  	/* panel ddc only if there is no bridge */
> >  	if (!channel->bridge) {
> > -		imx_ldb_con = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> > -		if (!imx_ldb_con)
> > -			return -ENOMEM;
> > +		imx_ldb_con = kzalloc(sizeof(*connector), GFP_KERNEL);
> > +		if (!imx_ldb_con) {
> > +			ret = -ENOMEM;
> > +			goto err_free;
> > +		}
> >  
> >  		ret = imx_ldb_panel_ddc(dev, channel, imx_ldb_con, child);
> >  		if (ret)
> > -			return ret;
> > +			goto err_free;
> >  
> >  		imx_ldb_con->ldb_channel = channel;
> >  		connector = &imx_ldb_con->connector;
> > @@ -628,7 +632,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> >  		ret = drm_bridge_attach(encoder, channel->bridge, NULL);
> >  		if (ret) {
> >  			DRM_ERROR("Failed to initialize bridge with drm\n");
> > -			goto err_put_ddc;
> > +			goto err_out;
> >  		}
> >  	} else {
> >  		drm_connector_attach_encoder(connector, encoder);
> > @@ -637,7 +641,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> >  	if (channel->panel) {
> >  		ret = drm_panel_attach(channel->panel, connector);
> >  		if (ret)
> > -			goto err_put_ddc;
> > +			goto err_out;
> >  	}
> >  
> >  	return 0;
> > @@ -645,6 +649,12 @@ static int imx_ldb_setup_channel(struct device *dev,
> >  err_put_ddc:
> >  	if (imx_ldb_con)
> >  		i2c_put_adapter(imx_ldb_con->ddc);
> > +err_free:
> > +	if (imx_ldb_con)
> > +		kfree(imx_ldb_con->edid);
> > +	kfree(imx_ldb_con);
> > +	kfree(channel);
> > +err_out:
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> > index a7a05c47f68b..15ff5f35ff0e 100644
> > --- a/drivers/gpu/drm/imx/imx-tve.c
> > +++ b/drivers/gpu/drm/imx/imx-tve.c
> > @@ -241,6 +241,7 @@ static void imx_tve_connector_destroy(struct drm_connector *connector)
> >  	i2c_put_adapter(tvec->ddc);
> >  	/* avoid dangling pointers */
> >  	tvec->tve = NULL;
> > +	kfree(tvec);
> >  }
> >  
> >  static int imx_tve_connector_get_modes(struct drm_connector *connector)
> > @@ -292,6 +293,7 @@ static void imx_tve_encoder_destroy(struct drm_encoder *encoder)
> >  	drm_encoder_cleanup(encoder);
> >  	/* avoid dangling pointers */
> >  	tvee->tve = NULL;
> > +	kfree(tvee);
> >  }
> >  
> >  static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
> > @@ -577,13 +579,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
> >  	struct imx_tve_connector *tvec;
> >  	int ret;
> >  
> > -	tvee = devm_kzalloc(dev, sizeof(*tvee), GFP_KERNEL);
> > +	tvee = kzalloc(sizeof(*tvee), GFP_KERNEL);
> >  	if (!tvee)
> >  		return -ENOMEM;
> >  
> > -	tvec = devm_kzalloc(dev, sizeof(*tvec), GFP_KERNEL);
> > -	if (!tvec)
> > -		return -ENOMEM;
> > +	tvec = kzalloc(sizeof(*tvec), GFP_KERNEL);
> > +	if (!tvec) {
> > +		ret = -ENOMEM;
> > +		goto err_free;
> > +	}
> >  
> >  	tvee->tve = imx_tve;
> >  	tvec->tve = imx_tve;
> > @@ -602,6 +606,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
> >  
> >  err_put_ddc:
> >  	i2c_put_adapter(tvec->ddc);
> > +err_free:
> > +	kfree(tvec);
> > +	kfree(tvee);
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > index 63c0284f8b3c..2d24677f7fef 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > @@ -105,6 +105,12 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
> >  	spin_unlock_irq(&crtc->dev->event_lock);
> >  }
> >  
> > +static void imx_drm_crtc_destroy(struct drm_crtc *crtc)
> > +{
> > +	drm_crtc_cleanup(crtc);
> > +	kfree(to_ipu_crtc(crtc));
> > +}
> > +
> >  static void imx_drm_crtc_reset(struct drm_crtc *crtc)
> >  {
> >  	struct imx_crtc_state *state;
> > @@ -166,7 +172,7 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
> >  
> >  static const struct drm_crtc_funcs ipu_crtc_funcs = {
> >  	.set_config = drm_atomic_helper_set_config,
> > -	.destroy = drm_crtc_cleanup,
> > +	.destroy = imx_drm_crtc_destroy,
> >  	.page_flip = drm_atomic_helper_page_flip,
> >  	.reset = imx_drm_crtc_reset,
> >  	.atomic_duplicate_state = imx_drm_crtc_duplicate_state,
> > @@ -357,7 +363,8 @@ static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
> >  }
> >  
> >  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
> > -	struct ipu_client_platformdata *pdata, struct drm_device *drm)
> > +			 struct ipu_client_platformdata *pdata,
> > +			 struct drm_device *drm)
> >  {
> >  	struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
> >  	struct drm_crtc *crtc = &ipu_crtc->base;
> > @@ -437,10 +444,11 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
> >  {
> >  	struct ipu_client_platformdata *pdata = dev->platform_data;
> >  	struct drm_device *drm = data;
> > +	struct drm_crtc *registered_crtc = NULL;
> >  	struct ipu_crtc *ipu_crtc;
> >  	int ret;
> >  
> > -	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
> > +	ipu_crtc = kzalloc(sizeof(*ipu_crtc), GFP_KERNEL);
> >  	if (!ipu_crtc)
> >  		return -ENOMEM;
> >  
> > @@ -448,11 +456,23 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
> >  
> >  	ret = ipu_crtc_init(ipu_crtc, pdata, drm);
> >  	if (ret)
> > -		return ret;
> > +		goto err;
> >  
> >  	dev_set_drvdata(dev, ipu_crtc);
> >  
> >  	return 0;
> > +
> > +err:
> > +	drm_for_each_crtc(registered_crtc, drm) {
> > +		/*
> > +		 * The crtc was registered with the drm core framework if we
> > +		 * enter here. So let the core .destroy() helper cleanup the
> > +		 * code.
> > +		 */
> > +		return ret;
> > +	}
> > +	kfree(ipu_crtc);
> > +	return ret;
> >  }
> >  
> >  static void ipu_drm_unbind(struct device *dev, struct device *master,
> > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > index 78703b15c7cf..3e247383a498 100644
> > --- a/drivers/gpu/drm/imx/parallel-display.c
> > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > @@ -55,6 +55,8 @@ static void imx_pd_connector_destroy(struct drm_connector *connector)
> >  	imx_drm_connector_destroy(connector);
> >  	/* avoid dangling pointer */
> >  	imxpc->imxpd = NULL;
> > +	kfree(imxpc->edid);
> > +	kfree(imxpc);
> >  }
> >  
> >  static int imx_pd_connector_get_modes(struct drm_connector *connector)
> > @@ -102,6 +104,7 @@ static void imx_pd_encoder_destroy(struct drm_encoder *encoder)
> >  	if (imxpd->panel)
> >  		drm_panel_detach(imxpd->panel);
> >  	drm_encoder_cleanup(encoder);
> > +	kfree(imxpd);
> >  }
> >  
> >  static void imx_pd_encoder_enable(struct drm_encoder *encoder)
> > @@ -225,31 +228,32 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> >  	struct device_node *np = dev->of_node;
> >  	const u8 *edidp;
> >  	struct imx_parallel_display *imxpd;
> > -	struct imx_parallel_connector *imxpc;
> > +	struct imx_parallel_connector *imxpc = NULL;
> >  	int ret;
> >  	u32 bus_format = 0;
> >  	const char *fmt;
> >  
> > -	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
> > +	imxpd = kzalloc(sizeof(*imxpd), GFP_KERNEL);
> >  	if (!imxpd)
> >  		return -ENOMEM;
> >  
> >  	/* port@1 is the output port */
> >  	ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
> >  	if (ret && ret != -ENODEV)
> > -		return ret;
> > +		goto err_free;
> >  
> >  	if (!imxpd->bridge) {
> > -		imxpc = devm_kzalloc(dev, sizeof(*imxpc), GFP_KERNEL);
> > -		if (!imxpc)
> > -			return -ENOMEM;
> > +		imxpc = kzalloc(sizeof(*imxpc), GFP_KERNEL);
> > +		if (!imxpc) {
> > +			ret = -ENOMEM;
> > +			goto err_free;
> > +		}
> >  
> >  		imxpc->imxpd = imxpd;
> >  
> >  		edidp = of_get_property(np, "edid", &imxpc->edid_len);
> >  		if (edidp)
> > -			imxpc->edid = devm_kmemdup(dev, edidp, imxpc->edid_len,
> > -						   GFP_KERNEL);
> > +			imxpc->edid = kmemdup(edidp, imxpc->edid_len, GFP_KERNEL);
> >  	}
> >  
> >  	ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
> > @@ -269,9 +273,17 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> >  
> >  	ret = imx_pd_register(drm, imxpd, imxpc);
> >  	if (ret)
> > -		return ret;
> > +		goto err_free;
> >  
> >  	return imx_pd_attach(drm, imxpd, imxpc);
> > +
> > +err_free:
> > +	if (imxpc)
> > +		kfree(imxpc->edid);
> > +	kfree(imxpc);
> > +	kfree(imxpd);
> > +
> > +	return ret;
> >  }
> >  
> >  static const struct component_ops imx_pd_ops = {
> > -- 
> > 2.20.1
> > 

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

Re: [PATCH 17/17] drm/imx: fix drm_mode_config_cleanup race condition

[Daniel Vetter]

On Thu, Feb 27, 2020 at 6:44 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Hi Daniel,
>
> On Do, 2020-02-27 at 18:29 +0100, Daniel Vetter wrote:
> > On Thu, Feb 27, 2020 at 05:21:25PM +0100, Marco Felsch wrote:
> > > Currently there is a race conditions if the panel can't be probed e.g.
> > > it is not connected [1]. There where several attempts to fix this [2,3]
> > > but non of them made it into mainline.
> > >
> > > The problem is the combination of the component framework and the drm
> > > framework, as Philipp already explained [1]. To fix this we need to
> > > drop the devres-kmalloc and move the plain kmalloc to let the drm
> > > framework free the resources upon a drm_mode_config_cleanup(). So we need
> > > to implement a .destroy() callback for each component. We also need to
> > > reorder the master.unbind() callback to ensure that the driver states
> > > are accessible during a component.unbind() call. This reordering also
> > > aligns the master.unbind() with the error-cleanup path during
> > > master.bind().
> > >
> > > [1] https://www.spinics.net/lists/dri-devel/msg189388.html
> > > [2] https://lkml.org/lkml/2018/10/16/1148
> > > [3] https://lkml.org/lkml/2019/4/2/612
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> >
> > I think this collides quite badly with my managed drm device resources
> > patch series I'm working on. Plus once we have that, you could use
> > drmm_kzalloc and wouldn't need to sprinkle kfree() over everything.
> >
> > I think at least, I haven't rolled much further than just getting the
> > baseline stuff figured out. So if it's not super-pressing to get this
> > patch here landed I think it'd be better to base this on top of the drmm
> > series. I'm working on sending out v3, I'll cc you on the imx parts so
> > you'll get pinged.
>
> IMO this part of imx-drm has been broken for far too long already, so
> we shouldn't delay this fixes series on a complete resource management
> rework.

Given it's patch 17/17 in a spring cleanup, and not patch 1/17 I'm not
entirely sure it's really that high priority. Anyway would be great if
you at least check out what the new drm managed resource stuff would
mean for imx here, since you're blowing on devm_kzalloc exactly in the
way that I'm trying to get sorted now (without tons of explicit
kfree() everywhere).
-Daniel

>
> Regards,
> Lucas
>
> > Cheers, Daniel
> >
> > > ---
> > >  drivers/gpu/drm/imx/dw_hdmi-imx.c      | 28 ++++++++++++---------
> > >  drivers/gpu/drm/imx/imx-drm-core.c     |  3 ++-
> > >  drivers/gpu/drm/imx/imx-ldb.c          | 34 +++++++++++++++++---------
> > >  drivers/gpu/drm/imx/imx-tve.c          | 15 +++++++++---
> > >  drivers/gpu/drm/imx/ipuv3-crtc.c       | 28 ++++++++++++++++++---
> > >  drivers/gpu/drm/imx/parallel-display.c | 30 ++++++++++++++++-------
> > >  6 files changed, 96 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > > index f22cfbf9353e..86a62796c151 100644
> > > --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > > +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > > @@ -137,6 +137,12 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
> > >     return 0;
> > >  }
> > >
> > > +static void dw_hdmi_imx_encoder_destroy(struct drm_encoder *encoder)
> > > +{
> > > +   drm_encoder_cleanup(encoder);
> > > +   kfree(enc_to_imx_hdmi(encoder));
> > > +}
> > > +
> > >  static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
> > >     .enable     = dw_hdmi_imx_encoder_enable,
> > >     .disable    = dw_hdmi_imx_encoder_disable,
> > > @@ -144,7 +150,7 @@ static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs =
> > >  };
> > >
> > >  static const struct drm_encoder_funcs dw_hdmi_imx_encoder_funcs = {
> > > -   .destroy = drm_encoder_cleanup,
> > > +   .destroy = dw_hdmi_imx_encoder_destroy,
> > >  };
> > >
> > >  static enum drm_mode_status
> > > @@ -212,7 +218,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> > >     if (!pdev->dev.of_node)
> > >             return -ENODEV;
> > >
> > > -   hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> > > +   hdmi = kzalloc(sizeof(*hdmi), GFP_KERNEL);
> > >     if (!hdmi)
> > >             return -ENOMEM;
> > >
> > > @@ -228,12 +234,16 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> > >      * not been registered yet.  Defer probing, and hope that
> > >      * the required CRTC is added later.
> > >      */
> > > -   if (encoder->possible_crtcs == 0)
> > > +   if (encoder->possible_crtcs == 0) {
> > > +           kfree(hdmi);
> > >             return -EPROBE_DEFER;
> > > +   }
> > >
> > >     ret = dw_hdmi_imx_parse_dt(hdmi);
> > > -   if (ret < 0)
> > > +   if (ret < 0) {
> > > +           kfree(hdmi);
> > >             return ret;
> > > +   }
> > >
> > >     drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
> > >     drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
> > > @@ -242,15 +252,9 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> > >     platform_set_drvdata(pdev, hdmi);
> > >
> > >     hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> > > -
> > > -   /*
> > > -    * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
> > > -    * which would have called the encoder cleanup.  Do it manually.
> > > -    */
> > > -   if (IS_ERR(hdmi->hdmi)) {
> > > +   /* Don't call kfree() here, this is done by the .destroy() handler. */
> > > +   if (IS_ERR(hdmi->hdmi))
> > >             ret = PTR_ERR(hdmi->hdmi);
> > > -           drm_encoder_cleanup(encoder);
> > > -   }
> > >
> > >     return ret;
> > >  }
> > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > index 9979547ca883..feab6eb9e7e5 100644
> > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > @@ -275,9 +275,10 @@ static void imx_drm_unbind(struct device *dev)
> > >
> > >     drm_kms_helper_poll_fini(drm);
> > >
> > > +   component_unbind_all(drm->dev, drm);
> > > +
> > >     drm_mode_config_cleanup(drm);
> > >
> > > -   component_unbind_all(drm->dev, drm);
> > >     dev_set_drvdata(dev, NULL);
> > >
> > >     drm_dev_put(drm);
> > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > > index 5e6c1b09dbfa..4a5d31da592a 100644
> > > --- a/drivers/gpu/drm/imx/imx-ldb.c
> > > +++ b/drivers/gpu/drm/imx/imx-ldb.c
> > > @@ -140,6 +140,8 @@ static void imx_ldb_connector_destroy(struct drm_connector *connector)
> > >     i2c_put_adapter(imx_ldb_con->ddc);
> > >     /* avoid dangling pointers */
> > >     imx_ldb_con->ldb_channel = NULL;
> > > +   kfree(imx_ldb_con->edid);
> > > +   kfree(imx_ldb_con);
> > >  }
> > >
> > >  static int imx_ldb_connector_get_modes(struct drm_connector *connector)
> > > @@ -184,6 +186,7 @@ static void imx_ldb_encoder_destroy(struct drm_encoder *encoder)
> > >     drm_encoder_cleanup(encoder);
> > >     /* avoid dangling pointers */
> > >     channel->ldb = NULL;
> > > +   kfree(channel);
> > >  }
> > >
> > >  static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
> > > @@ -502,9 +505,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
> > >             edidp = of_get_property(child, "edid",
> > >                                     &connector->edid_len);
> > >             if (edidp) {
> > > -                   connector->edid = devm_kmemdup(dev, edidp,
> > > -                                                  connector->edid_len,
> > > -                                                  GFP_KERNEL);
> > > +                   connector->edid = kmemdup(edidp, connector->edid_len,
> > > +                                             GFP_KERNEL);
> > >             } else if (!channel->panel) {
> > >                     /* fallback to display-timings node */
> > >                     ret = of_get_drm_display_mode(child,
> > > @@ -525,7 +527,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > >                              int channel_number)
> > >  {
> > >     struct imx_ldb_channel *channel;
> > > -   struct imx_ldb_connector *imx_ldb_con;
> > > +   struct imx_ldb_connector *imx_ldb_con = NULL;
> > >     struct drm_encoder *encoder;
> > >     struct drm_connector *connector = NULL;
> > >     int bus_format;
> > > @@ -537,7 +539,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > >      * 3) Register it with the DRM framework
> > >      * 4) Attach bridge or connector to encoder
> > >      */
> > > -   channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> > > +   channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > >     if (!channel)
> > >             return -ENOMEM;
> > >
> > > @@ -554,17 +556,19 @@ static int imx_ldb_setup_channel(struct device *dev,
> > >                                       ldb->lvds_mux ? 4 : 2, 0,
> > >                                       &channel->panel, &channel->bridge);
> > >     if (ret && ret != -ENODEV)
> > > -           return ret;
> > > +           goto err_free;
> > >
> > >     /* panel ddc only if there is no bridge */
> > >     if (!channel->bridge) {
> > > -           imx_ldb_con = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> > > -           if (!imx_ldb_con)
> > > -                   return -ENOMEM;
> > > +           imx_ldb_con = kzalloc(sizeof(*connector), GFP_KERNEL);
> > > +           if (!imx_ldb_con) {
> > > +                   ret = -ENOMEM;
> > > +                   goto err_free;
> > > +           }
> > >
> > >             ret = imx_ldb_panel_ddc(dev, channel, imx_ldb_con, child);
> > >             if (ret)
> > > -                   return ret;
> > > +                   goto err_free;
> > >
> > >             imx_ldb_con->ldb_channel = channel;
> > >             connector = &imx_ldb_con->connector;
> > > @@ -628,7 +632,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > >             ret = drm_bridge_attach(encoder, channel->bridge, NULL);
> > >             if (ret) {
> > >                     DRM_ERROR("Failed to initialize bridge with drm\n");
> > > -                   goto err_put_ddc;
> > > +                   goto err_out;
> > >             }
> > >     } else {
> > >             drm_connector_attach_encoder(connector, encoder);
> > > @@ -637,7 +641,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > >     if (channel->panel) {
> > >             ret = drm_panel_attach(channel->panel, connector);
> > >             if (ret)
> > > -                   goto err_put_ddc;
> > > +                   goto err_out;
> > >     }
> > >
> > >     return 0;
> > > @@ -645,6 +649,12 @@ static int imx_ldb_setup_channel(struct device *dev,
> > >  err_put_ddc:
> > >     if (imx_ldb_con)
> > >             i2c_put_adapter(imx_ldb_con->ddc);
> > > +err_free:
> > > +   if (imx_ldb_con)
> > > +           kfree(imx_ldb_con->edid);
> > > +   kfree(imx_ldb_con);
> > > +   kfree(channel);
> > > +err_out:
> > >     return ret;
> > >  }
> > >
> > > diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> > > index a7a05c47f68b..15ff5f35ff0e 100644
> > > --- a/drivers/gpu/drm/imx/imx-tve.c
> > > +++ b/drivers/gpu/drm/imx/imx-tve.c
> > > @@ -241,6 +241,7 @@ static void imx_tve_connector_destroy(struct drm_connector *connector)
> > >     i2c_put_adapter(tvec->ddc);
> > >     /* avoid dangling pointers */
> > >     tvec->tve = NULL;
> > > +   kfree(tvec);
> > >  }
> > >
> > >  static int imx_tve_connector_get_modes(struct drm_connector *connector)
> > > @@ -292,6 +293,7 @@ static void imx_tve_encoder_destroy(struct drm_encoder *encoder)
> > >     drm_encoder_cleanup(encoder);
> > >     /* avoid dangling pointers */
> > >     tvee->tve = NULL;
> > > +   kfree(tvee);
> > >  }
> > >
> > >  static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
> > > @@ -577,13 +579,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
> > >     struct imx_tve_connector *tvec;
> > >     int ret;
> > >
> > > -   tvee = devm_kzalloc(dev, sizeof(*tvee), GFP_KERNEL);
> > > +   tvee = kzalloc(sizeof(*tvee), GFP_KERNEL);
> > >     if (!tvee)
> > >             return -ENOMEM;
> > >
> > > -   tvec = devm_kzalloc(dev, sizeof(*tvec), GFP_KERNEL);
> > > -   if (!tvec)
> > > -           return -ENOMEM;
> > > +   tvec = kzalloc(sizeof(*tvec), GFP_KERNEL);
> > > +   if (!tvec) {
> > > +           ret = -ENOMEM;
> > > +           goto err_free;
> > > +   }
> > >
> > >     tvee->tve = imx_tve;
> > >     tvec->tve = imx_tve;
> > > @@ -602,6 +606,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
> > >
> > >  err_put_ddc:
> > >     i2c_put_adapter(tvec->ddc);
> > > +err_free:
> > > +   kfree(tvec);
> > > +   kfree(tvee);
> > >     return ret;
> > >  }
> > >
> > > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > index 63c0284f8b3c..2d24677f7fef 100644
> > > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > @@ -105,6 +105,12 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
> > >     spin_unlock_irq(&crtc->dev->event_lock);
> > >  }
> > >
> > > +static void imx_drm_crtc_destroy(struct drm_crtc *crtc)
> > > +{
> > > +   drm_crtc_cleanup(crtc);
> > > +   kfree(to_ipu_crtc(crtc));
> > > +}
> > > +
> > >  static void imx_drm_crtc_reset(struct drm_crtc *crtc)
> > >  {
> > >     struct imx_crtc_state *state;
> > > @@ -166,7 +172,7 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
> > >
> > >  static const struct drm_crtc_funcs ipu_crtc_funcs = {
> > >     .set_config = drm_atomic_helper_set_config,
> > > -   .destroy = drm_crtc_cleanup,
> > > +   .destroy = imx_drm_crtc_destroy,
> > >     .page_flip = drm_atomic_helper_page_flip,
> > >     .reset = imx_drm_crtc_reset,
> > >     .atomic_duplicate_state = imx_drm_crtc_duplicate_state,
> > > @@ -357,7 +363,8 @@ static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
> > >  }
> > >
> > >  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
> > > -   struct ipu_client_platformdata *pdata, struct drm_device *drm)
> > > +                    struct ipu_client_platformdata *pdata,
> > > +                    struct drm_device *drm)
> > >  {
> > >     struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
> > >     struct drm_crtc *crtc = &ipu_crtc->base;
> > > @@ -437,10 +444,11 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
> > >  {
> > >     struct ipu_client_platformdata *pdata = dev->platform_data;
> > >     struct drm_device *drm = data;
> > > +   struct drm_crtc *registered_crtc = NULL;
> > >     struct ipu_crtc *ipu_crtc;
> > >     int ret;
> > >
> > > -   ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
> > > +   ipu_crtc = kzalloc(sizeof(*ipu_crtc), GFP_KERNEL);
> > >     if (!ipu_crtc)
> > >             return -ENOMEM;
> > >
> > > @@ -448,11 +456,23 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
> > >
> > >     ret = ipu_crtc_init(ipu_crtc, pdata, drm);
> > >     if (ret)
> > > -           return ret;
> > > +           goto err;
> > >
> > >     dev_set_drvdata(dev, ipu_crtc);
> > >
> > >     return 0;
> > > +
> > > +err:
> > > +   drm_for_each_crtc(registered_crtc, drm) {
> > > +           /*
> > > +            * The crtc was registered with the drm core framework if we
> > > +            * enter here. So let the core .destroy() helper cleanup the
> > > +            * code.
> > > +            */
> > > +           return ret;
> > > +   }
> > > +   kfree(ipu_crtc);
> > > +   return ret;
> > >  }
> > >
> > >  static void ipu_drm_unbind(struct device *dev, struct device *master,
> > > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > > index 78703b15c7cf..3e247383a498 100644
> > > --- a/drivers/gpu/drm/imx/parallel-display.c
> > > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > > @@ -55,6 +55,8 @@ static void imx_pd_connector_destroy(struct drm_connector *connector)
> > >     imx_drm_connector_destroy(connector);
> > >     /* avoid dangling pointer */
> > >     imxpc->imxpd = NULL;
> > > +   kfree(imxpc->edid);
> > > +   kfree(imxpc);
> > >  }
> > >
> > >  static int imx_pd_connector_get_modes(struct drm_connector *connector)
> > > @@ -102,6 +104,7 @@ static void imx_pd_encoder_destroy(struct drm_encoder *encoder)
> > >     if (imxpd->panel)
> > >             drm_panel_detach(imxpd->panel);
> > >     drm_encoder_cleanup(encoder);
> > > +   kfree(imxpd);
> > >  }
> > >
> > >  static void imx_pd_encoder_enable(struct drm_encoder *encoder)
> > > @@ -225,31 +228,32 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> > >     struct device_node *np = dev->of_node;
> > >     const u8 *edidp;
> > >     struct imx_parallel_display *imxpd;
> > > -   struct imx_parallel_connector *imxpc;
> > > +   struct imx_parallel_connector *imxpc = NULL;
> > >     int ret;
> > >     u32 bus_format = 0;
> > >     const char *fmt;
> > >
> > > -   imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
> > > +   imxpd = kzalloc(sizeof(*imxpd), GFP_KERNEL);
> > >     if (!imxpd)
> > >             return -ENOMEM;
> > >
> > >     /* port@1 is the output port */
> > >     ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
> > >     if (ret && ret != -ENODEV)
> > > -           return ret;
> > > +           goto err_free;
> > >
> > >     if (!imxpd->bridge) {
> > > -           imxpc = devm_kzalloc(dev, sizeof(*imxpc), GFP_KERNEL);
> > > -           if (!imxpc)
> > > -                   return -ENOMEM;
> > > +           imxpc = kzalloc(sizeof(*imxpc), GFP_KERNEL);
> > > +           if (!imxpc) {
> > > +                   ret = -ENOMEM;
> > > +                   goto err_free;
> > > +           }
> > >
> > >             imxpc->imxpd = imxpd;
> > >
> > >             edidp = of_get_property(np, "edid", &imxpc->edid_len);
> > >             if (edidp)
> > > -                   imxpc->edid = devm_kmemdup(dev, edidp, imxpc->edid_len,
> > > -                                              GFP_KERNEL);
> > > +                   imxpc->edid = kmemdup(edidp, imxpc->edid_len, GFP_KERNEL);
> > >     }
> > >
> > >     ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
> > > @@ -269,9 +273,17 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> > >
> > >     ret = imx_pd_register(drm, imxpd, imxpc);
> > >     if (ret)
> > > -           return ret;
> > > +           goto err_free;
> > >
> > >     return imx_pd_attach(drm, imxpd, imxpc);
> > > +
> > > +err_free:
> > > +   if (imxpc)
> > > +           kfree(imxpc->edid);
> > > +   kfree(imxpc);
> > > +   kfree(imxpd);
> > > +
> > > +   return ret;
> > >  }
> > >
> > >  static const struct component_ops imx_pd_ops = {
> > > --
> > > 2.20.1
> > >
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/17] drm/imx: fix drm_mode_config_cleanup race condition

[Marco Felsch]

Hi Daniel,

On 20-02-27 19:14, Daniel Vetter wrote:
> On Thu, Feb 27, 2020 at 6:44 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> >
> > Hi Daniel,
> >
> > On Do, 2020-02-27 at 18:29 +0100, Daniel Vetter wrote:
> > > On Thu, Feb 27, 2020 at 05:21:25PM +0100, Marco Felsch wrote:
> > > > Currently there is a race conditions if the panel can't be probed e.g.
> > > > it is not connected [1]. There where several attempts to fix this [2,3]
> > > > but non of them made it into mainline.
> > > >
> > > > The problem is the combination of the component framework and the drm
> > > > framework, as Philipp already explained [1]. To fix this we need to
> > > > drop the devres-kmalloc and move the plain kmalloc to let the drm
> > > > framework free the resources upon a drm_mode_config_cleanup(). So we need
> > > > to implement a .destroy() callback for each component. We also need to
> > > > reorder the master.unbind() callback to ensure that the driver states
> > > > are accessible during a component.unbind() call. This reordering also
> > > > aligns the master.unbind() with the error-cleanup path during
> > > > master.bind().
> > > >
> > > > [1] https://www.spinics.net/lists/dri-devel/msg189388.html
> > > > [2] https://lkml.org/lkml/2018/10/16/1148
> > > > [3] https://lkml.org/lkml/2019/4/2/612
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > >
> > > I think this collides quite badly with my managed drm device resources
> > > patch series I'm working on. Plus once we have that, you could use
> > > drmm_kzalloc and wouldn't need to sprinkle kfree() over everything.
> > >
> > > I think at least, I haven't rolled much further than just getting the
> > > baseline stuff figured out. So if it's not super-pressing to get this
> > > patch here landed I think it'd be better to base this on top of the drmm
> > > series. I'm working on sending out v3, I'll cc you on the imx parts so
> > > you'll get pinged.
> >
> > IMO this part of imx-drm has been broken for far too long already, so
> > we shouldn't delay this fixes series on a complete resource management
> > rework.
> 
> Given it's patch 17/17 in a spring cleanup, and not patch 1/17 I'm not
> entirely sure it's really that high priority.

Sorry for the description but the alloc fixes are important. I'm with
Lucas, we should fix this now. I called it "spring cleanup" because it
moves a lot of code from a to b.

Regards,
  Marco

> Anyway would be great if
> you at least check out what the new drm managed resource stuff would
> mean for imx here, since you're blowing on devm_kzalloc exactly in the
> way that I'm trying to get sorted now (without tons of explicit
> kfree() everywhere).
> -Daniel
> 
> >
> > Regards,
> > Lucas
> >
> > > Cheers, Daniel
> > >
> > > > ---
> > > >  drivers/gpu/drm/imx/dw_hdmi-imx.c      | 28 ++++++++++++---------
> > > >  drivers/gpu/drm/imx/imx-drm-core.c     |  3 ++-
> > > >  drivers/gpu/drm/imx/imx-ldb.c          | 34 +++++++++++++++++---------
> > > >  drivers/gpu/drm/imx/imx-tve.c          | 15 +++++++++---
> > > >  drivers/gpu/drm/imx/ipuv3-crtc.c       | 28 ++++++++++++++++++---
> > > >  drivers/gpu/drm/imx/parallel-display.c | 30 ++++++++++++++++-------
> > > >  6 files changed, 96 insertions(+), 42 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > > > index f22cfbf9353e..86a62796c151 100644
> > > > --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > > > +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> > > > @@ -137,6 +137,12 @@ static int dw_hdmi_imx_atomic_check(struct drm_encoder *encoder,
> > > >     return 0;
> > > >  }
> > > >
> > > > +static void dw_hdmi_imx_encoder_destroy(struct drm_encoder *encoder)
> > > > +{
> > > > +   drm_encoder_cleanup(encoder);
> > > > +   kfree(enc_to_imx_hdmi(encoder));
> > > > +}
> > > > +
> > > >  static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs = {
> > > >     .enable     = dw_hdmi_imx_encoder_enable,
> > > >     .disable    = dw_hdmi_imx_encoder_disable,
> > > > @@ -144,7 +150,7 @@ static const struct drm_encoder_helper_funcs dw_hdmi_imx_encoder_helper_funcs =
> > > >  };
> > > >
> > > >  static const struct drm_encoder_funcs dw_hdmi_imx_encoder_funcs = {
> > > > -   .destroy = drm_encoder_cleanup,
> > > > +   .destroy = dw_hdmi_imx_encoder_destroy,
> > > >  };
> > > >
> > > >  static enum drm_mode_status
> > > > @@ -212,7 +218,7 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> > > >     if (!pdev->dev.of_node)
> > > >             return -ENODEV;
> > > >
> > > > -   hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> > > > +   hdmi = kzalloc(sizeof(*hdmi), GFP_KERNEL);
> > > >     if (!hdmi)
> > > >             return -ENOMEM;
> > > >
> > > > @@ -228,12 +234,16 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> > > >      * not been registered yet.  Defer probing, and hope that
> > > >      * the required CRTC is added later.
> > > >      */
> > > > -   if (encoder->possible_crtcs == 0)
> > > > +   if (encoder->possible_crtcs == 0) {
> > > > +           kfree(hdmi);
> > > >             return -EPROBE_DEFER;
> > > > +   }
> > > >
> > > >     ret = dw_hdmi_imx_parse_dt(hdmi);
> > > > -   if (ret < 0)
> > > > +   if (ret < 0) {
> > > > +           kfree(hdmi);
> > > >             return ret;
> > > > +   }
> > > >
> > > >     drm_encoder_helper_add(encoder, &dw_hdmi_imx_encoder_helper_funcs);
> > > >     drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
> > > > @@ -242,15 +252,9 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
> > > >     platform_set_drvdata(pdev, hdmi);
> > > >
> > > >     hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> > > > -
> > > > -   /*
> > > > -    * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
> > > > -    * which would have called the encoder cleanup.  Do it manually.
> > > > -    */
> > > > -   if (IS_ERR(hdmi->hdmi)) {
> > > > +   /* Don't call kfree() here, this is done by the .destroy() handler. */
> > > > +   if (IS_ERR(hdmi->hdmi))
> > > >             ret = PTR_ERR(hdmi->hdmi);
> > > > -           drm_encoder_cleanup(encoder);
> > > > -   }
> > > >
> > > >     return ret;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > index 9979547ca883..feab6eb9e7e5 100644
> > > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > @@ -275,9 +275,10 @@ static void imx_drm_unbind(struct device *dev)
> > > >
> > > >     drm_kms_helper_poll_fini(drm);
> > > >
> > > > +   component_unbind_all(drm->dev, drm);
> > > > +
> > > >     drm_mode_config_cleanup(drm);
> > > >
> > > > -   component_unbind_all(drm->dev, drm);
> > > >     dev_set_drvdata(dev, NULL);
> > > >
> > > >     drm_dev_put(drm);
> > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > > > index 5e6c1b09dbfa..4a5d31da592a 100644
> > > > --- a/drivers/gpu/drm/imx/imx-ldb.c
> > > > +++ b/drivers/gpu/drm/imx/imx-ldb.c
> > > > @@ -140,6 +140,8 @@ static void imx_ldb_connector_destroy(struct drm_connector *connector)
> > > >     i2c_put_adapter(imx_ldb_con->ddc);
> > > >     /* avoid dangling pointers */
> > > >     imx_ldb_con->ldb_channel = NULL;
> > > > +   kfree(imx_ldb_con->edid);
> > > > +   kfree(imx_ldb_con);
> > > >  }
> > > >
> > > >  static int imx_ldb_connector_get_modes(struct drm_connector *connector)
> > > > @@ -184,6 +186,7 @@ static void imx_ldb_encoder_destroy(struct drm_encoder *encoder)
> > > >     drm_encoder_cleanup(encoder);
> > > >     /* avoid dangling pointers */
> > > >     channel->ldb = NULL;
> > > > +   kfree(channel);
> > > >  }
> > > >
> > > >  static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
> > > > @@ -502,9 +505,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
> > > >             edidp = of_get_property(child, "edid",
> > > >                                     &connector->edid_len);
> > > >             if (edidp) {
> > > > -                   connector->edid = devm_kmemdup(dev, edidp,
> > > > -                                                  connector->edid_len,
> > > > -                                                  GFP_KERNEL);
> > > > +                   connector->edid = kmemdup(edidp, connector->edid_len,
> > > > +                                             GFP_KERNEL);
> > > >             } else if (!channel->panel) {
> > > >                     /* fallback to display-timings node */
> > > >                     ret = of_get_drm_display_mode(child,
> > > > @@ -525,7 +527,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > > >                              int channel_number)
> > > >  {
> > > >     struct imx_ldb_channel *channel;
> > > > -   struct imx_ldb_connector *imx_ldb_con;
> > > > +   struct imx_ldb_connector *imx_ldb_con = NULL;
> > > >     struct drm_encoder *encoder;
> > > >     struct drm_connector *connector = NULL;
> > > >     int bus_format;
> > > > @@ -537,7 +539,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > > >      * 3) Register it with the DRM framework
> > > >      * 4) Attach bridge or connector to encoder
> > > >      */
> > > > -   channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> > > > +   channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > > >     if (!channel)
> > > >             return -ENOMEM;
> > > >
> > > > @@ -554,17 +556,19 @@ static int imx_ldb_setup_channel(struct device *dev,
> > > >                                       ldb->lvds_mux ? 4 : 2, 0,
> > > >                                       &channel->panel, &channel->bridge);
> > > >     if (ret && ret != -ENODEV)
> > > > -           return ret;
> > > > +           goto err_free;
> > > >
> > > >     /* panel ddc only if there is no bridge */
> > > >     if (!channel->bridge) {
> > > > -           imx_ldb_con = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
> > > > -           if (!imx_ldb_con)
> > > > -                   return -ENOMEM;
> > > > +           imx_ldb_con = kzalloc(sizeof(*connector), GFP_KERNEL);
> > > > +           if (!imx_ldb_con) {
> > > > +                   ret = -ENOMEM;
> > > > +                   goto err_free;
> > > > +           }
> > > >
> > > >             ret = imx_ldb_panel_ddc(dev, channel, imx_ldb_con, child);
> > > >             if (ret)
> > > > -                   return ret;
> > > > +                   goto err_free;
> > > >
> > > >             imx_ldb_con->ldb_channel = channel;
> > > >             connector = &imx_ldb_con->connector;
> > > > @@ -628,7 +632,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > > >             ret = drm_bridge_attach(encoder, channel->bridge, NULL);
> > > >             if (ret) {
> > > >                     DRM_ERROR("Failed to initialize bridge with drm\n");
> > > > -                   goto err_put_ddc;
> > > > +                   goto err_out;
> > > >             }
> > > >     } else {
> > > >             drm_connector_attach_encoder(connector, encoder);
> > > > @@ -637,7 +641,7 @@ static int imx_ldb_setup_channel(struct device *dev,
> > > >     if (channel->panel) {
> > > >             ret = drm_panel_attach(channel->panel, connector);
> > > >             if (ret)
> > > > -                   goto err_put_ddc;
> > > > +                   goto err_out;
> > > >     }
> > > >
> > > >     return 0;
> > > > @@ -645,6 +649,12 @@ static int imx_ldb_setup_channel(struct device *dev,
> > > >  err_put_ddc:
> > > >     if (imx_ldb_con)
> > > >             i2c_put_adapter(imx_ldb_con->ddc);
> > > > +err_free:
> > > > +   if (imx_ldb_con)
> > > > +           kfree(imx_ldb_con->edid);
> > > > +   kfree(imx_ldb_con);
> > > > +   kfree(channel);
> > > > +err_out:
> > > >     return ret;
> > > >  }
> > > >
> > > > diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> > > > index a7a05c47f68b..15ff5f35ff0e 100644
> > > > --- a/drivers/gpu/drm/imx/imx-tve.c
> > > > +++ b/drivers/gpu/drm/imx/imx-tve.c
> > > > @@ -241,6 +241,7 @@ static void imx_tve_connector_destroy(struct drm_connector *connector)
> > > >     i2c_put_adapter(tvec->ddc);
> > > >     /* avoid dangling pointers */
> > > >     tvec->tve = NULL;
> > > > +   kfree(tvec);
> > > >  }
> > > >
> > > >  static int imx_tve_connector_get_modes(struct drm_connector *connector)
> > > > @@ -292,6 +293,7 @@ static void imx_tve_encoder_destroy(struct drm_encoder *encoder)
> > > >     drm_encoder_cleanup(encoder);
> > > >     /* avoid dangling pointers */
> > > >     tvee->tve = NULL;
> > > > +   kfree(tvee);
> > > >  }
> > > >
> > > >  static void imx_tve_encoder_mode_set(struct drm_encoder *encoder,
> > > > @@ -577,13 +579,15 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
> > > >     struct imx_tve_connector *tvec;
> > > >     int ret;
> > > >
> > > > -   tvee = devm_kzalloc(dev, sizeof(*tvee), GFP_KERNEL);
> > > > +   tvee = kzalloc(sizeof(*tvee), GFP_KERNEL);
> > > >     if (!tvee)
> > > >             return -ENOMEM;
> > > >
> > > > -   tvec = devm_kzalloc(dev, sizeof(*tvec), GFP_KERNEL);
> > > > -   if (!tvec)
> > > > -           return -ENOMEM;
> > > > +   tvec = kzalloc(sizeof(*tvec), GFP_KERNEL);
> > > > +   if (!tvec) {
> > > > +           ret = -ENOMEM;
> > > > +           goto err_free;
> > > > +   }
> > > >
> > > >     tvee->tve = imx_tve;
> > > >     tvec->tve = imx_tve;
> > > > @@ -602,6 +606,9 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
> > > >
> > > >  err_put_ddc:
> > > >     i2c_put_adapter(tvec->ddc);
> > > > +err_free:
> > > > +   kfree(tvec);
> > > > +   kfree(tvee);
> > > >     return ret;
> > > >  }
> > > >
> > > > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > > index 63c0284f8b3c..2d24677f7fef 100644
> > > > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > > @@ -105,6 +105,12 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
> > > >     spin_unlock_irq(&crtc->dev->event_lock);
> > > >  }
> > > >
> > > > +static void imx_drm_crtc_destroy(struct drm_crtc *crtc)
> > > > +{
> > > > +   drm_crtc_cleanup(crtc);
> > > > +   kfree(to_ipu_crtc(crtc));
> > > > +}
> > > > +
> > > >  static void imx_drm_crtc_reset(struct drm_crtc *crtc)
> > > >  {
> > > >     struct imx_crtc_state *state;
> > > > @@ -166,7 +172,7 @@ static void ipu_disable_vblank(struct drm_crtc *crtc)
> > > >
> > > >  static const struct drm_crtc_funcs ipu_crtc_funcs = {
> > > >     .set_config = drm_atomic_helper_set_config,
> > > > -   .destroy = drm_crtc_cleanup,
> > > > +   .destroy = imx_drm_crtc_destroy,
> > > >     .page_flip = drm_atomic_helper_page_flip,
> > > >     .reset = imx_drm_crtc_reset,
> > > >     .atomic_duplicate_state = imx_drm_crtc_duplicate_state,
> > > > @@ -357,7 +363,8 @@ static int ipu_get_resources(struct ipu_crtc *ipu_crtc,
> > > >  }
> > > >
> > > >  static int ipu_crtc_init(struct ipu_crtc *ipu_crtc,
> > > > -   struct ipu_client_platformdata *pdata, struct drm_device *drm)
> > > > +                    struct ipu_client_platformdata *pdata,
> > > > +                    struct drm_device *drm)
> > > >  {
> > > >     struct ipu_soc *ipu = dev_get_drvdata(ipu_crtc->dev->parent);
> > > >     struct drm_crtc *crtc = &ipu_crtc->base;
> > > > @@ -437,10 +444,11 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
> > > >  {
> > > >     struct ipu_client_platformdata *pdata = dev->platform_data;
> > > >     struct drm_device *drm = data;
> > > > +   struct drm_crtc *registered_crtc = NULL;
> > > >     struct ipu_crtc *ipu_crtc;
> > > >     int ret;
> > > >
> > > > -   ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
> > > > +   ipu_crtc = kzalloc(sizeof(*ipu_crtc), GFP_KERNEL);
> > > >     if (!ipu_crtc)
> > > >             return -ENOMEM;
> > > >
> > > > @@ -448,11 +456,23 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
> > > >
> > > >     ret = ipu_crtc_init(ipu_crtc, pdata, drm);
> > > >     if (ret)
> > > > -           return ret;
> > > > +           goto err;
> > > >
> > > >     dev_set_drvdata(dev, ipu_crtc);
> > > >
> > > >     return 0;
> > > > +
> > > > +err:
> > > > +   drm_for_each_crtc(registered_crtc, drm) {
> > > > +           /*
> > > > +            * The crtc was registered with the drm core framework if we
> > > > +            * enter here. So let the core .destroy() helper cleanup the
> > > > +            * code.
> > > > +            */
> > > > +           return ret;
> > > > +   }
> > > > +   kfree(ipu_crtc);
> > > > +   return ret;
> > > >  }
> > > >
> > > >  static void ipu_drm_unbind(struct device *dev, struct device *master,
> > > > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > > > index 78703b15c7cf..3e247383a498 100644
> > > > --- a/drivers/gpu/drm/imx/parallel-display.c
> > > > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > > > @@ -55,6 +55,8 @@ static void imx_pd_connector_destroy(struct drm_connector *connector)
> > > >     imx_drm_connector_destroy(connector);
> > > >     /* avoid dangling pointer */
> > > >     imxpc->imxpd = NULL;
> > > > +   kfree(imxpc->edid);
> > > > +   kfree(imxpc);
> > > >  }
> > > >
> > > >  static int imx_pd_connector_get_modes(struct drm_connector *connector)
> > > > @@ -102,6 +104,7 @@ static void imx_pd_encoder_destroy(struct drm_encoder *encoder)
> > > >     if (imxpd->panel)
> > > >             drm_panel_detach(imxpd->panel);
> > > >     drm_encoder_cleanup(encoder);
> > > > +   kfree(imxpd);
> > > >  }
> > > >
> > > >  static void imx_pd_encoder_enable(struct drm_encoder *encoder)
> > > > @@ -225,31 +228,32 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> > > >     struct device_node *np = dev->of_node;
> > > >     const u8 *edidp;
> > > >     struct imx_parallel_display *imxpd;
> > > > -   struct imx_parallel_connector *imxpc;
> > > > +   struct imx_parallel_connector *imxpc = NULL;
> > > >     int ret;
> > > >     u32 bus_format = 0;
> > > >     const char *fmt;
> > > >
> > > > -   imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
> > > > +   imxpd = kzalloc(sizeof(*imxpd), GFP_KERNEL);
> > > >     if (!imxpd)
> > > >             return -ENOMEM;
> > > >
> > > >     /* port@1 is the output port */
> > > >     ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge);
> > > >     if (ret && ret != -ENODEV)
> > > > -           return ret;
> > > > +           goto err_free;
> > > >
> > > >     if (!imxpd->bridge) {
> > > > -           imxpc = devm_kzalloc(dev, sizeof(*imxpc), GFP_KERNEL);
> > > > -           if (!imxpc)
> > > > -                   return -ENOMEM;
> > > > +           imxpc = kzalloc(sizeof(*imxpc), GFP_KERNEL);
> > > > +           if (!imxpc) {
> > > > +                   ret = -ENOMEM;
> > > > +                   goto err_free;
> > > > +           }
> > > >
> > > >             imxpc->imxpd = imxpd;
> > > >
> > > >             edidp = of_get_property(np, "edid", &imxpc->edid_len);
> > > >             if (edidp)
> > > > -                   imxpc->edid = devm_kmemdup(dev, edidp, imxpc->edid_len,
> > > > -                                              GFP_KERNEL);
> > > > +                   imxpc->edid = kmemdup(edidp, imxpc->edid_len, GFP_KERNEL);
> > > >     }
> > > >
> > > >     ret = of_property_read_string(np, "interface-pix-fmt", &fmt);
> > > > @@ -269,9 +273,17 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
> > > >
> > > >     ret = imx_pd_register(drm, imxpd, imxpc);
> > > >     if (ret)
> > > > -           return ret;
> > > > +           goto err_free;
> > > >
> > > >     return imx_pd_attach(drm, imxpd, imxpc);
> > > > +
> > > > +err_free:
> > > > +   if (imxpc)
> > > > +           kfree(imxpc->edid);
> > > > +   kfree(imxpc);
> > > > +   kfree(imxpd);
> > > > +
> > > > +   return ret;
> > > >  }
> > > >
> > > >  static const struct component_ops imx_pd_ops = {
> > > > --
> > > > 2.20.1
> > > >
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/17] drm/imx: fix drm_mode_config_cleanup race condition

[Philipp Zabel]

Hi,

On Thu, 2020-02-27 at 19:14 +0100, Daniel Vetter wrote:
> On Thu, Feb 27, 2020 at 6:44 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > Hi Daniel,
> > 
> > On Do, 2020-02-27 at 18:29 +0100, Daniel Vetter wrote:
> > > On Thu, Feb 27, 2020 at 05:21:25PM +0100, Marco Felsch wrote:
> > > > Currently there is a race conditions if the panel can't be probed e.g.
> > > > it is not connected [1]. There where several attempts to fix this [2,3]
> > > > but non of them made it into mainline.
> > > > 
> > > > The problem is the combination of the component framework and the drm
> > > > framework, as Philipp already explained [1]. To fix this we need to
> > > > drop the devres-kmalloc and move the plain kmalloc to let the drm
> > > > framework free the resources upon a drm_mode_config_cleanup(). So we need
> > > > to implement a .destroy() callback for each component. We also need to
> > > > reorder the master.unbind() callback to ensure that the driver states
> > > > are accessible during a component.unbind() call. This reordering also
> > > > aligns the master.unbind() with the error-cleanup path during
> > > > master.bind().
> > > > 
> > > > [1] https://www.spinics.net/lists/dri-devel/msg189388.html
> > > > [2] https://lkml.org/lkml/2018/10/16/1148
> > > > [3] https://lkml.org/lkml/2019/4/2/612
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > 
> > > I think this collides quite badly with my managed drm device resources
> > > patch series I'm working on. Plus once we have that, you could use
> > > drmm_kzalloc and wouldn't need to sprinkle kfree() over everything.

How does it collide, none of the patches touch imx-drm?

> > > I think at least, I haven't rolled much further than just getting the
> > > baseline stuff figured out. So if it's not super-pressing to get this
> > > patch here landed I think it'd be better to base this on top of the drmm
> > > series. I'm working on sending out v3, I'll cc you on the imx parts so
> > > you'll get pinged.
> > 
> > IMO this part of imx-drm has been broken for far too long already, so
> > we shouldn't delay this fixes series on a complete resource management
> > rework.

Right, the use-after-free fixes should not have to be rebased onto
WIP drmm code. But could we move the fixes to the front, with just
minimal necessary changes?
That way they could be backported to stable without the cleanup and code
shuffling patches in-between.
We could then migrate the rework to drm managed resources without hurry.

> Given it's patch 17/17 in a spring cleanup, and not patch 1/17 I'm not
> entirely sure it's really that high priority.

This series fixes crashes on probe in case of defective device trees or
missing component drivers. I wouldn't get too hung up on the "spring
cleanup" name, but the actual fix being the last of a series of 17
patches is a valid point.

> Anyway would be great if you at least check out what the new drm
> managed resource stuff would mean for imx here, since you're blowing
> on devm_kzalloc exactly in the way that I'm trying to get sorted now
> (without tons of explicit kfree() everywhere).

I concur. Marco, would the following workaround be enough to fix the
issue until we can switch to drm managed allocations?

----------8<----------
From b1c98a9d7b29fc052491d7fe0f7a1af91e48d866 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Mon, 2 Mar 2020 17:12:44 +0100
Subject: [PATCH] drm/imx: fix use after free

Component driver structures allocated with devm_kmalloc() in bind() are
freed automatically after unbind(). Since the contained drm structures
are accessed afterwards in drm_mode_config_cleanup(), move the
allocation into probe() to extend the driver structure's lifetime to the
lifetime of the device. This should eventually be changed to use drm
resource managed allocations with lifetime of the drm device.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/dw_hdmi-imx.c      | 15 ++++++++++-----
 drivers/gpu/drm/imx/imx-ldb.c          | 15 ++++++++++-----
 drivers/gpu/drm/imx/imx-tve.c          | 15 ++++++++++-----
 drivers/gpu/drm/imx/ipuv3-crtc.c       | 21 ++++++++++-----------
 drivers/gpu/drm/imx/parallel-display.c | 15 ++++++++++-----
 5 files changed, 50 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
index f22cfbf9353e..2e12a4a3bfa1 100644
--- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
+++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
@@ -212,9 +212,8 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	if (!pdev->dev.of_node)
 		return -ENODEV;
 
-	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
-	if (!hdmi)
-		return -ENOMEM;
+	hdmi = dev_get_drvdata(dev);
+	memset(hdmi, 0, sizeof(*hdmi));
 
 	match = of_match_node(dw_hdmi_imx_dt_ids, pdev->dev.of_node);
 	plat_data = match->data;
@@ -239,8 +238,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	platform_set_drvdata(pdev, hdmi);
-
 	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
 
 	/*
@@ -270,6 +267,14 @@ static const struct component_ops dw_hdmi_imx_ops = {
 
 static int dw_hdmi_imx_probe(struct platform_device *pdev)
 {
+	struct imx_hdmi *hdmi;
+
+	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	if (!hdmi)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, hdmi);
+
 	return component_add(&pdev->dev, &dw_hdmi_imx_ops);
 }
 
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 8cb2665b2c74..c42217fc9f47 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -594,9 +594,8 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 	int ret;
 	int i;
 
-	imx_ldb = devm_kzalloc(dev, sizeof(*imx_ldb), GFP_KERNEL);
-	if (!imx_ldb)
-		return -ENOMEM;
+	imx_ldb = dev_get_drvdata(dev);
+	memset(imx_ldb, 0, sizeof(*imx_ldb));
 
 	imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
 	if (IS_ERR(imx_ldb->regmap)) {
@@ -704,8 +703,6 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		}
 	}
 
-	dev_set_drvdata(dev, imx_ldb);
-
 	return 0;
 
 free_child:
@@ -737,6 +734,14 @@ static const struct component_ops imx_ldb_ops = {
 
 static int imx_ldb_probe(struct platform_device *pdev)
 {
+	struct imx_ldb *imx_ldb;
+
+	imx_ldb = devm_kzalloc(&pdev->dev, sizeof(*imx_ldb), GFP_KERNEL);
+	if (!imx_ldb)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, imx_ldb);
+
 	return component_add(&pdev->dev, &imx_ldb_ops);
 }
 
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 5bbfaa2cd0f4..6593e75fc16e 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -546,9 +546,8 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	int irq;
 	int ret;
 
-	tve = devm_kzalloc(dev, sizeof(*tve), GFP_KERNEL);
-	if (!tve)
-		return -ENOMEM;
+	tve = dev_get_drvdata(dev);
+	memset(tve, 0, sizeof(*tve));
 
 	tve->dev = dev;
 	spin_lock_init(&tve->lock);
@@ -659,8 +658,6 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	dev_set_drvdata(dev, tve);
-
 	return 0;
 }
 
@@ -680,6 +677,14 @@ static const struct component_ops imx_tve_ops = {
 
 static int imx_tve_probe(struct platform_device *pdev)
 {
+	struct imx_tve *tve;
+
+	tve = devm_kzalloc(dev, sizeof(*tve), GFP_KERNEL);
+	if (!tve)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, tve);
+
 	return component_add(&pdev->dev, &imx_tve_ops);
 }
 
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 63c0284f8b3c..2256c9789fc2 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -438,21 +438,13 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
 	struct ipu_client_platformdata *pdata = dev->platform_data;
 	struct drm_device *drm = data;
 	struct ipu_crtc *ipu_crtc;
-	int ret;
 
-	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
-	if (!ipu_crtc)
-		return -ENOMEM;
+	ipu_crtc = dev_get_drvdata(dev);
+	memset(ipu_crtc, 0, sizeof(*ipu_crtc));
 
 	ipu_crtc->dev = dev;
 
-	ret = ipu_crtc_init(ipu_crtc, pdata, drm);
-	if (ret)
-		return ret;
-
-	dev_set_drvdata(dev, ipu_crtc);
-
-	return 0;
+	return ipu_crtc_init(ipu_crtc, pdata, drm);
 }
 
 static void ipu_drm_unbind(struct device *dev, struct device *master,
@@ -474,6 +466,7 @@ static const struct component_ops ipu_crtc_ops = {
 static int ipu_drm_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct ipu_crtc *ipu_crtc;
 	int ret;
 
 	if (!dev->platform_data)
@@ -483,6 +476,12 @@ static int ipu_drm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
+	if (!ipu_crtc)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, ipu_crtc);
+
 	return component_add(dev, &ipu_crtc_ops);
 }
 
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index 3dca424059f7..e6e629bd4b2a 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -205,9 +205,8 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	u32 bus_format = 0;
 	const char *fmt;
 
-	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
-	if (!imxpd)
-		return -ENOMEM;
+	imxpd = dev_get_drvdata(dev);
+	memset(imxpd, 0, sizeof(*imxpd));
 
 	edidp = of_get_property(np, "edid", &imxpd->edid_len);
 	if (edidp)
@@ -237,8 +236,6 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
 	if (ret)
 		return ret;
 
-	dev_set_drvdata(dev, imxpd);
-
 	return 0;
 }
 
@@ -260,6 +257,14 @@ static const struct component_ops imx_pd_ops = {
 
 static int imx_pd_probe(struct platform_device *pdev)
 {
+	struct imx_parallel_display *imxpd;
+
+	imxpd = devm_kzalloc(&pdev->dev, sizeof(*imxpd), GFP_KERNEL);
+	if (!imxpd)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, imxpd);
+
 	return component_add(&pdev->dev, &imx_pd_ops);
 }
 

base-commit: 98d54f81e36ba3bf92172791eba5ca5bd813989b
-- 
2.20.1
---------->8----------

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

Re: [PATCH 17/17] drm/imx: fix drm_mode_config_cleanup race condition

[Daniel Vetter]

On Mon, Mar 02, 2020 at 07:07:31PM +0100, Philipp Zabel wrote:
> Hi,
> 
> On Thu, 2020-02-27 at 19:14 +0100, Daniel Vetter wrote:
> > On Thu, Feb 27, 2020 at 6:44 PM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > Hi Daniel,
> > > 
> > > On Do, 2020-02-27 at 18:29 +0100, Daniel Vetter wrote:
> > > > On Thu, Feb 27, 2020 at 05:21:25PM +0100, Marco Felsch wrote:
> > > > > Currently there is a race conditions if the panel can't be probed e.g.
> > > > > it is not connected [1]. There where several attempts to fix this [2,3]
> > > > > but non of them made it into mainline.
> > > > > 
> > > > > The problem is the combination of the component framework and the drm
> > > > > framework, as Philipp already explained [1]. To fix this we need to
> > > > > drop the devres-kmalloc and move the plain kmalloc to let the drm
> > > > > framework free the resources upon a drm_mode_config_cleanup(). So we need
> > > > > to implement a .destroy() callback for each component. We also need to
> > > > > reorder the master.unbind() callback to ensure that the driver states
> > > > > are accessible during a component.unbind() call. This reordering also
> > > > > aligns the master.unbind() with the error-cleanup path during
> > > > > master.bind().
> > > > > 
> > > > > [1] https://www.spinics.net/lists/dri-devel/msg189388.html
> > > > > [2] https://lkml.org/lkml/2018/10/16/1148
> > > > > [3] https://lkml.org/lkml/2019/4/2/612
> > > > > 
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > 
> > > > I think this collides quite badly with my managed drm device resources
> > > > patch series I'm working on. Plus once we have that, you could use
> > > > drmm_kzalloc and wouldn't need to sprinkle kfree() over everything.
> 
> How does it collide, none of the patches touch imx-drm?
> 
> > > > I think at least, I haven't rolled much further than just getting the
> > > > baseline stuff figured out. So if it's not super-pressing to get this
> > > > patch here landed I think it'd be better to base this on top of the drmm
> > > > series. I'm working on sending out v3, I'll cc you on the imx parts so
> > > > you'll get pinged.
> > > 
> > > IMO this part of imx-drm has been broken for far too long already, so
> > > we shouldn't delay this fixes series on a complete resource management
> > > rework.
> 
> Right, the use-after-free fixes should not have to be rebased onto
> WIP drmm code. But could we move the fixes to the front, with just
> minimal necessary changes?
> That way they could be backported to stable without the cleanup and code
> shuffling patches in-between.
> We could then migrate the rework to drm managed resources without hurry.
> 
> > Given it's patch 17/17 in a spring cleanup, and not patch 1/17 I'm not
> > entirely sure it's really that high priority.
> 
> This series fixes crashes on probe in case of defective device trees or
> missing component drivers. I wouldn't get too hung up on the "spring
> cleanup" name, but the actual fix being the last of a series of 17
> patches is a valid point.
> 
> > Anyway would be great if you at least check out what the new drm
> > managed resource stuff would mean for imx here, since you're blowing
> > on devm_kzalloc exactly in the way that I'm trying to get sorted now
> > (without tons of explicit kfree() everywhere).
> 
> I concur. Marco, would the following workaround be enough to fix the
> issue until we can switch to drm managed allocations?

So what would be really useful for managed allocations with drmm if
you folks could test-drive this with a component driver. I've started
looking at some of them, but the load sequence is fairly tricky so I'm not
sure whether we'll correctly unwind in all cases. But I do think since
you're just putting a kfree() into the various drm_mode_object->destroy
hooks it should work. At least as long as you keep the explicit
drm_mode_config_cleanup call still (I'm working on that problem).

Knowing that (with maybe some warts, but at least as a poc) drmm_kzalloc
works correctly for component.c based drivers would be really useful.
That's kinda why I jumped in here.
-Daniel

> 
> ----------8<----------
> From b1c98a9d7b29fc052491d7fe0f7a1af91e48d866 Mon Sep 17 00:00:00 2001
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Date: Mon, 2 Mar 2020 17:12:44 +0100
> Subject: [PATCH] drm/imx: fix use after free
> 
> Component driver structures allocated with devm_kmalloc() in bind() are
> freed automatically after unbind(). Since the contained drm structures
> are accessed afterwards in drm_mode_config_cleanup(), move the
> allocation into probe() to extend the driver structure's lifetime to the
> lifetime of the device. This should eventually be changed to use drm
> resource managed allocations with lifetime of the drm device.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/dw_hdmi-imx.c      | 15 ++++++++++-----
>  drivers/gpu/drm/imx/imx-ldb.c          | 15 ++++++++++-----
>  drivers/gpu/drm/imx/imx-tve.c          | 15 ++++++++++-----
>  drivers/gpu/drm/imx/ipuv3-crtc.c       | 21 ++++++++++-----------
>  drivers/gpu/drm/imx/parallel-display.c | 15 ++++++++++-----
>  5 files changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index f22cfbf9353e..2e12a4a3bfa1 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -212,9 +212,8 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	if (!pdev->dev.of_node)
>  		return -ENODEV;
>  
> -	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> -	if (!hdmi)
> -		return -ENOMEM;
> +	hdmi = dev_get_drvdata(dev);
> +	memset(hdmi, 0, sizeof(*hdmi));
>  
>  	match = of_match_node(dw_hdmi_imx_dt_ids, pdev->dev.of_node);
>  	plat_data = match->data;
> @@ -239,8 +238,6 @@ static int dw_hdmi_imx_bind(struct device *dev, struct device *master,
>  	drm_encoder_init(drm, encoder, &dw_hdmi_imx_encoder_funcs,
>  			 DRM_MODE_ENCODER_TMDS, NULL);
>  
> -	platform_set_drvdata(pdev, hdmi);
> -
>  	hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
>  
>  	/*
> @@ -270,6 +267,14 @@ static const struct component_ops dw_hdmi_imx_ops = {
>  
>  static int dw_hdmi_imx_probe(struct platform_device *pdev)
>  {
> +	struct imx_hdmi *hdmi;
> +
> +	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> +	if (!hdmi)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
>  	return component_add(&pdev->dev, &dw_hdmi_imx_ops);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 8cb2665b2c74..c42217fc9f47 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -594,9 +594,8 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
>  	int ret;
>  	int i;
>  
> -	imx_ldb = devm_kzalloc(dev, sizeof(*imx_ldb), GFP_KERNEL);
> -	if (!imx_ldb)
> -		return -ENOMEM;
> +	imx_ldb = dev_get_drvdata(dev);
> +	memset(imx_ldb, 0, sizeof(*imx_ldb));
>  
>  	imx_ldb->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
>  	if (IS_ERR(imx_ldb->regmap)) {
> @@ -704,8 +703,6 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
>  		}
>  	}
>  
> -	dev_set_drvdata(dev, imx_ldb);
> -
>  	return 0;
>  
>  free_child:
> @@ -737,6 +734,14 @@ static const struct component_ops imx_ldb_ops = {
>  
>  static int imx_ldb_probe(struct platform_device *pdev)
>  {
> +	struct imx_ldb *imx_ldb;
> +
> +	imx_ldb = devm_kzalloc(&pdev->dev, sizeof(*imx_ldb), GFP_KERNEL);
> +	if (!imx_ldb)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, imx_ldb);
> +
>  	return component_add(&pdev->dev, &imx_ldb_ops);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index 5bbfaa2cd0f4..6593e75fc16e 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -546,9 +546,8 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
>  	int irq;
>  	int ret;
>  
> -	tve = devm_kzalloc(dev, sizeof(*tve), GFP_KERNEL);
> -	if (!tve)
> -		return -ENOMEM;
> +	tve = dev_get_drvdata(dev);
> +	memset(tve, 0, sizeof(*tve));
>  
>  	tve->dev = dev;
>  	spin_lock_init(&tve->lock);
> @@ -659,8 +658,6 @@ static int imx_tve_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	dev_set_drvdata(dev, tve);
> -
>  	return 0;
>  }
>  
> @@ -680,6 +677,14 @@ static const struct component_ops imx_tve_ops = {
>  
>  static int imx_tve_probe(struct platform_device *pdev)
>  {
> +	struct imx_tve *tve;
> +
> +	tve = devm_kzalloc(dev, sizeof(*tve), GFP_KERNEL);
> +	if (!tve)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, tve);
> +
>  	return component_add(&pdev->dev, &imx_tve_ops);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 63c0284f8b3c..2256c9789fc2 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -438,21 +438,13 @@ static int ipu_drm_bind(struct device *dev, struct device *master, void *data)
>  	struct ipu_client_platformdata *pdata = dev->platform_data;
>  	struct drm_device *drm = data;
>  	struct ipu_crtc *ipu_crtc;
> -	int ret;
>  
> -	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
> -	if (!ipu_crtc)
> -		return -ENOMEM;
> +	ipu_crtc = dev_get_drvdata(dev);
> +	memset(ipu_crtc, 0, sizeof(*ipu_crtc));
>  
>  	ipu_crtc->dev = dev;
>  
> -	ret = ipu_crtc_init(ipu_crtc, pdata, drm);
> -	if (ret)
> -		return ret;
> -
> -	dev_set_drvdata(dev, ipu_crtc);
> -
> -	return 0;
> +	return ipu_crtc_init(ipu_crtc, pdata, drm);
>  }
>  
>  static void ipu_drm_unbind(struct device *dev, struct device *master,
> @@ -474,6 +466,7 @@ static const struct component_ops ipu_crtc_ops = {
>  static int ipu_drm_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct ipu_crtc *ipu_crtc;
>  	int ret;
>  
>  	if (!dev->platform_data)
> @@ -483,6 +476,12 @@ static int ipu_drm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ipu_crtc = devm_kzalloc(dev, sizeof(*ipu_crtc), GFP_KERNEL);
> +	if (!ipu_crtc)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, ipu_crtc);
> +
>  	return component_add(dev, &ipu_crtc_ops);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index 3dca424059f7..e6e629bd4b2a 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -205,9 +205,8 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  	u32 bus_format = 0;
>  	const char *fmt;
>  
> -	imxpd = devm_kzalloc(dev, sizeof(*imxpd), GFP_KERNEL);
> -	if (!imxpd)
> -		return -ENOMEM;
> +	imxpd = dev_get_drvdata(dev);
> +	memset(imxpd, 0, sizeof(*imxpd));
>  
>  	edidp = of_get_property(np, "edid", &imxpd->edid_len);
>  	if (edidp)
> @@ -237,8 +236,6 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	dev_set_drvdata(dev, imxpd);
> -
>  	return 0;
>  }
>  
> @@ -260,6 +257,14 @@ static const struct component_ops imx_pd_ops = {
>  
>  static int imx_pd_probe(struct platform_device *pdev)
>  {
> +	struct imx_parallel_display *imxpd;
> +
> +	imxpd = devm_kzalloc(&pdev->dev, sizeof(*imxpd), GFP_KERNEL);
> +	if (!imxpd)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, imxpd);
> +
>  	return component_add(&pdev->dev, &imx_pd_ops);
>  }
>  
> 
> base-commit: 98d54f81e36ba3bf92172791eba5ca5bd813989b
> -- 
> 2.20.1
> ---------->8----------
> 
> regards
> Philipp

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel