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>
> }
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
