Re: [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 21 Aug 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Aug 21, 2015 at 04:52:01PM +0300, Jani Nikula wrote:
>> From: David Weinehall <david.weinehall@linux.intel.com>
>> 
>> VBT version 196 increased the size of common_child_dev_config. The
>> parser code assumed that the size of this structure would not change.
>> 
>> The modified code now copies the amount needed based on the VBT version,
>> and emits a debug message if the VBT version is unknown (too new); since
>> the struct config block won't shrink in newer versions it should be
>> harmless to copy the maximum known size in such cases, so that's what we
>> do, but emitting the warning is probably sensible anyway.
>> 
>> In the longer run it might make sense to modify the parser code to use a
>> version/feature mapping, rather than hardcoding things like this, but
>> for now the variants are fairly managable.
>> 
>> This fixes a regression introduced in
>> 
>> commit 75067ddecf21271631bc018d2fb23ddd09b66aae
>> Author: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>> Date:   Fri Jul 10 14:10:55 2015 +0300
>> 
>>     drm/i915: Per-DDI I_boost override
>> 
>> since that commit changed the child device config size without updating
>> the checks and memcpy.
>> 
>> v2: Stricter size checks
>> 
>> v3 by Jani:
>> - Keep the checks strict, and warnigns verbose, but keep going anyway.
>> - Take care to copy the max amount of child device config we can.
>> - Fix the messages.
>> 
>> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 37 +++++++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_bios.h |  6 ++++--
>>  2 files changed, 37 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 64e5b15ae0b6..be83b77aa018 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -1051,17 +1051,39 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>  	const union child_device_config *p_child;
>>  	union child_device_config *child_dev_ptr;
>>  	int i, child_device_num, count;
>> -	u16	block_size;
>> +	u8 expected_size;
>> +	u16 block_size;
>>  
>>  	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>>  	if (!p_defs) {
>>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>>  		return;
>>  	}
>> -	if (p_defs->child_dev_size < sizeof(*p_child)) {
>> -		DRM_ERROR("General definiton block child device size is too small.\n");
>> +	if (bdb->version < 195) {
>> +		expected_size = sizeof(struct old_child_dev_config);
>> +	} else if (bdb->version == 195) {
>> +		expected_size = 37;
>> +	} else if (bdb->version <= 197) {
>> +		expected_size = 38;
>> +	} else {
>> +		expected_size = 38;
>> +		BUILD_BUG_ON(sizeof(*p_child) < 38);
>> +		DRM_DEBUG_DRIVER("Expected child device config size for VBT version %u not known; assuming %u\n",
>> +				 bdb->version, expected_size);
>> +	}
>> +
>> +	/* The legacy sized child device config is the minimum we need. */
>> +	if (p_defs->child_dev_size < sizeof(struct old_child_dev_config)) {
>> +		DRM_ERROR("Child device config size %u is too small.\n",
>> +			  p_defs->child_dev_size);
>>  		return;
>>  	}
>> +
>> +	/* Flag an error for unexpected size, but continue anyway. */
>> +	if (p_defs->child_dev_size != expected_size)
>> +		DRM_ERROR("Unexpected child device config size %u (expected %u for VBT version %u)\n",
>> +			  p_defs->child_dev_size, expected_size, bdb->version);
>> +
>>  	/* get the block size of general definitions */
>>  	block_size = get_blocksize(p_defs);
>>  	/* get the number of child device */
>> @@ -1106,7 +1128,14 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>  
>>  		child_dev_ptr = dev_priv->vbt.child_dev + count;
>>  		count++;
>> -		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>> +
>> +		/*
>> +		 * Copy as much as we know (sizeof) and is available
>> +		 * (child_dev_size) of the child device. Accessing the data must
>> +		 * depend on VBT version.
>> +		 */
>> +		memcpy(child_dev_ptr, p_child,
>> +		       min_t(size_t, p_defs->child_dev_size, sizeof(*p_child)));
>
> Looks good. Could maybe use a
> BUILD_BUG_ON(sizeof(struct old_child_dev_config) != 33)
> somewhere to make sure people don't make a mess of things.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to drm-intel-next-fixes, thanks for the review.

BR,
Jani.


>
>>  	}
>>  	return;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 6d909efbf43f..06d0dbde2be6 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -203,9 +203,11 @@ struct bdb_general_features {
>>  #define DEVICE_PORT_DVOB	0x01
>>  #define DEVICE_PORT_DVOC	0x02
>>  
>> -/* We used to keep this struct but without any version control. We should avoid
>> +/*
>> + * We used to keep this struct but without any version control. We should avoid
>>   * using it in the future, but it should be safe to keep using it in the old
>> - * code. */
>> + * code. Do not change; we rely on its size.
>> + */
>>  struct old_child_dev_config {
>>  	u16 handle;
>>  	u16 device_type;
>> -- 
>> 2.1.4
>
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx