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.
