| Message ID | 20191108211353.22288-3-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/bios: rename bios to oprom when mapping pci rom | expand |
On Fri, Nov 08, 2019 at 01:13:53PM -0800, Lucas De Marchi wrote: > When we map the VBT through pci_map_rom() we may not be allowed > to simply discard the address space and go on reading the memory. > That doesn't work on my test system, but by dumping the rom via > sysfs I can can get the correct vbt. So change our find_vbt() to do > the same as done by pci_read_rom(), i.e. use memcpy_fromio(). > > v2: the just the minimal changes by not bothering with the unaligned io > reads: this can be done on top (from Ville and Jani) > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/display/intel_bios.c | 51 +++++++++++++++++------ > 1 file changed, 39 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > index c79781e1ccbf..c079febae9c8 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -1811,28 +1811,52 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size) > return vbt; > } > > -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size) > +static const struct vbt_header *copy_vbt(void __iomem *oprom, size_t size) > { > + void __iomem *p = NULL; > + struct vbt_header *vbt; > + u16 vbt_size; > size_t i; > > /* Scour memory looking for the VBT signature. */ > for (i = 0; i + 4 < size; i++) { > - void *vbt; > - > if (ioread32(oprom + 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 *)oprom + i; > - if (intel_bios_is_valid_vbt(vbt, size - i)) > - return vbt; > - > + p = oprom + i; > + size -= i; > break; > } > > + if (!p) > + return NULL; > + > + if (sizeof(struct vbt_header) > size) { > + DRM_DEBUG_DRIVER("VBT header incomplete\n"); > + return NULL; > + } > + > + vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size)); > + if (vbt_size > size) { > + DRM_DEBUG_DRIVER("VBT incomplete (vbt_size overflows)\n"); > + return NULL; > + } > + > + /* The rest will be validated by intel_bios_is_valid_vbt() */ > + vbt = kmalloc(vbt_size, GFP_KERNEL); > + if (!vbt) > + return NULL; > + > + memcpy_fromio(vbt, p, vbt_size); > + > + if (!intel_bios_is_valid_vbt(vbt, vbt_size)) > + goto err_free_vbt; > + > + return vbt; > + > +err_free_vbt: > + kfree(vbt); > + > return NULL; > } > > @@ -1866,7 +1890,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv) > if (!oprom) > goto out; > > - vbt = find_vbt(oprom, size); > + vbt = copy_vbt(oprom, size); > if (!vbt) > goto out; > > @@ -1902,6 +1926,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv) > > if (oprom) > pci_unmap_rom(pdev, oprom); lgtm Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> As a followup we could now move the rom map/unamp into copy_vbt() as there's no longer any need to keep it mapped across the whole thing. > + > + if (vbt != dev_priv->opregion.vbt) > + kfree(vbt); > } > > /** > -- > 2.24.0
On Fri, Nov 15, 2019 at 07:40:15PM +0200, Ville Syrjälä wrote: >On Fri, Nov 08, 2019 at 01:13:53PM -0800, Lucas De Marchi wrote: >> When we map the VBT through pci_map_rom() we may not be allowed >> to simply discard the address space and go on reading the memory. >> That doesn't work on my test system, but by dumping the rom via >> sysfs I can can get the correct vbt. So change our find_vbt() to do >> the same as done by pci_read_rom(), i.e. use memcpy_fromio(). >> >> v2: the just the minimal changes by not bothering with the unaligned io >> reads: this can be done on top (from Ville and Jani) >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_bios.c | 51 +++++++++++++++++------ >> 1 file changed, 39 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c >> index c79781e1ccbf..c079febae9c8 100644 >> --- a/drivers/gpu/drm/i915/display/intel_bios.c >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c >> @@ -1811,28 +1811,52 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size) >> return vbt; >> } >> >> -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size) >> +static const struct vbt_header *copy_vbt(void __iomem *oprom, size_t size) just noticed this shouldn't be const anymore, since the caller is supposed to free the return >> { >> + void __iomem *p = NULL; >> + struct vbt_header *vbt; >> + u16 vbt_size; >> size_t i; >> >> /* Scour memory looking for the VBT signature. */ >> for (i = 0; i + 4 < size; i++) { >> - void *vbt; >> - >> if (ioread32(oprom + 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 *)oprom + i; >> - if (intel_bios_is_valid_vbt(vbt, size - i)) >> - return vbt; >> - >> + p = oprom + i; >> + size -= i; >> break; >> } >> >> + if (!p) >> + return NULL; >> + >> + if (sizeof(struct vbt_header) > size) { >> + DRM_DEBUG_DRIVER("VBT header incomplete\n"); >> + return NULL; >> + } >> + >> + vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size)); >> + if (vbt_size > size) { >> + DRM_DEBUG_DRIVER("VBT incomplete (vbt_size overflows)\n"); >> + return NULL; >> + } >> + >> + /* The rest will be validated by intel_bios_is_valid_vbt() */ >> + vbt = kmalloc(vbt_size, GFP_KERNEL); >> + if (!vbt) >> + return NULL; >> + >> + memcpy_fromio(vbt, p, vbt_size); >> + >> + if (!intel_bios_is_valid_vbt(vbt, vbt_size)) >> + goto err_free_vbt; >> + >> + return vbt; >> + >> +err_free_vbt: >> + kfree(vbt); >> + >> return NULL; >> } >> >> @@ -1866,7 +1890,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv) >> if (!oprom) >> goto out; >> >> - vbt = find_vbt(oprom, size); >> + vbt = copy_vbt(oprom, size); >> if (!vbt) >> goto out; >> >> @@ -1902,6 +1926,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv) >> >> if (oprom) >> pci_unmap_rom(pdev, oprom); > >lgtm >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >As a followup we could now move the rom map/unamp into copy_vbt() as >there's no longer any need to keep it mapped across the whole thing. yep, I will add this and a tentative to avoid the unaligned reads thanks Lucas De Marchi > >> + >> + if (vbt != dev_priv->opregion.vbt) >> + kfree(vbt); >> } >> >> /** >> -- >> 2.24.0 > >-- >Ville Syrjälä >Intel >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index c79781e1ccbf..c079febae9c8 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -1811,28 +1811,52 @@ bool intel_bios_is_valid_vbt(const void *buf, size_t size) return vbt; } -static const struct vbt_header *find_vbt(void __iomem *oprom, size_t size) +static const struct vbt_header *copy_vbt(void __iomem *oprom, size_t size) { + void __iomem *p = NULL; + struct vbt_header *vbt; + u16 vbt_size; size_t i; /* Scour memory looking for the VBT signature. */ for (i = 0; i + 4 < size; i++) { - void *vbt; - if (ioread32(oprom + 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 *)oprom + i; - if (intel_bios_is_valid_vbt(vbt, size - i)) - return vbt; - + p = oprom + i; + size -= i; break; } + if (!p) + return NULL; + + if (sizeof(struct vbt_header) > size) { + DRM_DEBUG_DRIVER("VBT header incomplete\n"); + return NULL; + } + + vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size)); + if (vbt_size > size) { + DRM_DEBUG_DRIVER("VBT incomplete (vbt_size overflows)\n"); + return NULL; + } + + /* The rest will be validated by intel_bios_is_valid_vbt() */ + vbt = kmalloc(vbt_size, GFP_KERNEL); + if (!vbt) + return NULL; + + memcpy_fromio(vbt, p, vbt_size); + + if (!intel_bios_is_valid_vbt(vbt, vbt_size)) + goto err_free_vbt; + + return vbt; + +err_free_vbt: + kfree(vbt); + return NULL; } @@ -1866,7 +1890,7 @@ void intel_bios_init(struct drm_i915_private *dev_priv) if (!oprom) goto out; - vbt = find_vbt(oprom, size); + vbt = copy_vbt(oprom, size); if (!vbt) goto out; @@ -1902,6 +1926,9 @@ void intel_bios_init(struct drm_i915_private *dev_priv) if (oprom) pci_unmap_rom(pdev, oprom); + + if (vbt != dev_priv->opregion.vbt) + kfree(vbt); } /**
When we map the VBT through pci_map_rom() we may not be allowed to simply discard the address space and go on reading the memory. That doesn't work on my test system, but by dumping the rom via sysfs I can can get the correct vbt. So change our find_vbt() to do the same as done by pci_read_rom(), i.e. use memcpy_fromio(). v2: the just the minimal changes by not bothering with the unaligned io reads: this can be done on top (from Ville and Jani) Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/display/intel_bios.c | 51 +++++++++++++++++------ 1 file changed, 39 insertions(+), 12 deletions(-)