On 10/12/2024 19:47, Keith Busch wrote:
+static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
+ struct nvme_ns_info *info, u8 fdp_idx)
+{
+ struct nvme_fdp_config_log hdr, *h;
+ struct nvme_fdp_config_desc *desc;
+ size_t size = sizeof(hdr);
+ int i, n, ret;
+ void *log;
+
+ ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
+ NVME_CSI_NVM, &hdr, size, 0, info->endgid);
+ if (ret) {
+ dev_warn(ctrl->device,
+ "FDP configs log header status:0x%x endgid:%x\n", ret,
+ info->endgid);
About endgid, I guess that there is a good reason but sometimes "0x" is
prefixed for hex prints and sometimes not. Maybe no prefix is used when
we know that the variable is to hold a value from a HW register / memory
structure - I don't know.
further nitpicking: And ret holds a kernel error code - the driver seems
inconsistent for printing this. Sometimes it's %d and sometimes 0x%x.
+ return ret;
+ }
+
+ size = le32_to_cpu(hdr.sze);
+ h = kzalloc(size, GFP_KERNEL);
+ if (!h) {
+ dev_warn(ctrl->device,
+ "failed to allocate %lu bytes for FDP config log\n",
+ size);
do we normally print ENOMEM messages? I see that the bytes is printed,
but I assume that this is a sane value (of little note).
+ return -ENOMEM;
+ }
+
+ ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
+ NVME_CSI_NVM, h, size, 0, info->endgid);
+ if (ret) {
+ dev_warn(ctrl->device,
+ "FDP configs log status:0x%x endgid:%x\n", ret,
+ info->endgid);
+ goto out;
+ }
+
+ n = le16_to_cpu(h->numfdpc) + 1;
+ if (fdp_idx > n) {
+ dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
+ fdp_idx, n);
+ /* Proceed without registering FDP streams */> + ret = 0;
nit: maybe you want to be explicit, but logically ret is already 0
+ goto out;
+ }
+
+ log = h + 1;
+ desc = log;
+ for (i = 0; i < fdp_idx; i++) {
+ log += le16_to_cpu(desc->dsze);
+ desc = log;
+ }
+
+ if (le32_to_cpu(desc->nrg) > 1) {
+ dev_warn(ctrl->device, "FDP NRG > 1 not supported\n");
+ ret = 0;
Same here
+ goto out;
+ }
+
+ info->runs = le64_to_cpu(desc->runs);
+out:
+ kfree(h);
+ return ret;
+}
