On Tue, Jun 16, 2020 at 11:58:59AM -0400, Martin K. Petersen wrote:
>
> Keith,
>
> > @@ -1113,8 +1126,9 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
> > status = nvme_submit_sync_cmd(ctrl->admin_q, &c, data,
> > NVME_IDENTIFY_DATA_SIZE);
> > if (status) {
> > - dev_warn(ctrl->device,
> > - "Identify Descriptors failed (%d)\n", status);
> > + if (ctrl->vs >= NVME_VS(1, 3, 0))
> > + dev_warn(ctrl->device,
> > + "Identify Descriptors failed (%d)\n", status);
>
> Not a biggie but maybe this should be a separate patch?
This function, nvme_identify_ns_descs(), was previously only
called if (ctrl->vs >= NVME_VS(1, 3, 0)).
Now it's called if (ctrl->vs >= NVME_VS(1, 3, 0) || nvme_multi_css(ctrl))
Because, if someone implements/enables multiple command sets feature
on an older base spec of NVMe, we still want to support/allow that.
(Although they would also need to implement the Namespace Identification
Descriptor list command.)
So I think that this change belongs in the this patch,
consider that this check wasn't needed before this patch.
>
> > @@ -1808,7 +1828,8 @@ static bool nvme_ns_ids_equal(struct nvme_ns_ids *a, struct nvme_ns_ids *b)
> > {
> > return uuid_equal(&a->uuid, &b->uuid) &&
> > memcmp(&a->nguid, &b->nguid, sizeof(a->nguid)) == 0 &&
> > - memcmp(&a->eui64, &b->eui64, sizeof(a->eui64)) == 0;
> > + memcmp(&a->eui64, &b->eui64, sizeof(a->eui64)) == 0 &&
> > + a->csi == b->csi;
> > }
>
> No objection to defensive programming. But wouldn't this be a broken
> device?
When you format a namespace, you specify a NSID, and a Command Set
Identifier (CSI), to say which is the main Command Set for this
namespace.
Someone could send NVMe format commands directly to the drive, using
e.g. nvme-cli to format a specific NSID, using a different CSI than
that NSID previously used.
In that case, the same NSID will now have another main CSI
associated, so it probably makes sense for revalidate disk to
detect that it is no longer the same disk.
Kind regards,
Niklas
