Re: [PATCH v2 1/4] drm/i915: Pass size to oprom_get_vbt

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 20 Feb 2024, Radhakrishna Sripada <radhakrishna.sripada@intel.com> wrote:
> oprom_get_vbt will later be used to show the contents of vbt for which
> the size of vbt is needed.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index fe52c06271ef..ceb6e4145c62 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -3008,38 +3008,39 @@ static struct vbt_header *spi_oprom_get_vbt(struct drm_i915_private *i915)
>  	return NULL;
>  }
>  
> -static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915)
> +static struct vbt_header *oprom_get_vbt(struct drm_i915_private *i915,
> +					size_t *size)
>  {
>  	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>  	void __iomem *p = NULL, *oprom;
>  	struct vbt_header *vbt;
>  	u16 vbt_size;
> -	size_t i, size;
> +	size_t i;
>  
> -	oprom = pci_map_rom(pdev, &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 += 4) {
> +	for (i = 0; i + 4 < *size; i += 4) {

Not a fan of using *size everywhere. Please just use size here, and do
something like this near the end:

	if (sizep)
		*sizep = size;

This also allows you to pass NULL when you don't need the size, like
intel_bios_init(), and you can drop the extra dummy local variable.

BR,
Jani.

>  		if (ioread32(oprom + i) != *((const u32 *)"$VBT"))
>  			continue;
>  
>  		p = oprom + i;
> -		size -= i;
> +		*size -= i;
>  		break;
>  	}
>  
>  	if (!p)
>  		goto err_unmap_oprom;
>  
> -	if (sizeof(struct vbt_header) > size) {
> +	if (sizeof(struct vbt_header) > *size) {
>  		drm_dbg(&i915->drm, "VBT header incomplete\n");
>  		goto err_unmap_oprom;
>  	}
>  
>  	vbt_size = ioread16(p + offsetof(struct vbt_header, vbt_size));
> -	if (vbt_size > size) {
> +	if (vbt_size > *size) {
>  		drm_dbg(&i915->drm,
>  			"VBT incomplete (vbt_size overflows)\n");
>  		goto err_unmap_oprom;
> @@ -3082,6 +3083,7 @@ void intel_bios_init(struct drm_i915_private *i915)
>  	const struct vbt_header *vbt;
>  	struct vbt_header *oprom_vbt = NULL;
>  	const struct bdb_header *bdb;
> +	size_t size;
>  
>  	INIT_LIST_HEAD(&i915->display.vbt.display_devices);
>  	INIT_LIST_HEAD(&i915->display.vbt.bdb_blocks);
> @@ -3106,7 +3108,7 @@ void intel_bios_init(struct drm_i915_private *i915)
>  	}
>  
>  	if (!vbt) {
> -		oprom_vbt = oprom_get_vbt(i915);
> +		oprom_vbt = oprom_get_vbt(i915, &size);
>  		vbt = oprom_vbt;
>  	}

-- 
Jani Nikula, Intel