On Mon, Dec 04, 2017 at 09:48:18PM +0100, Daniel Vetter wrote: > In > > commit 613051dac40da1751ab269572766d3348d45a197 > Author: Daniel Vetter <[email protected]> > Date: Wed Dec 14 00:08:06 2016 +0100 > > drm: locking&new iterators for connector_list > > we've went to extreme lengths to make sure connector iterations works > in any context, without introducing any additional locking context. > This worked, except for a small fumble in the implementation: > > When we actually race with a concurrent connector unplug event, and > our temporary connector reference turns out to be the final one, then > everything breaks: We call the connector release function from > whatever context we happen to be in, which can be an irq/atomic > context. And connector freeing grabs all kinds of locks and stuff. > > Fix this by creating a specially safe put function for connetor_iter, > which (in this rare case) punts the cleanup to a worker. > > Reported-by: Ben Widawsky <[email protected]> > Cc: Ben Widawsky <[email protected]> > Fixes: 613051dac40d ("drm: locking&new iterators for connector_list") > Cc: Dave Airlie <[email protected]> > Cc: Chris Wilson <[email protected]> > Cc: Sean Paul <[email protected]> > Cc: <[email protected]> # v4.11+ > Signed-off-by: Daniel Vetter <[email protected]>
s/savely/safely/ in the summary and Reviewed-by: Dave Airlie <[email protected]> from irc. CI is also happy, I'll merge as soon as Ben has approved this too. -Daniel > --- > drivers/gpu/drm/drm_connector.c | 28 ++++++++++++++++++++++++++-- > drivers/gpu/drm/drm_mode_config.c | 2 ++ > include/drm/drm_connector.h | 8 ++++++++ > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 25f4b2e9a44f..482014137953 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -152,6 +152,16 @@ static void drm_connector_free(struct kref *kref) > connector->funcs->destroy(connector); > } > > +static void drm_connector_free_work_fn(struct work_struct *work) > +{ > + struct drm_connector *connector = > + container_of(work, struct drm_connector, free_work); > + struct drm_device *dev = connector->dev; > + > + drm_mode_object_unregister(dev, &connector->base); > + connector->funcs->destroy(connector); > +} > + > /** > * drm_connector_init - Init a preallocated connector > * @dev: DRM device > @@ -181,6 +191,8 @@ int drm_connector_init(struct drm_device *dev, > if (ret) > return ret; > > + INIT_WORK(&connector->free_work, drm_connector_free_work_fn); > + > connector->base.properties = &connector->properties; > connector->dev = dev; > connector->funcs = funcs; > @@ -529,6 +541,18 @@ void drm_connector_list_iter_begin(struct drm_device > *dev, > } > EXPORT_SYMBOL(drm_connector_list_iter_begin); > > +/* > + * Extra-safe connector put function that works in any context. Should only > be > + * used from the connector_iter functions, where we never really expect to > + * actually release the connector when dropping our final reference. > + */ > +static void > +drm_connector_put_safe(struct drm_connector *conn) > +{ > + if (refcount_dec_and_test(&conn->base.refcount.refcount)) > + schedule_work(&conn->free_work); > +} > + > /** > * drm_connector_list_iter_next - return next connector > * @iter: connectr_list iterator > @@ -561,7 +585,7 @@ drm_connector_list_iter_next(struct > drm_connector_list_iter *iter) > spin_unlock_irqrestore(&config->connector_list_lock, flags); > > if (old_conn) > - drm_connector_put(old_conn); > + drm_connector_put_safe(old_conn); > > return iter->conn; > } > @@ -580,7 +604,7 @@ void drm_connector_list_iter_end(struct > drm_connector_list_iter *iter) > { > iter->dev = NULL; > if (iter->conn) > - drm_connector_put(iter->conn); > + drm_connector_put_safe(iter->conn); > lock_release(&connector_list_iter_dep_map, 0, _RET_IP_); > } > EXPORT_SYMBOL(drm_connector_list_iter_end); > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index 7623607c0f1e..346c19c6ce01 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -431,6 +431,8 @@ void drm_mode_config_cleanup(struct drm_device *dev) > drm_connector_put(connector); > } > drm_connector_list_iter_end(&conn_iter); > + /* connector_iter drops references in a work item. */ > + flush_scheduled_work(); > if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) { > drm_connector_list_iter_begin(dev, &conn_iter); > drm_for_each_connector_iter(connector, &conn_iter) > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 66d6c99d15e5..c5c753a1be85 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -926,6 +926,14 @@ struct drm_connector { > uint8_t num_h_tile, num_v_tile; > uint8_t tile_h_loc, tile_v_loc; > uint16_t tile_h_size, tile_v_size; > + > + /** > + * @free_work: > + * > + * Work used only by &drm_connector_iter to be able to clean up a > + * connector from any context. > + */ > + struct work_struct free_work; > }; > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > -- > 2.15.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
