nvme host memory buffer fixes and updates

nvme host memory buffer fixes and updates

[Christoph Hellwig]

Hi all,

this series fixes a few bugs in the HMB code, and adds support
for two trivial new tunables exposed by some devices.

[PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback

[Christoph Hellwig]

nvme_alloc_host_mem currently contains two loops that are interwinded,
and the outer retry loop turns out to be broken.  Fix this by untangling
the two.

Based on a report an initial patch from Akinobu Mita.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
Tested-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/nvme/host/pci.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 11874afb2422..e3252bcbc58e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1616,17 +1616,15 @@ static void nvme_free_host_mem(struct nvme_dev *dev)
 	dev->host_mem_descs = NULL;
 }
 
-static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+static int __nvme_alloc_host_mem(struct nvme_dev *dev, u64 preferred,
+		u32 chunk_size)
 {
 	struct nvme_host_mem_buf_desc *descs;
-	u32 chunk_size, max_entries, len;
+	u32 max_entries, len;
 	int i = 0;
 	void **bufs;
 	u64 size = 0, tmp;
 
-	/* start big and work our way down */
-	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
-retry:
 	tmp = (preferred + chunk_size - 1);
 	do_div(tmp, chunk_size);
 	max_entries = tmp;
@@ -1652,15 +1650,9 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 		i++;
 	}
 
-	if (!size || (min && size < min)) {
-		dev_warn(dev->ctrl.device,
-			"failed to allocate host memory buffer.\n");
+	if (!size)
 		goto out_free_bufs;
-	}
 
-	dev_info(dev->ctrl.device,
-		"allocated %lld MiB host memory buffer.\n",
-		size >> ilog2(SZ_1M));
 	dev->nr_host_mem_descs = i;
 	dev->host_mem_size = size;
 	dev->host_mem_descs = descs;
@@ -1679,15 +1671,28 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 out_free_descs:
 	kfree(descs);
 out:
-	/* try a smaller chunk size if we failed early */
-	if (chunk_size >= PAGE_SIZE * 2 && (i == 0 || size < min)) {
-		chunk_size /= 2;
-		goto retry;
-	}
 	dev->host_mem_descs = NULL;
 	return -ENOMEM;
 }
 
+static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+{
+	u32 chunk_size;
+
+	/* start big and work our way down */
+	for (chunk_size = min_t(u64, preferred, PAGE_SIZE << MAX_ORDER);
+	     chunk_size >= PAGE_SIZE * 2;
+	     chunk_size /= 2) {
+		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
+			if (!min || dev->host_mem_size >= min)
+				return 0;
+			nvme_free_host_mem(dev);
+		}
+	}
+
+	return -ENOMEM;
+}
+
 static void nvme_setup_host_mem(struct nvme_dev *dev)
 {
 	u64 max = (u64)max_host_mem_size_mb * SZ_1M;
@@ -1715,8 +1720,15 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 	}
 
 	if (!dev->host_mem_descs) {
-		if (nvme_alloc_host_mem(dev, min, preferred))
+		if (nvme_alloc_host_mem(dev, min, preferred)) {
+			dev_warn(dev->ctrl.device,
+				"failed to allocate host memory buffer.\n");
 			return;
+		}
+
+		dev_info(dev->ctrl.device,
+			"allocated %lld MiB host memory buffer.\n",
+			dev->host_mem_size >> ilog2(SZ_1M));
 	}
 
 	if (nvme_set_host_mem(dev, enable_bits))
-- 
2.11.0

[PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback

[Christoph Hellwig]

nvme_alloc_host_mem currently contains two loops that are interwinded,
and the outer retry loop turns out to be broken.  Fix this by untangling
the two.

Based on a report an initial patch from Akinobu Mita.

Signed-off-by: Christoph Hellwig <hch at lst.de>
Reported-by: Akinobu Mita <akinobu.mita at gmail.com>
Tested-by: Akinobu Mita <akinobu.mita at gmail.com>
Cc: stable at vger.kernel.org
---
 drivers/nvme/host/pci.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 11874afb2422..e3252bcbc58e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1616,17 +1616,15 @@ static void nvme_free_host_mem(struct nvme_dev *dev)
 	dev->host_mem_descs = NULL;
 }
 
-static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+static int __nvme_alloc_host_mem(struct nvme_dev *dev, u64 preferred,
+		u32 chunk_size)
 {
 	struct nvme_host_mem_buf_desc *descs;
-	u32 chunk_size, max_entries, len;
+	u32 max_entries, len;
 	int i = 0;
 	void **bufs;
 	u64 size = 0, tmp;
 
-	/* start big and work our way down */
-	chunk_size = min(preferred, (u64)PAGE_SIZE << MAX_ORDER);
-retry:
 	tmp = (preferred + chunk_size - 1);
 	do_div(tmp, chunk_size);
 	max_entries = tmp;
@@ -1652,15 +1650,9 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 		i++;
 	}
 
-	if (!size || (min && size < min)) {
-		dev_warn(dev->ctrl.device,
-			"failed to allocate host memory buffer.\n");
+	if (!size)
 		goto out_free_bufs;
-	}
 
-	dev_info(dev->ctrl.device,
-		"allocated %lld MiB host memory buffer.\n",
-		size >> ilog2(SZ_1M));
 	dev->nr_host_mem_descs = i;
 	dev->host_mem_size = size;
 	dev->host_mem_descs = descs;
@@ -1679,15 +1671,28 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 out_free_descs:
 	kfree(descs);
 out:
-	/* try a smaller chunk size if we failed early */
-	if (chunk_size >= PAGE_SIZE * 2 && (i == 0 || size < min)) {
-		chunk_size /= 2;
-		goto retry;
-	}
 	dev->host_mem_descs = NULL;
 	return -ENOMEM;
 }
 
+static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
+{
+	u32 chunk_size;
+
+	/* start big and work our way down */
+	for (chunk_size = min_t(u64, preferred, PAGE_SIZE << MAX_ORDER);
+	     chunk_size >= PAGE_SIZE * 2;
+	     chunk_size /= 2) {
+		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
+			if (!min || dev->host_mem_size >= min)
+				return 0;
+			nvme_free_host_mem(dev);
+		}
+	}
+
+	return -ENOMEM;
+}
+
 static void nvme_setup_host_mem(struct nvme_dev *dev)
 {
 	u64 max = (u64)max_host_mem_size_mb * SZ_1M;
@@ -1715,8 +1720,15 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 	}
 
 	if (!dev->host_mem_descs) {
-		if (nvme_alloc_host_mem(dev, min, preferred))
+		if (nvme_alloc_host_mem(dev, min, preferred)) {
+			dev_warn(dev->ctrl.device,
+				"failed to allocate host memory buffer.\n");
 			return;
+		}
+
+		dev_info(dev->ctrl.device,
+			"allocated %lld MiB host memory buffer.\n",
+			dev->host_mem_size >> ilog2(SZ_1M));
 	}
 
 	if (nvme_set_host_mem(dev, enable_bits))
-- 
2.11.0

Re: [PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback

[Keith Busch]

On Wed, Sep 06, 2017 at 03:55:29PM +0200, Christoph Hellwig wrote:
> nvme_alloc_host_mem currently contains two loops that are interwinded,
> and the outer retry loop turns out to be broken.  Fix this by untangling
> the two.
> 
> Based on a report an initial patch from Akinobu Mita.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Akinobu Mita <akinobu.mita@gmail.com>
> Tested-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: stable@vger.kernel.org
> ---

Looks good.

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

[PATCH 1/4] nvme-pci: fix host memory buffer allocation fallback

[Keith Busch]

On Wed, Sep 06, 2017@03:55:29PM +0200, Christoph Hellwig wrote:
> nvme_alloc_host_mem currently contains two loops that are interwinded,
> and the outer retry loop turns out to be broken.  Fix this by untangling
> the two.
> 
> Based on a report an initial patch from Akinobu Mita.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> Reported-by: Akinobu Mita <akinobu.mita at gmail.com>
> Tested-by: Akinobu Mita <akinobu.mita at gmail.com>
> Cc: stable at vger.kernel.org
> ---

Looks good.

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

[PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation

From: Akinobu Mita <akinobu.mita@gmail.com>

The initial chunk size for host memory buffer allocation is currently
PAGE_SIZE << MAX_ORDER.  MAX_ORDER order allocation is usually failed
without CONFIG_DMA_CMA.  So the HMB allocation is retried with chunk size
PAGE_SIZE << (MAX_ORDER - 1) in general, but there is no problem if the
retry allocation works correctly.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
[hch: rebased]
Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e3252bcbc58e..6d2acf06c180 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1680,7 +1680,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	u32 chunk_size;
 
 	/* start big and work our way down */
-	for (chunk_size = min_t(u64, preferred, PAGE_SIZE << MAX_ORDER);
+	for (chunk_size = min_t(u64, preferred, PAGE_SIZE * MAX_ORDER_NR_PAGES);
 	     chunk_size >= PAGE_SIZE * 2;
 	     chunk_size /= 2) {
 		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
-- 
2.11.0

[PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation

[Christoph Hellwig]

From: Akinobu Mita <akinobu.mita@gmail.com>

The initial chunk size for host memory buffer allocation is currently
PAGE_SIZE << MAX_ORDER.  MAX_ORDER order allocation is usually failed
without CONFIG_DMA_CMA.  So the HMB allocation is retried with chunk size
PAGE_SIZE << (MAX_ORDER - 1) in general, but there is no problem if the
retry allocation works correctly.

Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
[hch: rebased]
Signed-off-by: Christoph Hellwig <hch at lst.de>
Cc: stable at vger.kernel.org
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e3252bcbc58e..6d2acf06c180 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1680,7 +1680,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	u32 chunk_size;
 
 	/* start big and work our way down */
-	for (chunk_size = min_t(u64, preferred, PAGE_SIZE << MAX_ORDER);
+	for (chunk_size = min_t(u64, preferred, PAGE_SIZE * MAX_ORDER_NR_PAGES);
 	     chunk_size >= PAGE_SIZE * 2;
 	     chunk_size /= 2) {
 		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
-- 
2.11.0

Re: [PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation

[Keith Busch]

On Wed, Sep 06, 2017 at 03:55:30PM +0200, Christoph Hellwig wrote:
> From: Akinobu Mita <akinobu.mita@gmail.com>
> 
> The initial chunk size for host memory buffer allocation is currently
> PAGE_SIZE << MAX_ORDER.  MAX_ORDER order allocation is usually failed
> without CONFIG_DMA_CMA.  So the HMB allocation is retried with chunk size
> PAGE_SIZE << (MAX_ORDER - 1) in general, but there is no problem if the
> retry allocation works correctly.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> [hch: rebased]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Cc: stable@vger.kernel.org
> ---

Looks good.

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

[PATCH 2/4] nvme-pci: use appropriate initial chunk size for HMB allocation

[Keith Busch]

On Wed, Sep 06, 2017@03:55:30PM +0200, Christoph Hellwig wrote:
> From: Akinobu Mita <akinobu.mita at gmail.com>
> 
> The initial chunk size for host memory buffer allocation is currently
> PAGE_SIZE << MAX_ORDER.  MAX_ORDER order allocation is usually failed
> without CONFIG_DMA_CMA.  So the HMB allocation is retried with chunk size
> PAGE_SIZE << (MAX_ORDER - 1) in general, but there is no problem if the
> retry allocation works correctly.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita at gmail.com>
> [hch: rebased]
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> Cc: stable at vger.kernel.org
> ---

Looks good.

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

[PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Christoph Hellwig]

We want to catch command execution errors when resetting the device, so
propagate errors from the Set Features when setting up the host memory
buffer.  We keep ignoring memory allocation failures, as the spec
clearly says that the controller must work without a host memory buffer.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
---
 drivers/nvme/host/pci.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6d2acf06c180..06ea46801578 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1693,12 +1693,13 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	return -ENOMEM;
 }
 
-static void nvme_setup_host_mem(struct nvme_dev *dev)
+static int nvme_setup_host_mem(struct nvme_dev *dev)
 {
 	u64 max = (u64)max_host_mem_size_mb * SZ_1M;
 	u64 preferred = (u64)dev->ctrl.hmpre * 4096;
 	u64 min = (u64)dev->ctrl.hmmin * 4096;
 	u32 enable_bits = NVME_HOST_MEM_ENABLE;
+	int ret = 0;
 
 	preferred = min(preferred, max);
 	if (min > max) {
@@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 			"min host memory (%lld MiB) above limit (%d MiB).\n",
 			min >> ilog2(SZ_1M), max_host_mem_size_mb);
 		nvme_free_host_mem(dev);
-		return;
+		return 0;
 	}
 
 	/*
@@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 		if (nvme_alloc_host_mem(dev, min, preferred)) {
 			dev_warn(dev->ctrl.device,
 				"failed to allocate host memory buffer.\n");
-			return;
+			return 0; /* controller must work without HMB */
 		}
 
 		dev_info(dev->ctrl.device,
@@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 			dev->host_mem_size >> ilog2(SZ_1M));
 	}
 
-	if (nvme_set_host_mem(dev, enable_bits))
+	ret = nvme_set_host_mem(dev, enable_bits);
+	if (ret)
 		nvme_free_host_mem(dev);
+	return ret;
 }
 
 static int nvme_setup_io_queues(struct nvme_dev *dev)
@@ -2176,8 +2179,11 @@ static void nvme_reset_work(struct work_struct *work)
 				 "unable to allocate dma for dbbuf\n");
 	}
 
-	if (dev->ctrl.hmpre)
-		nvme_setup_host_mem(dev);
+	if (dev->ctrl.hmpre) {
+		result = nvme_setup_host_mem(dev);
+		if (result < 0)
+			goto out;
+	}
 
 	result = nvme_setup_io_queues(dev);
 	if (result)
-- 
2.11.0

[PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Christoph Hellwig]

We want to catch command execution errors when resetting the device, so
propagate errors from the Set Features when setting up the host memory
buffer.  We keep ignoring memory allocation failures, as the spec
clearly says that the controller must work without a host memory buffer.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6d2acf06c180..06ea46801578 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1693,12 +1693,13 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 	return -ENOMEM;
 }
 
-static void nvme_setup_host_mem(struct nvme_dev *dev)
+static int nvme_setup_host_mem(struct nvme_dev *dev)
 {
 	u64 max = (u64)max_host_mem_size_mb * SZ_1M;
 	u64 preferred = (u64)dev->ctrl.hmpre * 4096;
 	u64 min = (u64)dev->ctrl.hmmin * 4096;
 	u32 enable_bits = NVME_HOST_MEM_ENABLE;
+	int ret = 0;
 
 	preferred = min(preferred, max);
 	if (min > max) {
@@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 			"min host memory (%lld MiB) above limit (%d MiB).\n",
 			min >> ilog2(SZ_1M), max_host_mem_size_mb);
 		nvme_free_host_mem(dev);
-		return;
+		return 0;
 	}
 
 	/*
@@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 		if (nvme_alloc_host_mem(dev, min, preferred)) {
 			dev_warn(dev->ctrl.device,
 				"failed to allocate host memory buffer.\n");
-			return;
+			return 0; /* controller must work without HMB */
 		}
 
 		dev_info(dev->ctrl.device,
@@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
 			dev->host_mem_size >> ilog2(SZ_1M));
 	}
 
-	if (nvme_set_host_mem(dev, enable_bits))
+	ret = nvme_set_host_mem(dev, enable_bits);
+	if (ret)
 		nvme_free_host_mem(dev);
+	return ret;
 }
 
 static int nvme_setup_io_queues(struct nvme_dev *dev)
@@ -2176,8 +2179,11 @@ static void nvme_reset_work(struct work_struct *work)
 				 "unable to allocate dma for dbbuf\n");
 	}
 
-	if (dev->ctrl.hmpre)
-		nvme_setup_host_mem(dev);
+	if (dev->ctrl.hmpre) {
+		result = nvme_setup_host_mem(dev);
+		if (result < 0)
+			goto out;
+	}
 
 	result = nvme_setup_io_queues(dev);
 	if (result)
-- 
2.11.0

Re: [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Holger Hoffstätte]

On 09/06/17 15:55, Christoph Hellwig wrote:
> We want to catch command execution errors when resetting the device, so
> propagate errors from the Set Features when setting up the host memory
> buffer.  We keep ignoring memory allocation failures, as the spec
> clearly says that the controller must work without a host memory buffer.

Maybe I'm missing something but I think you forgot to update a bunch of
function signatures in terms of return type. Changing "return" to "return 0"
IMHO makes little sense otherwise.

-h

> @@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  			"min host memory (%lld MiB) above limit (%d MiB).\n",
>  			min >> ilog2(SZ_1M), max_host_mem_size_mb);
>  		nvme_free_host_mem(dev);
> -		return;
> +		return 0;

[..]

> @@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  		if (nvme_alloc_host_mem(dev, min, preferred)) {
>  			dev_warn(dev->ctrl.device,
>  				"failed to allocate host memory buffer.\n");
> -			return;
> +			return 0; /* controller must work without HMB */

[..]

> @@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  			dev->host_mem_size >> ilog2(SZ_1M));
>  	}
>  
> -	if (nvme_set_host_mem(dev, enable_bits))
> +	ret = nvme_set_host_mem(dev, enable_bits);
> +	if (ret)
>  		nvme_free_host_mem(dev);
> +	return ret;

[PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Holger Hoffstätte]

On 09/06/17 15:55, Christoph Hellwig wrote:
> We want to catch command execution errors when resetting the device, so
> propagate errors from the Set Features when setting up the host memory
> buffer.  We keep ignoring memory allocation failures, as the spec
> clearly says that the controller must work without a host memory buffer.

Maybe I'm missing something but I think you forgot to update a bunch of
function signatures in terms of return type. Changing "return" to "return 0"
IMHO makes little sense otherwise.

-h

> @@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  			"min host memory (%lld MiB) above limit (%d MiB).\n",
>  			min >> ilog2(SZ_1M), max_host_mem_size_mb);
>  		nvme_free_host_mem(dev);
> -		return;
> +		return 0;

[..]

> @@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  		if (nvme_alloc_host_mem(dev, min, preferred)) {
>  			dev_warn(dev->ctrl.device,
>  				"failed to allocate host memory buffer.\n");
> -			return;
> +			return 0; /* controller must work without HMB */

[..]

> @@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
>  			dev->host_mem_size >> ilog2(SZ_1M));
>  	}
>  
> -	if (nvme_set_host_mem(dev, enable_bits))
> +	ret = nvme_set_host_mem(dev, enable_bits);
> +	if (ret)
>  		nvme_free_host_mem(dev);
> +	return ret;

Re: [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Keith Busch]

On Wed, Sep 06, 2017 at 04:19:43PM +0200, Holger Hoffst�tte wrote:
> On 09/06/17 15:55, Christoph Hellwig wrote:
> > We want to catch command execution errors when resetting the device, so
> > propagate errors from the Set Features when setting up the host memory
> > buffer.  We keep ignoring memory allocation failures, as the spec
> > clearly says that the controller must work without a host memory buffer.
> 
> Maybe I'm missing something but I think you forgot to update a bunch of
> function signatures in terms of return type. Changing "return" to "return 0"
> IMHO makes little sense otherwise.
> 
> -h

The sig is updated:

 -static void nvme_setup_host_mem(struct nvme_dev *dev)
 +static int nvme_setup_host_mem(struct nvme_dev *dev)

The diff is just showing the old sig.

> > @@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  			"min host memory (%lld MiB) above limit (%d MiB).\n",
> >  			min >> ilog2(SZ_1M), max_host_mem_size_mb);
> >  		nvme_free_host_mem(dev);
> > -		return;
> > +		return 0;
> 
> [..]
> 
> > @@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  		if (nvme_alloc_host_mem(dev, min, preferred)) {
> >  			dev_warn(dev->ctrl.device,
> >  				"failed to allocate host memory buffer.\n");
> > -			return;
> > +			return 0; /* controller must work without HMB */
> 
> [..]
> 
> > @@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  			dev->host_mem_size >> ilog2(SZ_1M));
> >  	}
> >  
> > -	if (nvme_set_host_mem(dev, enable_bits))
> > +	ret = nvme_set_host_mem(dev, enable_bits);
> > +	if (ret)
> >  		nvme_free_host_mem(dev);
> > +	return ret;

[PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Keith Busch]

On Wed, Sep 06, 2017@04:19:43PM +0200, Holger Hoffst?tte wrote:
> On 09/06/17 15:55, Christoph Hellwig wrote:
> > We want to catch command execution errors when resetting the device, so
> > propagate errors from the Set Features when setting up the host memory
> > buffer.  We keep ignoring memory allocation failures, as the spec
> > clearly says that the controller must work without a host memory buffer.
> 
> Maybe I'm missing something but I think you forgot to update a bunch of
> function signatures in terms of return type. Changing "return" to "return 0"
> IMHO makes little sense otherwise.
> 
> -h

The sig is updated:

 -static void nvme_setup_host_mem(struct nvme_dev *dev)
 +static int nvme_setup_host_mem(struct nvme_dev *dev)

The diff is just showing the old sig.

> > @@ -1706,7 +1707,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  			"min host memory (%lld MiB) above limit (%d MiB).\n",
> >  			min >> ilog2(SZ_1M), max_host_mem_size_mb);
> >  		nvme_free_host_mem(dev);
> > -		return;
> > +		return 0;
> 
> [..]
> 
> > @@ -1723,7 +1724,7 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  		if (nvme_alloc_host_mem(dev, min, preferred)) {
> >  			dev_warn(dev->ctrl.device,
> >  				"failed to allocate host memory buffer.\n");
> > -			return;
> > +			return 0; /* controller must work without HMB */
> 
> [..]
> 
> > @@ -1731,8 +1732,10 @@ static void nvme_setup_host_mem(struct nvme_dev *dev)
> >  			dev->host_mem_size >> ilog2(SZ_1M));
> >  	}
> >  
> > -	if (nvme_set_host_mem(dev, enable_bits))
> > +	ret = nvme_set_host_mem(dev, enable_bits);
> > +	if (ret)
> >  		nvme_free_host_mem(dev);
> > +	return ret;

Re: [PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Keith Busch]

On Wed, Sep 06, 2017 at 03:55:31PM +0200, Christoph Hellwig wrote:
> We want to catch command execution errors when resetting the device, so
> propagate errors from the Set Features when setting up the host memory
> buffer.  We keep ignoring memory allocation failures, as the spec
> clearly says that the controller must work without a host memory buffer.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Cc: stable@vger.kernel.org

Looks good.

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

[PATCH 3/4] nvme-pci: propagate (some) errors from host memory buffer setup

[Keith Busch]

On Wed, Sep 06, 2017@03:55:31PM +0200, Christoph Hellwig wrote:
> We want to catch command execution errors when resetting the device, so
> propagate errors from the Set Features when setting up the host memory
> buffer.  We keep ignoring memory allocation failures, as the spec
> clearly says that the controller must work without a host memory buffer.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> Cc: stable at vger.kernel.org

Looks good.

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

[PATCH 4/4] nvme-pci: implement the HMB entry number and size limitations

[Christoph Hellwig]

Adds support for the new Host Memory Buffer Minimum Descriptor Entry Size
and Host Memory Maximum Descriptors Entries field that were added in
TP 4002 HMB Enhancements.  These allow the controller to advertise
limits for the usual number of segments in the host memory buffer, as
well as a minimum usable per-segment size.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c | 2 ++
 drivers/nvme/host/nvme.h | 3 +++
 drivers/nvme/host/pci.c  | 6 +++++-
 include/linux/nvme.h     | 4 +++-
 4 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 277a7a02cba5..407fc1c80d5b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1897,6 +1897,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		ctrl->cntlid = le16_to_cpu(id->cntlid);
 		ctrl->hmpre = le32_to_cpu(id->hmpre);
 		ctrl->hmmin = le32_to_cpu(id->hmmin);
+		ctrl->hmminds = le32_to_cpu(id->hmminds);
+		ctrl->hmmaxd = le16_to_cpu(id->hmmaxd);
 	}
 
 	kfree(id);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a19a587d60ed..432c543beaed 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -176,8 +176,11 @@ struct nvme_ctrl {
 	u64 ps_max_latency_us;
 	bool apst_enabled;
 
+	/* PCIe only: */
 	u32 hmpre;
 	u32 hmmin;
+	u32 hmminds;
+	u16 hmmaxd;
 
 	/* Fabrics only */
 	u16 sqsize;
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 06ea46801578..ef9093f617ac 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1628,6 +1628,10 @@ static int __nvme_alloc_host_mem(struct nvme_dev *dev, u64 preferred,
 	tmp = (preferred + chunk_size - 1);
 	do_div(tmp, chunk_size);
 	max_entries = tmp;
+
+	if (dev->ctrl.hmmaxd && dev->ctrl.hmmaxd < max_entries)
+		max_entries = dev->ctrl.hmmaxd;
+
 	descs = kcalloc(max_entries, sizeof(*descs), GFP_KERNEL);
 	if (!descs)
 		goto out;
@@ -1681,7 +1685,7 @@ static int nvme_alloc_host_mem(struct nvme_dev *dev, u64 min, u64 preferred)
 
 	/* start big and work our way down */
 	for (chunk_size = min_t(u64, preferred, PAGE_SIZE * MAX_ORDER_NR_PAGES);
-	     chunk_size >= PAGE_SIZE * 2;
+	     chunk_size >= max_t(u32, dev->ctrl.hmminds * 4096, PAGE_SIZE * 2);
 	     chunk_size /= 2) {
 		if (!__nvme_alloc_host_mem(dev, preferred, chunk_size)) {
 			if (!min || dev->host_mem_size >= min)
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 05560c2ecc83..c46f42269021 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -226,7 +226,9 @@ struct nvme_id_ctrl {
 	__le16			mntmt;
 	__le16			mxtmt;
 	__le32			sanicap;
-	__u8			rsvd332[180];
+	__le32			hmminds;
+	__le16			hmmaxd;
+	__u8			rsvd338[174];
 	__u8			sqes;
 	__u8			cqes;
 	__le16			maxcmd;
-- 
2.11.0

[PATCH 4/4] nvme-pci: implement the HMB entry number and size limitations

[Keith Busch]

On Wed, Sep 06, 2017@03:55:32PM +0200, Christoph Hellwig wrote:
> Adds support for the new Host Memory Buffer Minimum Descriptor Entry Size
> and Host Memory Maximum Descriptors Entries field that were added in
> TP 4002 HMB Enhancements.  These allow the controller to advertise
> limits for the usual number of segments in the host memory buffer, as
> well as a minimum usable per-segment size.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>

Looks good.

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