Re: [PATCH] fix: nvme_update_ns_info method should be called even if nvme_ms_ids_equal return false

金韬
2022-04-11T14:20:26Z

After applying the patch provided (based on official kernel 5.17.1), the
suspend issue has gone, and works!

dmesg shows below:
[ 0.489981] nvme 0000:03:00.0: platform quirk: setting simple suspend
[ 0.490002] nvme nvme0: pci function 0000:03:00.0
[ 0.494897] nvme nvme0: missing or invalid SUBNQN field.
[ 0.498902] nvme nvme0: allocated 16 MiB host memory buffer.
[ 0.537800] nvme nvme0: 8/0/0 default/read/poll queues
[ 0.543528] nvme nvme0: Ignoring bogus Namespace Identifiers
[ 0.546709] nvme0n1: p1 p2 p3 p4
[ 0.985096] EXT4-fs (nvme0n1p4): mounted filesystem with ordered data
mode. Quota mode: none.
[ 1.220554] EXT4-fs (nvme0n1p4): re-mounted. Quota mode: none.
[ 36.672605] nvme nvme0: 8/0/0 default/read/poll queues
[ 36.673717] nvme nvme0: Ignoring bogus Namespace Identifiers

btw, I checked the patch code, I noticed "MAXIO MAP1202" is hard coded
in pci.c. I think not only MAP1202 has the problem mentioned in this
thread, but also "MAXIO MAP1002"(Gloway Basic 1T NVMe SSD, which has the
same suspend issue).
> + { PCI_DEVICE(0x1e4B, 0x1202), /* MAXIO MAP1202 */
> + .driver_data = NVME_QUIRK_BOGUS_NID, },

I highly doubt that MAXIO SSD controller may all have the issue of
reporting nsid.

在 2022/4/11 14:07, Christoph Hellwig 写道:
> Plase try this patch:
>
> ---
> From 95cf9f00488ca5b203cb9205f108b8ec0c30d001 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Mon, 11 Apr 2022 08:05:27 +0200
> Subject: nvme: add a Namespace Identifiers quirk
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The MAXIO MAP1202 controllers reports completely bogus Namespace
> identifiers that even change after suspend cycles. Disable using
> the Identifiers entirely.
>
> Reported-by: 金韬 <me@kingtous.cn>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/core.c | 24 ++++++++++++++++++------
> drivers/nvme/host/nvme.h | 5 +++++
> drivers/nvme/host/pci.c | 2 ++
> 3 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index efb85c6d8e2d5..907f4a2b3de87 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1282,6 +1282,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
>
> switch (cur->nidt) {
> case NVME_NIDT_EUI64:
> + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
> + return -1;
> if (cur->nidl != NVME_NIDT_EUI64_LEN) {
> dev_warn(ctrl->device, "%s %d for NVME_NIDT_EUI64\n",
> warn_str, cur->nidl);
> @@ -1290,6 +1292,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
> memcpy(ids->eui64, data + sizeof(*cur), NVME_NIDT_EUI64_LEN);
> return NVME_NIDT_EUI64_LEN;
> case NVME_NIDT_NGUID:
> + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
> + return -1;
> if (cur->nidl != NVME_NIDT_NGUID_LEN) {
> dev_warn(ctrl->device, "%s %d for NVME_NIDT_NGUID\n",
> warn_str, cur->nidl);
> @@ -1298,6 +1302,8 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
> memcpy(ids->nguid, data + sizeof(*cur), NVME_NIDT_NGUID_LEN);
> return NVME_NIDT_NGUID_LEN;
> case NVME_NIDT_UUID:
> + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID)
> + return -1;
> if (cur->nidl != NVME_NIDT_UUID_LEN) {
> dev_warn(ctrl->device, "%s %d for NVME_NIDT_UUID\n",
> warn_str, cur->nidl);
> @@ -1399,12 +1405,18 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
> if ((*id)->ncap == 0) /* namespace not allocated or attached */
> goto out_free_id;
>
> - if (ctrl->vs >= NVME_VS(1, 1, 0) &&
> - !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
> - memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64));
> - if (ctrl->vs >= NVME_VS(1, 2, 0) &&
> - !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
> - memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
> +
> + if (ctrl->quirks & NVME_QUIRK_BOGUS_NID) {
> + dev_info(ctrl->device,
> + "Ignoring bogus Namespace Identifiers\n");
> + } else {
> + if (ctrl->vs >= NVME_VS(1, 1, 0) &&
> + !memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
> + memcpy(ids->eui64, (*id)->eui64, sizeof(ids->eui64));
> + if (ctrl->vs >= NVME_VS(1, 2, 0) &&
> + !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
> + memcpy(ids->nguid, (*id)->nguid, sizeof(ids->nguid));
> + }
>
> return 0;
>
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1393bbf82d71e..a2b53ca633359 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -144,6 +144,11 @@ enum nvme_quirks {
> * encoding the generation sequence number.
> */
> NVME_QUIRK_SKIP_CID_GEN = (1 << 17),
> +
> + /*
> + * Reports garbage in the namespace identifiers (eui64, nguid, uuid).
> + */
> + NVME_QUIRK_BOGUS_NID = (1 << 18),
> };
>
> /*
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index d817ca17463ed..c386c91483505 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -3447,6 +3447,8 @@ static const struct pci_device_id nvme_id_table[] = {
> .driver_data = NVME_QUIRK_NO_DEEPEST_PS, },
> { PCI_DEVICE(0x2646, 0x2263), /* KINGSTON A2000 NVMe SSD */
> .driver_data = NVME_QUIRK_NO_DEEPEST_PS, },
> + { PCI_DEVICE(0x1e4B, 0x1202), /* MAXIO MAP1202 */
> + .driver_data = NVME_QUIRK_BOGUS_NID, },
> { PCI_DEVICE(PCI_VENDOR_ID_AMAZON, 0x0061),
> .driver_data = NVME_QUIRK_DMA_ADDRESS_BITS_48, },
> { PCI_DEVICE(PCI_VENDOR_ID_AMAZON, 0x0065),