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

[Daniel Vetter]

On Thu, Aug 06, 2015 at 05:08:55PM +0300, David Weinehall wrote:
> On Thu, Aug 06, 2015 at 02:59:10PM +0100, Michel Thierry wrote:
> > 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.
> 
> Good catch, your analysis is indeed correct.
> 
> Daniel, will you do the fixup or should I submit a fixed version?

Fixed up locally.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch