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

[Keith Busch <kbusch@kernel.org>]

On Mon, Dec 09, 2024 at 02:18:19PM +0100, Christoph Hellwig wrote:
> > +	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++) {
> 		..

Yeah, okay. I was just trying to cleverly have a single place where the
descriptor is set. A for-loop needs to set it both within and after the
loop.

> > +	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.

I thought you could change the FDP configuration on a live namespace
with the Set Feature command, so needed to account for that. But the
spec really does restrict that feature to endurance groups without
namespaces, so I was mistaken and we can skip re-validiting FDP state
after the first scan.