On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Mon, Dec 14, 2015 at 12:50:50PM +0200, Jani Nikula wrote: >> Make the validation function a boolean operating on a buffer of given >> size, removing the extra pointer dances. >> >> Move the OpRegion based VBT validation to intel_opregion_setup(), only >> initializing opregion->vbt if it's valid. >> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_bios.c | 63 ++++++++++++++++++----------------- >> drivers/gpu/drm/i915/intel_opregion.c | 9 +++-- >> 3 files changed, 40 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index a2bd4c6a86a1..ef15f1710b50 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3354,6 +3354,7 @@ extern void intel_i2c_reset(struct drm_device *dev); >> >> /* intel_bios.c */ >> int intel_bios_init(struct drm_device *dev); >> +bool intel_bios_is_valid_vbt(const void *buf, size_t size); >> >> /* intel_opregion.c */ >> #ifdef CONFIG_ACPI >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >> index f2b79b7d12b8..007b2b759600 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -1221,37 +1221,40 @@ static const struct bdb_header *get_bdb_header(const struct vbt_header *vbt) >> return _vbt + vbt->bdb_offset; >> } >> >> -static const struct vbt_header *validate_vbt(const void *base, >> - size_t size, >> - const void *_vbt) >> +/** >> + * intel_bios_is_valid_vbt - does the given buffer contain a valid VBT >> + * @buf: pointer to a buffer to validate >> + * @size: size of the buffer >> + * >> + * Returns true on valid VBT. >> + */ >> +bool intel_bios_is_valid_vbt(const void *buf, size_t size) >> { >> - size_t offset = _vbt - base; >> - const struct vbt_header *vbt = _vbt; >> + const struct vbt_header *vbt = buf; >> const struct bdb_header *bdb; >> >> if (!vbt) >> - return NULL; >> + return false; >> >> - if (offset + sizeof(struct vbt_header) > size) { >> + if (sizeof(struct vbt_header) > size) { >> DRM_DEBUG_DRIVER("VBT header incomplete\n"); >> - return NULL; >> + return false; >> } >> >> if (memcmp(vbt->signature, "$VBT", 4)) { >> DRM_DEBUG_DRIVER("VBT invalid signature\n"); >> - return NULL; >> + return false; >> } >> >> - offset += vbt->bdb_offset; >> - if (offset + sizeof(struct bdb_header) > size) { >> + if (vbt->bdb_offset + sizeof(struct bdb_header) > size) { >> DRM_DEBUG_DRIVER("BDB header incomplete\n"); >> - return NULL; >> + return false; >> } >> >> bdb = get_bdb_header(vbt); >> - if (offset + bdb->bdb_size > size) { >> + if (vbt->bdb_offset + bdb->bdb_size > size) { >> DRM_DEBUG_DRIVER("BDB incomplete\n"); >> - return NULL; >> + return false; >> } >> >> return vbt; >> @@ -1259,26 +1262,27 @@ static const struct vbt_header *validate_vbt(const void *base, >> >> static const struct vbt_header *find_vbt(void __iomem *bios, size_t size) >> { >> - const struct vbt_header *vbt = NULL; >> size_t i; >> >> /* Scour memory looking for the VBT signature. */ >> for (i = 0; i + 4 < size; i++) { >> - if (ioread32(bios + i) == *((const u32 *) "$VBT")) { >> - /* >> - * This is the one place where we explicitly discard the >> - * address space (__iomem) of the BIOS/VBT. From now on >> - * everything is based on 'base', and treated as regular >> - * memory. >> - */ >> - void *_bios = (void __force *) bios; >> + void *vbt; >> >> - vbt = validate_vbt(_bios, size, _bios + i); >> - break; >> - } >> + if (ioread32(bios + i) != *((const u32 *) "$VBT")) >> + continue; >> + >> + /* >> + * This is the one place where we explicitly discard the address >> + * space (__iomem) of the BIOS/VBT. >> + */ >> + vbt = (void __force *) bios + i; >> + if (intel_bios_is_valid_vbt(vbt, size - i)) >> + return vbt; >> + >> + break; >> } >> >> - return vbt; >> + return NULL; >> } >> >> /** >> @@ -1295,7 +1299,7 @@ intel_bios_init(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct pci_dev *pdev = dev->pdev; >> - const struct vbt_header *vbt; >> + const struct vbt_header *vbt = dev_priv->opregion.vbt; >> const struct bdb_header *bdb; >> u8 __iomem *bios = NULL; >> >> @@ -1304,9 +1308,6 @@ intel_bios_init(struct drm_device *dev) >> >> init_vbt_defaults(dev_priv); >> >> - /* XXX Should this validation be moved to intel_opregion.c? */ >> - vbt = validate_vbt(dev_priv->opregion.header, OPREGION_SIZE, >> - dev_priv->opregion.vbt); >> if (vbt) { >> DRM_DEBUG_KMS("Using VBT from ACPI OpRegion: %20s\n", >> vbt->signature); > > Maybe the debug message should be moved too? It looks a bit out of place > after the validation gets moved. This is intentional (see patch 4). In a way this is where the decision is made to use the opregion VBT. The opregion code just validates it, but doesn't actually *use* it. BR, Jani. > >> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c >> index 5b9fc790d300..9f992e7c6185 100644 >> --- a/drivers/gpu/drm/i915/intel_opregion.c >> +++ b/drivers/gpu/drm/i915/intel_opregion.c >> @@ -986,8 +986,13 @@ int intel_opregion_setup(struct drm_device *dev) >> if (mboxes & MBOX_ASLE_EXT) >> DRM_DEBUG_DRIVER("ASLE extension supported\n"); >> >> - if (!dmi_check_system(intel_no_opregion_vbt)) >> - opregion->vbt = base + OPREGION_VBT_OFFSET; >> + if (!dmi_check_system(intel_no_opregion_vbt)) { >> + void *vbt = base + OPREGION_VBT_OFFSET; >> + u32 vbt_size = OPREGION_SIZE - OPREGION_VBT_OFFSET; >> + >> + if (intel_bios_is_valid_vbt(vbt, vbt_size)) >> + opregion->vbt = vbt; >> + } >> >> return 0; >> >> -- >> 2.1.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx