[PATCH] nvme: remove CC register read-back during enabling

[PATCH] nvme: remove CC register read-back during enabling

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Any non-posted read should flush the previous write, so we don't
necessarily need to read back the value we just wrote. I've found at
least some controllers that respond with 0 for short moments after
writing the CC register with EN (enable) cleared, so the read-back is
overwriting our valid ctrl_config value and ends up breaking on the
subsequent enabling.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 983909a600adb..3172d7dab43bb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2467,11 +2467,6 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	if (ret)
 		return ret;
 
-	/* Flush write to device (required if transport is PCI) */
-	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CC, &ctrl->ctrl_config);
-	if (ret)
-		return ret;
-
 	/* CAP value may change after initial CC write */
 	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
 	if (ret)
-- 
2.43.5

Re: [PATCH] nvme: remove CC register read-back during enabling

[Keith Busch]

On Wed, Sep 04, 2024 at 02:54:05PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Any non-posted read should flush the previous write, so we don't
> necessarily need to read back the value we just wrote. I've found at
> least some controllers that respond with 0 for short moments after
> writing the CC register with EN (enable) cleared, so the read-back is
> overwriting our valid ctrl_config value and ends up breaking on the
> subsequent enabling.

Ping on this one. Looks okay?
 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 983909a600adb..3172d7dab43bb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2467,11 +2467,6 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  	if (ret)
>  		return ret;
>  
> -	/* Flush write to device (required if transport is PCI) */
> -	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CC, &ctrl->ctrl_config);
> -	if (ret)
> -		return ret;
> -
>  	/* CAP value may change after initial CC write */
>  	ret = ctrl->ops->reg_read64(ctrl, NVME_REG_CAP, &ctrl->cap);
>  	if (ret)
> -- 
> 2.43.5
> 

Re: [PATCH] nvme: remove CC register read-back during enabling

[Christoph Hellwig]

On Mon, Sep 23, 2024 at 10:03:32AM +0200, Keith Busch wrote:
> On Wed, Sep 04, 2024 at 02:54:05PM -0700, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Any non-posted read should flush the previous write, so we don't
> > necessarily need to read back the value we just wrote. I've found at
> > least some controllers that respond with 0 for short moments after
> > writing the CC register with EN (enable) cleared, so the read-back is
> > overwriting our valid ctrl_config value and ends up breaking on the
> > subsequent enabling.
> 
> Ping on this one. Looks okay?

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>