Hello Keith,
On Wed, Sep 13, 2023 at 08:20:09AM -0700, Keith Busch wrote:
On Wed, Sep 13, 2023 at 12:25:29PM +0000, Niklas Cassel wrote:
if (ctrl->ctrl_config & NVME_CC_CRIME)timeout = max(timeout, NVME_CRTO_CRIMT(crto));elsetimeout = max(timeout, NVME_CRTO_CRWMT(crto));I saw the original bug report. But wasn't the problem that these were compared before NVME_CC_CRIME had been written?
i.e. is this max() check still needed for the bug reporter's NVMe drive, after NVME_CC_CRIME was been written and CAP has been re-read? (If so, would a quirk be better?)
The values reported in CRTO don't change with the CC writes. It's only CAP.TO that can change, so we still can't rely on CRTO at any point in the sequence for these devices.
Yes, I know that CRTO (which is the new register), doesn't change. It is supposed to have to values: CRTO.CRIMT CRTO.CRWMT
The hack to modify CAP.TO to be in sync with either CRTO.CRIMT or CRTO.CRWMT is really ugly.
Considering that CRTO.CRIMT and CRTO.CRWMT are also 16-bit, so wider than CAP.TO, suggests that software should read these if available, i.e. no need to ever read CAP.TO if CAP.CRMS.CRWMS is 1 or if CAP.CRMS.CRIMS is 1.
CRTO.CRIMT is allowed to be 0, if CAP.CRMS.CRIMS is 0. CRTO.CRWMT is not allowed to be 0 if CAP.CRMS.CRWMS is 1. (and this bit should be set to 1 according to NVMe 2.0).
Additionally, NVMe 2.0b, Figure 35, clearly states register CRTO as Mandatory for I/O and Admin controllers.
I guess that I just can't understand how a controller can set bit CAP.CRMS.CRWMS to 1, without setting a value in CRTO.CRWMT. Is it such a silly spec violation, that they seem to have not bothered to read what this bit means at all.
Such a controller is so obviously broken that it deserves a quirk.
Although, I understand that using quirks is not always the best solution for end users...
Kind regards, Niklas