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));elsetimeout = 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.