Re: [PATCH] drm: Warn if plane/crtc/encoder/connector index exceeds our 32bit bitmasks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 24, 2018 at 04:01:18PM -0500, Harry Wentland wrote:
> On 2018-01-24 01:37 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We use 32bit bitmasks to track planes/crtcs/encoders/connectors.
> > Naturally we can only do that if the index of those objects stays
> > below 32. Issue a warning whenever we exceed that limit, hopefully
> > prompting someone to fix the problem.
> > 
> > Or should we just outright fail the object initializatio perhaps?
> > 
> 
> This would make sense in my opinion. index >= 32 looks like it would lead to completely undefined behavior when trying to attach objects to each other.

I suppose. The counter argument is that this is basically the developer
shooting themselves in the foot intentionally, so it's a bit hard to
justify complicating the runtime error handling for this.

The connector case might be well justified though since the index there
comes from ida, and with MST connectors can come and go as they please.
So it might not be entirely impossible to overflow the bits there.

> 
> Harry
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_connector.c | 3 +++
> >  drivers/gpu/drm/drm_crtc.c      | 3 +++
> >  drivers/gpu/drm/drm_encoder.c   | 3 +++
> >  drivers/gpu/drm/drm_plane.c     | 3 +++
> >  4 files changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index b85a7749709d..9278a81c5d54 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -211,6 +211,9 @@ int drm_connector_init(struct drm_device *dev,
> >  	connector->index = ret;
> >  	ret = 0;
> >  
> > +	/* we have 32bit connector bitmasks */
> > +	WARN_ON(connector->index >= 32);
> > +
> >  	connector->connector_type = connector_type;
> >  	connector->connector_type_id =
> >  		ida_simple_get(connector_ida, 1, 0, GFP_KERNEL);
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index f0556e654116..e27ffa3561af 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -318,6 +318,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
> >  	list_add_tail(&crtc->head, &config->crtc_list);
> >  	crtc->index = config->num_crtc++;
> >  
> > +	/* we have 32bit crtc bitmasks */
> > +	WARN_ON(crtc->index >= 32);
> > +
> >  	crtc->primary = primary;
> >  	crtc->cursor = cursor;
> >  	if (primary && !primary->possible_crtcs)
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index 59e0ebe733f8..66d0cdd217fa 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -136,6 +136,9 @@ int drm_encoder_init(struct drm_device *dev,
> >  	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
> >  	encoder->index = dev->mode_config.num_encoder++;
> >  
> > +	/* we have 32bit encoder bitmasks */
> > +	WARN_ON(encoder->index >= 32);
> > +
> >  out_put:
> >  	if (ret)
> >  		drm_mode_object_unregister(dev, &encoder->base);
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 2c90519576a3..0a8d807603c1 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -242,6 +242,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >  	list_add_tail(&plane->head, &config->plane_list);
> >  	plane->index = config->num_total_plane++;
> >  
> > +	/* we have 32bit plane bitmasks */
> > +	WARN_ON(plane->index >= 32);
> > +
> >  	drm_object_attach_property(&plane->base,
> >  				   config->plane_type_property,
> >  				   plane->type);
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx