The race condition reported by [1] applies to the all drm-imx subcomponent drivers which implements an encoder:connector combination. The goal is to switch from the devres-kmalloc and the component framework to a 'normal' kmalloc and the drm framework to release the memory resources. So all acquired memory resoruces are freed by a drm_mode_config_cleanup() call and the ->destroy() callbacks. For this purpose we need to slit the drm_encoder and the drm_connector state. [1] https://www.spinics.net/lists/dri-devel/msg189388.html Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx> --- drivers/gpu/drm/imx/imx-ldb.c | 106 +++++++++++++++++++++------------- 1 file changed, 66 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c index 5ef9fcb9ae94..0e5a3c84df10 100644 --- a/drivers/gpu/drm/imx/imx-ldb.c +++ b/drivers/gpu/drm/imx/imx-ldb.c @@ -50,7 +50,6 @@ struct imx_ldb; struct imx_ldb_channel { struct imx_ldb *ldb; - struct drm_connector connector; struct drm_encoder encoder; /* Defines what is connected to the ldb, only one at a time */ @@ -58,19 +57,26 @@ struct imx_ldb_channel { struct drm_bridge *bridge; struct device_node *child; - struct i2c_adapter *ddc; int chno; + u32 bus_format; + u32 bus_flags; +}; + +struct imx_ldb_connector { + struct imx_ldb_channel *ldb_channel; + struct drm_connector connector; + + struct i2c_adapter *ddc; void *edid; int edid_len; + struct drm_display_mode mode; int mode_valid; - u32 bus_format; - u32 bus_flags; }; -static inline struct imx_ldb_channel *con_to_imx_ldb_ch(struct drm_connector *c) +static inline struct imx_ldb_connector *con_to_imx_ldb_con(struct drm_connector *c) { - return container_of(c, struct imx_ldb_channel, connector); + return container_of(c, struct imx_ldb_connector, connector); } static inline struct imx_ldb_channel *enc_to_imx_ldb_ch(struct drm_encoder *e) @@ -126,31 +132,41 @@ static void imx_ldb_ch_set_bus_format(struct imx_ldb_channel *imx_ldb_ch, } } +static void imx_ldb_connector_destroy(struct drm_connector *connector) +{ + struct imx_ldb_connector *imx_ldb_con = con_to_imx_ldb_con(connector); + + imx_drm_connector_destroy(connector); + i2c_put_adapter(imx_ldb_con->ddc); + /* avoid dangling pointers */ + imx_ldb_con->ldb_channel = NULL; +} + static int imx_ldb_connector_get_modes(struct drm_connector *connector) { - struct imx_ldb_channel *imx_ldb_ch = con_to_imx_ldb_ch(connector); + struct imx_ldb_connector *imx_ldb_con = con_to_imx_ldb_con(connector); + struct imx_ldb_channel *imx_ldb_ch = imx_ldb_con->ldb_channel; int num_modes; num_modes = drm_panel_get_modes(imx_ldb_ch->panel, connector); if (num_modes > 0) return num_modes; - if (!imx_ldb_ch->edid && imx_ldb_ch->ddc) - imx_ldb_ch->edid = drm_get_edid(connector, imx_ldb_ch->ddc); + if (!imx_ldb_con->edid && imx_ldb_con->ddc) + imx_ldb_con->edid = drm_get_edid(connector, imx_ldb_con->ddc); - if (imx_ldb_ch->edid) { - drm_connector_update_edid_property(connector, - imx_ldb_ch->edid); - num_modes = drm_add_edid_modes(connector, imx_ldb_ch->edid); + if (imx_ldb_con->edid) { + drm_connector_update_edid_property(connector, imx_ldb_con->edid); + num_modes = drm_add_edid_modes(connector, imx_ldb_con->edid); } - if (imx_ldb_ch->mode_valid) { + if (imx_ldb_con->mode_valid) { struct drm_display_mode *mode; mode = drm_mode_create(connector->dev); if (!mode) return -EINVAL; - drm_mode_copy(mode, &imx_ldb_ch->mode); + drm_mode_copy(mode, &imx_ldb_con->mode); mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); num_modes++; @@ -166,7 +182,8 @@ static void imx_ldb_encoder_destroy(struct drm_encoder *encoder) if (channel->panel) drm_panel_detach(channel->panel); drm_encoder_cleanup(encoder); - i2c_put_adapter(channel->ddc); + /* avoid dangling pointers */ + channel->ldb = NULL; } static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno, @@ -388,7 +405,7 @@ static int imx_ldb_encoder_atomic_check(struct drm_encoder *encoder, static const struct drm_connector_funcs imx_ldb_connector_funcs = { .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = imx_drm_connector_destroy, + .destroy = imx_ldb_connector_destroy, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, @@ -424,11 +441,13 @@ static int imx_ldb_get_clk(struct imx_ldb *ldb, int chno) return PTR_ERR_OR_ZERO(ldb->clk_pll[chno]); } -static int imx_ldb_register(struct drm_device *drm, - struct imx_ldb_channel *imx_ldb_ch) +static int imx_ldb_register_channel(struct drm_device *drm, + struct imx_ldb_channel *imx_ldb_ch, + struct imx_ldb_connector *imx_ldb_con) { struct imx_ldb *ldb = imx_ldb_ch->ldb; struct drm_encoder *encoder = &imx_ldb_ch->encoder; + struct drm_connector *connector = &imx_ldb_con->connector; int ret; ret = imx_drm_encoder_parse_of(drm, encoder, imx_ldb_ch->child); @@ -450,8 +469,7 @@ static int imx_ldb_register(struct drm_device *drm, DRM_MODE_ENCODER_LVDS, NULL); if (imx_ldb_ch->bridge) { - ret = drm_bridge_attach(&imx_ldb_ch->encoder, - imx_ldb_ch->bridge, NULL); + ret = drm_bridge_attach(encoder, imx_ldb_ch->bridge, NULL); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); return ret; @@ -463,18 +481,17 @@ static int imx_ldb_register(struct drm_device *drm, * historical reasons, the ldb driver can also work without * a panel. */ - drm_connector_helper_add(&imx_ldb_ch->connector, - &imx_ldb_connector_helper_funcs); - drm_connector_init_with_ddc(drm, &imx_ldb_ch->connector, + drm_connector_helper_add(connector, + &imx_ldb_connector_helper_funcs); + drm_connector_init_with_ddc(drm, connector, &imx_ldb_connector_funcs, DRM_MODE_CONNECTOR_LVDS, - imx_ldb_ch->ddc); - drm_connector_attach_encoder(&imx_ldb_ch->connector, encoder); + imx_ldb_con->ddc); + drm_connector_attach_encoder(connector, encoder); } if (imx_ldb_ch->panel) { - ret = drm_panel_attach(imx_ldb_ch->panel, - &imx_ldb_ch->connector); + ret = drm_panel_attach(imx_ldb_ch->panel, connector); if (ret) return ret; } @@ -518,7 +535,9 @@ static u32 of_get_bus_format(struct device *dev, struct device_node *np) } static int imx_ldb_panel_ddc(struct device *dev, - struct imx_ldb_channel *channel, struct device_node *child) + struct imx_ldb_channel *channel, + struct imx_ldb_connector *connector, + struct device_node *child) { struct device_node *ddc_node; const u8 *edidp; @@ -526,32 +545,32 @@ static int imx_ldb_panel_ddc(struct device *dev, ddc_node = of_parse_phandle(child, "ddc-i2c-bus", 0); if (ddc_node) { - channel->ddc = of_find_i2c_adapter_by_node(ddc_node); + connector->ddc = of_find_i2c_adapter_by_node(ddc_node); of_node_put(ddc_node); - if (!channel->ddc) { + if (!connector->ddc) { dev_warn(dev, "failed to get ddc i2c adapter\n"); return -EPROBE_DEFER; } } - if (!channel->ddc) { + if (!connector->ddc) { /* if no DDC available, fallback to hardcoded EDID */ dev_dbg(dev, "no ddc available\n"); edidp = of_get_property(child, "edid", - &channel->edid_len); + &connector->edid_len); if (edidp) { - channel->edid = devm_kmemdup(dev, edidp, - channel->edid_len, - GFP_KERNEL); + connector->edid = devm_kmemdup(dev, edidp, + connector->edid_len, + GFP_KERNEL); } else if (!channel->panel) { /* fallback to display-timings node */ ret = of_get_drm_display_mode(child, - &channel->mode, + &connector->mode, &channel->bus_flags, OF_USE_NATIVE_MODE); if (!ret) - channel->mode_valid = 1; + connector->mode_valid = 1; } } return 0; @@ -569,6 +588,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data) for_each_child_of_node(np, child) { struct imx_ldb_channel *channel; + struct imx_ldb_connector *connector; int bus_format; ret = of_property_read_u32(child, "reg", &i); @@ -604,9 +624,15 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data) /* panel ddc only if there is no bridge */ if (!channel->bridge) { - ret = imx_ldb_panel_ddc(dev, channel, child); + connector = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL); + if (!connector) + return -ENOMEM; + + ret = imx_ldb_panel_ddc(dev, channel, connector, child); if (ret) goto free_child; + + connector->ldb_channel = channel; } bus_format = of_get_bus_format(dev, child); @@ -628,7 +654,7 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data) channel->bus_format = bus_format; channel->child = child; - ret = imx_ldb_register(drm, channel); + ret = imx_ldb_register_channel(drm, channel, connector); if (ret) { channel->child = NULL; goto free_child; -- 2.20.1 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
