On Fri, 21 Aug 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Fri, Aug 21, 2015 at 04:52:01PM +0300, Jani Nikula wrote:
>> From: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
