Re: [PATCH V4 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 2 potential issue(s) to consider:
- [Medium] amdxdna: user-triggerable log spam in aie_update_counters ioctl path
- [Medium] amdxdna: uninitialized max_tops returned to userspace on telemetry failure
--

commit 9f91f5ba98bc4ba5e1f461f3136872d989e22936
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 tables,
and hardware resource information for AIE4/NPU3 devices. It moves the DPM
frequency table to the device structure and queries active DPM levels via
messages to the firmware.

> diff --git a/drivers/accel/amdxdna/aie4_pci.c b/drivers/accel/amdxdna/aie4_pci.c
> index 6eacdc026ec7f..5593dfb6c7ed5 100644
> --- a/drivers/accel/amdxdna/aie4_pci.c
> +++ b/drivers/accel/amdxdna/aie4_pci.c
> @@ -299,6 +299,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);
> +	}

[Severity: Medium]
When aie4_init_dpm_freq_table() successfully queries the DPM table from
firmware, the fallback call to set_dpm() is skipped. Doesn't this leave
aie->max_tops and aie->curr_tops uninitialized (0) at startup?

[ ... ]

> @@ -635,11 +646,71 @@ static int aie4_get_power_mode(struct amdxdna_client *client,
>  	return 0;
>  }
>  
> +static int aie4_query_clock_metadata(struct amdxdna_client *client,
> +				     struct amdxdna_drm_get_info *args)
> +{
> +	struct amdxdna_drm_query_clock_metadata *clock;
> +	struct amdxdna_dev *xdna = client->xdna;
> +	struct amdxdna_dev_hdl *ndev;
> +	int ret = 0;
> +	u32 buf_sz;
> +
> +	ndev = xdna->dev_handle;
> +	clock = kzalloc_obj(*clock);
> +	if (!clock)
> +		return -ENOMEM;
> +
> +	aie_update_counters(ndev);

[Severity: Medium]
Because this ioctl can be polled rapidly by unprivileged userspace monitoring
tools, does calling aie_update_counters() without checking its return value
provide a vector for local denial of service via kernel log spam if the
telemetry query in npu3_update_counters() fails?

[ ... ]

> +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;
> +	res_info.npu_tops_curr = ndev->aie.curr_tops;

[Severity: Medium]
Here we ignore the return value of aie_update_counters() as well. If the
underlying telemetry query fails and returns early without populating
aie->max_tops, will this copy the uninitialized 0 values to userspace? 

Userspace might receive 0 for static hardware limits and clock frequencies,
causing incorrect statistics in clients that rely on accurate resource bounds.

> diff --git a/drivers/accel/amdxdna/npu3_regs.c b/drivers/accel/amdxdna/npu3_regs.c
> index c82433d629012..cff99ddd3a552 100644
> --- a/drivers/accel/amdxdna/npu3_regs.c
> +++ b/drivers/accel/amdxdna/npu3_regs.c
[ ... ]
> +static int npu3_update_counters(struct aie_device *aie)
> +{
> +	struct amdxdna_dev_hdl *ndev = aie->xdna->dev_handle;
> +	u32 aieclk_level, npuhclk_level;
> +	int ret;
> +
> +	/* Use firmware DPM table or pre-defined table, never mixed. */
> +	ret = aie4_query_dpm_level(ndev, &aieclk_level, &npuhclk_level);
> +	if (ret) {
> +		XDNA_WARN(aie->xdna, "Failed to get DPM level from fw: %d", ret);
> +		return ret;
> +	}

[Severity: Medium]
This is where the un-ratelimited warning is printed upon a telemetry failure in
the ioctl path. If the firmware is older and lacks support for
AIE4_MSG_OP_GET_CURRENT_DPM_LEVEL, or the management channel times out, could
this result in unbounded kernel log spam?

If this telemetry query fails, max_tops is never computed. Combined with the
uninitialized state from aie4_query() during probe, does this ensure 0 is
propagated to the caller?

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