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

[Ville Syrjälä]

On Wed, Jan 24, 2018 at 11:24:05PM +0200, Ville Syrjälä wrote:
> 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 at 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.

And after giving this one second of actual though I figured it won't
actually complicate anything if we do it early enough. These functions
can't be safely called concurrently anyway, and so we can ignore any
races that sort of thing would bring.

> 
> 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC