| Message ID | 20200227162125.10450-18-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM: imx spring-cleaning | expand |
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 >
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 > >
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 > > > >
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 >
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
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
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 = {
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(-)