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 <[email protected]> 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/[email protected]?part=8
