| Message ID | 20191120234608.17775-3-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/bios: do not discard address space | expand |
On Wed, 20 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > The unaligned ioread32() will make us read byte by byte looking for the > vbt. We could just as well have done a ioread8() + a shift and avoid the > extra confusion on how we are looking for "$VBT". > > However when using ACPI it's guaranteed the VBT is 4-byte aligned > per spec, so we can probably assume it here as well. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/display/intel_bios.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > index aa9b182efee5..6bf57b1ad056 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -1902,27 +1902,20 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *dev_priv) > void __iomem *p = NULL, *oprom; > struct vbt_header *vbt; > u16 vbt_size; > - size_t i, size; > + size_t size; > > oprom = pci_map_rom(pdev, &size); > if (!oprom) > return NULL; > > /* Scour memory looking for the VBT signature. */ > - for (i = 0; i + 4 < size; i++) { > - if (ioread32(oprom + i) != *((const u32 *)"$VBT")) > - continue; > - > - p = oprom + i; > - size -= i; > - break; > - } > - > - if (!p) > - goto err_unmap_oprom; > + for (p = oprom; size >= 4; p += 4, size -= 4) > + if (ioread32(p) == *((const u32 *)"$VBT")) > + break; I think the original is easier to read. You only really need to change "i++" to "i += 4" and be done with it. BR, Jani. > > if (sizeof(struct vbt_header) > size) { > - DRM_DEBUG_DRIVER("VBT header incomplete\n"); > + if (size >= 4) > + DRM_DEBUG_DRIVER("VBT header incomplete\n"); > goto err_unmap_oprom; > }
On Thu, Nov 21, 2019 at 10:54:29AM -0800, Lucas De Marchi wrote: > On Thu, Nov 21, 2019 at 03:09:03PM +0200, Jani Nikula wrote: > >On Wed, 20 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > >> The unaligned ioread32() will make us read byte by byte looking for the > >> vbt. We could just as well have done a ioread8() + a shift and avoid the > >> extra confusion on how we are looking for "$VBT". > >> > >> However when using ACPI it's guaranteed the VBT is 4-byte aligned > >> per spec, so we can probably assume it here as well. > >> > >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > >> --- > >> drivers/gpu/drm/i915/display/intel_bios.c | 19 ++++++------------- > >> 1 file changed, 6 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > >> index aa9b182efee5..6bf57b1ad056 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_bios.c > >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c > >> @@ -1902,27 +1902,20 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *dev_priv) > >> void __iomem *p = NULL, *oprom; > >> struct vbt_header *vbt; > >> u16 vbt_size; > >> - size_t i, size; > >> + size_t size; > >> > >> oprom = pci_map_rom(pdev, &size); > >> if (!oprom) > >> return NULL; > >> > >> /* Scour memory looking for the VBT signature. */ > >> - for (i = 0; i + 4 < size; i++) { > >> - if (ioread32(oprom + i) != *((const u32 *)"$VBT")) > >> - continue; > >> - > >> - p = oprom + i; > >> - size -= i; > >> - break; > >> - } > >> - > >> - if (!p) > >> - goto err_unmap_oprom; > >> + for (p = oprom; size >= 4; p += 4, size -= 4) > >> + if (ioread32(p) == *((const u32 *)"$VBT")) > >> + break; > > > >I think the original is easier to read. You only really need to change > >"i++" to "i += 4" and be done with it. > > I really liked this version much more... shorter and with only one control > variable rather than keeping the control in 3 different places (i, size > and p). I think I'm with Jani here. Generally not a huge fan of pointer arithmetic, and having just one variable modified by the loop is more customary so usually doesn't require me to read more than once. This thing I had to read a few times to make sure I understood what it's doing.
On Fri, Nov 22, 2019 at 03:55:32PM +0200, Ville Syrjälä wrote: >On Thu, Nov 21, 2019 at 10:54:29AM -0800, Lucas De Marchi wrote: >> On Thu, Nov 21, 2019 at 03:09:03PM +0200, Jani Nikula wrote: >> >On Wed, 20 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> >> The unaligned ioread32() will make us read byte by byte looking for the >> >> vbt. We could just as well have done a ioread8() + a shift and avoid the >> >> extra confusion on how we are looking for "$VBT". >> >> >> >> However when using ACPI it's guaranteed the VBT is 4-byte aligned >> >> per spec, so we can probably assume it here as well. >> >> >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/display/intel_bios.c | 19 ++++++------------- >> >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c >> >> index aa9b182efee5..6bf57b1ad056 100644 >> >> --- a/drivers/gpu/drm/i915/display/intel_bios.c >> >> +++ b/drivers/gpu/drm/i915/display/intel_bios.c >> >> @@ -1902,27 +1902,20 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *dev_priv) >> >> void __iomem *p = NULL, *oprom; >> >> struct vbt_header *vbt; >> >> u16 vbt_size; >> >> - size_t i, size; >> >> + size_t size; >> >> >> >> oprom = pci_map_rom(pdev, &size); >> >> if (!oprom) >> >> return NULL; >> >> >> >> /* Scour memory looking for the VBT signature. */ >> >> - for (i = 0; i + 4 < size; i++) { >> >> - if (ioread32(oprom + i) != *((const u32 *)"$VBT")) >> >> - continue; >> >> - >> >> - p = oprom + i; >> >> - size -= i; >> >> - break; >> >> - } >> >> - >> >> - if (!p) >> >> - goto err_unmap_oprom; >> >> + for (p = oprom; size >= 4; p += 4, size -= 4) >> >> + if (ioread32(p) == *((const u32 *)"$VBT")) >> >> + break; >> > >> >I think the original is easier to read. You only really need to change >> >"i++" to "i += 4" and be done with it. >> >> I really liked this version much more... shorter and with only one control >> variable rather than keeping the control in 3 different places (i, size >> and p). > >I think I'm with Jani here. Generally not a huge fan of pointer >arithmetic, and having just one variable modified by the loop is >more customary so usually doesn't require me to read more than we were previously modifying 3: i, p and size. And additionally using oprom.... versus we only care about p inside the loop, which points to whatever we just read... and we we keep updates on size to control the stop condition. >once. This thing I had to read a few times to make sure I >understood what it's doing. Ok, 2 against 1. I will respin this. Lucas De Marchi > >-- >Ville Syrjälä >Intel
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index aa9b182efee5..6bf57b1ad056 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -1902,27 +1902,20 @@ static struct vbt_header *oprom_get_vbt(struct drm_i915_private *dev_priv) void __iomem *p = NULL, *oprom; struct vbt_header *vbt; u16 vbt_size; - size_t i, size; + size_t size; oprom = pci_map_rom(pdev, &size); if (!oprom) return NULL; /* Scour memory looking for the VBT signature. */ - for (i = 0; i + 4 < size; i++) { - if (ioread32(oprom + i) != *((const u32 *)"$VBT")) - continue; - - p = oprom + i; - size -= i; - break; - } - - if (!p) - goto err_unmap_oprom; + for (p = oprom; size >= 4; p += 4, size -= 4) + if (ioread32(p) == *((const u32 *)"$VBT")) + break; if (sizeof(struct vbt_header) > size) { - DRM_DEBUG_DRIVER("VBT header incomplete\n"); + if (size >= 4) + DRM_DEBUG_DRIVER("VBT header incomplete\n"); goto err_unmap_oprom; }
The unaligned ioread32() will make us read byte by byte looking for the vbt. We could just as well have done a ioread8() + a shift and avoid the extra confusion on how we are looking for "$VBT". However when using ACPI it's guaranteed the VBT is 4-byte aligned per spec, so we can probably assume it here as well. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/display/intel_bios.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)