[Intel-gfx] [PATCH 1/2 v2] drm/i915: Allow parsing of variable size child device entries from VBT

[Michel Thierry]

On 8/5/2015 9:59 AM, Daniel Vetter wrote:
> On Tue, Aug 04, 2015 at 04:55:52PM +0300, David Weinehall wrote:
>> 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.
>>
>> v2: Stricter size checks
>>
>> Signed-off-by: David Weinehall <david.weinehall at linux.intel.com>
>
> Since Chris mentioned that this should fix a regression I applied it to
> drm-intel-fixes.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/intel_bios.c |   26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 2ff9eb00fdec..8a1f3e1fc598 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -1015,15 +1015,33 @@ 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 = 33;
>> +     } else if (bdb->version == 195) {
>> +             expected_size = 37;
>> +     } else if (bdb->version <= 197) {
>> +             expected_size = 38;
>> +     } else {
>> +             expected_size = 38;
>> +             DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n", expected_size);

Hi, the line above throws a warning because there are two '%u' but only 
one variable. Looks like bdb->version should be the 1st printed value.

>> +     }
>> +
>> +     if (expected_size > sizeof(*p_child)) {
>> +             DRM_ERROR("child_device_config cannot fit in p_child\n");
>> +             return;
>> +     }
>> +
>> +     if (p_defs->child_dev_size != expected_size) {
>> +             DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
>> +                       p_defs->child_dev_size, expected_size, bdb->version);
>>                return;
>>        }
>>        /* get the block size of general definitions */
>> @@ -1070,7 +1088,7 @@ 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));
>> +             memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>>        }
>>        return;
>>   }
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>