* [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness
@ 2016-06-01 17:54 Guilherme G. Piccoli
0 siblings, 0 replies; 5+ messages in thread
From: Guilherme G. Piccoli @ 2016-06-01 17:54 UTC (permalink / raw)
When disabling the controller, the specification says the register
NVME_REG_CC should be written and then driver needs to wait the
adapter to be ready, which is checked by reading another register
bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
so in case this timeout is reached the driver gives up and removes
the adapter from the system.
After a firmware activation procedure, the PCI_DEVICE(0x1c58, 0x0003)
(HGST adapter) end up being removed if we issue a reset_controller,
because driver keeps verifying the NVME_REG_CSTS until the timeout is
reached. This patch adds a necessary quirk for this adapter, by
introducing a delay before nvme_wait_ready(), so the reset procedure
is able to be completed. This quirk is needed because just increasing
the timeout is not enough in case of this adapter - the driver must
wait before start reading NVME_REG_CSTS register on this specific
device.
Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
drivers/nvme/host/core.c | 7 +++++++
drivers/nvme/host/nvme.h | 13 +++++++++++++
drivers/nvme/host/pci.c | 2 ++
3 files changed, 22 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1a51584..53e344d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -991,6 +991,13 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
if (ret)
return ret;
+
+ /* Checking for ctrl->tagset is a trick to avoid sleeping on module
+ * load, since we only need the quirk on reset_controller.
+ */
+ if ((ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY) && ctrl->tagset)
+ msleep(NVME_QUIRK_DELAY_AMOUNT);
+
return nvme_wait_ready(ctrl, cap, false);
}
EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1daa048..97ac7d1 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -65,8 +65,21 @@ enum nvme_quirks {
* logical blocks.
*/
NVME_QUIRK_DISCARD_ZEROES = (1 << 2),
+
+ /*
+ * The controller needs a delay before starts checking the device
+ * readiness, which is done by reading the NVME_CSTS_RDY bit.
+ */
+ NVME_QUIRK_DELAY_BEFORE_CHK_RDY = (1 << 3),
};
+/* The below value is the specific amount of delay needed before checking
+ * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
+ * NVME_QUIRK_DELAY_BEFORE_CHK_RDY quirk enabled. The value (in ms) was
+ * found empirically.
+ */
+#define NVME_QUIRK_DELAY_AMOUNT 2000
+
enum nvme_ctrl_state {
NVME_CTRL_NEW,
NVME_CTRL_LIVE,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 78dca31..d02255c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2117,6 +2117,8 @@ static const struct pci_device_id nvme_id_table[] = {
NVME_QUIRK_DISCARD_ZEROES, },
{ PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */
.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
+ { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */
+ .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
{ 0, }
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness
[not found] <201606011754.u51HsNPs003785@mx0a-001b2d01.pphosted.com>
@ 2016-06-02 8:22 ` Christoph Hellwig
2016-06-02 13:30 ` Guilherme G. Piccoli
0 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2016-06-02 8:22 UTC (permalink / raw)
Hi Guilherme,
the code looks fine code wise. But based on the previous disccusion
we only need the delay after a firmware activation, right? Even if
we don't have a good way to implement that (yet) it would be good
to document that fact in a comment.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness
2016-06-02 8:22 ` Christoph Hellwig
@ 2016-06-02 13:30 ` Guilherme G. Piccoli
0 siblings, 0 replies; 5+ messages in thread
From: Guilherme G. Piccoli @ 2016-06-02 13:30 UTC (permalink / raw)
On 06/02/2016 05:22 AM, Christoph Hellwig wrote:
> Hi Guilherme,
>
> the code looks fine code wise. But based on the previous disccusion
> we only need the delay after a firmware activation, right? Even if
> we don't have a good way to implement that (yet) it would be good
> to document that fact in a comment.
Thanks Christoph, I agree with you.
Will send a v3 with a comment about the quirk's need only in
reset_controller after firmware activation.
Cheers,
Guilherme
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness
[not found] <201606011754.u51HsKE4032778@mx0a-001b2d01.pphosted.com>
@ 2016-06-13 13:57 ` Jeff Lien
2016-06-13 14:43 ` Guilherme G. Piccoli
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Lien @ 2016-06-13 13:57 UTC (permalink / raw)
I have reviewed this patch and approve it.
----------------------------------------------------------
Jeff Lien
Linux Device Driver Development
Device Host Apps and Drivers
Western Digital Corporation
e. jeff.lien at hgst.com
o. +1-507-322-2416
m. +1-507-273-9124
________________________________________
From: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
Sent: Wednesday, June 01, 2016 12:54:20 PM
To: linux-nvme at lists.infradead.org
Cc: keith.busch at intel.com; axboe at fb.com; hch at infradead.org; brking at linux.vnet.ibm.com; krisman at linux.vnet.ibm.com; halves at linux.vnet.ibm.com; mniyer at us.ibm.com; David Darrington; Jeff Lien; Guilherme G. Piccoli
Subject: [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness
When disabling the controller, the specification says the register
NVME_REG_CC should be written and then driver needs to wait the
adapter to be ready, which is checked by reading another register
bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
so in case this timeout is reached the driver gives up and removes
the adapter from the system.
After a firmware activation procedure, the PCI_DEVICE(0x1c58, 0x0003)
(HGST adapter) end up being removed if we issue a reset_controller,
because driver keeps verifying the NVME_REG_CSTS until the timeout is
reached. This patch adds a necessary quirk for this adapter, by
introducing a delay before nvme_wait_ready(), so the reset procedure
is able to be completed. This quirk is needed because just increasing
the timeout is not enough in case of this adapter - the driver must
wait before start reading NVME_REG_CSTS register on this specific
device.
Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
drivers/nvme/host/core.c | 7 +++++++
drivers/nvme/host/nvme.h | 13 +++++++++++++
drivers/nvme/host/pci.c | 2 ++
3 files changed, 22 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1a51584..53e344d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -991,6 +991,13 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
if (ret)
return ret;
+
+ /* Checking for ctrl->tagset is a trick to avoid sleeping on module
+ * load, since we only need the quirk on reset_controller.
+ */
+ if ((ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY) && ctrl->tagset)
+ msleep(NVME_QUIRK_DELAY_AMOUNT);
+
return nvme_wait_ready(ctrl, cap, false);
}
EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1daa048..97ac7d1 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -65,8 +65,21 @@ enum nvme_quirks {
* logical blocks.
*/
NVME_QUIRK_DISCARD_ZEROES = (1 << 2),
+
+ /*
+ * The controller needs a delay before starts checking the device
+ * readiness, which is done by reading the NVME_CSTS_RDY bit.
+ */
+ NVME_QUIRK_DELAY_BEFORE_CHK_RDY = (1 << 3),
};
+/* The below value is the specific amount of delay needed before checking
+ * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
+ * NVME_QUIRK_DELAY_BEFORE_CHK_RDY quirk enabled. The value (in ms) was
+ * found empirically.
+ */
+#define NVME_QUIRK_DELAY_AMOUNT 2000
+
enum nvme_ctrl_state {
NVME_CTRL_NEW,
NVME_CTRL_LIVE,
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 78dca31..d02255c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2117,6 +2117,8 @@ static const struct pci_device_id nvme_id_table[] = {
NVME_QUIRK_DISCARD_ZEROES, },
{ PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */
.driver_data = NVME_QUIRK_IDENTIFY_CNS, },
+ { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */
+ .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
{ 0, }
--
2.1.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness
2016-06-13 13:57 ` [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness Jeff Lien
@ 2016-06-13 14:43 ` Guilherme G. Piccoli
0 siblings, 0 replies; 5+ messages in thread
From: Guilherme G. Piccoli @ 2016-06-13 14:43 UTC (permalink / raw)
On 06/13/2016 10:57 AM, Jeff Lien wrote:
> I have reviewed this patch and approve it.
>
Thanks Jeff, I'll sent a v3 implementing Christoph's suggestion.
Cheers,
Guilherme
> ----------------------------------------------------------
> Jeff Lien
> Linux Device Driver Development
> Device Host Apps and Drivers
> Western Digital Corporation
> e. jeff.lien at hgst.com
> o. +1-507-322-2416
> m. +1-507-273-9124
>
>
>
>
>
> ________________________________________
> From: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
> Sent: Wednesday, June 01, 2016 12:54:20 PM
> To: linux-nvme at lists.infradead.org
> Cc: keith.busch at intel.com; axboe at fb.com; hch at infradead.org; brking at linux.vnet.ibm.com; krisman at linux.vnet.ibm.com; halves at linux.vnet.ibm.com; mniyer at us.ibm.com; David Darrington; Jeff Lien; Guilherme G. Piccoli
> Subject: [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness
>
> When disabling the controller, the specification says the register
> NVME_REG_CC should be written and then driver needs to wait the
> adapter to be ready, which is checked by reading another register
> bit (NVME_CSTS_RDY). There's a timeout validation in this checking,
> so in case this timeout is reached the driver gives up and removes
> the adapter from the system.
>
> After a firmware activation procedure, the PCI_DEVICE(0x1c58, 0x0003)
> (HGST adapter) end up being removed if we issue a reset_controller,
> because driver keeps verifying the NVME_REG_CSTS until the timeout is
> reached. This patch adds a necessary quirk for this adapter, by
> introducing a delay before nvme_wait_ready(), so the reset procedure
> is able to be completed. This quirk is needed because just increasing
> the timeout is not enough in case of this adapter - the driver must
> wait before start reading NVME_REG_CSTS register on this specific
> device.
>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
> ---
> drivers/nvme/host/core.c | 7 +++++++
> drivers/nvme/host/nvme.h | 13 +++++++++++++
> drivers/nvme/host/pci.c | 2 ++
> 3 files changed, 22 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 1a51584..53e344d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -991,6 +991,13 @@ int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap)
> ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
> if (ret)
> return ret;
> +
> + /* Checking for ctrl->tagset is a trick to avoid sleeping on module
> + * load, since we only need the quirk on reset_controller.
> + */
> + if ((ctrl->quirks & NVME_QUIRK_DELAY_BEFORE_CHK_RDY) && ctrl->tagset)
> + msleep(NVME_QUIRK_DELAY_AMOUNT);
> +
> return nvme_wait_ready(ctrl, cap, false);
> }
> EXPORT_SYMBOL_GPL(nvme_disable_ctrl);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 1daa048..97ac7d1 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -65,8 +65,21 @@ enum nvme_quirks {
> * logical blocks.
> */
> NVME_QUIRK_DISCARD_ZEROES = (1 << 2),
> +
> + /*
> + * The controller needs a delay before starts checking the device
> + * readiness, which is done by reading the NVME_CSTS_RDY bit.
> + */
> + NVME_QUIRK_DELAY_BEFORE_CHK_RDY = (1 << 3),
> };
>
> +/* The below value is the specific amount of delay needed before checking
> + * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the
> + * NVME_QUIRK_DELAY_BEFORE_CHK_RDY quirk enabled. The value (in ms) was
> + * found empirically.
> + */
> +#define NVME_QUIRK_DELAY_AMOUNT 2000
> +
> enum nvme_ctrl_state {
> NVME_CTRL_NEW,
> NVME_CTRL_LIVE,
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 78dca31..d02255c 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2117,6 +2117,8 @@ static const struct pci_device_id nvme_id_table[] = {
> NVME_QUIRK_DISCARD_ZEROES, },
> { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */
> .driver_data = NVME_QUIRK_IDENTIFY_CNS, },
> + { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */
> + .driver_data = NVME_QUIRK_DELAY_BEFORE_CHK_RDY, },
> { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
> { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
> { 0, }
> --
> 2.1.0
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-13 14:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201606011754.u51HsKE4032778@mx0a-001b2d01.pphosted.com>
2016-06-13 13:57 ` [PATCH v2] nvme/quirk: Add a delay before checking for adapter readiness Jeff Lien
2016-06-13 14:43 ` Guilherme G. Piccoli
[not found] <201606011754.u51HsNPs003785@mx0a-001b2d01.pphosted.com>
2016-06-02 8:22 ` Christoph Hellwig
2016-06-02 13:30 ` Guilherme G. Piccoli
2016-06-01 17:54 Guilherme G. Piccoli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.