On Mon, 14 Dec 2015, Ville Syrjälä <ville.syrj...@linux.intel.com> 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.nik...@intel.com>
>> ---
>> 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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
