[PATCHv2] nvme: avoid bogus CRTO values

[PATCHv2] nvme: avoid bogus CRTO values

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Some devices are reporting Controller Ready Modes Supported, but return
0 for CRTO. These devices require a much higher time to ready than that,
so they are failing to initialize after the driver started preferring
that value over CAP.TO.

The spec requires CAP.TO match the appropritate CRTO value, or be set to
0xff if CRTO is larger than that. This means that CAP.TO can be used to
validate if CRTO is reliable, and provides an appropriate fallback for
setting the timeout value if not. Use whichever is larger.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217863
Reported-by: Cláudio Sampaio <patola@gmail.com>
Reported-by: Felix Yan <felixonmars@archlinux.org>
Based-on-a-patch-by: Felix Yan <felixonmars@archlinux.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Cc: stable@vger.kernel.org
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v1->v2:
  Warn once if driver isn't relying on CRTO values

 drivers/nvme/host/core.c | 54 ++++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 37b6fa7466620..0685ed4f2dc49 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2245,25 +2245,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	else
 		ctrl->ctrl_config = NVME_CC_CSS_NVM;
 
-	if (ctrl->cap & NVME_CAP_CRMS_CRWMS) {
-		u32 crto;
-
-		ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CRTO, &crto);
-		if (ret) {
-			dev_err(ctrl->device, "Reading CRTO failed (%d)\n",
-				ret);
-			return ret;
-		}
-
-		if (ctrl->cap & NVME_CAP_CRMS_CRIMS) {
-			ctrl->ctrl_config |= NVME_CC_CRIME;
-			timeout = NVME_CRTO_CRIMT(crto);
-		} else {
-			timeout = NVME_CRTO_CRWMT(crto);
-		}
-	} else {
-		timeout = NVME_CAP_TIMEOUT(ctrl->cap);
-	}
+	if (ctrl->cap & NVME_CAP_CRMS_CRWMS && ctrl->cap & NVME_CAP_CRMS_CRIMS)
+		ctrl->ctrl_config |= NVME_CC_CRIME;
 
 	ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
 	ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
@@ -2277,6 +2260,39 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	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)
+		return ret;
+
+	timeout = NVME_CAP_TIMEOUT(ctrl->cap);
+	if (ctrl->cap & NVME_CAP_CRMS_CRWMS) {
+		u32 crto, ready_timeout;
+
+		ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CRTO, &crto);
+		if (ret) {
+			dev_err(ctrl->device, "Reading CRTO failed (%d)\n",
+				ret);
+			return ret;
+		}
+
+		/*
+		 * CRTO should always be greater or equal to CAP.TO, but some
+		 * devices are known to get this wrong. Use the larger of the
+		 * two values.
+		 */
+		if (ctrl->ctrl_config & NVME_CC_CRIME)
+			ready_timeout = NVME_CRTO_CRIMT(crto);
+		else
+			ready_timeout = NVME_CRTO_CRWMT(crto);
+
+		if (ready_timeout < timeout)
+			dev_warn_once(ctrl->device, "bad crto:%x cap:%llx\n",
+				      crto, ctrl->cap);
+		else
+			timeout = ready_timeout;
+	}
+
 	ctrl->ctrl_config |= NVME_CC_ENABLE;
 	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
 	if (ret)
-- 
2.34.1

Re: [PATCHv2] nvme: avoid bogus CRTO values

[Keith Busch]

On Wed, Sep 13, 2023 at 01:28:10PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Some devices are reporting Controller Ready Modes Supported, but return
> 0 for CRTO. These devices require a much higher time to ready than that,
> so they are failing to initialize after the driver started preferring
> that value over CAP.TO.
> 
> The spec requires CAP.TO match the appropritate CRTO value, or be set to
> 0xff if CRTO is larger than that. This means that CAP.TO can be used to
> validate if CRTO is reliable, and provides an appropriate fallback for
> setting the timeout value if not. Use whichever is larger.

I need to send a pull request out today since we're quite a bit behind
as it is. I've applied this for nvme-6.6 now since it fixes a regression
that apparently quite a few people are encountering. If there are any
objections, please let me know by EOD and I'll remove it from the queue.

Re: [PATCHv2] nvme: avoid bogus CRTO values

[Felix Yan]

[-- Attachment #1.1: Type: text/plain, Size: 3801 bytes --]

On 9/13/23 23:28, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Some devices are reporting Controller Ready Modes Supported, but return
> 0 for CRTO. These devices require a much higher time to ready than that,
> so they are failing to initialize after the driver started preferring
> that value over CAP.TO.
> 
> The spec requires CAP.TO match the appropritate CRTO value, or be set to
> 0xff if CRTO is larger than that. This means that CAP.TO can be used to
> validate if CRTO is reliable, and provides an appropriate fallback for
> setting the timeout value if not. Use whichever is larger.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217863
> Reported-by: Cláudio Sampaio <patola@gmail.com>
> Reported-by: Felix Yan <felixonmars@archlinux.org>
> Based-on-a-patch-by: Felix Yan <felixonmars@archlinux.org>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Cc: stable@vger.kernel.org
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> v1->v2:
>    Warn once if driver isn't relying on CRTO values
> 
>   drivers/nvme/host/core.c | 54 ++++++++++++++++++++++++++--------------
>   1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 37b6fa7466620..0685ed4f2dc49 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2245,25 +2245,8 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>   	else
>   		ctrl->ctrl_config = NVME_CC_CSS_NVM;
>   
> -	if (ctrl->cap & NVME_CAP_CRMS_CRWMS) {
> -		u32 crto;
> -
> -		ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CRTO, &crto);
> -		if (ret) {
> -			dev_err(ctrl->device, "Reading CRTO failed (%d)\n",
> -				ret);
> -			return ret;
> -		}
> -
> -		if (ctrl->cap & NVME_CAP_CRMS_CRIMS) {
> -			ctrl->ctrl_config |= NVME_CC_CRIME;
> -			timeout = NVME_CRTO_CRIMT(crto);
> -		} else {
> -			timeout = NVME_CRTO_CRWMT(crto);
> -		}
> -	} else {
> -		timeout = NVME_CAP_TIMEOUT(ctrl->cap);
> -	}
> +	if (ctrl->cap & NVME_CAP_CRMS_CRWMS && ctrl->cap & NVME_CAP_CRMS_CRIMS)
> +		ctrl->ctrl_config |= NVME_CC_CRIME;
>   
>   	ctrl->ctrl_config |= (NVME_CTRL_PAGE_SHIFT - 12) << NVME_CC_MPS_SHIFT;
>   	ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE;
> @@ -2277,6 +2260,39 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>   	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)
> +		return ret;
> +
> +	timeout = NVME_CAP_TIMEOUT(ctrl->cap);
> +	if (ctrl->cap & NVME_CAP_CRMS_CRWMS) {
> +		u32 crto, ready_timeout;
> +
> +		ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CRTO, &crto);
> +		if (ret) {
> +			dev_err(ctrl->device, "Reading CRTO failed (%d)\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		/*
> +		 * CRTO should always be greater or equal to CAP.TO, but some
> +		 * devices are known to get this wrong. Use the larger of the
> +		 * two values.
> +		 */
> +		if (ctrl->ctrl_config & NVME_CC_CRIME)
> +			ready_timeout = NVME_CRTO_CRIMT(crto);
> +		else
> +			ready_timeout = NVME_CRTO_CRWMT(crto);
> +
> +		if (ready_timeout < timeout)
> +			dev_warn_once(ctrl->device, "bad crto:%x cap:%llx\n",
> +				      crto, ctrl->cap);
> +		else
> +			timeout = ready_timeout;
> +	}
> +
>   	ctrl->ctrl_config |= NVME_CC_ENABLE;
>   	ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, ctrl->ctrl_config);
>   	if (ret)

Thanks, verified that it works well here.

I noticed only one very small issue: dev_warn_once seems to only print 
once when multiple devices are affected. It may be more ideal if it 
prints once for each device, but I don't know how to really achieve that...

-- 
Regards,
Felix Yan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCHv2] nvme: avoid bogus CRTO values

[Keith Busch]

On Thu, Sep 14, 2023 at 10:48:55PM +0300, Felix Yan wrote:
> 
> Thanks, verified that it works well here.

Thanks, okay if I append your Tested-by: in the patch?
 
> I noticed only one very small issue: dev_warn_once seems to only print once
> when multiple devices are affected. It may be more ideal if it prints once
> for each device, but I don't know how to really achieve that...

There's no good way to do that, unfortunately. We'd have to create a
custom "print once" based on some driver specific flag for this path,
but that's overkill for this issue, IMO. I feel it should be sufficient
just to know that the fallback is happening, and doesn't really matter
for an admin scanning the logs to see it appear for each device. My main
concern was printing it on every reset; that level of repitition would
definitely cause alarm for some people.

Re: [PATCHv2] nvme: avoid bogus CRTO values

[Felix Yan]

[-- Attachment #1.1: Type: text/plain, Size: 1024 bytes --]

On 9/14/23 22:58, Keith Busch wrote:
> On Thu, Sep 14, 2023 at 10:48:55PM +0300, Felix Yan wrote:
>>
>> Thanks, verified that it works well here.
> 
> Thanks, okay if I append your Tested-by: in the patch?

Sure :)

>> I noticed only one very small issue: dev_warn_once seems to only print once
>> when multiple devices are affected. It may be more ideal if it prints once
>> for each device, but I don't know how to really achieve that...
> 
> There's no good way to do that, unfortunately. We'd have to create a
> custom "print once" based on some driver specific flag for this path,
> but that's overkill for this issue, IMO. I feel it should be sufficient
> just to know that the fallback is happening, and doesn't really matter
> for an admin scanning the logs to see it appear for each device. My main
> concern was printing it on every reset; that level of repitition would
> definitely cause alarm for some people.

I see. I'm okay with the current solution then.

-- 
Regards,
Felix Yan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]