Re: [PATCH 05/14] drm: Check locking in drm_for_each_connector

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Thursday 09 July 2015 23:44:28 Daniel Vetter wrote:
> Because of DP MST connectors can now be hotplugged and we must hold
> the right lock when walking the connector lists.  Enforce this by
> checking the locking in our shiny new list walking macros.
> 
> v2: Extract the locking check into a small static inline helper to
> help readability. This will be more important when we make the
> read list access rules more complicated in later patches. Inspired by
> comments from Chris. Unfortunately, due to header loops around the
> definition of struct drm_device the function interface is a bit funny.
> 
> v3: Encoders aren't hotadded/removed. For each dp mst encoder we
> statically create one fake encoder per pipe so that we can support as
> many mst sinks as the hw can (Dave).
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  include/drm/drm_crtc.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 7c95a7df6065..499562274353 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1589,8 +1589,18 @@ static inline struct drm_property
> *drm_property_find(struct drm_device *dev, #define drm_for_each_crtc(crtc,
> dev) \
>  	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head)
> 
> +static inline void
> +assert_drm_connector_list_read_locked(struct drm_mode_config *mode_config)
> +{
> +	WARN_ON(!mutex_is_locked(&mode_config->mutex));

I believe this introduced a regression for all drivers that call 
drm_mode_config_reset() at .load() time (and there's lots of them), as the 
mode config mutex isn't locked then.

> +}
> +
>  #define drm_for_each_connector(connector, dev) \
> -	list_for_each_entry(connector, &(dev)->mode_config.connector_list, head)
> +	for (assert_drm_connector_list_read_locked(&(dev)->mode_config),	\
> +	     connector = list_first_entry(&(dev)->mode_config.connector_list,	
\
> +					  struct drm_connector, head);		\
> +	     &connector->head != (&(dev)->mode_config.connector_list);		\
> +	     connector = list_next_entry(connector, head))
> 
>  #define drm_for_each_encoder(encoder, dev) \
>  	list_for_each_entry(encoder, &(dev)->mode_config.encoder_list, head)

-- 
Regards,

Laurent Pinchart

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