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

[Harry Wentland]

On 2018-01-24 04:24 PM, 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.
> 

True, if this needs extensive runtime error handling a WARN_ON is probably better.

> 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.
> 

Right, I was trying to think of a real-life case where this might happen. MST is a good one (although still quite rare in the vast majority of cases).

Harry

>>
>> 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);
>>>
>