[PATCH 1/3] nvme-pci: consistencly use ctrl->device for logging

[PATCH 1/3] nvme-pci: consistencly use ctrl->device for logging

[Christoph Hellwig]

This is what most of the code already does and gives much more useful
prefixes than the device embedded in the pci_dev.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index fed803232edc..a150b2dd7851 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -263,7 +263,7 @@ static void nvme_dbbuf_set(struct nvme_dev *dev)
 	c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf_eis_dma_addr);
 
 	if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0)) {
-		dev_warn(dev->dev, "unable to set dbbuf\n");
+		dev_warn(dev->ctrl.device, "unable to set dbbuf\n");
 		/* Free memory and continue on */
 		nvme_dbbuf_dma_free(dev);
 	}
@@ -1394,11 +1394,11 @@ static void nvme_warn_reset(struct nvme_dev *dev, u32 csts)
 	result = pci_read_config_word(to_pci_dev(dev->dev), PCI_STATUS,
 				      &pci_status);
 	if (result == PCIBIOS_SUCCESSFUL)
-		dev_warn(dev->dev,
+		dev_warn(dev->ctrl.device,
 			 "controller is down; will reset: CSTS=0x%x, PCI_STATUS=0x%hx\n",
 			 csts, pci_status);
 	else
-		dev_warn(dev->dev,
+		dev_warn(dev->ctrl.device,
 			 "controller is down; will reset: CSTS=0x%x, PCI_STATUS read failed (%d)\n",
 			 csts, result);
 }
@@ -1735,8 +1735,8 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	 */
 	if (pdev->vendor == PCI_VENDOR_ID_APPLE && pdev->device == 0x2001) {
 		dev->q_depth = 2;
-		dev_warn(dev->dev, "detected Apple NVMe controller, set "
-			"queue depth=%u to work around controller resets\n",
+		dev_warn(dev->ctrl.device, "detected Apple NVMe controller, "
+			"set queue depth=%u to work around controller resets\n",
 			dev->q_depth);
 	}
 
@@ -1754,7 +1754,7 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 		if (dev->cmbsz) {
 			if (sysfs_add_file_to_group(&dev->ctrl.device->kobj,
 						    &dev_attr_cmb.attr, NULL))
-				dev_warn(dev->dev,
+				dev_warn(dev->ctrl.device,
 					 "failed to add sysfs attribute for CMB\n");
 		}
 	}
-- 
2.11.0

[PATCH 2/3] nvme: replace is_flags field in nvme_ctrl_ops with a flags field

[Christoph Hellwig]

So that we can have more flags for transport-specific behavior.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c   | 2 +-
 drivers/nvme/host/fc.c     | 2 +-
 drivers/nvme/host/nvme.h   | 3 ++-
 drivers/nvme/host/rdma.c   | 2 +-
 drivers/nvme/target/loop.c | 2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d5e0906262ea..79b78bd67b07 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1605,7 +1605,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	}
 	memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
 
-	if (ctrl->ops->is_fabrics) {
+	if (ctrl->ops->flags & NVME_F_FABRICS) {
 		ctrl->icdoff = le16_to_cpu(id->icdoff);
 		ctrl->ioccsz = le32_to_cpu(id->ioccsz);
 		ctrl->iorcsz = le32_to_cpu(id->iorcsz);
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 70e689bf1cad..f126f16722e5 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2645,7 +2645,7 @@ nvme_fc_reset_nvme_ctrl(struct nvme_ctrl *nctrl)
 static const struct nvme_ctrl_ops nvme_fc_ctrl_ops = {
 	.name			= "fc",
 	.module			= THIS_MODULE,
-	.is_fabrics		= true,
+	.flags			= NVME_F_FABRICS,
 	.reg_read32		= nvmf_reg_read32,
 	.reg_read64		= nvmf_reg_read64,
 	.reg_write32		= nvmf_reg_write32,
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 29c708ca9621..7c4b0f6636c5 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -208,7 +208,8 @@ struct nvme_ns {
 struct nvme_ctrl_ops {
 	const char *name;
 	struct module *module;
-	bool is_fabrics;
+	unsigned int flags;
+#define NVME_F_FABRICS			(1 << 0)
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
 	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index dd1c6deef82f..d05ee21f6400 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1782,7 +1782,7 @@ static int nvme_rdma_reset_ctrl(struct nvme_ctrl *nctrl)
 static const struct nvme_ctrl_ops nvme_rdma_ctrl_ops = {
 	.name			= "rdma",
 	.module			= THIS_MODULE,
-	.is_fabrics		= true,
+	.flags			= NVME_F_FABRICS,
 	.reg_read32		= nvmf_reg_read32,
 	.reg_read64		= nvmf_reg_read64,
 	.reg_write32		= nvmf_reg_write32,
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index feb497134aee..e503cfff0337 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -558,7 +558,7 @@ static int nvme_loop_reset_ctrl(struct nvme_ctrl *nctrl)
 static const struct nvme_ctrl_ops nvme_loop_ctrl_ops = {
 	.name			= "loop",
 	.module			= THIS_MODULE,
-	.is_fabrics		= true,
+	.flags			= NVME_F_FABRICS,
 	.reg_read32		= nvmf_reg_read32,
 	.reg_read64		= nvmf_reg_read64,
 	.reg_write32		= nvmf_reg_write32,
-- 
2.11.0

[PATCH 3/3] nvme: only setup block integrity if supported by the driver

[Christoph Hellwig]

Currently only the PCIe driver supports metadata, so we should not claim
integrity support for the other drivers.  This prevents nasty crashes
with targets that advertise metadata support on fabrics.

Also use the opportunity to factor out some code into a separate helper
that isn't even compiled if CONFIG_BLK_DEV_INTEGRITY is disabled.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 50 ++++++++++++++++++++++++++++++------------------
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  |  1 +
 3 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 79b78bd67b07..f6dd4fed56a4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -925,6 +925,29 @@ static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 }
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
+static void nvme_prep_integrity(struct gendisk *disk, struct nvme_id_ns *id,
+		u16 bs)
+{
+	struct nvme_ns *ns = disk->private_data;
+	u16 old_ms = ns->ms;
+	u8 pi_type = 0;
+
+	ns->ms = le16_to_cpu(id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ms);
+	ns->ext = ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT);
+
+	/* PI implementation requires metadata equal t10 pi tuple size */
+	if (ns->ms == sizeof(struct t10_pi_tuple))
+		pi_type = id->dps & NVME_NS_DPS_PI_MASK;
+
+	if (blk_get_integrity(disk) &&
+	    (ns->pi_type != pi_type || ns->ms != old_ms ||
+	     bs != queue_logical_block_size(disk->queue) ||
+	     (ns->ms && ns->ext)))
+		blk_integrity_unregister(disk);
+
+	ns->pi_type = pi_type;
+}
+
 static void nvme_init_integrity(struct nvme_ns *ns)
 {
 	struct blk_integrity integrity;
@@ -951,6 +974,10 @@ static void nvme_init_integrity(struct nvme_ns *ns)
 	blk_queue_max_integrity_segments(ns->queue, 1);
 }
 #else
+static void nvme_prep_integrity(struct gendisk *disk, struct nvme_id_ns *id,
+		u16 bs)
+{
+}
 static void nvme_init_integrity(struct nvme_ns *ns)
 {
 }
@@ -997,37 +1024,22 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
 static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
 {
 	struct nvme_ns *ns = disk->private_data;
-	u8 lbaf, pi_type;
-	u16 old_ms;
-	unsigned short bs;
-
-	old_ms = ns->ms;
-	lbaf = id->flbas & NVME_NS_FLBAS_LBA_MASK;
-	ns->lba_shift = id->lbaf[lbaf].ds;
-	ns->ms = le16_to_cpu(id->lbaf[lbaf].ms);
-	ns->ext = ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT);
+	u16 bs;
 
 	/*
 	 * If identify namespace failed, use default 512 byte block size so
 	 * block layer can use before failing read/write for 0 capacity.
 	 */
+	ns->lba_shift = id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ds;
 	if (ns->lba_shift == 0)
 		ns->lba_shift = 9;
 	bs = 1 << ns->lba_shift;
-	/* XXX: PI implementation requires metadata equal t10 pi tuple size */
-	pi_type = ns->ms == sizeof(struct t10_pi_tuple) ?
-					id->dps & NVME_NS_DPS_PI_MASK : 0;
 
 	blk_mq_freeze_queue(disk->queue);
-	if (blk_get_integrity(disk) && (ns->pi_type != pi_type ||
-				ns->ms != old_ms ||
-				bs != queue_logical_block_size(disk->queue) ||
-				(ns->ms && ns->ext)))
-		blk_integrity_unregister(disk);
 
-	ns->pi_type = pi_type;
+	if (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED)
+		nvme_prep_integrity(disk, id, bs);
 	blk_queue_logical_block_size(ns->queue, bs);
-
 	if (ns->ms && !blk_get_integrity(disk) && !ns->ext)
 		nvme_init_integrity(ns);
 	if (ns->ms && !(ns->ms == 8 && ns->pi_type) && !blk_get_integrity(disk))
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7c4b0f6636c5..9d6a070d4391 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -210,6 +210,7 @@ struct nvme_ctrl_ops {
 	struct module *module;
 	unsigned int flags;
 #define NVME_F_FABRICS			(1 << 0)
+#define NVME_F_METADATA_SUPPORTED	(1 << 1)
 	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
 	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a150b2dd7851..283efc2e8921 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2041,6 +2041,7 @@ static int nvme_pci_reset_ctrl(struct nvme_ctrl *ctrl)
 static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.name			= "pcie",
 	.module			= THIS_MODULE,
+	.flags			= NVME_F_METADATA_SUPPORTED,
 	.reg_read32		= nvme_pci_reg_read32,
 	.reg_write32		= nvme_pci_reg_write32,
 	.reg_read64		= nvme_pci_reg_read64,
-- 
2.11.0

[PATCH 3/3] nvme: only setup block integrity if supported by the driver

[Christoph Hellwig]

Any chance to get a review for this one?  Without it we'll reliably
crash the kernel if a fabrics target advertises metadata support.

On Sat, May 20, 2017@03:14:45PM +0200, Christoph Hellwig wrote:
> Currently only the PCIe driver supports metadata, so we should not claim
> integrity support for the other drivers.  This prevents nasty crashes
> with targets that advertise metadata support on fabrics.
> 
> Also use the opportunity to factor out some code into a separate helper
> that isn't even compiled if CONFIG_BLK_DEV_INTEGRITY is disabled.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>  drivers/nvme/host/core.c | 50 ++++++++++++++++++++++++++++++------------------
>  drivers/nvme/host/nvme.h |  1 +
>  drivers/nvme/host/pci.c  |  1 +
>  3 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 79b78bd67b07..f6dd4fed56a4 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -925,6 +925,29 @@ static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  }
>  
>  #ifdef CONFIG_BLK_DEV_INTEGRITY
> +static void nvme_prep_integrity(struct gendisk *disk, struct nvme_id_ns *id,
> +		u16 bs)
> +{
> +	struct nvme_ns *ns = disk->private_data;
> +	u16 old_ms = ns->ms;
> +	u8 pi_type = 0;
> +
> +	ns->ms = le16_to_cpu(id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ms);
> +	ns->ext = ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT);
> +
> +	/* PI implementation requires metadata equal t10 pi tuple size */
> +	if (ns->ms == sizeof(struct t10_pi_tuple))
> +		pi_type = id->dps & NVME_NS_DPS_PI_MASK;
> +
> +	if (blk_get_integrity(disk) &&
> +	    (ns->pi_type != pi_type || ns->ms != old_ms ||
> +	     bs != queue_logical_block_size(disk->queue) ||
> +	     (ns->ms && ns->ext)))
> +		blk_integrity_unregister(disk);
> +
> +	ns->pi_type = pi_type;
> +}
> +
>  static void nvme_init_integrity(struct nvme_ns *ns)
>  {
>  	struct blk_integrity integrity;
> @@ -951,6 +974,10 @@ static void nvme_init_integrity(struct nvme_ns *ns)
>  	blk_queue_max_integrity_segments(ns->queue, 1);
>  }
>  #else
> +static void nvme_prep_integrity(struct gendisk *disk, struct nvme_id_ns *id,
> +		u16 bs)
> +{
> +}
>  static void nvme_init_integrity(struct nvme_ns *ns)
>  {
>  }
> @@ -997,37 +1024,22 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
>  static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
>  {
>  	struct nvme_ns *ns = disk->private_data;
> -	u8 lbaf, pi_type;
> -	u16 old_ms;
> -	unsigned short bs;
> -
> -	old_ms = ns->ms;
> -	lbaf = id->flbas & NVME_NS_FLBAS_LBA_MASK;
> -	ns->lba_shift = id->lbaf[lbaf].ds;
> -	ns->ms = le16_to_cpu(id->lbaf[lbaf].ms);
> -	ns->ext = ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT);
> +	u16 bs;
>  
>  	/*
>  	 * If identify namespace failed, use default 512 byte block size so
>  	 * block layer can use before failing read/write for 0 capacity.
>  	 */
> +	ns->lba_shift = id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ds;
>  	if (ns->lba_shift == 0)
>  		ns->lba_shift = 9;
>  	bs = 1 << ns->lba_shift;
> -	/* XXX: PI implementation requires metadata equal t10 pi tuple size */
> -	pi_type = ns->ms == sizeof(struct t10_pi_tuple) ?
> -					id->dps & NVME_NS_DPS_PI_MASK : 0;
>  
>  	blk_mq_freeze_queue(disk->queue);
> -	if (blk_get_integrity(disk) && (ns->pi_type != pi_type ||
> -				ns->ms != old_ms ||
> -				bs != queue_logical_block_size(disk->queue) ||
> -				(ns->ms && ns->ext)))
> -		blk_integrity_unregister(disk);
>  
> -	ns->pi_type = pi_type;
> +	if (ns->ctrl->ops->flags & NVME_F_METADATA_SUPPORTED)
> +		nvme_prep_integrity(disk, id, bs);
>  	blk_queue_logical_block_size(ns->queue, bs);
> -
>  	if (ns->ms && !blk_get_integrity(disk) && !ns->ext)
>  		nvme_init_integrity(ns);
>  	if (ns->ms && !(ns->ms == 8 && ns->pi_type) && !blk_get_integrity(disk))
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 7c4b0f6636c5..9d6a070d4391 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -210,6 +210,7 @@ struct nvme_ctrl_ops {
>  	struct module *module;
>  	unsigned int flags;
>  #define NVME_F_FABRICS			(1 << 0)
> +#define NVME_F_METADATA_SUPPORTED	(1 << 1)
>  	int (*reg_read32)(struct nvme_ctrl *ctrl, u32 off, u32 *val);
>  	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
>  	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index a150b2dd7851..283efc2e8921 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2041,6 +2041,7 @@ static int nvme_pci_reset_ctrl(struct nvme_ctrl *ctrl)
>  static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
>  	.name			= "pcie",
>  	.module			= THIS_MODULE,
> +	.flags			= NVME_F_METADATA_SUPPORTED,
>  	.reg_read32		= nvme_pci_reg_read32,
>  	.reg_write32		= nvme_pci_reg_write32,
>  	.reg_read64		= nvme_pci_reg_read64,
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
---end quoted text---

[PATCH 1/3] nvme-pci: consistencly use ctrl->device for logging

[Keith Busch]

On Sat, May 20, 2017@03:14:43PM +0200, Christoph Hellwig wrote:
> This is what most of the code already does and gives much more useful
> prefixes than the device embedded in the pci_dev.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>

Looks good.

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH 2/3] nvme: replace is_flags field in nvme_ctrl_ops with a flags field

[Keith Busch]

On Sat, May 20, 2017@03:14:44PM +0200, Christoph Hellwig wrote:
> So that we can have more flags for transport-specific behavior.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>

Looks good.

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH 3/3] nvme: only setup block integrity if supported by the driver

[Keith Busch]

On Sat, May 20, 2017@03:14:45PM +0200, Christoph Hellwig wrote:
> Currently only the PCIe driver supports metadata, so we should not claim
> integrity support for the other drivers.  This prevents nasty crashes
> with targets that advertise metadata support on fabrics.
> 
> Also use the opportunity to factor out some code into a separate helper
> that isn't even compiled if CONFIG_BLK_DEV_INTEGRITY is disabled.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>

Looks good.

Reviewed-by: Keith Busch <keith.busch at intel.com>