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 = {