[PATCH] nvme: add bug report info for global duplicate id

[PATCH] nvme: add bug report info for global duplicate id

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The recent global id check is finding poorly implemented devices in the
wild. Include relavant device information in the output to help quicken
an appropriate quirk patch.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 24165daee3c8..4cda5024a352 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3852,6 +3852,13 @@ static int nvme_global_check_duplicate_ids(struct nvme_subsystem *this,
 	return ret;
 }
 
+static int nvme_str_len(char *s, int len)
+{
+	while (s[len - 1] == ' ')
+		len--;
+	return len;
+}
+
 static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 		struct nvme_ns_ids *ids, bool is_shared)
 {
@@ -3861,8 +3868,18 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 
 	ret = nvme_global_check_duplicate_ids(ctrl->subsys, ids);
 	if (ret) {
+		struct nvme_subsystem *subsys = ctrl->subsys;
+
 		dev_err(ctrl->device,
 			"globally duplicate IDs for nsid %d\n", nsid);
+		dev_err(ctrl->device,
+			"bug report info: VID:%04x model:%.*s firmware:%.*s\n",
+			subsys->vendor_id,
+			nvme_str_len(subsys->model, sizeof(subsys->model)),
+			subsys->model,
+			nvme_str_len(subsys->firmware_rev,
+				     sizeof(subsys->firmware_rev)),
+			subsys->firmware_rev);
 		return ret;
 	}
 
-- 
2.30.2

Re: [PATCH] nvme: add bug report info for global duplicate id

[Chaitanya Kulkarni]

On 6/6/22 09:40, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The recent global id check is finding poorly implemented devices in the
> wild. Include relavant device information in the output to help quicken
> an appropriate quirk patch.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] nvme: add bug report info for global duplicate id

[Christoph Hellwig]

On Mon, Jun 06, 2022 at 09:40:47AM -0700, Keith Busch wrote:
>  	if (ret) {
> +		struct nvme_subsystem *subsys = ctrl->subsys;
> +
>  		dev_err(ctrl->device,
>  			"globally duplicate IDs for nsid %d\n", nsid);
> +		dev_err(ctrl->device,
> +			"bug report info: VID:%04x model:%.*s firmware:%.*s\n",
> +			subsys->vendor_id,
> +			nvme_str_len(subsys->model, sizeof(subsys->model)),
> +			subsys->model,
> +			nvme_str_len(subsys->firmware_rev,
> +				     sizeof(subsys->firmware_rev)),
> +			subsys->firmware_rev);

So I like the idea here, but this won't give us the PCI device id
that we usually need for quirks.

Re: [PATCH] nvme: add bug report info for global duplicate id

[Keith Busch]

On Tue, Jun 07, 2022 at 12:15:26PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 06, 2022 at 09:40:47AM -0700, Keith Busch wrote:
> >  	if (ret) {
> > +		struct nvme_subsystem *subsys = ctrl->subsys;
> > +
> >  		dev_err(ctrl->device,
> >  			"globally duplicate IDs for nsid %d\n", nsid);
> > +		dev_err(ctrl->device,
> > +			"bug report info: VID:%04x model:%.*s firmware:%.*s\n",
> > +			subsys->vendor_id,
> > +			nvme_str_len(subsys->model, sizeof(subsys->model)),
> > +			subsys->model,
> > +			nvme_str_len(subsys->firmware_rev,
> > +				     sizeof(subsys->firmware_rev)),
> > +			subsys->firmware_rev);
> 
> So I like the idea here, but this won't give us the PCI device id
> that we usually need for quirks.

Yeah, we don't have the DID from core. Should I add a driver specific op to
print device info? Just a quick rough idea here:

---
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9b72b6ecf33c..8daf13eaddac 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -503,6 +503,7 @@ struct nvme_ctrl_ops {
 	void (*submit_async_event)(struct nvme_ctrl *ctrl);
 	void (*delete_ctrl)(struct nvme_ctrl *ctrl);
 	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
+	void (*print_device_info)(struct nvme_ctrl *ctrl);
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 9d963f6cdbae..709793efe16c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2984,6 +2984,18 @@ static int nvme_pci_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
 	return snprintf(buf, size, "%s\n", dev_name(&pdev->dev));
 }
 
+
+static void nvme_pci_print_device_info(struct nvme_ctrl *ctrl)
+{
+	struct pci_dev *pdev = to_pci_dev(to_nvme_dev(ctrl)->dev);
+	struct nvme_subsystem *subsys = ctrl->subsys;
+
+	dev_err(ctrl->device,
+		"VID:DID %04x:%04x model:%.*s firmware:%.*s\n",
+		pdev->vendor, pdev->device, 20, subsys->model, 8,
+		subsys->firmware_rev);
+}
+
 static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.name			= "pcie",
 	.module			= THIS_MODULE,
@@ -2995,6 +3007,7 @@ static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.free_ctrl		= nvme_pci_free_ctrl,
 	.submit_async_event	= nvme_pci_submit_async_event,
 	.get_address		= nvme_pci_get_address,
+	.print_device_info	= nvme_pci_print_device_info,
 };
 
 static int nvme_dev_map(struct nvme_dev *dev)
--

Re: [PATCH] nvme: add bug report info for global duplicate id

[Christoph Hellwig]

On Tue, Jun 07, 2022 at 08:23:13AM -0600, Keith Busch wrote:
> > So I like the idea here, but this won't give us the PCI device id
> > that we usually need for quirks.
> 
> Yeah, we don't have the DID from core. Should I add a driver specific op to
> print device info? Just a quick rough idea here:

Yes, something like this looks reasonable to me.