[PATCH] nvme: avoid bogus CRTO values

[Keith Busch]

On Wed, Sep 13, 2023 at 12:50:15PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 12, 2023 at 02:47:33PM -0700, Keith Busch wrote:
> >  1 file changed, 29 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> 
> > +	if (ctrl->cap & NVME_CAP_CRMS_CRWMS && ctrl->cap & NVME_CAP_CRMS_CRIMS)
> 
> I don't think the NVME_CAP_CRMS_CRWMS check here makes sense, this
> should only need the NVME_CAP_CRMS_CRIMS one.

I think you're right, but we currently only enable CRIME if both are
set, so that would be a functional change snuck into this sanity check.
 
> > +	timeout = NVME_CAP_TIMEOUT(ctrl->cap);
> > +	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;
> > +		}
> > +
> > +		/*
> > +		 * 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)
> > +			timeout = max(timeout, NVME_CRTO_CRIMT(crto));
> > +		else
> > +			timeout = max(timeout, NVME_CRTO_CRWMT(crto));
> 
> Should we at least log a harmless one-liner warning if the new timeouts
> are too small?

Felix had something like that in an earlier patch, but it would print on
every reset. That could get a bit spammy on the kernel logs, but I can
add a dev_warn_once() instead.