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.
