Re: [PATCH] accel/ivpu: Read preemption buffer size from boot params

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel/ivpu: ivpu_preemption_config_parse() rejects zero secondary buffer size
--

commit e2bae997f3bdfe346d64fb0ce52874d7284d8b32
Author: Andrzej Kacprowski <andrzej.kacprowski@linux.intel.com>

accel/ivpu: Read preemption buffer size from boot params

This commit updates the ivpu driver to read the preemption buffer size from
the firmware boot parameters after cold boot during device initialization,
falling back to the sizes in the firmware image header if not reported. It
also updates the image header parsing logic to use
preemption_buffer_2_max_size even when it is zero if
preemption_buffer_1_max_size is set, and updates the firmware boot API header
to version 3.39.1.

> diff --git a/drivers/accel/ivpu/ivpu_fw.c b/drivers/accel/ivpu/ivpu_fw.c
> index 96f5be23a8673..6607a5da19340 100644
> --- a/drivers/accel/ivpu/ivpu_fw.c
> +++ b/drivers/accel/ivpu/ivpu_fw.c
[ ... ]
> @@ -160,35 +165,31 @@ ivpu_fw_sched_mode_select(struct ivpu_device *vdev)
>  static void
>  ivpu_preemption_config_parse(struct ivpu_device *vdev, const struct vpu_firmware_header *fw_hdr)
>  {
> -	struct ivpu_fw_info *fw = vdev->fw;
>  	u32 primary_preempt_buf_size, secondary_preempt_buf_size;
>  
> -	if (fw_hdr->preemption_buffer_1_max_size)
> +	if (fw_hdr->preemption_buffer_1_max_size) {
>  		primary_preempt_buf_size = fw_hdr->preemption_buffer_1_max_size;
> -	else
> -		primary_preempt_buf_size = fw_hdr->preemption_buffer_1_size;
> -
> -	if (fw_hdr->preemption_buffer_2_max_size)
>  		secondary_preempt_buf_size = fw_hdr->preemption_buffer_2_max_size;
> -	else
> +	} else {
> +		primary_preempt_buf_size = fw_hdr->preemption_buffer_1_size;
>  		secondary_preempt_buf_size = fw_hdr->preemption_buffer_2_size;
> -
> -	ivpu_dbg(vdev, FW_BOOT, "Preemption buffer size, primary: %u, secondary: %u\n",
> -		 primary_preempt_buf_size, secondary_preempt_buf_size);
> +	}
>  
>  	if (primary_preempt_buf_size < FW_PREEMPT_BUF_MIN_SIZE ||
>  	    secondary_preempt_buf_size < FW_PREEMPT_BUF_MIN_SIZE) {
> -		ivpu_warn(vdev, "Preemption buffers size too small\n");
> +		ivpu_warn(vdev, "Preemption buffers size too small, primary %u, secondary %u\n",
> +			  primary_preempt_buf_size, secondary_preempt_buf_size);
>  		return;
>  	}

[Severity: Medium]
When fw_hdr->preemption_buffer_1_max_size is non-zero and
fw_hdr->preemption_buffer_2_max_size is 0, won't the
secondary_preempt_buf_size < FW_PREEMPT_BUF_MIN_SIZE check in
ivpu_preemption_config_parse() in drivers/accel/ivpu/ivpu_fw.c always trigger
a warning and return early?

The commit message notes that when preemption_buffer_1_max_size is set in the
image header, preemption_buffer_2_max_size should be used even if it is zero,
and ivpu_preemption_buffers_create() in drivers/accel/ivpu/ivpu_job.c is
updated to handle vdev->fw->secondary_preempt_buf_size == 0.

However, during ivpu_dev_init() -> ivpu_fw_init() -> ivpu_fw_parse() ->
ivpu_preemption_config_parse(), if secondary_preempt_buf_size is 0, checking
secondary_preempt_buf_size < FW_PREEMPT_BUF_MIN_SIZE (where
FW_PREEMPT_BUF_MIN_SIZE is SZ_4K) evaluates to true and returns before
reaching:

	vdev->fw->primary_preempt_buf_size = ALIGN(primary_preempt_buf_size, PAGE_SIZE);
	vdev->fw->secondary_preempt_buf_size = ALIGN(secondary_preempt_buf_size, PAGE_SIZE);

For firmware that relies on the image header sizes
(boot_params->preemption_buffer_size == 0) with
preemption_buffer_1_max_size > 0 and preemption_buffer_2_max_size == 0,
vdev->fw->primary_preempt_buf_size remains 0, ivpu_fw_postboot_init() warns
that hardware scheduling has no preemption buffer, and
ivpu_preemption_buffers_create() skips allocating the primary preemption
buffer.

In addition, on newer firmware where the deprecated image header preemption
buffer fields are zeroed in favor of boot_params->preemption_buffer_size,
won't ivpu_preemption_config_parse() also emit a false-positive warning on
every probe before the boot params are read in ivpu_fw_postboot_init()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026093020.228233-1-andrzej.kacprowski@linux.intel.com?part=1