Re: [PATCHv12 11/12] nvme: register fdp parameters with the block layer

[Christoph Hellwig <hch@lst.de>]

> +static int nvme_check_fdp(struct nvme_ns *ns, struct nvme_ns_info *info,
> +			  u8 fdp_idx)

Maybe nvme_query_fdp_runs or something else that makes it clear this
is trying to find the runs field might make sense to name this a little
bit more descriptively.



> +{
> +	struct nvme_fdp_config_log hdr, *h;
> +	struct nvme_fdp_config_desc *desc;
> +	size_t size = sizeof(hdr);
> +	int i, n, ret;
> +	void *log;
> +
> +	info->runs = 0;
> +	ret = nvme_get_log_lsi(ns->ctrl, 0, NVME_LOG_FDP_CONFIGS, 0, NVME_CSI_NVM,

Overly long line here, and same for the second call below.

> +			   (void *)&hdr, size, 0, info->endgid);

And this cast isn't actually needed.

> +	n = le16_to_cpu(h->numfdpc) + 1;
> +	if (fdp_idx > n)
> +		goto out;
> +
> +	log = h + 1;
> +	do {
> +		desc = log;
> +		log += le16_to_cpu(desc->dsze);
> +	} while (i++ < fdp_idx);

Maybe a for loop makes it easier to avoid the uninitialized variable,
e.g.

	for (i = 0; i < fdp_index; i++) {
		..

> +	if (ns->ctrl->ctratt & NVME_CTRL_ATTR_FDPS) {
> +		ret = nvme_query_fdp_info(ns, info);
> +		if (ret)
> +			dev_warn(ns->ctrl->device,
> +				"FDP failure status:0x%x\n", ret);
> +		if (ret < 0)
> +			goto out;
> +	}

Looking at the full series with the next patch applied I'm a bit
confused about the handling when rescanning.  AFAIK the code now always
goes into nvme_query_fdp_info when NVME_CTRL_ATTR_FDPS even if
head->plids/head->nr_plids is already set, and that will then simply
override them, even if they were already set.

Also the old freeing of head->plids in nvme_free_ns_head seems gone in
this version.

Last not but least "FDP failure" is probably not a very helpful message
when it could come from about half a dozen different commands sent to
the device.