Re: [PATCH V3 08/19] accel/amdxdna: Add clock, DPM frequency, and resource info queries for AIE4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] accel/amdxdna: uninitialized max_tops exposed if aie_update_counters fails
--

commit ce83bedb3ca3d2c33f7c97c3535a3c4783281e8b
Author: David Zhang <yidong.zhang@amd.com>

accel/amdxdna: Add clock, DPM frequency, and resource info queries for AIE4

This commit adds support for querying clock metadata, DPM frequency table,
and hardware resource information for AIE4/NPU3 devices. It moves common
clock/TOPS counters into aie_device and adds firmware queries for DPM levels
and frequency tables.

> diff --git a/drivers/accel/amdxdna/aie4_pci.c b/drivers/accel/amdxdna/aie4_pci.c
> index c905dc75c0764..fdcf6b8602e9e 100644
> --- a/drivers/accel/amdxdna/aie4_pci.c
> +++ b/drivers/accel/amdxdna/aie4_pci.c
> @@ -298,6 +298,17 @@ static int aie4_query(struct amdxdna_dev_hdl *ndev)
>  	if (ret)
>  		return ret;
>  
> +	ndev->total_col = min_t(u32, AIE4_TOTAL_COLUMN, ndev->aie.metadata.cols);
> +
> +	ret = aie4_init_dpm_freq_table(ndev);
> +	if (ret) {
> +		if (ret == -ETIME)
> +			return ret;
> +		/* if query dpm from fw failed, using default value */
> +		if (ndev->priv->hw_ops && ndev->priv->hw_ops->set_dpm)
> +			(void)ndev->priv->hw_ops->set_dpm(&ndev->aie, 0);
> +	}
> +
>  	return 0;
>  }

[Severity: Low]
If aie4_init_dpm_freq_table() succeeds during AIE4 hardware start (e.g.,
for NPU3 devices), the fallback to set_dpm() is bypassed. Will this leave
ndev->aie.max_tops uninitialized?

[ ... ]

> +static int aie4_query_resource_info(struct amdxdna_client *client,
> +				    struct amdxdna_drm_get_info *args)
> +{
> +	struct amdxdna_drm_get_resource_info res_info = {};
> +	struct amdxdna_dev_hdl *ndev;
> +	struct amdxdna_dev *xdna;
> +	u32 buf_sz;
> +
> +	xdna = client->xdna;
> +	ndev = xdna->dev_handle;
> +
> +	aie_update_counters(ndev);
> +	res_info.npu_clk_max = ndev->dpm_clk_tbl[ndev->max_npuhclk_level].hclk;
> +	res_info.npu_tops_max = ndev->aie.max_tops;

[Severity: Low]
If ndev->aie.max_tops was left uninitialized (at 0) during startup, and
aie_update_counters() experiences a transient firmware failure, the return
value of aie_update_counters() is ignored. 

Does this result in successfully copying 0 to userspace for the static
capability field npu_tops_max?

> +	res_info.npu_tops_curr = ndev->aie.curr_tops;
> +	/* Context accounting is populated in later patches in the series. */
> +
> +	buf_sz = min_t(u32, args->buffer_size, sizeof(res_info));
> +	if (copy_to_user(u64_to_user_ptr(args->buffer), &res_info, buf_sz))
> +		return -EFAULT;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008032348.2044667-1-yidong.zhang@amd.com?part=8