Re: [PATCH v11 4/8] drm/i915/bios: de/allocate VS/PE-O buffers for each port

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Memory leak of devdata->entries during driver removal.
- [Low] Missing initialization of `num_entries` in `devdata->vspeo`.
--

--- Patch [4]: [PATCH v11 4/8] drm/i915/bios: de/allocate VS/PE-O buffers for each port ---
commit f4e52d6386e66ed2695365f0776e18c426ae4237
Author: Michał Grzelak <[email protected]>

drm/i915/bios: de/allocate VS/PE-O buffers for each port
    
This patch allocates dedicated VS/PE-O buffers for each port if an
override is requested and supported. It adds intel_ddi_buf_trans and
entries fields to intel_bios_encoder_data and handles their allocation
and deallocation.

> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
> index 9610b794bc147..a491b85006117 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
[ ... ]
> @@ -2648,6 +2651,29 @@ static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
> +static void allocate_vswing_preemph_override(struct intel_bios_encoder_data *devdata)
> +{
> +	int num_rows = devdata->display->vbt.vspeo.num_rows;
> +	union intel_ddi_buf_trans_entry *entries;
> +	struct intel_ddi_buf_trans *vspeo;
> +
> +	if (!intel_bios_encoder_requests_vspeo(devdata))
> +		return;
> +
> +	vspeo = kzalloc_obj(*vspeo);
> +	if (!vspeo)
> +		return;
> +
> +	entries = kzalloc_objs(*entries, num_rows);
> +	if (!entries) {
> +		kfree(vspeo);
> +		return;
> +	}
> +
> +	devdata->vspeo = vspeo;
> +	devdata->entries = entries;

[Severity: Low]
Is this code missing the initialization of devdata->vspeo->num_entries?

The commit message notes for v4->v5 say "- set devdata->vspeo->num_entries in
intel_bios.c", but this doesn't seem to be happening here. Leaving this at 0
would make the table appear empty.

> +}
[ ... ]
> @@ -3403,6 +3430,11 @@ void intel_bios_driver_remove(struct intel_display *display)
>  	list_for_each_entry_safe(devdata, nd, &display->vbt.display_devices,
>  				 node) {
>  		list_del(&devdata->node);
> +
> +		if (devdata->vspeo)
> +			kfree(devdata->vspeo->entries);

[Severity: Medium]
Will this leak the entries buffer allocated in
allocate_vswing_preemph_override()?

Because devdata->vspeo->entries is never initialized in this patch, it remains
NULL, and kfree(NULL) is a no-op. Should this be kfree(devdata->entries)
instead?

> +
> +		kfree(devdata->vspeo);
>  		kfree(devdata->dsc);
>  		kfree(devdata);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4