* [PATCH 0/1] vbt child device size fix @ 2015-08-21 13:52 Jani Nikula 2015-08-21 13:52 ` [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT Jani Nikula 0 siblings, 1 reply; 4+ messages in thread From: Jani Nikula @ 2015-08-21 13:52 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula Since I had to revert David's commit from v4.2, we'll have to add it back to drm-intel-next-fixes for v4.3. So it's a good occasion to reflect on it. I think it's problematic to be so strict with the VBT child device size. We generally go for really verbose error and debug messages in the dmesg and plunge on even when almost all hope is lost; it seems rather silly to effectively stop trying to get displays lit up if the VBT child device size is larger than expected. Hey, new stuff gets added at the end all the time. So log what happened, and move on. Here's a modified version of David's patch to do just that. BR, Jani. David Weinehall (1): drm/i915: Allow parsing of variable size child device entries from VBT drivers/gpu/drm/i915/intel_bios.c | 37 +++++++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_bios.h | 6 ++++-- 2 files changed, 37 insertions(+), 6 deletions(-) -- 2.1.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT 2015-08-21 13:52 [PATCH 0/1] vbt child device size fix Jani Nikula @ 2015-08-21 13:52 ` Jani Nikula 2015-08-21 14:24 ` Ville Syrjälä 0 siblings, 1 reply; 4+ messages in thread From: Jani Nikula @ 2015-08-21 13:52 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula 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))); } 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT 2015-08-21 13:52 ` [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT Jani Nikula @ 2015-08-21 14:24 ` Ville Syrjälä 2015-08-24 7:40 ` Jani Nikula 0 siblings, 1 reply; 4+ messages in thread From: Ville Syrjälä @ 2015-08-21 14:24 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx 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> > } > 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT 2015-08-21 14:24 ` Ville Syrjälä @ 2015-08-24 7:40 ` Jani Nikula 0 siblings, 0 replies; 4+ messages in thread From: Jani Nikula @ 2015-08-24 7:40 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-08-24 7:37 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-21 13:52 [PATCH 0/1] vbt child device size fix Jani Nikula 2015-08-21 13:52 ` [PATCH 1/1] drm/i915: Allow parsing of variable size child device entries from VBT Jani Nikula 2015-08-21 14:24 ` Ville Syrjälä 2015-08-24 7:40 ` Jani Nikula
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox