[PATCH] NVMe: Change nvme_enable_ctrl behavior and track CC

[PATCH] NVMe: Change nvme_enable_ctrl behavior and track CC

[Dan McLeran]

In the current implementation, nvme_enable_ctrl does not actually enable the controller.
It waits for the controller, which has previously been enabled from nvme_configure_admin_queue,
to become ready to receive commands. This patch moves the logic of enabling the controller into
nvme_enable_ctrl. This patch also ensure that shutdown flags are not passed as part of enabling
or disabling the controller. This patch also ensures the software representation of the CC
register remains in sync with actual reads and writes to the hardware.

Signed-off-by: Dan McLeran <daniel.mcleran at intel.com>
---
 drivers/block/nvme-core.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 02351e2..3e31b67 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1408,15 +1408,27 @@ static int nvme_wait_ready(struct nvme_dev *dev, u64 cap, bool enabled)
  */
 static int nvme_disable_ctrl(struct nvme_dev *dev, u64 cap)
 {
-	u32 cc = readl(&dev->bar->cc);
+	u32 cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK);
+
+	if (cc & NVME_CC_ENABLE) {
+		cc &= ~NVME_CC_ENABLE;
+		writel(cc, &dev->bar->cc);
+		dev->ctrl_config = cc;
+	}
 
-	if (cc & NVME_CC_ENABLE)
-		writel(cc & ~NVME_CC_ENABLE, &dev->bar->cc);
 	return nvme_wait_ready(dev, cap, false);
 }
 
 static int nvme_enable_ctrl(struct nvme_dev *dev, u64 cap)
 {
+	u32 cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK);
+
+	if (!(cc & NVME_CC_ENABLE)) {
+		cc |= NVME_CC_ENABLE | NVME_CC_CSS_NVM;
+		writel(cc, &dev->bar->cc);
+		dev->ctrl_config = cc;
+	}
+
 	return nvme_wait_ready(dev, cap, true);
 }
 
@@ -1427,6 +1439,7 @@ static int nvme_shutdown_ctrl(struct nvme_dev *dev)
 
 	cc = (readl(&dev->bar->cc) & ~NVME_CC_SHN_MASK) | NVME_CC_SHN_NORMAL;
 	writel(cc, &dev->bar->cc);
+	dev->ctrl_config = cc;
 
 	timeout = 2 * HZ + jiffies;
 	while ((readl(&dev->bar->csts) & NVME_CSTS_SHST_MASK) !=
@@ -1465,7 +1478,6 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	aqa = nvmeq->q_depth - 1;
 	aqa |= aqa << 16;
 
-	dev->ctrl_config = NVME_CC_ENABLE | NVME_CC_CSS_NVM;
 	dev->ctrl_config |= (PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
 	dev->ctrl_config |= NVME_CC_ARB_RR | NVME_CC_SHN_NONE;
 	dev->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
-- 
1.7.10.4

[PATCH] NVMe: Change nvme_enable_ctrl behavior and track CC

[Matthew Wilcox]

On Thu, Jun 19, 2014@09:42:55AM -0600, Dan McLeran wrote:
> In the current implementation, nvme_enable_ctrl does not actually enable the controller.
> It waits for the controller, which has previously been enabled from nvme_configure_admin_queue,
> to become ready to receive commands. This patch moves the logic of enabling the controller into
> nvme_enable_ctrl. This patch also ensure that shutdown flags are not passed as part of enabling
> or disabling the controller. This patch also ensures the software representation of the CC
> register remains in sync with actual reads and writes to the hardware.

Please wrap your changelog entries to 75-ish characters.  You've also
got three consecutive sentences starting with 'This patch' which reads
a bit funny.

I don't understand why we'd want to clear the current settings of SHN
when we write CC.  Wouldn't the controller infer from that the shutdown
has been cancelled?  Leaving the bits the same seems like the right
course of action.

Looking at the meat of the patch causes me to wonder ... why are we
reading ->cc anywhere?  We program it; the controller isn't allowed
to modify any part of it.  This register should be write-only for us,
using the ctrl_config soft-copy.

> @@ -1465,7 +1478,6 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
>  	aqa = nvmeq->q_depth - 1;
>  	aqa |= aqa << 16;
>  
> -	dev->ctrl_config = NVME_CC_ENABLE | NVME_CC_CSS_NVM;
>  	dev->ctrl_config |= (PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
>  	dev->ctrl_config |= NVME_CC_ARB_RR | NVME_CC_SHN_NONE;
>  	dev->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;

I don't think this is right.  You're reading in the register settings
in nvme_disable_ctrl(), but the point of this routine is to initialise
the settings in ctrl_config, not to preserve the old settings.  What
if the BIOS had, for example, configured IOCQES to a different size?

[PATCH] NVMe: Change nvme_enable_ctrl behavior and track CC

[Dan McLeran]

nvme_probe as it is written today essentially clears out any the shutdown 
bits during enable. If you unload/reload the nvme module you'll see 
that the normal shutdown bits are set before the controller gets 
enabled. The nvme_remove path calls nvme_shutdown_ctrl but not 
nvme_disable_ctrl. Maybe the nvme_probe path should always disable the 
controller first before enabling to ensure we're starting clean?

On Fri, 20 Jun 2014, Matthew Wilcox wrote:

> On Thu, Jun 19, 2014@09:42:55AM -0600, Dan McLeran wrote:
>> In the current implementation, nvme_enable_ctrl does not actually enable the controller.
>> It waits for the controller, which has previously been enabled from nvme_configure_admin_queue,
>> to become ready to receive commands. This patch moves the logic of enabling the controller into
>> nvme_enable_ctrl. This patch also ensure that shutdown flags are not passed as part of enabling
>> or disabling the controller. This patch also ensures the software representation of the CC
>> register remains in sync with actual reads and writes to the hardware.
>
> Please wrap your changelog entries to 75-ish characters.  You've also
> got three consecutive sentences starting with 'This patch' which reads
> a bit funny.
>
> I don't understand why we'd want to clear the current settings of SHN
> when we write CC.  Wouldn't the controller infer from that the shutdown
> has been cancelled?  Leaving the bits the same seems like the right
> course of action.
>
> Looking at the meat of the patch causes me to wonder ... why are we
> reading ->cc anywhere?  We program it; the controller isn't allowed
> to modify any part of it.  This register should be write-only for us,
> using the ctrl_config soft-copy.
>
>> @@ -1465,7 +1478,6 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
>>  	aqa = nvmeq->q_depth - 1;
>>  	aqa |= aqa << 16;
>>
>> -	dev->ctrl_config = NVME_CC_ENABLE | NVME_CC_CSS_NVM;
>>  	dev->ctrl_config |= (PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
>>  	dev->ctrl_config |= NVME_CC_ARB_RR | NVME_CC_SHN_NONE;
>>  	dev->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES;
>
> I don't think this is right.  You're reading in the register settings
> in nvme_disable_ctrl(), but the point of this routine is to initialise
> the settings in ctrl_config, not to preserve the old settings.  What
> if the BIOS had, for example, configured IOCQES to a different size?
>

[PATCH] NVMe: Change nvme_enable_ctrl behavior and track CC

[Keith Busch]

On Fri, 20 Jun 2014, Dan McLeran wrote:
> nvme_probe as it is written today essentially clears out any the shutdown 
> bits during enable. If you unload/reload the nvme module you'll see that the 
> normal shutdown bits are set before the controller gets enabled. The 
> nvme_remove path calls nvme_shutdown_ctrl but not nvme_disable_ctrl. Maybe 
> the nvme_probe path should always disable the controller first before 
> enabling to ensure we're starting clean?

But we already do that. We probably want nvme_disable_ctrl to also clear
the CC.SHN bits in addition to CC.EN, though.

[PATCH] NVMe: Change nvme_enable_ctrl behavior and track CC

[Dan McLeran]

Hmm. You're right. I guess I thought setting EN to 0 would have cleared 
out the shutdown bits. I guess not.

On Fri, 20 Jun 2014, Keith Busch wrote:

> On Fri, 20 Jun 2014, Dan McLeran wrote:
>> nvme_probe as it is written today essentially clears out any the shutdown 
>> bits during enable. If you unload/reload the nvme module you'll see that 
>> the normal shutdown bits are set before the controller gets enabled. The 
>> nvme_remove path calls nvme_shutdown_ctrl but not nvme_disable_ctrl. Maybe 
>> the nvme_probe path should always disable the controller first before 
>> enabling to ensure we're starting clean?
>
> But we already do that. We probably want nvme_disable_ctrl to also clear
> the CC.SHN bits in addition to CC.EN, though.
>