Skip to contentSkip to conversation

NVMe: Change nvme_enable_ctrl behavior and track CC

PATCH New Dan McLeran 3 participants 5 messages Jun 19, 2014 – Jun 20, 2014

Subsystems (2)
Block layer NVMe
lore
Share & tools
lore

Dan McLeran

author
#0 lore raw

PATCH 1/1 NVMe: Change nvme_enable_ctrl behavior and track CC

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(-)
drivers/block/nvme-core.c +16 -4
index 02351e2..3e31b67 100644
@@ -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;
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/1 NVMe: Change nvme_enable_ctrl behavior and track CC

Quoted 6 lines · 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.

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.

drivers/block/nvme-core.c static int nvme_configure_admin_queue(struct nvme_dev *dev) · line 1483

Quoted 8 lines
drivers/block/nvme-core.c +0 -1
@@ -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?

author
#2 lore raw

PATCH 1/1 NVMe: Change nvme_enable_ctrl behavior and track CC

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?

Quoted 36 lines · Matthew Wilcox
Quoted 6 lines · 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.

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.

drivers/block/nvme-core.c static int nvme_configure_admin_queue(struct nvme_dev *dev) · line 1483

Quoted 8 lines
drivers/block/nvme-core.c +0 -1
@@ -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?

#3 lore raw

PATCH 1/1 NVMe: Change nvme_enable_ctrl behavior and track CC

Quoted 6 lines · 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?

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

author
#4 lore raw

PATCH 1/1 NVMe: Change nvme_enable_ctrl behavior and track CC

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

Quoted 11 lines · Keith Busch
Quoted 6 lines · 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?

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