[PATCH v2 0/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[PATCH v2 0/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[gjoyce]

From: Greg Joyce <gjoyce@linux.ibm.com>

NVMe 2.0 introduced a configuration setting CC.CRIME that allows the 
controller to set CSTS.RDY before the media is fully enabled. In this
case commands listed in Figure 103 may return the status
NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY. The NVMe driver is not coded
to handle these failures so initialization errors occur.

Since the current driver does not:
	1) retry media not ready errors
	2) utilize any parallel initialization that CRIME may provide

setting NVME_CC_CRIME provides no value.

The NVMe 2.0 spec says this about CC.CRIME:
	"If the CAP.CRMS field is set to 11b, then both controller
	 ready modes are supported, and the host may select the
	 controller ready mode by modifying the value of the
	 CC.CRIME bit."

This the CC.CRIME feature is optional and this patch removes it.

Changelog
v2:	- remove NVME_CC_CRIME from timeout calculations (Keith Busch)

Greg Joyce (1):
  nvme: disable CC.CRIME (NVME_CC_CRIME)

 drivers/nvme/host/core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
gjoyce@linux.ibm.com

[PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[gjoyce]

From: Greg Joyce <gjoyce@linux.ibm.com>

Disable NVME_CC_CRIME so that CSTS.RDY indicates that the media
is ready and able to handle commands without returning
NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY.

Signed-off-by: Greg Joyce <gjoyce@linux.ibm.com>
---
 drivers/nvme/host/core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ba6508455e18..10eca7660ca7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2458,8 +2458,13 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 	else
 		ctrl->ctrl_config = NVME_CC_CSS_NVM;
 
-	if (ctrl->cap & NVME_CAP_CRMS_CRWMS && ctrl->cap & NVME_CAP_CRMS_CRIMS)
-		ctrl->ctrl_config |= NVME_CC_CRIME;
+	/*
+	 * Setting CRIME results in CSTS.RDY before the media is ready. This
+	 * make it possible for media related commands to return the error
+	 * NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY. Until the driver is
+	 * restructurade to handle retries, disable CC.CRIME.
+	 */
+	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;
@@ -2489,10 +2494,7 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
 		 * 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);
+		ready_timeout = NVME_CRTO_CRWMT(crto);
 
 		if (ready_timeout < timeout)
 			dev_warn_once(ctrl->device, "bad crto:%x cap:%llx\n",
-- 
gjoyce@linux.ibm.com

Re: [PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[Nilay Shroff]

On 10/8/24 01:03, gjoyce@linux.ibm.com wrote:
> From: Greg Joyce <gjoyce@linux.ibm.com>
> 
> Disable NVME_CC_CRIME so that CSTS.RDY indicates that the media
> is ready and able to handle commands without returning
> NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY.
> 
> Signed-off-by: Greg Joyce <gjoyce@linux.ibm.com>
> ---
>  drivers/nvme/host/core.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ba6508455e18..10eca7660ca7 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2458,8 +2458,13 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  	else
>  		ctrl->ctrl_config = NVME_CC_CSS_NVM;
>  
> -	if (ctrl->cap & NVME_CAP_CRMS_CRWMS && ctrl->cap & NVME_CAP_CRMS_CRIMS)
> -		ctrl->ctrl_config |= NVME_CC_CRIME;
> +	/*
> +	 * Setting CRIME results in CSTS.RDY before the media is ready. This
> +	 * make it possible for media related commands to return the error
> +	 * NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY. Until the driver is
> +	 * restructurade to handle retries, disable CC.CRIME.
> +	 */
> +	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;
> @@ -2489,10 +2494,7 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl)
>  		 * 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);
> +		ready_timeout = NVME_CRTO_CRWMT(crto);
>  
>  		if (ready_timeout < timeout)
>  			dev_warn_once(ctrl->device, "bad crto:%x cap:%llx\n",

Looks good to me.

Reviewed-by: nilay@linux.ibm.com

Tested the above patch on Kioxia disk supporting TCG Opal SED. My test 
confirmed that the patch works as expected and we don't see 
NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY error during opal discovery or 
any admin command executed during disk initialization.

Tested-by: nilay@linux.ibm.com

Re: [PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[Keith Busch]

On Mon, Oct 07, 2024 at 02:33:24PM -0500, gjoyce@linux.ibm.com wrote:
> From: Greg Joyce <gjoyce@linux.ibm.com>
> 
> Disable NVME_CC_CRIME so that CSTS.RDY indicates that the media
> is ready and able to handle commands without returning
> NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY.

I'm okay with this. We just need to see if Christoph wants to chime in
since he added the without media ready support. I never saw the value in
having Linux use it, but maybe there's a good reason to do it.

Re: [PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[Christoph Hellwig]

On Tue, Oct 08, 2024 at 11:35:59AM -0600, Keith Busch wrote:
> > Disable NVME_CC_CRIME so that CSTS.RDY indicates that the media
> > is ready and able to handle commands without returning
> > NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY.
> 
> I'm okay with this. We just need to see if Christoph wants to chime in
> since he added the without media ready support. I never saw the value in
> having Linux use it, but maybe there's a good reason to do it.

It think it is useful not to block bringing the admin queue up on
the media timeout.  But given how fucked up implementations are we
might as well remove the support for now.

Re: [PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[Keith Busch]

On Mon, Oct 07, 2024 at 02:33:24PM -0500, gjoyce@linux.ibm.com wrote:
> From: Greg Joyce <gjoyce@linux.ibm.com>
> 
> Disable NVME_CC_CRIME so that CSTS.RDY indicates that the media
> is ready and able to handle commands without returning
> NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY.

Thanks, applied to nvme-6.12.

Re: [PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[Keith Busch]

On Mon, Oct 07, 2024 at 02:33:24PM -0500, gjoyce@linux.ibm.com wrote:
> +	/*
> +	 * Setting CRIME results in CSTS.RDY before the media is ready. This
> +	 * make it possible for media related commands to return the error
> +	 * NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY. Until the driver is
> +	 * restructurade to handle retries, disable CC.CRIME.
> +	 */
> +	ctrl->ctrl_config &= ~NVME_CC_CRIME;

We could just delete this section entirely. Nothing else sets CRIME, so
no need to explicitly clear it here. I guess it serves to provide a
place for the explanation, I think we can remove the flag setting
anyway.

Re: [PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[Greg Joyce]

On Wed, 2024-10-09 at 16:04 -0600, Keith Busch wrote:
> On Mon, Oct 07, 2024 at 02:33:24PM -0500, gjoyce@linux.ibm.com wrote:
> > +	/*
> > +	 * Setting CRIME results in CSTS.RDY before the media is
> > ready. This
> > +	 * make it possible for media related commands to return
> > the error
> > +	 * NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY. Until the driver
> > is
> > +	 * restructurade to handle retries, disable CC.CRIME.
> > +	 */
> > +	ctrl->ctrl_config &= ~NVME_CC_CRIME;
> 
> We could just delete this section entirely. Nothing else sets CRIME,
> so
> no need to explicitly clear it here. I guess it serves to provide a
> place for the explanation, I think we can remove the flag setting
> anyway.

I went back on forth on that. What I do like is that explicitly shows
that CRIME is not being set and why. But you're right, it's really more
of an elaborate comment. 

I have no problems with removing it. Would you like me to send a new
patch?

Greg

Re: [PATCH v2 1/1] nvme: disable CC.CRIME (NVME_CC_CRIME)

[Keith Busch]

On Thu, Oct 10, 2024 at 11:11:43AM -0500, Greg Joyce wrote:
> On Wed, 2024-10-09 at 16:04 -0600, Keith Busch wrote:
> > On Mon, Oct 07, 2024 at 02:33:24PM -0500, gjoyce@linux.ibm.com wrote:
> > > +	/*
> > > +	 * Setting CRIME results in CSTS.RDY before the media is
> > > ready. This
> > > +	 * make it possible for media related commands to return
> > > the error
> > > +	 * NVME_SC_ADMIN_COMMAND_MEDIA_NOT_READY. Until the driver
> > > is
> > > +	 * restructurade to handle retries, disable CC.CRIME.
> > > +	 */
> > > +	ctrl->ctrl_config &= ~NVME_CC_CRIME;
> > 
> > We could just delete this section entirely. Nothing else sets CRIME,
> > so
> > no need to explicitly clear it here. I guess it serves to provide a
> > place for the explanation, I think we can remove the flag setting
> > anyway.
> 
> I went back on forth on that. What I do like is that explicitly shows
> that CRIME is not being set and why. But you're right, it's really more
> of an elaborate comment. 
> 
> I have no problems with removing it. Would you like me to send a new
> patch?

Let's just leave it. It's already applied and I ought to send out the
next pull request soon. I did, however, fix the restructured spelling :)