[PATCH 1/2] nvme: Ensure char members of struct nvme_id_ctrl are null-terminated

[PATCH 1/2] nvme: Ensure char members of struct nvme_id_ctrl are null-terminated

[Erwan Velu]

When issueing an identify command, the resulting data structure stored
as a nvme_id_ctrl struct, contains a couple of char members.

These char members features:
- the serial number (sn)
- the model number (mn)
- the firmware revision (fr)

As per the current code, nothing enforce these members to be
null-terminated which causes issues when manipulating them with regular
string manupulation functions.

This commit ensure the three of them are always null-terminated.

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/nvme/host/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 24165daee3c8..0f7e625e8bd0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1274,6 +1274,13 @@ static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 			sizeof(struct nvme_id_ctrl));
 	if (error)
 		kfree(*id);
+	else {
+		/* Ensure that model, serial and firmware fields are always null-terminated */
+		(*id)->mn[sizeof((*id)->mn)-1] = 0;
+		(*id)->sn[sizeof((*id)->sn)-1] = 0;
+		(*id)->fr[sizeof((*id)->fr)-1] = 0;
+	}
+
 	return error;
 }
 
-- 
2.35.3

[PATCH 1/2] nvme: Ensure char members of struct nvme_id_ctrl are null-terminated

[Erwan Velu]

When issueing an identify command, the resulting data structure stored
as a nvme_id_ctrl struct, contains a couple of char members.

These char members features:
- the serial number (sn)
- the model number (mn)
- the firmware revision (fr)

As per the current code, nothing enforce these members to be
null-terminated which causes issues when manipulating them with regular
string manupulation functions.

This commit ensure the three of them are always null-terminated.

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/nvme/host/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 24165daee3c8..0f7e625e8bd0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1274,6 +1274,13 @@ static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
 			sizeof(struct nvme_id_ctrl));
 	if (error)
 		kfree(*id);
+	else {
+		/* Ensure that model, serial and firmware fields are always null-terminated */
+		(*id)->mn[sizeof((*id)->mn)-1] = 0;
+		(*id)->sn[sizeof((*id)->sn)-1] = 0;
+		(*id)->fr[sizeof((*id)->fr)-1] = 0;
+	}
+
 	return error;
 }
 
-- 
2.35.3

[PATCH 2/2] nvme: Report model,sn,fw,pci device information during init

[Erwan Velu]

SCSI-based device get their identify properties being printed when initialized like :

[    1.245357] scsi 0:0:0:0: Direct-Access     ATA      HGST HTE721010A9 A3M0 PQ: 0 ANSI: 5

When initializing nvme devices, no identification message is reported
making difficult to identify them during the boot process. If the system
crashes during boot process or if the init phase fail, it could be very diffcult
to identify the faulty disk.

This patch reports model, serial, firmware version and pci information
as soon as possible making this early identifying task possible.

A typical output looks like:
[    0.383353] nvme nvme0: pci function 0000:00:03.0
[    0.418184] nvme nvme0: MODEL:QEMU NVMe Ctrl                          SN:deadbeef            FW:1.0     PCI_ID:1b36:1af4
[    0.422020] nvme nvme0: 1/0/0 default/read/poll queues

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/nvme/host/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0f7e625e8bd0..f73ca92412a9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2982,6 +2982,14 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		return -EIO;
 	}
 
+	/* Reporting model, serial, firmware and pci info */
+	dev_info(ctrl->device, "MODEL:%s SN:%s FW:%s PCI_ID:%04x:%04x\n",
+			id->mn,
+			id->sn,
+			id->fr,
+			le16_to_cpu(id->vid),
+			le16_to_cpu(id->ssvid));
+
 	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
 		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
 		if (ret < 0)
-- 
2.35.3

[PATCH 2/2] nvme: Report model,sn,fw,pci device information during init

[Erwan Velu]

SCSI-based device get their identify properties being printed when initialized like :

[    1.245357] scsi 0:0:0:0: Direct-Access     ATA      HGST HTE721010A9 A3M0 PQ: 0 ANSI: 5

When initializing nvme devices, no identification message is reported
making difficult to identify them during the boot process. If the system
crashes during boot process or if the init phase fail, it could be very diffcult
to identify the faulty disk.

This patch reports model, serial, firmware version and pci information
as soon as possible making this early identifying task possible.

A typical output looks like:
[    0.383353] nvme nvme0: pci function 0000:00:03.0
[    0.418184] nvme nvme0: MODEL:QEMU NVMe Ctrl                          SN:deadbeef            FW:1.0     PCI_ID:1b36:1af4
[    0.422020] nvme nvme0: 1/0/0 default/read/poll queues

Signed-off-by: Erwan Velu <e.velu@criteo.com>
---
 drivers/nvme/host/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0f7e625e8bd0..f73ca92412a9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2982,6 +2982,14 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		return -EIO;
 	}
 
+	/* Reporting model, serial, firmware and pci info */
+	dev_info(ctrl->device, "MODEL:%s SN:%s FW:%s PCI_ID:%04x:%04x\n",
+			id->mn,
+			id->sn,
+			id->fr,
+			le16_to_cpu(id->vid),
+			le16_to_cpu(id->ssvid));
+
 	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
 		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
 		if (ret < 0)
-- 
2.35.3

Re: [PATCH 1/2] nvme: Ensure char members of struct nvme_id_ctrl are null-terminated

[Keith Busch]

On Tue, Jun 14, 2022 at 11:09:01PM +0200, Erwan Velu wrote:
> +	else {
> +		/* Ensure that model, serial and firmware fields are always null-terminated */
> +		(*id)->mn[sizeof((*id)->mn)-1] = 0;
> +		(*id)->sn[sizeof((*id)->sn)-1] = 0;
> +		(*id)->fr[sizeof((*id)->fr)-1] = 0;
> +	}

But the last byte here is owned by the controller. You may be truncating
information.

Re: [PATCH 2/2] nvme: Report model,sn,fw,pci device information during init

[Keith Busch]

On Tue, Jun 14, 2022 at 11:09:02PM +0200, Erwan Velu wrote:
> @@ -2982,6 +2982,14 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
>  		return -EIO;
>  	}
>  
> +	/* Reporting model, serial, firmware and pci info */
> +	dev_info(ctrl->device, "MODEL:%s SN:%s FW:%s PCI_ID:%04x:%04x\n",
> +			id->mn,
> +			id->sn,
> +			id->fr,
> +			le16_to_cpu(id->vid),
> +			le16_to_cpu(id->ssvid));

We don't need to print this on every controller reset, so I think this needs to
be within the "if (!ctrl->identified)" block.

And since you can't just null terminate the these strings, you should use the
"%*s" format.

I'm unsure if the serial number should be logged. Firmware and model are
probably fine.

Re: [PATCH 2/2] nvme: Report model,sn,fw,pci device information during init

[Christoph Hellwig]

On Tue, Jun 14, 2022 at 02:30:08PM -0700, Keith Busch wrote:
> I'm unsure if the serial number should be logged. Firmware and model are
> probably fine.

I would much prefer to not print it all.  The Linux boot is way to spammy
to start with and this doesn't add any real value.

Re: [PATCH 2/2] nvme: Report model,sn,fw,pci device information during init

[Christoph Hellwig]

On Wed, Jun 15, 2022 at 09:57:05AM +0200, Erwan Velu wrote:
> > I would much prefer to not print it all.  The Linux boot is way to spammy
> > to start with and this doesn't add any real value.
> >
> 
> I know the boot is a bit spammy but when systems crash because of nvme
> drives, that's very handy to get a trace showing who was the culprit and
> what drive is installed (including the fw version which is usually a major
> source of troubles).

Well, usually they are all the same or a very small set of different
SKUs compared to the total number of drives.

Re: [PATCH 2/2] nvme: Report model,sn,fw,pci device information during init

[Jens Axboe]

On 6/15/22 4:53 AM, Christoph Hellwig wrote:
> On Wed, Jun 15, 2022 at 09:57:05AM +0200, Erwan Velu wrote:
>>> I would much prefer to not print it all.  The Linux boot is way to spammy
>>> to start with and this doesn't add any real value.
>>>
>>
>> I know the boot is a bit spammy but when systems crash because of nvme
>> drives, that's very handy to get a trace showing who was the culprit and
>> what drive is installed (including the fw version which is usually a major
>> source of troubles).
> 
> Well, usually they are all the same or a very small set of different
> SKUs compared to the total number of drives.

Agree that we don't want to add unnecessary spam to the boot messages,
and we don't need tons of lines like eg SCSI does. But it does seem
handy to have a single line of the basic model/fw/sn for each drive
along with the enumeration we already print.

-- 
Jens Axboe