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