Re: [PATCHv13 10/11] nvme: register fdp parameters with the block layer

[John Garry <john.g.garry@oracle.com>]

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;
> +}